[PATCH 1/4] X86: Revamp reboot behaviour to match Windows more closely

[PATCH 1/4] X86: Revamp reboot behaviour to match Windows more closely

[Matthew Garrett]

Windows reboots by hitting the ACPI reboot vector (if available), trying
the keyboard controller, hitting the ACPI reboot vector again and then
giving the keyboard controller one last go. Rework our reboot process a
little to default to matching this behaviour, although we'll fall through
to attempting a triple fault if nothing else works.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 arch/x86/kernel/reboot.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 715037c..2b8d03c 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -35,7 +35,7 @@ EXPORT_SYMBOL(pm_power_off);
 
 static const struct desc_ptr no_idt = {};
 static int reboot_mode;
-enum reboot_type reboot_type = BOOT_KBD;
+enum reboot_type reboot_type = BOOT_ACPI;
 int reboot_force;
 
 #if defined(CONFIG_X86_32) && defined(CONFIG_SMP)
@@ -547,9 +547,23 @@ void __attribute__((weak)) mach_reboot_fixups(void)
 {
 }
 
+/*
+ * Windows does the following on reboot:
+ * 1) If the FADT has the ACPI reboot register flag set, try it
+ * 2) If still alive, write to the keyboard controller
+ * 3) If still alive, write to the ACPI reboot register again
+ * 4) If still alive, write to the keyboard controller again
+ *
+ * If the machine is still alive at this stage, it gives up. We default to
+ * following the same pattern, except that if we're still alive after (4) we'll
+ * try to force a triple fault and then cycle between hitting the keyboard
+ * controller and doing that
+ */
 static void native_machine_emergency_restart(void)
 {
 	int i;
+	int attempt = 0;
+	int orig_reboot_type = reboot_type;
 
 	if (reboot_emergency)
 		emergency_vmx_disable_all();
@@ -571,6 +585,13 @@ static void native_machine_emergency_restart(void)
 				outb(0xfe, 0x64); /* pulse reset low */
 				udelay(50);
 			}
+			if (attempt == 0 && orig_reboot_type == BOOT_ACPI) {
+				attempt = 1;
+				reboot_type = BOOT_ACPI;
+			} else {
+				reboot_type = BOOT_TRIPLE;
+			}
+			break;
 
 		case BOOT_TRIPLE:
 			load_idt(&no_idt);
-- 
1.7.4.1

[PATCH 2/4] ACPICA: Fix access width for reset vector

[Matthew Garrett]

Section 4.7.3.6 of the ACPI specification requires that the register width
of the reset vector be 8 bits. Windows simply hardcodes the access to be
a byte and ignores the width provided in the FADT, so make sure that we
do the same.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/acpica/hwxface.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpica/hwxface.c b/drivers/acpi/acpica/hwxface.c
index 6f98d21..f75f81a 100644
--- a/drivers/acpi/acpica/hwxface.c
+++ b/drivers/acpi/acpica/hwxface.c
@@ -80,14 +80,14 @@ acpi_status acpi_reset(void)
 
 	if (reset_reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
 		/*
-		 * For I/O space, write directly to the OSL. This bypasses the port
-		 * validation mechanism, which may block a valid write to the reset
-		 * register.
+		 * For I/O space, write directly to the OSL. This
+		 * bypasses the port validation mechanism, which may
+		 * block a valid write to the reset register. Spec
+		 * section 4.7.3.6 requires register width to be 8.
 		 */
 		status =
 		    acpi_os_write_port((acpi_io_address) reset_reg->address,
-				       acpi_gbl_FADT.reset_value,
-				       reset_reg->bit_width);
+				       acpi_gbl_FADT.reset_value, 8);
 	} else {
 		/* Write the reset value to the reset register */
 
-- 
1.7.4.1

Re: [PATCH 2/4] ACPICA: Fix access width for reset vector

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH 3/4] ACPI: Bug compatibility for Windows on the ACPI reboot vector

[Matthew Garrett]

Windows ignores the bit_offset and bit_width, despite the spec requiring
that they be validated. Drop the checks so that we match this behaviour.
Windows also goes straight for the keyboard controller if the ACPI reboot
fails, so we shouldn't sleep if we're still alive.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/reboot.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/reboot.c b/drivers/acpi/reboot.c
index 93f9114..4870aaa 100644
--- a/drivers/acpi/reboot.c
+++ b/drivers/acpi/reboot.c
@@ -15,9 +15,10 @@ void acpi_reboot(void)
 
 	rr = &acpi_gbl_FADT.reset_register;
 
-	/* Is the reset register supported? */
-	if (!(acpi_gbl_FADT.flags & ACPI_FADT_RESET_REGISTER) ||
-	    rr->bit_width != 8 || rr->bit_offset != 0)
+	/* Is the reset register supported? The spec says we should be
+	 * checking the bit width and bit offset, but Windows ignores
+	 * these fields */
+	if (!(acpi_gbl_FADT.flags & ACPI_FADT_RESET_REGISTER))
 		return;
 
 	reset_value = acpi_gbl_FADT.reset_value;
@@ -45,6 +46,4 @@ void acpi_reboot(void)
 		acpi_reset();
 		break;
 	}
-	/* Wait ten seconds */
-	acpi_os_stall(10000000);
 }
-- 
1.7.4.1

Re: [PATCH 3/4] ACPI: Bug compatibility for Windows on the ACPI reboot vector

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH 4/4] ACPI: Make sure the FADT is at least rev 2 before using the reset register

[Matthew Garrett]

The reset register was only introduced with version 2 of the FADT, so we
should check that the FADT revision before trusting its contents.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/reboot.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/reboot.c b/drivers/acpi/reboot.c
index 4870aaa..a6c77e8b 100644
--- a/drivers/acpi/reboot.c
+++ b/drivers/acpi/reboot.c
@@ -15,6 +15,11 @@ void acpi_reboot(void)
 
 	rr = &acpi_gbl_FADT.reset_register;
 
+	/* ACPI reset register was only introduced with v2 of the FADT */
+
+	if (acpi_gbl_FADT.header.revision < 2)
+		return;
+
 	/* Is the reset register supported? The spec says we should be
 	 * checking the bit width and bit offset, but Windows ignores
 	 * these fields */
-- 
1.7.4.1

Re: [PATCH 4/4] ACPI: Make sure the FADT is at least rev 2 before using the reset register

[Len Brown]

On Fri, 11 Mar 2011, Matthew Garrett wrote:

> The reset register was only introduced with version 2 of the FADT, so we
> should check that the FADT revision before trusting its contents.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/acpi/reboot.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/acpi/reboot.c b/drivers/acpi/reboot.c
> index 4870aaa..a6c77e8b 100644
> --- a/drivers/acpi/reboot.c
> +++ b/drivers/acpi/reboot.c
> @@ -15,6 +15,11 @@ void acpi_reboot(void)
>  
>  	rr = &acpi_gbl_FADT.reset_register;
>  
> +	/* ACPI reset register was only introduced with v2 of the FADT */
> +
> +	if (acpi_gbl_FADT.header.revision < 2)
> +		return;
> +

Isn't this check redundant with the check just below this hunk?

>  	/* Is the reset register supported? The spec says we should be
>  	 * checking the bit width and bit offset, but Windows ignores
>  	 * these fields */


        if (!(acpi_gbl_FADT.flags & ACPI_FADT_RESET_REGISTER))
                return;

For it not to be redundant, there would have to be FADTs out there
of revision 1 that set what was then a reserved bit in the flags.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 4/4] ACPI: Make sure the FADT is at least rev 2 before using the reset register

[Matthew Garrett]

On Wed, Mar 23, 2011 at 12:01:50AM -0400, Len Brown wrote:
>         if (!(acpi_gbl_FADT.flags & ACPI_FADT_RESET_REGISTER))
>                 return;
> 
> For it not to be redundant, there would have to be FADTs out there
> of revision 1 that set what was then a reserved bit in the flags.

Yes. You trust them not to have set reserved flags to random garbage?

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

Re: [PATCH 1/4] X86: Revamp reboot behaviour to match Windows more closely

[Rafael J. Wysocki]

On Friday, March 11, 2011, Matthew Garrett wrote:
> Windows reboots by hitting the ACPI reboot vector (if available), trying
> the keyboard controller, hitting the ACPI reboot vector again and then
> giving the keyboard controller one last go. Rework our reboot process a
> little to default to matching this behaviour, although we'll fall through
> to attempting a triple fault if nothing else works.

Does this fix a particular problem observed in practice?

Rafael


> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  arch/x86/kernel/reboot.c |   23 ++++++++++++++++++++++-
>  1 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 715037c..2b8d03c 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -35,7 +35,7 @@ EXPORT_SYMBOL(pm_power_off);
>  
>  static const struct desc_ptr no_idt = {};
>  static int reboot_mode;
> -enum reboot_type reboot_type = BOOT_KBD;
> +enum reboot_type reboot_type = BOOT_ACPI;
>  int reboot_force;
>  
>  #if defined(CONFIG_X86_32) && defined(CONFIG_SMP)
> @@ -547,9 +547,23 @@ void __attribute__((weak)) mach_reboot_fixups(void)
>  {
>  }
>  
> +/*
> + * Windows does the following on reboot:
> + * 1) If the FADT has the ACPI reboot register flag set, try it
> + * 2) If still alive, write to the keyboard controller
> + * 3) If still alive, write to the ACPI reboot register again
> + * 4) If still alive, write to the keyboard controller again
> + *
> + * If the machine is still alive at this stage, it gives up. We default to
> + * following the same pattern, except that if we're still alive after (4) we'll
> + * try to force a triple fault and then cycle between hitting the keyboard
> + * controller and doing that
> + */
>  static void native_machine_emergency_restart(void)
>  {
>  	int i;
> +	int attempt = 0;
> +	int orig_reboot_type = reboot_type;
>  
>  	if (reboot_emergency)
>  		emergency_vmx_disable_all();
> @@ -571,6 +585,13 @@ static void native_machine_emergency_restart(void)
>  				outb(0xfe, 0x64); /* pulse reset low */
>  				udelay(50);
>  			}
> +			if (attempt == 0 && orig_reboot_type == BOOT_ACPI) {
> +				attempt = 1;
> +				reboot_type = BOOT_ACPI;
> +			} else {
> +				reboot_type = BOOT_TRIPLE;
> +			}
> +			break;
>  
>  		case BOOT_TRIPLE:
>  			load_idt(&no_idt);
> 

Re: [PATCH 1/4] X86: Revamp reboot behaviour to match Windows more closely

[Matthew Garrett]

On Fri, Mar 11, 2011 at 11:08:18PM +0100, Rafael J. Wysocki wrote:
> On Friday, March 11, 2011, Matthew Garrett wrote:
> > Windows reboots by hitting the ACPI reboot vector (if available), trying
> > the keyboard controller, hitting the ACPI reboot vector again and then
> > giving the keyboard controller one last go. Rework our reboot process a
> > little to default to matching this behaviour, although we'll fall through
> > to attempting a triple fault if nothing else works.
> 
> Does this fix a particular problem observed in practice?

Yup. We're seeing an increasing number of machines that don't implement 
the legacy keyboard controller at all and fail to reboot if you poke it. 
The expectation appears to be that you use the ACPI reboot vector on 
these machines.

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

Re: [PATCH 1/4] X86: Revamp reboot behaviour to match Windows more closely

[Rafael J. Wysocki]

On Friday, March 11, 2011, Matthew Garrett wrote:
> On Fri, Mar 11, 2011 at 11:08:18PM +0100, Rafael J. Wysocki wrote:
> > On Friday, March 11, 2011, Matthew Garrett wrote:
> > > Windows reboots by hitting the ACPI reboot vector (if available), trying
> > > the keyboard controller, hitting the ACPI reboot vector again and then
> > > giving the keyboard controller one last go. Rework our reboot process a
> > > little to default to matching this behaviour, although we'll fall through
> > > to attempting a triple fault if nothing else works.
> > 
> > Does this fix a particular problem observed in practice?
> 
> Yup. We're seeing an increasing number of machines that don't implement 
> the legacy keyboard controller at all and fail to reboot if you poke it. 
> The expectation appears to be that you use the ACPI reboot vector on 
> these machines.

So perhaps you can put a pointer or two into the changelog?  That would
show precisely that it's not just for pure Windows compatibility.

Thanks,
Rafael

Re: [PATCH 1/4] X86: Revamp reboot behaviour to match Windows more closely

[Len Brown]

> > > Does this fix a particular problem observed in practice?
> > 
> > Yup. We're seeing an increasing number of machines that don't implement 
> > the legacy keyboard controller at all and fail to reboot if you poke it. 
> > The expectation appears to be that you use the ACPI reboot vector on 
> > these machines.
> 
> So perhaps you can put a pointer or two into the changelog?  That would
> show precisely that it's not just for pure Windows compatibility.

We've been through this before -- many times, in fact.
There are lots of machines that reset only via ACPI,
but when we cut the default over to ACPI, we had
some regressions, and so that change was reverted.

The most recent round started off as a DMI workaround:
http://lkml.org/lkml/2010/1/3/56
and it almost went upstream, but the expected refreshed
patch never materialized.

I think that Matthew's approach is more thorough than
anything else we've tried before wrt compatibility.

But we've found this area to be somewhat fragile in the past,
and so if he succeeds in not breaking any machines,
Matthew is officially my hero:-)

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 1/4] X86: Revamp reboot behaviour to match Windows more closely

[Len Brown]

On Fri, 11 Mar 2011, Matthew Garrett wrote:

> Windows reboots by hitting the ACPI reboot vector (if available), trying
> the keyboard controller, hitting the ACPI reboot vector again and then
> giving the keyboard controller one last go. Rework our reboot process a
> little to default to matching this behaviour, although we'll fall through
> to attempting a triple fault if nothing else works.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Acked-by: Len Brown <len.brown@intel.com>

This looks like "tip" material.

thanks,
Len Brown, Intel Open Source Technology Center