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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Sep 19, 2023 at 11:31:07AM +0200, Jan Beulich wrote:
> On 18.09.2023 18:00, Roger Pau Monné wrote:
> > On Mon, Sep 18, 2023 at 05:44:47PM +0200, Jan Beulich wrote:
> >> On 18.09.2023 17:09, Roger Pau Monné wrote:
> >>> On Mon, Sep 18, 2023 at 02:26:51PM +0200, Jan Beulich wrote:
> >>>> On 15.09.2023 09:43, 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.
> >>>>
> >>>> I certainly appreciate this as a goal. However, ...
> >>>>
> >>>>> Add a special case for one Acer model that does require being rebooted using
> >>>>> ResetSystem().  See Linux commit 0082517fa4bce for rationale.
> >>>>
> >>>> ... this is precisely what I'd like to avoid: Needing workarounds on spec-
> >>>> conforming systems.
> >>>
> >>> I wouldn't call that platform spec-conforming when ACPI reboot doesn't
> >>> work reliably on it either.  I haven't been able to find a wording on
> >>> the UEFI specification that mandates using ResetSystem() in order to
> >>> reset the platform.  I've only found this wording:
> >>>
> >>> "... then the UEFI OS Loader has taken control of the platform, and
> >>> EFI will not regain control of the system until the platform is reset.
> >>> One method of resetting the platform is through the EFI Runtime
> >>> Service ResetSystem()."
> >>>
> >>> And this reads to me as a mere indication that one option is to use
> >>> ResetSystem(), but that there are likely other platform specific reset
> >>> methods that are suitable to be used for OSes and still be compliant
> >>> with the UEFI spec.
> >>
> >> See my reference to ia64.
> > 
> > Right, I understand that on ia64 things might have been different, due
> > to the platform lacking any other reboot method, but I don't see how
> > this applies to x86 where there are other reboot methods.
> > 
> >> With ACPI_FADT_RESET_REGISTER not set, I don't
> >> think there would have been any other non-custom reboot method there. So
> >> while perhaps not mandated, it's still the designated abstraction layer.
> > 
> > Again the spec doesn't mention that ResetSystem() must be used, so
> > while it would make sense if it was reliable, it clearly isn't.  In
> > which case resorting to the more reliable method should always be
> > preferred, specially if the spec is so lax as to call ResetSystem()
> > "One method of resetting the platform".
> 
> That wording wasn't there in 1.02, but I can see it all the way back to
> at least 2.1. So yes, you have a point. Yet - adding onto an earlier
> remark of mine - EFI_RESET_NOTIFICATION_PROTOCOL is pretty useless if
> use of ResetSystem() was optional.

See the note in
EFI_RESET_NOTIFICATION_PROTOCOL.RegisterResetNotify():

"The list of registered reset notification functions are processed if
ResetSystem() is called before ExitBootServices(). The list of
registered reset notification functions is ignored if ResetSystem() is
called after ExitBootServices()."

Those handlers are only called before ExitBootServices(), so for our
use-case it doesn't make a difference, as we call ResetSystem() after
having exited boot services.

Thanks, Roger.