Re: [PATCH v2] arm64: move efi_reboot to restart handler

Re: [PATCH v2] arm64: move efi_reboot to restart handler

[Alexander Sverdlin]

Hello Ard, Mark,

On 02/02/2022 15:01, Ard Biesheuvel wrote:
>>>> In other words, I would like to be able to run a restart handler on EFI
>>>> based ARM64 systems, just like I can on other systems, just for its
>>>> "side effects", not to do the actual reboot. Current code disables this
>>>> possibility on an ARM64 EFI system.
>>>
>>> It sounds like two things are being conflated here:
>>>
>>> 1) A *notification* that a restart will subsequently occur.
>>> 2) A *request* to initiate a restart.
>>>
>>> IIUC (1) is supposed to be handled by the existing reboot notifier mechanism
>>> (see the reboot_notifier_list) which *is* invoked prior to the EFI reboot
>>> today.
>>>
>>> IMO, using restart handlers as notifiers is an abuse of the interface, and
>>> that's the fundamental problem.
>>>
>>> What am I missing?
>>
>> You are completly right. It is possible that I would like to be able to
>> *abuse* the restart handlers as notifier. You are right that we have a
>> reboot_notifier but it is not good enough for my usecase - it is only
>> called, well, on reboot. It is not called in case of emergency_restart()
>> so in case of a panic, this won't happen. It also is called much earlier
>> than restart handlers which also makes a difference in some cases. So I
>> see no other choice than to abuse the restart_handler mechanism for that.
>>
> 
> Why would such a platform implement ResetSystem() in the first place
> if it cannot be used?
> 
> So the right solution here is for the firmware to publish a
> EFI_RT_PROPERTIES_TABLE that describes ResetSystem() as unsupported,
> and Linux will happily disregard it and try something else.

Let me outline the use case once again:

we have cases where different SoCs are integrated into more complicated
FPGA based systems where FPGA controls the reset sequence of the whole
system. Those SoCs are of different ARCHs and only one of them currently
has EFI.

So the implementation is largely platform-independent and is implemented
in Linux.

Do you suggest that in EFI case we cannot implement a conditional
reset implementation (native EFI reset vs system-wide FPGA-controlled)
in Linux any longer (because register_restart_handler() doesn't work
for EFI based ARM64 system) and need to reimplement very special
solution in EFI? 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2] arm64: move efi_reboot to restart handler

[Alexander Sverdlin]

Hello Mark, Ard,

On 01/02/2022 14:58, Mark Rutland wrote:
>> You could argue that restart handlers were not created for that but they
>> suit this purpose ideally and it wouldn't make much sense (in my
>> opinion) to add yet another notifier chain that would run before reset
>> notifiers, for code that is not supposed to reset the whole system and
>> this is exacly what I would have to do if efi_reboot() is forced to be
>> called before all handlers.
> 
> As above, I think that's just using the wrong interface, and the reboot
> notifier mechanism *already* exists, so I'm really confused here.
> 
> Have I misunderstood what you're trying to achieve?
> 
> Is there some problem with the reboot notifier mechanism that I am unaware of?
> e.g. do we bypass them in some case where you think they're needed?
> 
> Are you simply unaware of reboot notifiers?

Could you please check the simple case of pwrseq_emmc.c?

While that's currently the only example of this kind upstream I can imagine
further use-cases, especially in storage area like above.

Would you suggest it's illegal usage of register_restart_handler()?
Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain
which will be called before restart handlers, so that it works on
emergency_restart()?

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2] arm64: move efi_reboot to restart handler

[Guenter Roeck]

