Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Robert Hancock]

Matthew Garrett wrote:
> On Fri, Nov 07, 2008 at 09:01:19AM +0800, Zhao Yakui wrote:
> 
>> With the help of KVM I find that the windows will be rebooted by writing
>> RESET_VALUE to RESET_REG I/O port if the RESET_REG_SUP bit is not
>> zero(It indicates whether ACPI reboot is supported).
>> IMO maybe the ACPI reboot is the first choice. If it can't, then it will
>> fall back to other mode.
> 
> Hmm. But we're seeing some machines that end up very confused if 
> rebooted via ACPI. I guess we need to run Vista on them to find out how 
> they behave. What OSI strings did your KVM setup expose? We know that 
> Windows changes behaviour under various circumstances depending on which 
> OS the firmware requests, so it's almost possible that this is another 
> of those cases.

Given that Windows behavior, this patch seems suspicious:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8fd145917fb62368a9b80db59562c20576238f5a

This patch ignores the RESET_REG_SUP flag and just tries using the reset 
register anyway if it thinks it's valid. So we may attempt ACPI reset on 
machines which don't indicate it's supported.

The patch description mentioned that some machines didn't reboot after 
S3 suspend without this patch. However, we recently had this patch merged:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=a68823ee5285e65b51ceb96f8b13a5b4f99a6888

Is it possible that the problem fixed there is the true cause of this 
reboot after S3 problem?

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Matthew Garrett]

On Thu, Nov 06, 2008 at 07:30:43PM -0600, Robert Hancock wrote:

> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8fd145917fb62368a9b80db59562c20576238f5a
> 
> This patch ignores the RESET_REG_SUP flag and just tries using the reset 
> register anyway if it thinks it's valid. So we may attempt ACPI reset on 
> machines which don't indicate it's supported.

Yeah, that sounds very wrong.

> The patch description mentioned that some machines didn't reboot after 
> S3 suspend without this patch. However, we recently had this patch merged:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=a68823ee5285e65b51ceb96f8b13a5b4f99a6888
> 
> Is it possible that the problem fixed there is the true cause of this 
> reboot after S3 problem?

Oh, yeah, could be. Given the From:, I should really have thought of 
that :)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Len Brown]

On Fri, 7 Nov 2008, Matthew Garrett wrote:

> On Thu, Nov 06, 2008 at 07:30:43PM -0600, Robert Hancock wrote:
> 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8fd145917fb62368a9b80db59562c20576238f5a
> > 
> > This patch ignores the RESET_REG_SUP flag and just tries using the reset 
> > register anyway if it thinks it's valid. So we may attempt ACPI reset on 
> > machines which don't indicate it's supported.
> 
> Yeah, that sounds very wrong.

As it turns out, it was an incorrect guess on our part on how to be "bug 
compatible" and I'm reverting it per the regression report here:

http://bugzilla.kernel.org/show_bug.cgi?id=11942

-Len

> > The patch description mentioned that some machines didn't reboot after 
> > S3 suspend without this patch. However, we recently had this patch merged:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=a68823ee5285e65b51ceb96f8b13a5b4f99a6888
> > 
> > Is it possible that the problem fixed there is the true cause of this 
> > reboot after S3 problem?
> 
> Oh, yeah, could be. Given the From:, I should really have thought of 
> that :)
> 
> -- 
> Matthew Garrett | mjg59@srcf.ucam.org
> 

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Rafael J. Wysocki]

