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

[mingo@kernel.org (Ingo Molnar)]

* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> On 10 March 2016 at 16:03, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Ard Biesheuvel <ard.biesheuvel@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__);

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.

> 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.

Thanks,

	Ingo