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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan