[U-Boot] [PATCH 0/2] efi_loader: Implement reset on RPi

Fri Aug 12 23:20:41 CEST 2016

> On 12 Aug 2016, at 22:07, Simon Glass <sjg at chromium.org> wrote:
> 
> Hi Alex,
> 
> On 12 August 2016 at 12:50, Alexander Graf <agraf at suse.de> wrote:
>> 
>> 
>> On 12.08.16 19:21, Simon Glass wrote:
>>> Hi Alex,
>>> 
>>> On 11 August 2016 at 05:49, Alexander Graf <agraf at suse.de> wrote:
>>>> 
>>>> 
>>>> On 08.08.16 23:44, Simon Glass wrote:
>>>>> Hi,
>>>>> 
>>>>> On 7 August 2016 at 10:59, Andreas Färber <afaerber at suse.de> wrote:
>>>>>> Am 14.07.2016 um 08:18 schrieb Alexander Graf:
>>>>>>>> Am 14.07.2016 um 06:48 schrieb Andreas Färber <afaerber at suse.de>:
>>>>>>>> 
>>>>>>>> Hi Alex,
>>>>>>>> 
>>>>>>>>> Am 05.06.2016 um 23:17 schrieb Alexander Graf:
>>>>>>>>> If Linux finds an EFI implementation it always uses the EFI reset handler to
>>>>>>>>> reboot or power down the system.
>>>>>>>> 
>>>>>>>> Hm, I thought my powerdown issues on the Jetson TK1 were for lack of
>>>>>>>> CONFIG_AS3277_RESET - sounds like it could be due to EFI instead?
>>>>>>> 
>>>>>>> It depends. IIRC the TK1 is 32bit, where you're probably using grub2 which is not efi Linux aware, but instead boots over the zImage protocol. In that case Linux doesn't know about efi runtime services.
>>>>>> 
>>>>>> We've confirmed in the meantime that the Jetson TK1 issues were
>>>>>> unrelated to EFI and could be worked around by enabling some as3722
>>>>>> kernel option.
>>>>>> 
>>>>>>>>> Unfortunately we haven't implemented that one yet. In fact, while we prepared
>>>>>>>>> for RTS handling, we never actually implemented a single user.
>>>>>>>>> 
>>>>>>>>> This is going to change today. This simple patch set enables RTS reset support
>>>>>>>>> for the RPi systems, allowing you to reboot and shut down the rpi if booted
>>>>>>>>> via EFI.
>>>>>>>>> 
>>>>>>>>> It also lays the groundwork to show how to implement that functionality at all,
>>>>>>>>> so I expect more boards to follow.
>>>>>>>>> 
>>>>>>>>> Alexander Graf (2):
>>>>>>>>> efi_loader: Allow boards to implement get_time and reset_system
>>>>>>>>> ARM: bcm283x: Implement EFI RTS reset_system
>>>>>>>>> 
>>>>>>>>> arch/arm/mach-bcm283x/include/mach/wdog.h |   2 +-
>>>>>>>>> arch/arm/mach-bcm283x/reset.c             |  59 +++++++++++++++--
>>>>>>>>> cmd/bootefi.c                             |   4 ++
>>>>>>>>> include/efi_loader.h                      |  18 ++++++
>>>>>>>>> lib/efi_loader/efi_runtime.c              | 101 ++++++++++++++++++++++++++----
>>>>>>>>> 5 files changed, 166 insertions(+), 18 deletions(-)
>>>>>>>> 
>>>>>>>> This all looks very non-generic and would mean that every board will
>>>>>>>> need to be patched individually, which is unrealistic to be tested by
>>>>>>>> just the two of us.
>>>>>>>> 
>>>>>>>> Can't you patch the reset_cpu() declaration (common.h/sysreset.h)
>>>>>>>> instead of all its implementations? We might still need to patch
>>>>>>>> individual implementations but I don't see why any reset_cpu()
>>>>>>>> implementation should be in a different section than others.
>>>>>>> 
>>>>>>> Hmm. There are 2 minor problems:
>>>>>>> 
>>>>>>>  1) Efi also supports power off on top of reset
>>>>>>>  2) We would have to convert all boards at once, rather than one by one, as we couldn't distinguish between efi aware and unaware ones
>>>>>> 
>>>>>> I don't see why we would need to convert everything at once either way.
>>>>>> 
>>>>>>> 
>>>>>>> And one major issue:
>>>>>>> 
>>>>>>> All device memory pointers used by the reset functions need to be marked as such in the efi memory map and live relocated when entering runtime mode. So we need to manually touch every function either way.
>>>>> 
>>>>> I'm worried about this. It means that any code used from this run-time
>>>>> needs to be so marked. This could be large tracts of U-Boot. In
>>>> 
>>>> We only need to mark the few bits that are actually executed and used
>>>> within RTS. That's usually just 2 or 3 functions.
>>>> 
>>> 
>>> At present you only have reset, and you've only implemented it for one
>>> board. Are there other calls that we need to implement? This
>>> EFI_RUNTIME is transitive - anything it calls must be in the runtime.
>>> Does the linker prevent us from screwing up?
>> 
>> It doesn't (I couldn't find a way), but the runtime relocation code does:
>> 
>> 
>> https://github.com/trini/u-boot/blob/master/lib/efi_loader/efi_runtime.c#L205
>> 
>>> 
>>>> Also, moving forward, we'll see more and more systems implement PSCI
>>>> which means we can implement a generic PSCI RTS for reset/shutdown.
>>> 
>>> What system can I get that uses that today?
>> 
>> Anything that runs ATF:
>> 
>>  https://github.com/ARM-software/arm-trusted-firmware
>> 
>> in EL3:
>> 
>>  - Pine64
>>  - ZynqMP
>>  - S905X
>>  - Hikey
>>  - probably many many more
>> 
>> I think the only AArch64 systems that I'm aware of that do *not* run ATF
>> in EL3 are
>> 
>>  - Anything FSL LS
>>  - RPi3
>>  - uniphier
>> 
>> and in all 3 cases it's a bad design decision IMHO, but it's one that
>> their respective creators did.
> 
> So there is an interface in ATF to shut the machine down? Is that
> because of security / permission? I really have not looked into ATF
> unfortunately.

The main interface that we care about in ATF is PSCI:

  http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf

which implements interesting functions like

  SYSTEM_OFF and
  SYSTEM_RESET

So yes, it basically implements everything we need. IIUC the NXP folks want to implement their own PSCI backend in U-Boot, so we could use the same infrastructure. I hope that unipher moves to something that implements PSCI as well. That would only leave the RPi3 as non-PSCI aarch64 platform in mainline U-Boot.

The only case where not implementing PSCI is a reasonable compromise is that APM X-Gene, which does not implement EL3. But given that I have neither seen any efforts from any side to push their U-Boot code upstream nor any need for the U-Boot EFI support, as they have a working edk2 port, I doubt anyone really cares :).

> 
>> 
>>> 
>>>> 
>>>>> particular, as I have mentioned a few times, I think the UEFI tables
>>>>> should be 'live' and not just created before booting, which means that
>>>>> much of driver/core needs to be in the UEFI section.
>>>> 
>>>> I don't fully understand what you're aiming for here. The tables are
>>>> always static in a uEFI world. I don't see how they could get more
>>>> "live" than creating them right before boot.
>>> 
>>> I'd prefer to see the EFI requests be processed as they are received,
>>> rather than with pre-canned data. Your original justification (e.g.
>>> for efi_disk_add_dev()) was that there was not authoritative list of
>>> block devices in U-Boot. But there is now.
>>> 
>>> So do you think it would be feasible to drop these tables, and efi_obj_list?
>> 
>> Ah, we're talking about different tables. I was thinking "tables" as in
>> what EFI calls tables. That would be the device tree or SMBIOS tables.
>> Those are really just pointers to memory.
>> 
>>> What happens with the tables if I run an app and then come back to U-Boot?
>> 
>> So let me try to grasp what you mean by tables. I guess you mean the
>> list of devices? It gets regenerated on every bootefi invocation. Or did
>> you mean something different?
> 
> Yes that's right - I mean the list of devices, and all the stuff in there.
> 
> If I had a function like:
> 
> struct efi_object *dev_get_efi(struct udevice *dev)
> 
> would that be enough to handle the conversion? Basically I'm wondering
> whether we can scan through the DM tables and obtain EFI objects as
> needed (on the fly), or whether this is prohibitively expensive. DM
> doesn't like competition!

I’m not 100% sure how we should model it yet. One of the nice things of the separate object tree is that we have no (memory & cpu) overhead for people who don’t call bootefi. I doubt it really makes a big difference though.

I don’t have a good grasp on how well the two object models overlap. If we want to get rid of efi_obj_list we’d need to put at least the uuid and class type maps into the actual udevice objects I guess. So I suppose what you’re thinking of is something along the lines of

  struct udevice {
    […];
    struct efi_object;
  }

which would basically give us exactly that? We’d then still have to actually initialize the objects with data somehow. I’m not sure how that would happen. Maybe call into something like efi_disk_add_dev() from the generic block init code?

Alex