On 2/15/22 00:44, Alexander Sverdlin wrote:
> Hello Mark, Ard,
> 
> On 01/02/2022 14:58, Mark Rutland wrote:
>>> You could argue that restart handlers were not created for that but they
>>> suit this purpose ideally and it wouldn't make much sense (in my
>>> opinion) to add yet another notifier chain that would run before reset
>>> notifiers, for code that is not supposed to reset the whole system and
>>> this is exacly what I would have to do if efi_reboot() is forced to be
>>> called before all handlers.
>>
>> As above, I think that's just using the wrong interface, and the reboot
>> notifier mechanism *already* exists, so I'm really confused here.
>>
>> Have I misunderstood what you're trying to achieve?
>>
>> Is there some problem with the reboot notifier mechanism that I am unaware of?
>> e.g. do we bypass them in some case where you think they're needed?
>>
>> Are you simply unaware of reboot notifiers?
> 
> Could you please check the simple case of pwrseq_emmc.c?
> 
> While that's currently the only example of this kind upstream I can imagine
> further use-cases, especially in storage area like above.
> 
> Would you suggest it's illegal usage of register_restart_handler()?
> Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain
> which will be called before restart handlers, so that it works on
> emergency_restart()?
> 

Abuse isn't just about using an API for something it isn't originally intended
for, abuse is also to intentionally _not_ use an API, as it is currently done
by the EFI restart code for arm64. Also, keep in mind that the same argument
(our restart handler _must_ be executed under all circumstances and does therefore
not use the restart API) is likely going to be used again in the future. All
it takes is for some other standard (or chip vendor, for that matter) to declare
their restart handler mandatory if present. That means there will be other
non-users of the restart API in the future, and you simply can not rely on it
being used. That was clearly not the intention of the restart API - quite the
opposite - but it is what it is.

Given that, I'd suggest to cut your losses and try to find another solution
for your problem. That may be to introduce yet another API, one that is called
before the EFI restart handling but that is always called, unlike reboot
notifiers, or simply stick with out-of-tree code.

Guenter

Re: [PATCH v2] arm64: move efi_reboot to restart handler

[Krzysztof Adamski]

Dnia Tue, Feb 15, 2022 at 06:30:26AM -0800, Guenter Roeck napisał(a):
>On 2/15/22 00:44, Alexander Sverdlin wrote:
>>Hello Mark, Ard,
>>
>>On 01/02/2022 14:58, Mark Rutland wrote:
>>>>You could argue that restart handlers were not created for that but they
>>>>suit this purpose ideally and it wouldn't make much sense (in my
>>>>opinion) to add yet another notifier chain that would run before reset
>>>>notifiers, for code that is not supposed to reset the whole system and
>>>>this is exacly what I would have to do if efi_reboot() is forced to be
>>>>called before all handlers.
>>>
>>>As above, I think that's just using the wrong interface, and the reboot
>>>notifier mechanism *already* exists, so I'm really confused here.
>>>
>>>Have I misunderstood what you're trying to achieve?
>>>
>>>Is there some problem with the reboot notifier mechanism that I am unaware of?
>>>e.g. do we bypass them in some case where you think they're needed?
>>>
>>>Are you simply unaware of reboot notifiers?
>>
>>Could you please check the simple case of pwrseq_emmc.c?
>>
>>While that's currently the only example of this kind upstream I can imagine
>>further use-cases, especially in storage area like above.
>>
>>Would you suggest it's illegal usage of register_restart_handler()?
>>Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain
>>which will be called before restart handlers, so that it works on
>>emergency_restart()?
>>
>
>Abuse isn't just about using an API for something it isn't originally intended
>for, abuse is also to intentionally _not_ use an API, as it is currently done
>by the EFI restart code for arm64. Also, keep in mind that the same argument
>(our restart handler _must_ be executed under all circumstances and does therefore
>not use the restart API) is likely going to be used again in the future. All
>it takes is for some other standard (or chip vendor, for that matter) to declare
>their restart handler mandatory if present.

Wait, but it is up to us to decide if we want to follow such standard or
not. If we want to have code that is more flexible, nobody can refuse us
to do so, right? None of the standards says that we can't support
restart handlers in case of EFI on ARM64, it was decided by kernel
developers, not some vendors. We can change that.

>Given that, I'd suggest to cut your losses and try to find another
>solution for your problem. That may be to introduce yet another API,
>one that is called before the EFI restart handling but that is always
>called, unlike reboot notifiers, or simply stick with out-of-tree code.

Sure I could create yet another API like you suggest but in practice it
would be a copy of existing API as i would have to work exactly the
same - would be called at the same time in the same situations. The only
thing that would be different would be separate chain.
But if we want to prevent registering some custom code to be run before
efi_reboot(), that new API would have to be rejected as well, for the
same reason. So what is the point?

Krzysztof

Re: [PATCH v2] arm64: move efi_reboot to restart handler

[Guenter Roeck]

On 2/15/22 07:01, Krzysztof Adamski wrote:
> Dnia Tue, Feb 15, 2022 at 06:30:26AM -0800, Guenter Roeck napisał(a):
>> On 2/15/22 00:44, Alexander Sverdlin wrote:
>>> Hello Mark, Ard,
>>>
>>> On 01/02/2022 14:58, Mark Rutland wrote:
>>>>> You could argue that restart handlers were not created for that but they
>>>>> suit this purpose ideally and it wouldn't make much sense (in my
>>>>> opinion) to add yet another notifier chain that would run before reset
>>>>> notifiers, for code that is not supposed to reset the whole system and
>>>>> this is exacly what I would have to do if efi_reboot() is forced to be
>>>>> called before all handlers.
>>>>
>>>> As above, I think that's just using the wrong interface, and the reboot
>>>> notifier mechanism *already* exists, so I'm really confused here.
>>>>
>>>> Have I misunderstood what you're trying to achieve?
>>>>
>>>> Is there some problem with the reboot notifier mechanism that I am unaware of?
>>>> e.g. do we bypass them in some case where you think they're needed?
>>>>
>>>> Are you simply unaware of reboot notifiers?
>>>
>>> Could you please check the simple case of pwrseq_emmc.c?
>>>
>>> While that's currently the only example of this kind upstream I can imagine
>>> further use-cases, especially in storage area like above.
>>>
>>> Would you suggest it's illegal usage of register_restart_handler()?
>>> Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain
>>> which will be called before restart handlers, so that it works on
>>> emergency_restart()?
>>>
>>
>> Abuse isn't just about using an API for something it isn't originally intended
>> for, abuse is also to intentionally _not_ use an API, as it is currently done
>> by the EFI restart code for arm64. Also, keep in mind that the same argument
>> (our restart handler _must_ be executed under all circumstances and does therefore
>> not use the restart API) is likely going to be used again in the future. All
>> it takes is for some other standard (or chip vendor, for that matter) to declare
>> their restart handler mandatory if present.
> 
> Wait, but it is up to us to decide if we want to follow such standard or
> not. If we want to have code that is more flexible, nobody can refuse us
> to do so, right? None of the standards says that we can't support
> restart handlers in case of EFI on ARM64, it was decided by kernel
> developers, not some vendors. We can change that.
> 

Of course it was decided by kernel developers. Point is that they use
the EFI standard as argument for bypassing the API. What I am saying is
that others can (and likely will, since the flood doors have been opened)
do the same in the future, using the same line of arguments.

>> Given that, I'd suggest to cut your losses and try to find another
>> solution for your problem. That may be to introduce yet another API,
>> one that is called before the EFI restart handling but that is always
>> called, unlike reboot notifiers, or simply stick with out-of-tree code.
> 
> Sure I could create yet another API like you suggest but in practice it
> would be a copy of existing API as i would have to work exactly the
> same - would be called at the same time in the same situations. The only
> thing that would be different would be separate chain.

Correct.

> But if we want to prevent registering some custom code to be run before > efi_reboot(), that new API would have to be rejected as well, for the
> same reason. So what is the point?
> 

Ah, yes, you are right. The emmc example does reset the emmc, after all,
which one could use as argument that it "violates" the EFI mandate.
Sorry, I guess you'll be stuck with out-of-tree code (and, realistically,
so is everyone using emmc in an arm64 based system with an EFI restart
handler which does not implement emmc reset). Actually, turns out that
the emmc restart handling code is not reliable anyway, since for example
x86 doesn't use/support the restart handler call chain, and neither
do several other architectures.

Interestingly, in this context, x86 isn't as inflexible as arm64 and does
support other means to reset the system even in the presence of EFI
(and actually seems to prefer ACPI reset over EFI reset unless I
misunderstand the code).

Other options for you might be to disable EFI restart handling in your
system (assuming that is possible), or to implement the necessary code
as part of the EFI restart handler, ie outside Linux, again if that is
possible.

Guenter

Re: [PATCH v2] arm64: move efi_reboot to restart handler

[Ard Biesheuvel]

On Tue, 15 Feb 2022 at 17:57, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 2/15/22 07:01, Krzysztof Adamski wrote:
> > Dnia Tue, Feb 15, 2022 at 06:30:26AM -0800, Guenter Roeck napisał(a):
> >> On 2/15/22 00:44, Alexander Sverdlin wrote:
> >>> Hello Mark, Ard,
> >>>
> >>> On 01/02/2022 14:58, Mark Rutland wrote:
> >>>>> You could argue that restart handlers were not created for that but they
> >>>>> suit this purpose ideally and it wouldn't make much sense (in my
> >>>>> opinion) to add yet another notifier chain that would run before reset
> >>>>> notifiers, for code that is not supposed to reset the whole system and
> >>>>> this is exacly what I would have to do if efi_reboot() is forced to be
> >>>>> called before all handlers.
> >>>>
> >>>> As above, I think that's just using the wrong interface, and the reboot
> >>>> notifier mechanism *already* exists, so I'm really confused here.
> >>>>
> >>>> Have I misunderstood what you're trying to achieve?
> >>>>
> >>>> Is there some problem with the reboot notifier mechanism that I am unaware of?
> >>>> e.g. do we bypass them in some case where you think they're needed?
> >>>>
> >>>> Are you simply unaware of reboot notifiers?
> >>>
> >>> Could you please check the simple case of pwrseq_emmc.c?
> >>>
> >>> While that's currently the only example of this kind upstream I can imagine
> >>> further use-cases, especially in storage area like above.
> >>>
> >>> Would you suggest it's illegal usage of register_restart_handler()?
> >>> Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain
> >>> which will be called before restart handlers, so that it works on
> >>> emergency_restart()?
> >>>
> >>
> >> Abuse isn't just about using an API for something it isn't originally intended
> >> for, abuse is also to intentionally _not_ use an API, as it is currently done
> >> by the EFI restart code for arm64. Also, keep in mind that the same argument
> >> (our restart handler _must_ be executed under all circumstances and does therefore
> >> not use the restart API) is likely going to be used again in the future. All
> >> it takes is for some other standard (or chip vendor, for that matter) to declare
> >> their restart handler mandatory if present.
> >
> > Wait, but it is up to us to decide if we want to follow such standard or
> > not. If we want to have code that is more flexible, nobody can refuse us
> > to do so, right? None of the standards says that we can't support
> > restart handlers in case of EFI on ARM64, it was decided by kernel
> > developers, not some vendors. We can change that.
> >
>
> Of course it was decided by kernel developers. Point is that they use
> the EFI standard as argument for bypassing the API. What I am saying is
> that others can (and likely will, since the flood doors have been opened)
> do the same in the future, using the same line of arguments.
>

I don't think anyone was doing what you describe here. My primary
point was that platforms should not implement and expose EFI reset if
that implementation fails to deal with the platform's peripherals
adequately.

> >> Given that, I'd suggest to cut your losses and try to find another
> >> solution for your problem. That may be to introduce yet another API,
> >> one that is called before the EFI restart handling but that is always
> >> called, unlike reboot notifiers, or simply stick with out-of-tree code.
> >
> > Sure I could create yet another API like you suggest but in practice it
> > would be a copy of existing API as i would have to work exactly the
> > same - would be called at the same time in the same situations. The only
> > thing that would be different would be separate chain.
>
> Correct.
>
> > But if we want to prevent registering some custom code to be run before > efi_reboot(), that new API would have to be rejected as well, for the
> > same reason. So what is the point?
> >
>
> Ah, yes, you are right. The emmc example does reset the emmc, after all,
> which one could use as argument that it "violates" the EFI mandate.
> Sorry, I guess you'll be stuck with out-of-tree code (and, realistically,
> so is everyone using emmc in an arm64 based system with an EFI restart
> handler which does not implement emmc reset). Actually, turns out that
> the emmc restart handling code is not reliable anyway, since for example
> x86 doesn't use/support the restart handler call chain, and neither
> do several other architectures.
>

