[PATCH RFC] efi: By default use the BOOT_ACPI method instead of BOOT_EFI unless on reduced ACPI hardware.

[PATCH RFC] efi: By default use the BOOT_ACPI method instead of BOOT_EFI unless on reduced ACPI hardware.

[Konrad Rzeszutek Wilk]

This mimics the behavior of the Linux kernel in which the reboot
sequence by default under EFI booted kernels is first ACPI.

EFI reboot is only tried if the user supplied it or if the hardware
is an ACPI 5.0 (or higher) reduced hardware board.

This fixes the EFI firmware crashing on Lenovo ThinkCentre M57

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 docs/misc/xen-command-line.markdown |  5 ++++-
 xen/arch/x86/shutdown.c             | 17 +++++++++++++----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a061aa4..f037e0f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1099,7 +1099,7 @@ The following resources are available:
   * `rmid_max` indicates the max value for rmid.
 
 ### reboot
-> `= t[riple] | k[bd] | a[cpi] | p[ci] | n[o] [, [w]arm | [c]old]`
+> `= t[riple] | k[bd] | a[cpi] | p[ci] | n[o] | [e]fi [, [w]arm | [c]old]`
 
 > Default: `0`
 
@@ -1119,6 +1119,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 ACPI method first).
+
 ### sched
 > `= credit | credit2 | sedf | arinc653`
 
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 21f6cf5..f17ae9b 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -32,12 +32,13 @@ enum reboot_type {
         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,6 +46,7 @@ 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 void __init set_reboot_type(char *str)
@@ -66,6 +68,7 @@ static void __init set_reboot_type(char *str)
         case 'k':
         case 't':
         case 'p':
+        case 'e':
             reboot_type = *str;
             break;
         }
@@ -452,7 +455,11 @@ static struct dmi_system_id __initdata reboot_dmi_table[] = {
 
 static int __init reboot_init(void)
 {
-    dmi_check_system(reboot_dmi_table);
+    int rv;
+
+    rv = dmi_check_system(reboot_dmi_table);
+    if ( !rv && acpi_gbl_reduced_hardware )
+        reboot_type = BOOT_EFI;
     return 0;
 }
 __initcall(reboot_init);
@@ -504,8 +511,6 @@ void machine_restart(unsigned int delay_millisecs)
         tboot_shutdown(TB_SHUTDOWN_REBOOT);
     }
 
-    efi_reset_system(reboot_mode != 0);
-
     /* Rebooting needs to touch the page at absolute address 0. */
     *((unsigned short *)__va(0x472)) = reboot_mode;
 
@@ -532,6 +537,10 @@ void machine_restart(unsigned int delay_millisecs)
             reboot_type = (((attempt == 1) && (orig_reboot_type == BOOT_ACPI))
                            ? BOOT_ACPI : BOOT_TRIPLE);
             break;
+        case BOOT_EFI:
+            efi_reset_system(reboot_mode != 0);
+            reboot_type = BOOT_ACPI;
+            break;
         case BOOT_TRIPLE:
             asm volatile ("lidt %0; int3" : : "m" (no_idt));
             reboot_type = BOOT_KBD;
-- 
2.1.0

Re: [PATCH RFC] efi: By default use the BOOT_ACPI method instead of BOOT_EFI unless on reduced ACPI hardware.

[Jan Beulich]

>>> On 21.01.15 at 22:53, <konrad.wilk@oracle.com> wrote:
> This mimics the behavior of the Linux kernel in which the reboot
> sequence by default under EFI booted kernels is first ACPI.

Which is contrary to the EFI spec. I.e. NAK.

> EFI reboot is only tried if the user supplied it or if the hardware
> is an ACPI 5.0 (or higher) reduced hardware board.
> 
> This fixes the EFI firmware crashing on Lenovo ThinkCentre M57

Buggy firmware should be worked around with "reboot="; I'd
certainly accept a patch to bypass efi_reset_system() in that
case.

Jan

Re: [PATCH RFC] efi: By default use the BOOT_ACPI method instead of BOOT_EFI unless on reduced ACPI hardware.

[Konrad Rzeszutek Wilk]

On Thu, Jan 22, 2015 at 09:49:08AM +0000, Jan Beulich wrote:
> >>> On 21.01.15 at 22:53, <konrad.wilk@oracle.com> wrote:
> > This mimics the behavior of the Linux kernel in which the reboot
> > sequence by default under EFI booted kernels is first ACPI.
> 
> Which is contrary to the EFI spec. I.e. NAK.

I am failing to see that in the spec. I see that it says what
the ResetSystem() call does, but nothing about "MUST".

I see this at the start of the spec:

" Together, these provide a standard environment for booting an OS. This
specification is designed as a pure interface specification. As such,
the specification defines the set of interface s and structures that
platform firmware must implement. "

(which talks about 'booting an OS' - which this is not, and interestingly
enough - it does say implement, but not where it must implement it
correctly!).

But I have not dug that deep in the spec to find something
that says you MUST not use existing other specs? Perhaps you
remember where the contrary part is?


Also, why do we want to be different that Windows and Linux when doing
EFI operations?



> 
> > EFI reboot is only tried if the user supplied it or if the hardware
> > is an ACPI 5.0 (or higher) reduced hardware board.
> > 
> > This fixes the EFI firmware crashing on Lenovo ThinkCentre M57
> 
> Buggy firmware should be worked around with "reboot="; I'd
> certainly accept a patch to bypass efi_reset_system() in that
> case.

Independent of the conversation above I will work the patch
that way.  Would you also be OK if I stuck the DMI data for the
ThinkCentre to make this automatic?

> 
> Jan
> 

Re: [PATCH RFC] efi: By default use the BOOT_ACPI method instead of BOOT_EFI unless on reduced ACPI hardware.

[Jan Beulich]

>>> On 22.01.15 at 16:01, <konrad.wilk@oracle.com> wrote:
> On Thu, Jan 22, 2015 at 09:49:08AM +0000, Jan Beulich wrote:
>> >>> On 21.01.15 at 22:53, <konrad.wilk@oracle.com> wrote:
>> > This mimics the behavior of the Linux kernel in which the reboot
>> > sequence by default under EFI booted kernels is first ACPI.
>> 
>> Which is contrary to the EFI spec. I.e. NAK.
> 
> I am failing to see that in the spec. I see that it says what
> the ResetSystem() call does, but nothing about "MUST".
> 
> I see this at the start of the spec:
> 
> " Together, these provide a standard environment for booting an OS. This
> specification is designed as a pure interface specification. As such,
> the specification defines the set of interface s and structures that
> platform firmware must implement. "
> 
> (which talks about 'booting an OS' - which this is not, and interestingly
> enough - it does say implement, but not where it must implement it
> correctly!).
> 
> But I have not dug that deep in the spec to find something
> that says you MUST not use existing other specs? Perhaps you
> remember where the contrary part is?

The whole spirit of EFI is to get the OS away from doing things
some custom (and hence possibly fragile) way.

> Also, why do we want to be different that Windows and Linux when doing
> EFI operations?

Because just because they do things a certain way doesn't mean
they do it right. Linux having actively removed using the time
related runtime services functions is something that I for example
absolutely can't agree with. If there are firmware vendors not
getting their act together, having ways to work around that is
acceptable, but outright violation of the spec is wrong imo.

>> > EFI reboot is only tried if the user supplied it or if the hardware
>> > is an ACPI 5.0 (or higher) reduced hardware board.
>> > 
>> > This fixes the EFI firmware crashing on Lenovo ThinkCentre M57
>> 
>> Buggy firmware should be worked around with "reboot="; I'd
>> certainly accept a patch to bypass efi_reset_system() in that
>> case.
> 
> Independent of the conversation above I will work the patch
> that way.  Would you also be OK if I stuck the DMI data for the
> ThinkCentre to make this automatic?

Yes, as long as the match isn't too generic (i.e. wouldn't match all
furure models too).

Jan

Re: [PATCH RFC] efi: By default use the BOOT_ACPI method instead of BOOT_EFI unless on reduced ACPI hardware.

[Konrad Rzeszutek Wilk]

On Thu, Jan 22, 2015 at 03:22:10PM +0000, Jan Beulich wrote:
> >>> On 22.01.15 at 16:01, <konrad.wilk@oracle.com> wrote:
> > On Thu, Jan 22, 2015 at 09:49:08AM +0000, Jan Beulich wrote:
> >> >>> On 21.01.15 at 22:53, <konrad.wilk@oracle.com> wrote:
> >> > This mimics the behavior of the Linux kernel in which the reboot
> >> > sequence by default under EFI booted kernels is first ACPI.
> >> 
> >> Which is contrary to the EFI spec. I.e. NAK.
> > 
> > I am failing to see that in the spec. I see that it says what
> > the ResetSystem() call does, but nothing about "MUST".
> > 
> > I see this at the start of the spec:
> > 
> > " Together, these provide a standard environment for booting an OS. This
> > specification is designed as a pure interface specification. As such,
> > the specification defines the set of interface s and structures that
> > platform firmware must implement. "
> > 
> > (which talks about 'booting an OS' - which this is not, and interestingly
> > enough - it does say implement, but not where it must implement it
> > correctly!).
> > 
> > But I have not dug that deep in the spec to find something
> > that says you MUST not use existing other specs? Perhaps you
> > remember where the contrary part is?
> 
> The whole spirit of EFI is to get the OS away from doing things
> some custom (and hence possibly fragile) way.

ACPI being 'custom'? I am not sure if there is a way on x86 to run
without ACPI at all. I can see it under ARM where you have the
Device Tree to complement it - but either way EFI is not an
full solution to manage the system.
> 
> > Also, why do we want to be different that Windows and Linux when doing
> > EFI operations?
> 
> Because just because they do things a certain way doesn't mean
> they do it right. Linux having actively removed using the time
> related runtime services functions is something that I for example
> absolutely can't agree with. If there are firmware vendors not
> getting their act together, having ways to work around that is
> acceptable, but outright violation of the spec is wrong imo.

There is the spec and there is the implementation (Windows) that
every motherboard manufacturer follows and caters to. It does not
matter if the they (Microsoft) violate the spec - by them violating
it or not using certain things - it makes it an de-facto specification.

Now I don't know if Linux ignoring the time runtime services has
been due to bugs or just following what Windows did - but either way
having it done that way - makes the firmware vendors that care
about Linux - implement it to work under Linux (so expose the
legacy timers even in EFI mode).

> 
> >> > EFI reboot is only tried if the user supplied it or if the hardware
> >> > is an ACPI 5.0 (or higher) reduced hardware board.
> >> > 
> >> > This fixes the EFI firmware crashing on Lenovo ThinkCentre M57
> >> 
> >> Buggy firmware should be worked around with "reboot="; I'd
> >> certainly accept a patch to bypass efi_reset_system() in that
> >> case.
> > 
> > Independent of the conversation above I will work the patch
> > that way.  Would you also be OK if I stuck the DMI data for the
> > ThinkCentre to make this automatic?
> 
> Yes, as long as the match isn't too generic (i.e. wouldn't match all
> furure models too).

I've sent you two patches for this. I have no clue what the future
models of this particular hardware have for product id - but it looks
quite specific to be for just this hardware.
> 
> Jan
> 

Re: [PATCH RFC] efi: By default use the BOOT_ACPI method instead of BOOT_EFI unless on reduced ACPI hardware.

[Jan Beulich]

>>> On 23.01.15 at 03:37, <konrad.wilk@oracle.com> wrote:
> On Thu, Jan 22, 2015 at 03:22:10PM +0000, Jan Beulich wrote:
>> >>> On 22.01.15 at 16:01, <konrad.wilk@oracle.com> wrote:
>> > On Thu, Jan 22, 2015 at 09:49:08AM +0000, Jan Beulich wrote:
>> >> >>> On 21.01.15 at 22:53, <konrad.wilk@oracle.com> wrote:
>> >> > This mimics the behavior of the Linux kernel in which the reboot
>> >> > sequence by default under EFI booted kernels is first ACPI.
>> >> 
>> >> Which is contrary to the EFI spec. I.e. NAK.
>> > 
>> > I am failing to see that in the spec. I see that it says what
>> > the ResetSystem() call does, but nothing about "MUST".
>> > 
>> > I see this at the start of the spec:
>> > 
>> > " Together, these provide a standard environment for booting an OS. This
>> > specification is designed as a pure interface specification. As such,
>> > the specification defines the set of interface s and structures that
>> > platform firmware must implement. "
>> > 
>> > (which talks about 'booting an OS' - which this is not, and interestingly
>> > enough - it does say implement, but not where it must implement it
>> > correctly!).
>> > 
>> > But I have not dug that deep in the spec to find something
>> > that says you MUST not use existing other specs? Perhaps you
>> > remember where the contrary part is?
>> 
>> The whole spirit of EFI is to get the OS away from doing things
>> some custom (and hence possibly fragile) way.
> 
> ACPI being 'custom'? I am not sure if there is a way on x86 to run
> without ACPI at all. I can see it under ARM where you have the
> Device Tree to complement it - but either way EFI is not an
> full solution to manage the system.

>From a EFI pov, EFI runtime services are to be preferred over
ACPI methods. While particularly relevant for cases where there's
no ACPI (which you validly say is not an practical option on x86),
it's a general conceptual thing to follow.

>> > Also, why do we want to be different that Windows and Linux when doing
>> > EFI operations?
>> 
>> Because just because they do things a certain way doesn't mean
>> they do it right. Linux having actively removed using the time
>> related runtime services functions is something that I for example
>> absolutely can't agree with. If there are firmware vendors not
>> getting their act together, having ways to work around that is
>> acceptable, but outright violation of the spec is wrong imo.
> 
> There is the spec and there is the implementation (Windows) that
> every motherboard manufacturer follows and caters to. It does not
> matter if the they (Microsoft) violate the spec - by them violating
> it or not using certain things - it makes it an de-facto specification.
> 
> Now I don't know if Linux ignoring the time runtime services has
> been due to bugs or just following what Windows did - but either way
> having it done that way - makes the firmware vendors that care
> about Linux - implement it to work under Linux (so expose the
> legacy timers even in EFI mode).

