Honey, I shrunk the EFI stub
cover Honey, I shrunk the EFI stub
Demonstrate the code reduction attainable by efi_call_proto() which was proffered in a patch I've posted a few minutes ago.
For this to work, all three protocol variants (_32_t and _64_t for x86 and _t for ARM) need to be declared as typedefs. The declaration and naming of protocols in include/linux/efi.h currently isn't consistent, some are declared as typedefs and some aren't, some use a "_t" suffix and some don't. These inconsistencies need to be straightened out when converting to efi_call_proto(). It should be noted that checkpatch complains about newly introduced typedefs. It would be possible to retool efi_call_proto() to work without typedef declarations as long as it's done consistently.
In __file_size32() all protocol calls are currently cast to unsigned long, which is 64 bit when compiled on x86_64. Matt has said that the register needs to be loaded with a 32 bit address, so it looks to me like this is currently broken for mixed-mode. Patch [1/2] should fix this. E.g.:
efi_file_handle_32_t *h, *fh = __fh; [...] status = efi_early->call((unsigned long)h->get_info, h, &info_guid, &info_sz, NULL);
Another oddity is that info_sz is declared u32 in __file_size32(), yet the spec says that the third argument to EFI_FILE_PROTOCOL.GetInfo() is of type UINTN, which I assume is 64 bit regardless of mixed-mode, or am I missing something? Patch [1/2] uses an unsigned long instead.
Thanks,
Lukas
Lukas Wunner (2):
efi: Deduplicate efi_file_size() / _read() / _close() x86/efi: Deduplicate efi_char16_printk()
arch/x86/boot/compressed/eboot.c | 174 +------------------------ drivers/firmware/efi/libstub/arm-stub.c | 69 ---------- drivers/firmware/efi/libstub/efi-stub-helper.c | 63 +++++++++ drivers/firmware/efi/libstub/efistub.h | 8 -- include/linux/efi.h | 8 +- 5 files changed, 69 insertions(+), 253 deletions(-)
Signature · 1 line
2.10.1
A message that is not in the archive (sent off-list or not yet received): cover.1478510356.git.lukas-JFq808J9C/[email protected]
PATCH 1/2 efi: Deduplicate efi_file_size() / _read() / _close()
There's one ARM, one x86_32 and one x86_64 version which can be folded into a single shared version by masking their differences with the shiny new efi_call_proto() macro.
No functional change intended.
--- arch/x86/boot/compressed/eboot.c | 148 ------------------------- drivers/firmware/efi/libstub/arm-stub.c | 69 ------------ drivers/firmware/efi/libstub/efi-stub-helper.c | 63 +++++++++++ drivers/firmware/efi/libstub/efistub.h | 8 -- 4 files changed, 63 insertions(+), 225 deletions(-)
arch/x86/boot/compressed/eboot.c +0 -148
@@ -38,154 +38,6 @@ static void setup_boot_services##bits(struct efi_config *c) \BOOT_SERVICES (32 );BOOT_SERVICES (64 ); -void efi_char16_printk (efi_system_table_t *,efi_char16_t *); - -static efi_status_t -__file_size32 (void *__fh,efi_char16_t *filename_16, -void **handle,u64 *file_sz) -{ -efi_file_handle_32_t *h, *fh = __fh; -efi_file_info_t *info; -efi_status_t status; -efi_guid_t info_guid =EFI_FILE_INFO_ID ; -u32 info_sz; - - status = efi_early->call ((unsigned long )fh->open, fh, &h, filename_16, -EFI_FILE_MODE_READ , (u64 )0 ); -if (status !=EFI_SUCCESS ) { -efi_printk (sys_table,"Failed to open file: " ); -efi_char16_printk (sys_table, filename_16); -efi_printk (sys_table,"\n" ); -return status; - } - - *handle = h; - - info_sz =0 ; - status = efi_early->call ((unsigned long )h->get_info, h, &info_guid, - &info_sz,NULL ); -if (status !=EFI_BUFFER_TOO_SMALL ) { -efi_printk (sys_table,"Failed to get file info size\n" ); -return status; - } - -grow: - status =efi_call_early (allocate_pool,EFI_LOADER_DATA , - info_sz, (void **)&info); -if (status !=EFI_SUCCESS ) { -efi_printk (sys_table,"Failed to alloc mem for file info\n" ); -return status; - } - - status = efi_early->call ((unsigned long )h->get_info, h, &info_guid, - &info_sz, info); -if (status ==EFI_BUFFER_TOO_SMALL ) { -efi_call_early (free_pool, info); - goto grow; - } - - *file_sz = info->file_size; -efi_call_early (free_pool, info); - -if (status !=EFI_SUCCESS ) -efi_printk (sys_table,"Failed to get initrd info\n" ); - -return status; -} - -static efi_status_t -__file_size64 (void *__fh,efi_char16_t *filename_16, -void **handle,u64 *file_sz) -{ -efi_file_handle_64_t *h, *fh = __fh; -efi_file_info_t *info; -efi_status_t status; -efi_guid_t info_guid =EFI_FILE_INFO_ID ; -u64 info_sz; - - status = efi_early->call ((unsigned long )fh->open, fh, &h, filename_16, -EFI_FILE_MODE_READ , (u64 )0 ); -if (status !=EFI_SUCCESS ) { -efi_printk (sys_table,"Failed to open file: " ); -efi_char16_printk (sys_table, filename_16); -efi_printk (sys_table,"\n" ); -return status; - } - - *handle = h; - - info_sz =0 ; - status = efi_early->call ((unsigned long )h->get_info, h, &info_guid, - &info_sz,NULL ); -if (status !=EFI_BUFFER_TOO_SMALL ) { -efi_printk (sys_table,"Failed to get file info size\n" ); -return status; - } - -grow: - status =efi_call_early (allocate_pool,EFI_LOADER_DATA , - info_sz, (void **)&info); -if (status !=EFI_SUCCESS ) { -efi_printk (sys_table,"Failed to alloc mem for file info\n" ); -return status; - } - - status = efi_early->call ((unsigned long )h->get_info, h, &info_guid, - &info_sz, info); -if (status ==EFI_BUFFER_TOO_SMALL ) { -efi_call_early (free_pool, info); - goto grow; - } - - *file_sz = info->file_size; -efi_call_early (free_pool, info); - -if (status !=EFI_SUCCESS ) -efi_printk (sys_table,"Failed to get initrd info\n" ); - -return status; -} -efi_status_t -efi_file_size (efi_system_table_t *sys_table,void *__fh, -efi_char16_t *filename_16,void **handle,u64 *file_sz) -{ -if (efi_early->is64) -return __file_size64 (__fh, filename_16, handle, file_sz); - -return __file_size32 (__fh, filename_16, handle, file_sz); -} - -efi_status_t -efi_file_read (void *handle,unsigned long *size,void *addr) -{ -unsigned long func; - -if (efi_early->is64) { -efi_file_handle_64_t *fh = handle; - - func = (unsigned long )fh->read; -return efi_early->call (func, handle, size, addr); - }else { -efi_file_handle_32_t *fh = handle; - - func = (unsigned long )fh->read; -return efi_early->call (func, handle, size, addr); - } -} - -efi_status_t efi_file_close (void *handle) -{ -if (efi_early->is64) { -efi_file_handle_64_t *fh = handle; - -return efi_early->call ((unsigned long )fh->close, handle); - }else { -efi_file_handle_32_t *fh = handle; - -return efi_early->call ((unsigned long )fh->close, handle); - } -} -static inline efi_status_t __open_volume32 (void *__image,void **__fh) {efi_file_io_interface_t *io;
drivers/firmware/efi/libstub/arm-stub.c +0 -69
75 lines not on this page · raw message
drivers/firmware/efi/libstub/efi-stub-helper.c +63 -0
69 lines not on this page · raw message
drivers/firmware/efi/libstub/efistub.h +0 -8
@@ -29,14 +29,6 @@ void efi_char16_printk(efi_system_table_t *, efi_char16_t *);efi_status_t efi_open_volume (efi_system_table_t *sys_table_arg,void *__image,void **__fh); -efi_status_t efi_file_size (efi_system_table_t *sys_table_arg,void *__fh, -efi_char16_t *filename_16,void **handle, -u64 *file_sz); - -efi_status_t efi_file_read (void *handle,unsigned long *size,void *addr); - -efi_status_t efi_file_close (void *handle); -unsigned long get_dram_base (efi_system_table_t *sys_table_arg); efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
Signature · 1 line
2.10.1
PATCH 2/2 x86/efi: Deduplicate efi_char16_printk()
In reply to a message not in the archiveEliminate the separate 32 bit and 64 bit code paths by way of the shiny new efi_call_proto() macro.
No functional change intended.
--- arch/x86/boot/compressed/eboot.c | 26 ++------------------------ include/linux/efi.h | 8 ++++---- 2 files changed, 6 insertions(+), 28 deletions(-)
arch/x86/boot/compressed/eboot.c +2 -24
@@ -101,30 +101,8 @@ efi_open_volume(efi_system_table_t *sys_table, void *__image, void **__fh)void efi_char16_printk (efi_system_table_t *table,efi_char16_t *str) { -unsigned long output_string; -size_t offset; - -if (efi_early->is64) { -struct efi_simple_text_output_protocol_64 *out; -u64 *func; - - offset = offsetof(typeof(*out), output_string); - output_string = efi_early->text_output + offset; - out = (typeof (out ))(unsigned long )efi_early->text_output; - func = (u64 *)output_string; - - efi_early->call (*func, out, str); - }else { -struct efi_simple_text_output_protocol_32 *out; -u32 *func; - - offset = offsetof(typeof(*out), output_string); - output_string = efi_early->text_output + offset; - out = (typeof (out ))(unsigned long )efi_early->text_output; - func = (u32 *)output_string; - - efi_early->call (*func, out, str); - } +efi_call_proto (efi_simple_text_output_protocol, output_string, + efi_early->text_output, str); } static efi_status_t
include/linux/efi.h +4 -4
@@ -1238,17 +1238,17 @@ struct efivar_entry {bool deleting; }; -struct efi_simple_text_output_protocol_32 { +typedef struct {u32 reset;u32 output_string;u32 test_string; -}; +}efi_simple_text_output_protocol_32_t ; -struct efi_simple_text_output_protocol_64 { +typedef struct {u64 reset;u64 output_string;u64 test_string; -}; +}efi_simple_text_output_protocol_64_t ;struct efi_simple_text_output_protocol {void *reset;
Signature · 1 line
2.10.1
Quoted 12 lines · Lukas Wunner
Demonstrate the code reduction attainable by
efi_call_proto()which was proffered in a patch I've posted a few minutes ago.For this to work, all three protocol variants (
_32_tand_64_tfor x86 and _t for ARM) need to be declared as typedefs. The declaration and naming of protocols ininclude/linux/efi.hcurrently isn't consistent, some are declared as typedefs and some aren't, some use a "_t" suffix and some don't. These inconsistencies need to be straightened out when converting toefi_call_proto(). It should be noted that checkpatch complains about newly introduced typedefs. It would be possible to retoolefi_call_proto()to work without typedef declarations as long as it's done consistently.
This is probably v4.11 material. We *may* be able to get this into v4.10 if I review and merge this soon, but it definitely isn't going to be included in the imminent pull request.
I do like the general idea though.
Quoted 9 lines
In
__file_size32()all protocol calls are currently cast to unsigned long, which is 64 bit when compiled onx86_64. Matt has said that the register needs to be loaded with a 32 bit address, so it looks to me like this is currently broken for mixed-mode. Patch [1/2] should fix this. E.g.:efi_file_handle_32_t *h, *fh = __fh; [...] status = efi_early->call((unsigned long)h->get_info, h, &info_guid, &info_sz, NULL);
There's a subtle distinction here between 32-bit address and 32-bit value. A 64-bit value can be a valid 32-bit address, provided that the upper 32-bits are zero, e.g. 0x00000000ffffffff.
So when I say "32-bit address" I really just mean some value where only the lower 32-bits are important.
That is why using unsigned long in mixed-mode is OK for the early call code.
Quoted 4 lines
Another oddity is that
info_szis declared u32 in__file_size32(), yet the spec says that the third argument toEFI_FILE_PROTOCOL.GetInfo()is of type UINTN, which I assume is 64 bit regardless of mixed-mode, or am I missing something? Patch [1/2] uses an unsigned long instead.
UINTN is an unsigned value of native width as seen by the firmware. On 32-bit firmware that's 32-bits and 64-bit firmware 64-bits.
Using 'u32' in __file_size32() is correct, unsigned long is not.
A message that is not in the archive (sent off-list or not yet received): 20161112205514.GA2373-mF/unelCI9GS6iBeEJttW/[email protected]
Quoted 14 lines · Matt Fleming
Quoted 12 lines · Lukas Wunner
Demonstrate the code reduction attainable by
efi_call_proto()which was proffered in a patch I've posted a few minutes ago.For this to work, all three protocol variants (
_32_tand_64_tfor x86 and _t for ARM) need to be declared as typedefs. The declaration and naming of protocols ininclude/linux/efi.hcurrently isn't consistent, some are declared as typedefs and some aren't, some use a "_t" suffix and some don't. These inconsistencies need to be straightened out when converting toefi_call_proto(). It should be noted that checkpatch complains about newly introduced typedefs. It would be possible to retoolefi_call_proto()to work without typedef declarations as long as it's done consistently.
Quoted 5 lines
This is probably v4.11 material. We *may* be able to get this into v4.10 if I review and merge this soon, but it definitely isn't going to be included in the imminent pull request.
I do like the general idea though.
Yes, this was posted quite late in the cycle so I didn't expect it to make it into 4.10 really. It was meant as a demonstration, I can respin this into a series that deduplicates these redundancies more thoroughly, but I wanted to gauge the reaction of the community first. Ard should probably also weigh in since it touches ARM code.
By the way, you mentioned that you have a MacBook2,1, is this the Late 2006 version and would you be able to test changes to the efistub on that machine? I was thinking about obtaining such a machine myself on ebay since right now I can only test x86_64, not mixed mode. If noone else is able to perform tests I might just do that. (Only the Late 2006 version uses mixed mode, the Mid 2007 has a native 64-bit EFI.)
Quoted 25 lines
Quoted 9 lines
In
__file_size32()all protocol calls are currently cast to unsigned long, which is 64 bit when compiled onx86_64. Matt has said that the register needs to be loaded with a 32 bit address, so it looks to me like this is currently broken for mixed-mode. Patch [1/2] should fix this. E.g.:efi_file_handle_32_t *h, *fh = __fh; [...] status = efi_early->call((unsigned long)h->get_info, h, &info_guid, &info_sz, NULL);There's a subtle distinction here between 32-bit address and 32-bit value. A 64-bit value can be a valid 32-bit address, provided that the upper 32-bits are zero, e.g.
0x00000000ffffffff.So when I say "32-bit address" I really just mean some value where only the lower 32-bits are important.
That is why using unsigned long in mixed-mode is OK for the early call code.
Quoted 4 lines
Another oddity is that
info_szis declared u32 in__file_size32(), yet the spec says that the third argument toEFI_FILE_PROTOCOL.GetInfo()is of type UINTN, which I assume is 64 bit regardless of mixed-mode, or am I missing something? Patch [1/2] uses an unsigned long instead.
Quoted 4 lines
UINTN is an unsigned value of native width as seen by the firmware. On 32-bit firmware that's 32-bits and 64-bit firmware 64-bits.
Using 'u32' in
__file_size32()is correct, unsigned long is not.
Okay since this is all little endian, it should be okay to have a 64 bit wide variable on the stack whose address is passed to GetInfo() as BufferSize argument. But I guess I need to initialize it to 0 upon declaration so that the upper 32 bit are zeroed out in mixed mode, right? That would be a bug in patch [1/2] then.
Thanks,
Lukas
A message that is not in the archive (sent off-list or not yet received): 20161114111906.GA9938-JFq808J9C/[email protected]
Quoted 10 lines · Lukas Wunner
Quoted 9 lines · Matt Fleming
Quoted 4 lines · Lukas Wunner
Another oddity is that
info_szis declared u32 in__file_size32(), yet the spec says that the third argument toEFI_FILE_PROTOCOL.GetInfo()is of type UINTN, which I assume is 64 bit regardless of mixed-mode, or am I missing something? Patch [1/2] uses an unsigned long instead.UINTN is an unsigned value of native width as seen by the firmware. On 32-bit firmware that's 32-bits and 64-bit firmware 64-bits.
Quoted 7 lines
Quoted 1 line
Using 'u32' in
__file_size32()is correct, unsigned long is not.Okay since this is all little endian, it should be okay to have a 64 bit wide variable on the stack whose address is passed to
GetInfo()as BufferSize argument. But I guess I need to initialize it to 0 upon declaration so that the upper 32 bit are zeroed out in mixed mode, right? That would be a bug in patch [1/2] then.
Oh the info_sz variable *is* already initialized to 0 before the first invocation of GetInfo(). So the patch should be fine, reviewed it once more with the explanations in mind that you provided and couldn't see anything else that would cause issues in mixed-mode.
Thanks,
Lukas
A message that is not in the archive (sent off-list or not yet received): 20161114153231.GB10141-JFq808J9C/[email protected]
Quoted 5 lines · Lukas Wunner
Oh the
info_szvariable *is* already initialized to 0 before the first invocation ofGetInfo(). So the patch should be fine, reviewed it once more with the explanations in mind that you provided and couldn't see anything else that would cause issues in mixed-mode.
Thanks for double-checking that. These patches look good to me and pass all my testing. Queued for v4.11, thanks Lukas!