[PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Ard Biesheuvel]

On 10 March 2016 at 16:25, Ingo Molnar <mingo at kernel.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
>
>> On 10 March 2016 at 16:03, Ingo Molnar <mingo at kernel.org> wrote:
>> >
>> > * Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
>> >
>> >> > So screen_info_guid should probably not be a 'const': you have to cast it away
>> >> > anyway, adding artificial linebreaks and uglifying the code. It's also a bad
>> >> > practice to cast away const-ness, it hinders move-consts-to-readonly-sections
>> >> > efforts.
>> >>
>> >> The problem here is that the UEFI spec never uses const qualifiers in
>> >> its APIs for by-ref parameters that are obviously never modified by
>> >> the caller, such as these GUIDs. [...]
>> >
>> > Ah, ok. Two related thoughts came up:
>> >
>> > 1)
>> >
>> > While I was looking at this code and was asking myself why the EFI runtime is
>> > generally invoked via a relatively fragile, non-type-checking vararg construct.
>> >
>> > Wouldn't you be better off by explicitly defining all the API variants, and then
>> > internally calling the EFI runtime?
>> >
>> > That would neatly solve such const artifacts as well.
>> >
>> > So instead of:
>> >
>> >
>> > +       status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
>> > +                               sizeof(*si), (void **)&si);
>> >
>> > we could have something like:
>> >
>> >         status = efi_early__allocate_pool(EFI_RUNTIME_SERVICES_DATA, sizeof(*si), &si);
>> >
>> >         ...
>> >
>> >         efi_early__free_pool(si);
>> >
>> >
>> > i.e. it would look a lot more like a properly distributed, typed, structured
>> > family of C function APIs, instead of this single central bastard of an ioctl()
>> > interface.
>> >
>> > There's over 100 invocations of the EFI runtime in the Linux kernel, I think it
>> > would be worth the effort. The wrapper inlines should be mostly trivial.
>> >
>> > That would also add an opportunity to actually document most of these calls.
>> >
>>
>> If only. The ARM and arm64 wrappers actually simply resolve to
>> correctly typed calls. While I am not an expert on the x86 side of
>> things, I think the vararg stuff is needed to be able to perform the
>> thunking required to do function calls using the MS ABI for x86_64.
>
> So at least the efi_early calls seem to just be using a function pointer:
>
>   arch/x86/include/asm/efi.h:     __efi_early()->call(__efi_early()->f, __VA_ARGS__);
>

Indeed. I think the efi_early struct is a separate structure that is
set up by the early boot code, and has a number of function pointers
of EFI boot services copied into it. Matt should know the details.

> Furthermore, my suggestion would work with arbitrarily structured thunking: my
> suggestion was to put a typed C layer in there - and the layer itself could then
> call the vararg construct internally. It's a C type demuxing, only a syntactic
> effort, it does not change any real call signature.
>

I would welcome any improvement in this regard. Happy to contribute as well.

>> As far as documentation is concerned, all of these functions and
>> protocol methods are documented in the UEFI spec, which is freely
>> accessible. It may be somewhat redundant to have our own documentation
>> for them.
>
> I mean a properly split up typed C interface 'documents itself' to a large degree
> - while with opaque varargs bugs can slip in more easily.
>
> This is really an obvious argument ... it's the well known 'why ioctl()s are bad'
> API argument.
>
> It has no bearing on this series obviously, it's a cleanup suggestion that could
> be done separately.
>

I will take this up with Matt. There is lots of room for improvement
and consolidation between x86 on the one hand and ARM/arm64 on the
other hand in other ways as well.

Thanks,
Ard.