On Friday, 7 of November 2008, Robert Hancock wrote:
> Matthew Garrett wrote:
> > On Fri, Nov 07, 2008 at 09:01:19AM +0800, Zhao Yakui wrote:
> > 
> >> With the help of KVM I find that the windows will be rebooted by writing
> >> RESET_VALUE to RESET_REG I/O port if the RESET_REG_SUP bit is not
> >> zero(It indicates whether ACPI reboot is supported).
> >> IMO maybe the ACPI reboot is the first choice. If it can't, then it will
> >> fall back to other mode.
> > 
> > Hmm. But we're seeing some machines that end up very confused if 
> > rebooted via ACPI. I guess we need to run Vista on them to find out how 
> > they behave. What OSI strings did your KVM setup expose? We know that 
> > Windows changes behaviour under various circumstances depending on which 
> > OS the firmware requests, so it's almost possible that this is another 
> > of those cases.
> 
> Given that Windows behavior, this patch seems suspicious:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8fd145917fb62368a9b80db59562c20576238f5a
> 
> This patch ignores the RESET_REG_SUP flag and just tries using the reset 
> register anyway if it thinks it's valid. So we may attempt ACPI reset on 
> machines which don't indicate it's supported.
> 
> The patch description mentioned that some machines didn't reboot after 
> S3 suspend without this patch. However, we recently had this patch merged:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=a68823ee5285e65b51ceb96f8b13a5b4f99a6888
> 
> Is it possible that the problem fixed there is the true cause of this 
> reboot after S3 problem?

Generally, it is.

Should it regarded as -stable material, BTW, or is it already in -stable?

Matthew?

Thanks,
Rafael

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Matthew Garrett]

On Sun, Nov 09, 2008 at 12:11:03PM +0200, Avi Kivity wrote:
> Matthew Garrett wrote:
> >Hmm. But we're seeing some machines that end up very confused if 
> >rebooted via ACPI. I guess we need to run Vista on them to find out how 
> >they behave. What OSI strings did your KVM setup expose? We know that 
> >Windows changes behaviour under various circumstances depending on which 
> >OS the firmware requests, so it's almost possible that this is another 
> >of those cases.
> >  
> 
> Isn't it the other way around?  The firmware changes behavior depending 
> on how the OS identifies itself?

That also happens, yes.

> Reboot is a fixed feature IIRC, so it cannot change depending on 
> identification strings.

The ID strings that the firmware requests give a good idea about which 
operating systems the machine has been tested with. If Vista uses the 
ACPI method then having the firmware request the OSI string for Vista 
gives us a good indication that it's safe to use the ACPI method.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Avi Kivity]

Matthew Garrett wrote:
> Hmm. But we're seeing some machines that end up very confused if 
> rebooted via ACPI. I guess we need to run Vista on them to find out how 
> they behave. What OSI strings did your KVM setup expose? We know that 
> Windows changes behaviour under various circumstances depending on which 
> OS the firmware requests, so it's almost possible that this is another 
> of those cases.
>   

Isn't it the other way around?  The firmware changes behavior depending 
on how the OS identifies itself?

Reboot is a fixed feature IIRC, so it cannot change depending on 
identification strings.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Zhao Yakui]

On Fri, 2008-11-07 at 06:17 +0800, Len Brown wrote:
> 
> > > My expectation is that with the ACPI default, our problem
> > > is working around a finite list of old machines that don't work;
> > > while with the default KBD, our problem is working around
> > > a potentially unbounded list of yet to be shipped machines
> > > who may only be tested and work using the ACPI method.
> > 
> > Does Windows default to using the ACPI method now?
> 
> That is my guess, based on the fact that we've seen
> newer machines that don't reboot w/o using the ACPI reset reg.
With the help of KVM I find that the windows will be rebooted by writing
RESET_VALUE to RESET_REG I/O port if the RESET_REG_SUP bit is not
zero(It indicates whether ACPI reboot is supported).
IMO maybe the ACPI reboot is the first choice. If it can't, then it will
fall back to other mode.

Thanks.
> 
> -Len
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Matthew Garrett]

On Fri, Nov 07, 2008 at 09:01:19AM +0800, Zhao Yakui wrote:

> With the help of KVM I find that the windows will be rebooted by writing
> RESET_VALUE to RESET_REG I/O port if the RESET_REG_SUP bit is not
> zero(It indicates whether ACPI reboot is supported).
> IMO maybe the ACPI reboot is the first choice. If it can't, then it will
> fall back to other mode.

