On 10 March 2016 at 16:03, Ingo Molnar <mingo@xxxxxxxxxx> wrote: > > * Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> 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. 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. > 2) > > Another suggestion: would it make sense to unify the 'EFI' and 'EFI early' calls - > is there any deep reason why they are invoked via separate names? Why not use a > single namespace: > > efi__allocate_pool() > efi__free_pool() > > and have a 'current EFI configuration' pointer internaly that can be switched from > the early to the later variant during bootup. The various typed API wrappers would > use this pointer. > Actually, having statically initialized pointer variables is more of a concern. The nice thing about plain function calls is that they can usually be resolved at link time to relative branches on most architectures, rather than require GOT fixups or other magic to be able to execute in the UEFI environment. Thanks, Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
