On 10 March 2016 at 16:25, Ingo Molnar [off-list ref] wrote:

* Ard Biesheuvel [off-list ref] wrote:

quoted

On 10 March 2016 at 16:03, Ingo Molnar [off-list ref] wrote:

quoted

* Ard Biesheuvel [off-list ref] wrote:

quoted

quoted

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.

quoted

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.