Re: [PATCH v2] x86/EFI: allow reboot= overrides when running under EFI

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Mar 23, 2015 at 09:21:14AM +0000, Ross Lagerwall wrote:
> On 03/12/2015 04:32 PM, Jan Beulich wrote:
> >By default we will always use EFI reboot mechanism when
> >running under EFI platforms. However some EFI platforms
> >are buggy and need to use the ACPI mechanism to
> >reboot (such as Lenovo ThinkCentre M57). As such
> >respect the 'reboot=' override and DMI overrides
> >for EFI platforms.
> >
> >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> >- BOOT_INVALID is just zero
> >- also consider acpi_disabled in BOOT_INVALID resolution
> >- duplicate BOOT_INVALID resolution in machine_restart()
> >- don't fall back from BOOT_ACPI to BOOT_EFI (if it was overridden, it
> >   surely was for a reason)
> >- adjust doc change formatting
> >
> >Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >---
> >v2: Update reboot_type prior to calling efi_reset_system(), as
> >     requested by Andrew.
> >
> >--- a/docs/misc/xen-command-line.markdown
> >+++ b/docs/misc/xen-command-line.markdown
> >@@ -1145,7 +1145,7 @@ The following resources are available:
> >    Technology.
> >
> >  ### reboot
> >-> `= t[riple] | k[bd] | a[cpi] | p[ci] | n[o] [, [w]arm | [c]old]`
> >+> `= t[riple] | k[bd] | a[cpi] | p[ci] | e[fi] | n[o] [, [w]arm | [c]old]`
> >
> >  > Default: `0`
> >
> >@@ -1165,6 +1165,9 @@ Specify the host reboot method.
> >
> >  `pci` instructs Xen to reboot the host using PCI reset register (port CF9).
> >
> >+'efi' instructs Xen to reboot using the EFI reboot call (in EFI mode by
> >+ default it will use that method first).
> >+
> >  ### ro-hpet
> >  > `= <boolean>`
> >
> >--- a/xen/arch/x86/shutdown.c
> >+++ b/xen/arch/x86/shutdown.c
> >@@ -28,16 +28,18 @@
> >  #include <asm/apic.h>
> >
> >  enum reboot_type {
> >+        BOOT_INVALID,
> >          BOOT_TRIPLE = 't',
> >          BOOT_KBD = 'k',
> >          BOOT_ACPI = 'a',
> >          BOOT_CF9 = 'p',
> >+        BOOT_EFI = 'e',
> >  };
> >
> >  static int reboot_mode;
> >
> >  /*
> >- * reboot=t[riple] | k[bd] | a[cpi] | p[ci] | n[o] [, [w]arm | [c]old]
> >+ * reboot=t[riple] | k[bd] | a[cpi] | p[ci] | n[o] | [e]fi [, [w]arm | [c]old]
> >   * warm   Don't set the cold reboot flag
> >   * cold   Set the cold reboot flag
> >   * no     Suppress automatic reboot after panics or crashes
> >@@ -45,8 +47,9 @@ static int reboot_mode;
> >   * kbd    Use the keyboard controller. cold reset (default)
> >   * acpi   Use the RESET_REG in the FADT
> >   * pci    Use the so-called "PCI reset register", CF9
> >+ * efi    Use the EFI reboot (if running under EFI)
> >   */
> >-static enum reboot_type reboot_type = BOOT_ACPI;
> >+static enum reboot_type reboot_type = BOOT_INVALID;
> >  static void __init set_reboot_type(char *str)
> >  {
> >      for ( ; ; )
> >@@ -63,6 +66,7 @@ static void __init set_reboot_type(char
> >              reboot_mode = 0x0;
> >              break;
> >          case 'a':
> >+        case 'e':
> >          case 'k':
> >          case 't':
> >          case 'p':
> >@@ -106,6 +110,14 @@ void machine_halt(void)
> >      __machine_halt(NULL);
> >  }
> >
> >+static void default_reboot_type(void)
> >+{
> >+    if ( reboot_type == BOOT_INVALID )
> >+        reboot_type = efi_enabled ? BOOT_EFI
> >+                                  : acpi_disabled ? BOOT_KBD
> >+                                                  : BOOT_ACPI;
> >+}
> >+
> >  static int __init override_reboot(struct dmi_system_id *d)
> >  {
> >      enum reboot_type type = (long)d->driver_data;
> >@@ -452,6 +464,7 @@ static struct dmi_system_id __initdata r
> >
> >  static int __init reboot_init(void)
> >  {
> >+    default_reboot_type();
> >      dmi_check_system(reboot_dmi_table);
> >      return 0;
> >  }
> >@@ -465,7 +478,7 @@ static void noreturn __machine_restart(v
> >  void machine_restart(unsigned int delay_millisecs)
> >  {
> >      unsigned int i, attempt;
> >-    enum reboot_type orig_reboot_type = reboot_type;
> >+    enum reboot_type orig_reboot_type;
> >      const struct desc_ptr no_idt = { 0 };
> >
> >      watchdog_disable();
> >@@ -504,15 +517,20 @@ void machine_restart(unsigned int delay_
> >          tboot_shutdown(TB_SHUTDOWN_REBOOT);
> >      }
> >
> >-    efi_reset_system(reboot_mode != 0);
> >+    /* Just in case reboot_init() didn't run yet. */
> >+    default_reboot_type();
> >+    orig_reboot_type = reboot_type;
> >
> >      /* Rebooting needs to touch the page at absolute address 0. */
> >-    *((unsigned short *)__va(0x472)) = reboot_mode;
> >+    if ( reboot_type != BOOT_EFI )
> >+        *((unsigned short *)__va(0x472)) = reboot_mode;
> >
> >      for ( attempt = 0; ; attempt++ )
> >      {
> >          switch ( reboot_type )
> >          {
> >+        case BOOT_INVALID:
> >+            ASSERT_UNREACHABLE();
> >          case BOOT_KBD:
> >              /* Pulse the keyboard reset line. */
> >              for ( i = 0; i < 100; i++ )
> >@@ -532,6 +550,11 @@ void machine_restart(unsigned int delay_
> >              reboot_type = (((attempt == 1) && (orig_reboot_type == BOOT_ACPI))
> >                             ? BOOT_ACPI : BOOT_TRIPLE);
> >              break;
> >+        case BOOT_EFI:
> >+            reboot_type = acpi_disabled ? BOOT_KBD : BOOT_ACPI;
> >+            efi_reset_system(reboot_mode != 0);
> >+            *((unsigned short *)__va(0x472)) = reboot_mode;
> 
> This has broken rebooting on EFI systems because acpi_disabled is marked as
> __init_data so it causes a page fault trying to access it.
> 
> acpi_disabled is also used in default_reboot_type() which is called from
> machine_restart().

Bummer! I presume this patch fixes it for you?


diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 384df03..b4b7b91 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -106,7 +106,7 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
 
 unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
 
-bool_t __initdata acpi_disabled;
+bool_t acpi_disabled;
 bool_t __initdata acpi_force;
 static char __initdata acpi_param[10] = "";
 static void __init parse_acpi_param(char *s)

> 
> >+            break;
> >          case BOOT_TRIPLE:
> >              asm volatile ("lidt %0; int3" : : "m" (no_idt));
> >              reboot_type = BOOT_KBD;
> >
> >
> 
> Regards
> -- 
> Ross Lagerwall