Argumentation along those lines is what I hear all time long. But how
do you envision firmware vendors to become aware of and fix their
bugs if everyone blindly works around them? By not adding
workarounds where it's not explicitly known they're needed, we at
least can point out to them that they have issues. Making things
more user friendly by making available (default-off) command line
overrides (or, as you suggest in this case, detecting the need via
DMI matching) is certainly desirable.

Jan

Re: [PATCH RFC] efi: By default use the BOOT_ACPI method instead of BOOT_EFI unless on reduced ACPI hardware.

[Konrad Rzeszutek Wilk]

On Fri, Jan 23, 2015 at 09:18:32AM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 03:37, <konrad.wilk@oracle.com> wrote:
> > On Thu, Jan 22, 2015 at 03:22:10PM +0000, Jan Beulich wrote:
> >> >>> On 22.01.15 at 16:01, <konrad.wilk@oracle.com> wrote:
> >> > On Thu, Jan 22, 2015 at 09:49:08AM +0000, Jan Beulich wrote:
> >> >> >>> On 21.01.15 at 22:53, <konrad.wilk@oracle.com> wrote:
> >> >> > This mimics the behavior of the Linux kernel in which the reboot
> >> >> > sequence by default under EFI booted kernels is first ACPI.
> >> >> 
> >> >> Which is contrary to the EFI spec. I.e. NAK.
> >> > 
> >> > I am failing to see that in the spec. I see that it says what
> >> > the ResetSystem() call does, but nothing about "MUST".
> >> > 
> >> > I see this at the start of the spec:
> >> > 
> >> > " Together, these provide a standard environment for booting an OS. This
> >> > specification is designed as a pure interface specification. As such,
> >> > the specification defines the set of interface s and structures that
> >> > platform firmware must implement. "
> >> > 
> >> > (which talks about 'booting an OS' - which this is not, and interestingly
> >> > enough - it does say implement, but not where it must implement it
> >> > correctly!).
> >> > 
> >> > But I have not dug that deep in the spec to find something
> >> > that says you MUST not use existing other specs? Perhaps you
> >> > remember where the contrary part is?
> >> 
> >> The whole spirit of EFI is to get the OS away from doing things
> >> some custom (and hence possibly fragile) way.
> > 
> > ACPI being 'custom'? I am not sure if there is a way on x86 to run
> > without ACPI at all. I can see it under ARM where you have the
> > Device Tree to complement it - but either way EFI is not an
> > full solution to manage the system.
> 
> >From a EFI pov, EFI runtime services are to be preferred over
> ACPI methods. While particularly relevant for cases where there's
> no ACPI (which you validly say is not an practical option on x86),
> it's a general conceptual thing to follow.
> 
> >> > Also, why do we want to be different that Windows and Linux when doing
> >> > EFI operations?
> >> 
> >> Because just because they do things a certain way doesn't mean
> >> they do it right. Linux having actively removed using the time
> >> related runtime services functions is something that I for example
> >> absolutely can't agree with. If there are firmware vendors not
> >> getting their act together, having ways to work around that is
> >> acceptable, but outright violation of the spec is wrong imo.
> > 
> > There is the spec and there is the implementation (Windows) that
> > every motherboard manufacturer follows and caters to. It does not
> > matter if the they (Microsoft) violate the spec - by them violating
> > it or not using certain things - it makes it an de-facto specification.
> > 
> > Now I don't know if Linux ignoring the time runtime services has
> > been due to bugs or just following what Windows did - but either way
> > having it done that way - makes the firmware vendors that care
> > about Linux - implement it to work under Linux (so expose the
> > legacy timers even in EFI mode).
> 
> Argumentation along those lines is what I hear all time long. But how
> do you envision firmware vendors to become aware of and fix their
> bugs if everyone blindly works around them? By not adding

That is a very good point. However my experience with hardware vendors
is that they have limited amount of resources and have hardly any
resources to even fix issues that occur when running with Linux.

Big vendors like IBM, Oracle, etc certainly have an incentive, but 
other less so.

> workarounds where it's not explicitly known they're needed, we at
> least can point out to them that they have issues. Making things
> more user friendly by making available (default-off) command line
> overrides (or, as you suggest in this case, detecting the need via
> DMI matching) is certainly desirable.

I would love to tell Lenovo engineers about their bugs on this
platform (EFI reboot not implemented, EHCI controller doing some wonky
reads in EFI_RSV - Elena seeing that) - but I have not had any luck
getting a contact person.

If you have a contact email I shall file them detailed logs of what
I see on this little workstation I have.
> 
> Jan
>