| Message ID | 1457588408-19309-7-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 10 March 2016 at 15:25, Ingo Molnar <mingo@kernel.org> wrote: > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c >> index 495ebd657e38..0fbe00f13186 100644 >> --- a/drivers/firmware/efi/libstub/arm32-stub.c >> +++ b/drivers/firmware/efi/libstub/arm32-stub.c >> @@ -9,6 +9,44 @@ >> #include <linux/efi.h> >> #include <asm/efi.h> >> >> +static const efi_guid_t screen_info_guid = LINUX_ARM32_SCREEN_INFO_TABLE_GUID; >> + >> +struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg) >> +{ >> + struct screen_info *si; >> + efi_status_t status; >> + >> + /* >> + * Unlike on arm64, where we can directly fill out the screen_info >> + * structure from the stub, we need to allocate a buffer to hold >> + * its contents while we hand over to the kernel proper from the >> + * decompressor. >> + */ >> + status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA, >> + sizeof(*si), (void **)&si); >> + >> + if (status != EFI_SUCCESS) >> + return NULL; >> + >> + status = efi_call_early(install_configuration_table, >> + (efi_guid_t *)&screen_info_guid, si); >> + if (status == EFI_SUCCESS) >> + return si; >> + >> + efi_call_early(free_pool, si); >> + return NULL; >> +} >> + >> +void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si) >> +{ >> + if (!si) >> + return; >> + >> + efi_call_early(install_configuration_table, >> + (efi_guid_t *)&screen_info_guid, NULL); >> + efi_call_early(free_pool, si); >> +} >> + >> efi_status_t handle_kernel_image(efi_system_table_t *sys_table, >> unsigned long *image_addr, >> unsigned long *image_size, > > 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. This is __init code regardless (either due to the fact that it is linked into the stub rather than the core kernel, or because we __init-ize it using objcopy) so I don't care deeply, and I am happy to remove it. > Otherwise the series looks mostly good to me. Time for new v4.6 merges is short, > but if Matt acks it I can still take it into tip:efi/core. > Thank you. I am perfectly happy exercising some more caution here, though, considering that our past experiences involving reshuffling stub code for x86 were not a huge success (and broke Linus's laptop, among others') I was very careful not to introduce cross-object data symbol references (which were the culprit before), and I tested both i386 and x86_64 to the extent feasible to me, but we'd need to confirm that those things are still fully correct before queuing this. Thanks, Ard. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 10 March 2016 at 16:25, Ingo Molnar <mingo@kernel.org> wrote: > > * 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__); > 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h index daebcfe6c35a..6329b5be1eca 100644 --- a/arch/arm/include/asm/efi.h +++ b/arch/arm/include/asm/efi.h @@ -63,6 +63,18 @@ void efi_virtmap_unload(void); #define __efi_call_early(f, ...) f(__VA_ARGS__) #define efi_is_64bit() (0) +struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg); +void free_screen_info(efi_system_table_t *sys_table, struct screen_info *si); + +/* + * This GUID is used to pass to the kernel proper the struct screen_info + * structure that was populated by the stub based on the GOP protocol instance + * associated with ConOut + */ +#define LINUX_ARM32_SCREEN_INFO_TABLE_GUID \ + EFI_GUID(0xe03fc20a, 0x85dc, 0x406e, \ + 0xb9, 0xe, 0x4a, 0xb5, 0x02, 0x37, 0x1d, 0x95) + /* * A reasonable upper bound for the uncompressed kernel size is 32 MBytes, * so we will reserve that amount of memory. We have no easy way to tell what diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index 7d0cba6f1cc5..149a7787b40f 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -881,7 +881,8 @@ static void __init request_standard_resources(const struct machine_desc *mdesc) request_resource(&ioport_resource, &lp2); } -#if defined(CONFIG_VGA_CONSOLE) || defined(CONFIG_DUMMY_CONSOLE) +#if defined(CONFIG_VGA_CONSOLE) || defined(CONFIG_DUMMY_CONSOLE) || \ + defined(CONFIG_EFI) struct screen_info screen_info = { .orig_video_lines = 30, .orig_video_cols = 80, diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index e3d707131bba..a6c14f29b970 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -54,6 +54,9 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md); #define __efi_call_early(f, ...) f(__VA_ARGS__) #define efi_is_64bit() (1) +#define alloc_screen_info(x...) &screen_info +#define free_screen_info(x...) + #define EFI_ALLOC_ALIGN SZ_64K /* diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index b6abc852f2a1..6f4b3ea359db 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -17,6 +17,9 @@ #include <asm/efi.h> +/* we will fill this structure from the stub, so don't put it in .bss */ +struct screen_info screen_info __section(.data); + int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) { pteval_t prot_val; diff --git a/arch/arm64/kernel/image.h b/arch/arm64/kernel/image.h index 5ff892f40a0a..fa52ebdfc93e 100644 --- a/arch/arm64/kernel/image.h +++ b/arch/arm64/kernel/image.h @@ -115,6 +115,7 @@ __efistub___memset = KALLSYMS_HIDE(__pi_memset); __efistub__text = KALLSYMS_HIDE(_text); __efistub__end = KALLSYMS_HIDE(_end); __efistub__edata = KALLSYMS_HIDE(_edata); +__efistub_screen_info = KALLSYMS_HIDE(screen_info); #endif diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c index 4deb3e7faa0e..94bd51cfb71e 100644 --- a/drivers/firmware/efi/libstub/arm-stub.c +++ b/drivers/firmware/efi/libstub/arm-stub.c @@ -147,6 +147,25 @@ void efi_char16_printk(efi_system_table_t *sys_table_arg, out->output_string(out, str); } +static struct screen_info *setup_graphics(efi_system_table_t *sys_table_arg) +{ + efi_guid_t gop_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID; + efi_status_t status; + unsigned long size; + void **gop_handle = NULL; + struct screen_info *si = NULL; + + size = 0; + status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL, + &gop_proto, NULL, &size, gop_handle); + if (status == EFI_BUFFER_TOO_SMALL) { + si = alloc_screen_info(sys_table_arg); + if (!si) + return NULL; + efi_setup_gop(sys_table_arg, si, &gop_proto, size); + } + return si; +} /* * This function handles the architcture specific differences between arm and @@ -185,6 +204,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table, efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID; unsigned long reserve_addr = 0; unsigned long reserve_size = 0; + struct screen_info *si; /* Check if we were booted by the EFI firmware */ if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE) @@ -233,6 +253,8 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table, __nokaslr = true; } + si = setup_graphics(sys_table); + status = handle_kernel_image(sys_table, image_addr, &image_size, &reserve_addr, &reserve_size, @@ -305,6 +327,7 @@ fail_free_image: efi_free(sys_table, image_size, *image_addr); efi_free(sys_table, reserve_size, reserve_addr); fail_free_cmdline: + free_screen_info(sys_table, si); efi_free(sys_table, cmdline_size, (unsigned long)cmdline_ptr); fail: return EFI_ERROR; diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c index 495ebd657e38..0fbe00f13186 100644 --- a/drivers/firmware/efi/libstub/arm32-stub.c +++ b/drivers/firmware/efi/libstub/arm32-stub.c @@ -9,6 +9,44 @@ #include <linux/efi.h> #include <asm/efi.h> +static const efi_guid_t screen_info_guid = LINUX_ARM32_SCREEN_INFO_TABLE_GUID; + +struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg) +{ + struct screen_info *si; + efi_status_t status; + + /* + * Unlike on arm64, where we can directly fill out the screen_info + * structure from the stub, we need to allocate a buffer to hold + * its contents while we hand over to the kernel proper from the + * decompressor. + */ + status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA, + sizeof(*si), (void **)&si); + + if (status != EFI_SUCCESS) + return NULL; + + status = efi_call_early(install_configuration_table, + (efi_guid_t *)&screen_info_guid, si); + if (status == EFI_SUCCESS) + return si; + + efi_call_early(free_pool, si); + return NULL; +} + +void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si) +{ + if (!si) + return; + + efi_call_early(install_configuration_table, + (efi_guid_t *)&screen_info_guid, NULL); + efi_call_early(free_pool, si); +} + efi_status_t handle_kernel_image(efi_system_table_t *sys_table, unsigned long *image_addr, unsigned long *image_size,
This enables the EFI stub side of handling the EFI Graphics Output Protocol. It involves invoking the generic GOP code to discover the GOP handle that also drives the console, and using its metadata to populate the screen_info structure. For arm64, the stub and the kernel proper live in the same executable, so we can simply access screen_info directly. For ARM, we need to allocate a struct screen_info in the stub, and install it as a configuration table so that the core kernel can go and fetch it to copy its contents into the real screen_info structure. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm/include/asm/efi.h | 12 +++++++ arch/arm/kernel/setup.c | 3 +- arch/arm64/include/asm/efi.h | 3 ++ arch/arm64/kernel/efi.c | 3 ++ arch/arm64/kernel/image.h | 1 + drivers/firmware/efi/libstub/arm-stub.c | 23 ++++++++++++ drivers/firmware/efi/libstub/arm32-stub.c | 38 ++++++++++++++++++++ 7 files changed, 82 insertions(+), 1 deletion(-) -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel