From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Lagerwall Subject: Re: [PATCH v2] x86/EFI: allow reboot= overrides when running under EFI Date: Mon, 23 Mar 2015 16:06:51 +0000 Message-ID: <55103A1B.7060906@citrix.com> References: <5501CD9302000078000694AB@mail.emea.novell.com> <550FDB0A.9040809@citrix.com> <20150323131302.GB8723@l.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Ya55f-0001V3-Jo for xen-devel@lists.xenproject.org; Mon, 23 Mar 2015 16:20:35 +0000 In-Reply-To: <20150323131302.GB8723@l.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: xen-devel , Jan Beulich , Andrew Cooper List-Id: xen-devel@lists.xenproject.org On 03/23/2015 01:13 PM, Konrad Rzeszutek Wilk wrote: > 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 >>> >>> - 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 >>> Reviewed-by: Konrad Rzeszutek Wilk >>> --- >>> 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 >>> > `= ` >>> >>> --- a/xen/arch/x86/shutdown.c >>> +++ b/xen/arch/x86/shutdown.c >>> @@ -28,16 +28,18 @@ >>> #include >>> >>> 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) > >> Yeah, that fixes it. -- Ross Lagerwall