Hmm. But we're seeing some machines that end up very confused if 
rebooted via ACPI. I guess we need to run Vista on them to find out how 
they behave. What OSI strings did your KVM setup expose? We know that 
Windows changes behaviour under various circumstances depending on which 
OS the firmware requests, so it's almost possible that this is another 
of those cases.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Matthew Garrett]

On Thu, Nov 06, 2008 at 05:17:25PM -0500, Len Brown wrote:
> > Does Windows default to using the ACPI method now?
> 
> That is my guess, based on the fact that we've seen
> newer machines that don't reboot w/o using the ACPI reset reg.

We've seen machines that won't reboot for a variety of reasons in the 
past. In some cases this has seemed to be due to hardware being in a 
state that the BIOS didn't expect. Hitting the SMI trap behind the ACPI 
register might work around this, but I suspect that in many cases we 
could achieve the same effect by spending more time trying to work out 
whether there's any common themes in the failures.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Len Brown]

> > My expectation is that with the ACPI default, our problem
> > is working around a finite list of old machines that don't work;
> > while with the default KBD, our problem is working around
> > a potentially unbounded list of yet to be shipped machines
> > who may only be tested and work using the ACPI method.
> 
> Does Windows default to using the ACPI method now?

That is my guess, based on the fact that we've seen
newer machines that don't reboot w/o using the ACPI reset reg.

-Len

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Matthew Garrett]

On Thu, Nov 06, 2008 at 02:50:06PM -0500, Len Brown wrote:

> My expectation is that with the ACPI default, our problem
> is working around a finite list of old machines that don't work;
> while with the default KBD, our problem is working around
> a potentially unbounded list of yet to be shipped machines
> who may only be tested and work using the ACPI method.

Does Windows default to using the ACPI method now?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Len Brown]

> > This reverts commit c7ffa6c26277b403920e2255d10df849bd613380.

I agree that the 2.6.27 default was changed to ACPI for the wrong reason.

However, I think it was the right thing to do,
and if you didn't propose it, I would.

My expectation is that with the ACPI default, our problem
is working around a finite list of old machines that don't work;
while with the default KBD, our problem is working around
a potentially unbounded list of yet to be shipped machines
who may only be tested and work using the ACPI method.

So I recommend leaving the default as ACPI for a while to
see how it goes.

thanks,
-Len

ps. please cc: linux-acpi@vger.kernel.org on ACPI related changes.

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

[I had to trim direct recipients as my provider would refuse deliver
claiming it is spam]

On Thursday 06 November 2008, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > Andrey Borzenkov's patch, for example, adds a new DMI entry 
> > > because reboot=acpi breaks his keyboard (even without KVM, I 
> > > guess). Andrey, was that the case?
> > 
> > hm, IIRC the problem was KVM in his case too.
> 
> actually, Andrey's problem seems to be unrelated. So i've queued up 
> the revert below in tip/x86/urgent for v2.6.28. Thanks guys!
> 

Yes, I do not use KVM. Actually my CPU (PIII) does not even support
virtualization.

> 	Ingo
> 
> ---------------->
> From 8d00450d296dedec9ada38d43b83e79cca6fd5a3 Mon Sep 17 00:00:00 2001
> From: Eduardo Habkost <ehabkost@redhat.com>
> Date: Tue, 4 Nov 2008 12:52:44 -0200
> Subject: [PATCH] Revert "x86: default to reboot via ACPI"
> 
> This reverts commit c7ffa6c26277b403920e2255d10df849bd613380.
> 
> the assumptio of this change was that this would not break
> any existing machine. Andrey Borzenkov reported troubles with
> the ACPI reboot method: the system would hang on reboot, necessiating
> a power cycle. Probably more systems are affected as well.
> 

To be precise - system reboots but keyboard is non-functional after that.
Power off is required to clear this condition.

I am fine with either way (revert or DMI). But if problem which ACPI
reboot fixed (or worked around) is not solved differently I think
reverting to old way is better.