Rich firmware like EFI and ACPI implies that the firmware knows how to
manage the hardware.

> Interestingly, in this context, x86 isn't as inflexible as arm64 and does
> support other means to reset the system even in the presence of EFI
> (and actually seems to prefer ACPI reset over EFI reset unless I
> misunderstand the code).
>

No, correct. This is primarily for historical reasons and for parity
with Windows/x86 which does the same (which means that on many cheap
Wintel laptops, ACPI reset is the only thing that works)

> Other options for you might be to disable EFI restart handling in your
> system (assuming that is possible), or to implement the necessary code
> as part of the EFI restart handler, ie outside Linux, again if that is
> possible.
>

Either implement EFI reset properly, or not at all. Adding code to the
OS that forces it to reason about whether or not EFI reset can be
called safely simply means that the EFI implementation is broken and
should probably be avoided entirely.

Re: [PATCH v2] arm64: move efi_reboot to restart handler

[Krzysztof Adamski]

Dnia Tue, Feb 15, 2022 at 06:03:30PM +0100, Ard Biesheuvel napisał(a):
>> > But if we want to prevent registering some custom code to be run before > efi_reboot(), that new API would have to be rejected as well, for the
>> > same reason. So what is the point?
>> >
>>
>> Ah, yes, you are right. The emmc example does reset the emmc, after all,
>> which one could use as argument that it "violates" the EFI mandate.
>> Sorry, I guess you'll be stuck with out-of-tree code (and, realistically,
>> so is everyone using emmc in an arm64 based system with an EFI restart
>> handler which does not implement emmc reset). Actually, turns out that
>> the emmc restart handling code is not reliable anyway, since for example
>> x86 doesn't use/support the restart handler call chain, and neither
>> do several other architectures.
>>
>
>Rich firmware like EFI and ACPI implies that the firmware knows how to
>manage the hardware.
>

In an ideal world, probably yes. When I move there, I will probably have
less concerns :)

>
>> Other options for you might be to disable EFI restart handling in your
>> system (assuming that is possible), or to implement the necessary code
>> as part of the EFI restart handler, ie outside Linux, again if that is
>> possible.
>>
>
>Either implement EFI reset properly, or not at all. Adding code to the
>OS that forces it to reason about whether or not EFI reset can be
>called safely simply means that the EFI implementation is broken and
>should probably be avoided entirely.

That is black-and-white thinking. There are shades of gray, too :)

Imagine this sitation - you have a SoC and the vendor delivers this SoC
together with support and code for that SoC. You use it to build some
platform around that SoC but that platform is much more complicated than
the basic one. It is custom and specific to your needs. The vendor
provided firmware, together with the EFI implementation can handle
reseting the SoC without problems but you also have some additional
parts in your platform you would like to handle. Why, in such a case, I
should not use the existing EFI implementation and just build on top of
that? Avoiding using it entirely does not seem reasonable to me. It
might also not be possible as you might not know all the details to
reimplement it.

On PC/Server world, that approach might be feasible - the vendor knows
the usecase for their product well enough to support all the cases. But
ARM64 is being often used for other, so called "embedded", usecases.
People working in "embedded" often have unique needs and I do understand
that mainline kernel cannot support all of them. But when supporting
them is easy and does not require some ugly hacks to be added, why not?

For years we were implementing ways to overcome limitations in
hardware and vendor software implementations. restart_handler mechanism
is quite pretty example of that, one which isn't really hacky, does not
introduce maintainabity problems if widely used. Why not support it on
ARM64 as well? Only because in ideal world it wouldn't be needed? :)

I understand it should not be needed in EFI world and there are
approaches that could be used instead of that one, but they all seem
like taking a sledgehammer to crack a nut.

Krzysztof