1. Patchew
  2. Xen
  3. View series

[PATCH v4] x86/shutdown: change default reboot method preference

Roger Pau Monne posted 1 patch 3 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240802105613.99197-1-roger.pau@citrix.com
CHANGELOG.md            |  2 ++
xen/arch/x86/shutdown.c | 19 +++++++++++++++----
2 files changed, 17 insertions(+), 4 deletions(-)
[PATCH v4] x86/shutdown: change default reboot method preference
Posted by Roger Pau Monne 3 months, 3 weeks ago 
The current logic to chose the preferred reboot method is based on the mode Xen
has been booted into, so if the box is booted from UEFI, the preferred reboot
method will be to use the ResetSystem() run time service call.

However, that method seems to be widely untested, and quite often leads to a
result similar to:

Hardware Dom0 shutdown: rebooting machine
----[ Xen-4.18-unstable  x86_64  debug=y  Tainted:   C    ]----
CPU:    0
RIP:    e008:[<0000000000000017>] 0000000000000017
RFLAGS: 0000000000010202   CONTEXT: hypervisor
[...]
Xen call trace:
   [<0000000000000017>] R 0000000000000017
   [<ffff83207eff7b50>] S ffff83207eff7b50
   [<ffff82d0403525aa>] F machine_restart+0x1da/0x261
   [<ffff82d04035263c>] F apic_wait_icr_idle+0/0x37
   [<ffff82d040233689>] F smp_call_function_interrupt+0xc7/0xcb
   [<ffff82d040352f05>] F call_function_interrupt+0x20/0x34
   [<ffff82d04033b0d5>] F do_IRQ+0x150/0x6f3
   [<ffff82d0402018c2>] F common_interrupt+0x132/0x140
   [<ffff82d040283d33>] F arch/x86/acpi/cpu_idle.c#acpi_idle_do_entry+0x113/0x129
   [<ffff82d04028436c>] F arch/x86/acpi/cpu_idle.c#acpi_processor_idle+0x3eb/0x5f7
   [<ffff82d04032a549>] F arch/x86/domain.c#idle_loop+0xec/0xee

****************************************
Panic on CPU 0:
FATAL TRAP: vector = 6 (invalid opcode)
****************************************

Which in most cases does lead to a reboot, however that's unreliable.

Change the default reboot preference to prefer ACPI over UEFI if available and
not in reduced hardware mode.

This is in line to what Linux does, so it's unlikely to cause issues on current
and future hardware, since there's a much higher chance of vendors testing
hardware with Linux rather than Xen.

Add a special case for one Acer model that does require being rebooted using
ResetSystem().  See Linux commit 0082517fa4bce for rationale.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes since v3:
 - Add changelog entry.

Changes since v2:
 - Rebase and remove incorrect paragraph from the commit message.

Changes since v1:
 - Add special case for Acer model to use UEFI reboot.
 - Adjust commit message.
---
 CHANGELOG.md            |  2 ++
 xen/arch/x86/shutdown.c | 19 +++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index eeaaa8b2741d..5521ae5bb37a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 ## [4.20.0 UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
 
 ### Changed
+ - On x86:
+   - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
 
 ### Added
 
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index acceec2a385d..fa60d1638d58 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -150,19 +150,20 @@ static void default_reboot_type(void)
 
     if ( xen_guest )
         reboot_type = BOOT_XEN;
+    else if ( !acpi_disabled && !acpi_gbl_reduced_hardware )
+        reboot_type = BOOT_ACPI;
     else if ( efi_enabled(EFI_RS) )
         reboot_type = BOOT_EFI;
-    else if ( acpi_disabled )
-        reboot_type = BOOT_KBD;
     else
-        reboot_type = BOOT_ACPI;
+        reboot_type = BOOT_KBD;
 }
 
 static int __init cf_check override_reboot(const struct dmi_system_id *d)
 {
     enum reboot_type type = (long)d->driver_data;
 
-    if ( type == BOOT_ACPI && acpi_disabled )
+    if ( (type == BOOT_ACPI && acpi_disabled) ||
+         (type == BOOT_EFI && !efi_enabled(EFI_RS)) )
         type = BOOT_KBD;
 
     if ( reboot_type != type )
@@ -172,6 +173,7 @@ static int __init cf_check override_reboot(const struct dmi_system_id *d)
             [BOOT_KBD]  = "keyboard controller",
             [BOOT_ACPI] = "ACPI",
             [BOOT_CF9]  = "PCI",
+            [BOOT_EFI]  = "UEFI",
         };
 
         reboot_type = type;
@@ -492,6 +494,15 @@ static const struct dmi_system_id __initconstrel reboot_dmi_table[] = {
             DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
             DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R740")),
     },
+    {    /* Handle problems with rebooting on Acer TravelMate X514-51T. */
+        .callback = override_reboot,
+        .driver_data = (void *)(long)BOOT_EFI,
+        .ident = "Acer TravelMate X514-51T",
+        .matches = {
+            DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+            DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate X514-51T"),
+        },
+    },
     { }
 };
 
-- 
2.45.2
Re: [PATCH v4] x86/shutdown: change default reboot method preference
Posted by Jan Beulich 3 months, 2 weeks ago 
On 02.08.2024 12:56, Roger Pau Monne wrote:
> @@ -492,6 +494,15 @@ static const struct dmi_system_id __initconstrel reboot_dmi_table[] = {
>              DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>              DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R740")),
>      },
> +    {    /* Handle problems with rebooting on Acer TravelMate X514-51T. */
> +        .callback = override_reboot,
> +        .driver_data = (void *)(long)BOOT_EFI,
> +        .ident = "Acer TravelMate X514-51T",
> +        .matches = {
> +            DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> +            DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate X514-51T"),
> +        },
> +    },
>      { }
>  };
>  

Sadly this wasn't properly re-based over the introduction of DMI_MATCH2()
and friends. I guess I'll make a patch, hoping to find someone to ack it
before you return.

Jan
Re: [PATCH v4] x86/shutdown: change default reboot method preference
Posted by oleksii.kurochko@gmail.com 3 months, 3 weeks ago 
On Fri, 2024-08-02 at 12:56 +0200, Roger Pau Monne wrote:
> The current logic to chose the preferred reboot method is based on
> the mode Xen
> has been booted into, so if the box is booted from UEFI, the
> preferred reboot
> method will be to use the ResetSystem() run time service call.
> 
> However, that method seems to be widely untested, and quite often
> leads to a
> result similar to:
> 
> Hardware Dom0 shutdown: rebooting machine
> ----[ Xen-4.18-unstable  x86_64  debug=y  Tainted:   C    ]----
> CPU:    0
> RIP:    e008:[<0000000000000017>] 0000000000000017
> RFLAGS: 0000000000010202   CONTEXT: hypervisor
> [...]
> Xen call trace:
>    [<0000000000000017>] R 0000000000000017
>    [<ffff83207eff7b50>] S ffff83207eff7b50
>    [<ffff82d0403525aa>] F machine_restart+0x1da/0x261
>    [<ffff82d04035263c>] F apic_wait_icr_idle+0/0x37
>    [<ffff82d040233689>] F smp_call_function_interrupt+0xc7/0xcb
>    [<ffff82d040352f05>] F call_function_interrupt+0x20/0x34
>    [<ffff82d04033b0d5>] F do_IRQ+0x150/0x6f3
>    [<ffff82d0402018c2>] F common_interrupt+0x132/0x140
>    [<ffff82d040283d33>] F
> arch/x86/acpi/cpu_idle.c#acpi_idle_do_entry+0x113/0x129
>    [<ffff82d04028436c>] F
> arch/x86/acpi/cpu_idle.c#acpi_processor_idle+0x3eb/0x5f7
>    [<ffff82d04032a549>] F arch/x86/domain.c#idle_loop+0xec/0xee
> 
> ****************************************
> Panic on CPU 0:
> FATAL TRAP: vector = 6 (invalid opcode)
> ****************************************
> 
> Which in most cases does lead to a reboot, however that's unreliable.
> 
> Change the default reboot preference to prefer ACPI over UEFI if
> available and
> not in reduced hardware mode.
> 
> This is in line to what Linux does, so it's unlikely to cause issues
> on current
> and future hardware, since there's a much higher chance of vendors
> testing
> hardware with Linux rather than Xen.
> 
> Add a special case for one Acer model that does require being
> rebooted using
> ResetSystem().  See Linux commit 0082517fa4bce for rationale.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com>
> ---
> Changes since v3:
>  - Add changelog entry.
> 
> Changes since v2:
>  - Rebase and remove incorrect paragraph from the commit message.
> 
> Changes since v1:
>  - Add special case for Acer model to use UEFI reboot.
>  - Adjust commit message.
> ---
>  CHANGELOG.md            |  2 ++
>  xen/arch/x86/shutdown.c | 19 +++++++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index eeaaa8b2741d..5521ae5bb37a 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -7,6 +7,8 @@ The format is based on [Keep a
> Changelog](https://keepachangelog.com/en/1.0.0/)
>  ## [4.20.0
> UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortl
> og;h=staging) - TBD
>  
>  ### Changed
> + - On x86:
> +   - Prefer ACPI reboot over UEFI ResetSystem() run time service
> call.
Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

>  
>  ### Added
>  
> diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
> index acceec2a385d..fa60d1638d58 100644
> --- a/xen/arch/x86/shutdown.c
> +++ b/xen/arch/x86/shutdown.c
> @@ -150,19 +150,20 @@ static void default_reboot_type(void)
>  
>      if ( xen_guest )
>          reboot_type = BOOT_XEN;
> +    else if ( !acpi_disabled && !acpi_gbl_reduced_hardware )
> +        reboot_type = BOOT_ACPI;
>      else if ( efi_enabled(EFI_RS) )
>          reboot_type = BOOT_EFI;
> -    else if ( acpi_disabled )
> -        reboot_type = BOOT_KBD;
>      else
> -        reboot_type = BOOT_ACPI;
> +        reboot_type = BOOT_KBD;
>  }
>  
>  static int __init cf_check override_reboot(const struct
> dmi_system_id *d)
>  {
>      enum reboot_type type = (long)d->driver_data;
>  
> -    if ( type == BOOT_ACPI && acpi_disabled )
> +    if ( (type == BOOT_ACPI && acpi_disabled) ||
> +         (type == BOOT_EFI && !efi_enabled(EFI_RS)) )
>          type = BOOT_KBD;
>  
>      if ( reboot_type != type )
> @@ -172,6 +173,7 @@ static int __init cf_check override_reboot(const
> struct dmi_system_id *d)
>              [BOOT_KBD]  = "keyboard controller",
>              [BOOT_ACPI] = "ACPI",
>              [BOOT_CF9]  = "PCI",
> +            [BOOT_EFI]  = "UEFI",
>          };
>  
>          reboot_type = type;
> @@ -492,6 +494,15 @@ static const struct dmi_system_id __initconstrel
> reboot_dmi_table[] = {
>              DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>              DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R740")),
>      },
> +    {    /* Handle problems with rebooting on Acer TravelMate X514-
> 51T. */
> +        .callback = override_reboot,
> +        .driver_data = (void *)(long)BOOT_EFI,
> +        .ident = "Acer TravelMate X514-51T",
> +        .matches = {
> +            DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> +            DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate X514-51T"),
> +        },
> +    },
>      { }
>  };
>  

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.