> Also, there are patches queued up for v2.6.29 to disable virtualization
> on emergency_restart() - which was the original motivation of
> this change.
> 
> Reported-by: Andrey Borzenkov <arvidjaar@mail.ru>
> Bisected-by: Andrey Borzenkov <arvidjaar@mail.ru>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Acked-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/reboot.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index f4c93f1..724adfc 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -29,11 +29,7 @@ EXPORT_SYMBOL(pm_power_off);
>  
>  static const struct desc_ptr no_idt = {};
>  static int reboot_mode;
> -/*
> - * Keyboard reset and triple fault may result in INIT, not RESET, which
> - * doesn't work when we're in vmx root mode.  Try ACPI first.
> - */
> -enum reboot_type reboot_type = BOOT_ACPI;
> +enum reboot_type reboot_type = BOOT_KBD;
>  int reboot_force;
>  
>  #if defined(CONFIG_X86_32) && defined(CONFIG_SMP)
> 
> 



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Avi Kivity]

Eric W. Biederman wrote:
> If there are a number of problems with reboot and reset
> I'm wondering if we should investigate using an
> outb to port 0x92.  With the right bit set you can trigger
> a toggle of the reset line on many motherboards.
>   

Most likely that's what the ACPI reset describes.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Eric W. Biederman]

Ingo Molnar <mingo@elte.hu> writes:

> * Ingo Molnar <mingo@elte.hu> wrote:
>
>> > Andrey Borzenkov's patch, for example, adds a new DMI entry 
>> > because reboot=acpi breaks his keyboard (even without KVM, I 
>> > guess). Andrey, was that the case?
>> 
>> hm, IIRC the problem was KVM in his case too.
>
> actually, Andrey's problem seems to be unrelated. So i've queued up 
> the revert below in tip/x86/urgent for v2.6.28. Thanks guys!

If there are a number of problems with reboot and reset
I'm wondering if we should investigate using an
outb to port 0x92.  With the right bit set you can trigger
a toggle of the reset line on many motherboards.

Eric

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > Andrey Borzenkov's patch, for example, adds a new DMI entry 
> > because reboot=acpi breaks his keyboard (even without KVM, I 
> > guess). Andrey, was that the case?
> 
> hm, IIRC the problem was KVM in his case too.

actually, Andrey's problem seems to be unrelated. So i've queued up 
the revert below in tip/x86/urgent for v2.6.28. Thanks guys!

	Ingo

---------------->
>From 8d00450d296dedec9ada38d43b83e79cca6fd5a3 Mon Sep 17 00:00:00 2001
From: Eduardo Habkost <ehabkost@redhat.com>
Date: Tue, 4 Nov 2008 12:52:44 -0200
Subject: [PATCH] Revert "x86: default to reboot via ACPI"

This reverts commit c7ffa6c26277b403920e2255d10df849bd613380.

the assumptio of this change was that this would not break
any existing machine. Andrey Borzenkov reported troubles with
the ACPI reboot method: the system would hang on reboot, necessiating
a power cycle. Probably more systems are affected as well.

Also, there are patches queued up for v2.6.29 to disable virtualization
on emergency_restart() - which was the original motivation of
this change.

Reported-by: Andrey Borzenkov <arvidjaar@mail.ru>
Bisected-by: Andrey Borzenkov <arvidjaar@mail.ru>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/reboot.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index f4c93f1..724adfc 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -29,11 +29,7 @@ EXPORT_SYMBOL(pm_power_off);
 
 static const struct desc_ptr no_idt = {};
 static int reboot_mode;
-/*
- * Keyboard reset and triple fault may result in INIT, not RESET, which
- * doesn't work when we're in vmx root mode.  Try ACPI first.
- */
-enum reboot_type reboot_type = BOOT_ACPI;
+enum reboot_type reboot_type = BOOT_KBD;
 int reboot_force;
 
 #if defined(CONFIG_X86_32) && defined(CONFIG_SMP)

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Ingo Molnar]

* Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Nov 06, 2008 at 08:14:45AM +0100, Ingo Molnar wrote:
> > 
> > * Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > This reverts commit c7ffa6c26277b403920e2255d10df849bd613380.
> > > 
> > > Now that we have the hooks to disable virtualization on
> > > emergency_restart(), we can get back to the BOOT_KBD reboot_type default.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > hm, why revert this? There's nothing wrong with the ACPI reboot 
> > method, it's just that we surprise the BIOS by exiting with an unclean 
> > VMX state in certain circumstances.
> > 
> > if the ACPI reboot method does not work we do the KBD method as the 
> > next thing in the reboot chain.
> 
> I suppose there are cases where the new default broke without KVM, 
> and the suggestion was to disable VMX before rebooting instead of 
> changing the default.
> 
> Avi changed the default because on some machines reboot=kbd breaks 
> when VMX is enabled, but the regressions caused by the new default 
> doesn't necessarily involve VMX.

there's another reason as well: a growing quirk-list of machines where 
ACPI is the best (sometimes only) method to reboot.

> Andrey Borzenkov's patch, for example, adds a new DMI entry because 
> reboot=acpi breaks his keyboard (even without KVM, I guess). Andrey, 
> was that the case?

hm, IIRC the problem was KVM in his case too.

	Ingo

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Eduardo Habkost]

On Thu, Nov 06, 2008 at 08:14:45AM +0100, Ingo Molnar wrote:
> 
> * Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This reverts commit c7ffa6c26277b403920e2255d10df849bd613380.
> > 
> > Now that we have the hooks to disable virtualization on
> > emergency_restart(), we can get back to the BOOT_KBD reboot_type default.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> hm, why revert this? There's nothing wrong with the ACPI reboot 
> method, it's just that we surprise the BIOS by exiting with an unclean 
> VMX state in certain circumstances.
> 
> if the ACPI reboot method does not work we do the KBD method as the 
> next thing in the reboot chain.

I suppose there are cases where the new default broke without KVM, and
the suggestion was to disable VMX before rebooting instead of changing
the default.

Avi changed the default because on some machines reboot=kbd breaks when
VMX is enabled, but the regressions caused by the new default doesn't
necessarily involve VMX.

Andrey Borzenkov's patch, for example, adds a new DMI entry because
reboot=acpi breaks his keyboard (even without KVM, I guess). Andrey,
was that the case?

-- 
Eduardo

Re: [PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Ingo Molnar]

* Eduardo Habkost <ehabkost@redhat.com> wrote:

> This reverts commit c7ffa6c26277b403920e2255d10df849bd613380.
> 
> Now that we have the hooks to disable virtualization on
> emergency_restart(), we can get back to the BOOT_KBD reboot_type default.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

hm, why revert this? There's nothing wrong with the ACPI reboot 
method, it's just that we surprise the BIOS by exiting with an unclean 
VMX state in certain circumstances.

if the ACPI reboot method does not work we do the KBD method as the 
next thing in the reboot chain.

	Ingo

[PATCH 15/15] Revert "x86: default to reboot via ACPI"

[Eduardo Habkost]

This reverts commit c7ffa6c26277b403920e2255d10df849bd613380.

Now that we have the hooks to disable virtualization on
emergency_restart(), we can get back to the BOOT_KBD reboot_type default.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 arch/x86/kernel/reboot.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 3240491..260c131 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -32,11 +32,7 @@ EXPORT_SYMBOL(pm_power_off);
 
 static const struct desc_ptr no_idt = {};
 static int reboot_mode;
-/*
- * Keyboard reset and triple fault may result in INIT, not RESET, which
- * doesn't work when we're in vmx root mode.  Try ACPI first.
- */
-enum reboot_type reboot_type = BOOT_ACPI;
+enum reboot_type reboot_type = BOOT_KBD;
 int reboot_force;
 
 #if defined(CONFIG_X86_32) && defined(CONFIG_SMP)
-- 
1.5.5.GIT