[U-Boot] [PATCH 1/5] efi_loader: Allow boards to implement get_time and reset_system

[Simon Glass]

Hi Alex,

On 18 August 2016 at 15:02, Alexander Graf <agraf at suse.de> wrote:
>
>> On 18 Aug 2016, at 08:43, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Alex,
>>
>> On 17 August 2016 at 22:49, Alexander Graf <agraf at suse.de> wrote:
>>>
>>>
>>>> Am 18.08.2016 um 05:44 schrieb Simon Glass <sjg at chromium.org>:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 16 August 2016 at 13:08, Alexander Graf <agraf at suse.de> wrote:
>>>>> EFI allows an OS to leverage firmware drivers while the OS is running. In the
>>>>> generic code we so far had to stub those implementations out, because we would
>>>>> need board specific knowledge about MMIO setups for it.
>>>>>
>>>>> However, boards can easily implement those themselves. This patch provides the
>>>>> framework so that a board can implement its own versions of get_time and
>>>>> reset_system which would actually do something useful.
>>>>>
>>>>> While at it we also introduce a simple way for code to reserve MMIO pointers
>>>>> as runtime available.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf at suse.de>
>>>>> ---
>>>>> cmd/bootefi.c                |   4 ++
>>>>> include/efi_loader.h         |  18 ++++++++
>>>>> lib/efi_loader/efi_runtime.c | 101 ++++++++++++++++++++++++++++++++++++++-----
>>>>> 3 files changed, 112 insertions(+), 11 deletions(-)
>>>>
>>>> Instead of weak functions, can you use the existing driver model
>>>> sysreset uclass?
>>>
>>> Unfortunately not, at least not generically.
>>>
>>> The EFI RTS code gets dynamically relocated by the executing OS (like Linux) and expects any pointers to dynamically move to where the OS remaps things to. So memory at 0x1234 during boot time might become 0x75995234 in RTS.
>>>
>>> While we can automatically catch all linker relocations, dynamically created objects or pointers that rely on the 1:1 map will fail to work once we're in Linux because of the above.
>>>
>>> So the "obvious" path forward would be for systems that implement the sysreset uclass to provide an efi rts reset function which calls into their sysreset device. In a patch that enables this, whoever does it would also have to go through all the the dm code, annotate it as efi runtime and wrap all pointers in the dynamic efi relocation call. The same goes for pointers to device memory that needs to get accessed, which then also needs to get annotated as efi mmio regions in the efi memory map.
>>>
>>> This is insanely tedious and very hard to get right. The less complexity we have in RTS code, the better we are off.
>>
>> Yes I thought this would be the issue. I've done something similar
>> before where part of U-Boot was linked to be in one image and another
>> part was loaded later only if needed. It was pretty brittle.
>>
>> I agree that annotating functions that are used is tedious. In fact as
>> mentioned I'm really not keen on that in any case. It would be great
>> to drop the runtime annotation altogether.
>>
>> Some options:
>> - Include all of U-Boot in the run-time portion (at least for now)
>> - Include drivers/{core,sysreset,sysreset} (which should be enough)
>> - link the image once with just the EFI loader binary and its
>> dependencies (using --gc-sections) and then used the list of included
>> object files as your list to put in the EFI runtime region
>>
>> I suspect the middle one might be quite easy?
>
> All of the above would be reasonably easy to do with linker scripts. The hard part are pointers that are not tracked by relocations, so we can’t easily relocate them when going into Linux land. And without that solved, I don’t think it’s worth thinking about over engineering run time support. I’d much prefer to put as much as we can in very trivial helpers, generically with PSCI where possible.

Yes that's a whole other problem that I hadn't considered. It means
that the currently implementation is a barrier to adoption of driver
model. This really need some thought. How do you deal with pointers at
present? It may need do be a new execution phase, where we set up a
new global_data and init driver model. Pretty simple I suppose (we do
this in spl_init() at present), but I'm sure there would be fishhooks.

If we continue down the path you have created, it is only going to get
harder to fix this up. If I had known we would end up here I would
have been much less comfortable with the implementation.

Let's figure this out now. I'm happy to help with refactoring or
whatever is needed.

>
>> My concern with what you have is that you are working around driver
>> model, creating a second ad-hoc driver approach. I can certainly see
>> your reasons (short-term expediency mostly :-), but it might not be
>> that hard to just bring in the DM code that is needed. Remember that
>> each board should keep its driver in the correct drivers/...
>> directory, so this does not need to be board-specific.
>>
>>>
>>> The less obvious - but imho more sane - route would be to just implement PSCI on armv8 systems. York has patches in the works to make U-Boot be a PSCI provider on armv8, armv7 already has PSCI provider code. In EL3 / TZ you can maintain your 1:1 map, don't need to swizzle pointers and could just reuse dm as much as you like. Linux gets secondary smp bootup (and core power off for power saving) for free and efi reset would "just work" with this patch set. Even better yet, it would "just work" without efi too ;).
>>
>> Interesting - then ATF would not provide any run-time services? I
>> suppose that means that the code is maintained in two places, but it
>> sounds reasonable to me. It's probably not a lot of code.
>
> Well, ATF would still provide PSCI which Linux can directly call into ;). But U-Boot wouldn’t have to maintain anything internally for that functionality.

Ah I see, so in what way is U-Boot the provider in this case? Does it
mean U-Boot does not have run-time code anymore?

Regards,
Simon