| Message ID | 20180712122154.13819-1-ard.biesheuvel@linaro.org |
|---|---|
| Headers | show |
| Series | efi/x86 mixed mode cleanups | expand |
Hi, On 12-07-18 14:21, Ard Biesheuvel wrote: > This series contains some fixes and cleanups for mixed-mode UEFI on x86. > > Patch #1 adds the missing locking in the runtime service wrapper code for > mixed mode. This was found by inspection rather than having been reported > but could be a candidate for -stable nonetheless. > > Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci > routine. > > Patches #3 and #4 do the same for the UGA draw protocol discovery routines. > > Patch #5 fixes a latent bug in the UGA draw code. > > Patch #6 helps unused code paths to be optimized away on configurations > that don't need them (32-bit only and 64-bit only) I've given a kernel with these patches a quick spin running in mixed mode on a Bay Trail based tablet: Tested-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 July 2018 at 16:26, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 12-07-18 14:21, Ard Biesheuvel wrote: >> >> This series contains some fixes and cleanups for mixed-mode UEFI on x86. >> >> Patch #1 adds the missing locking in the runtime service wrapper code for >> mixed mode. This was found by inspection rather than having been reported >> but could be a candidate for -stable nonetheless. >> >> Patch #2 merges the remaining 32/64-bit specific parts of the >> setup_efi_pci >> routine. >> >> Patches #3 and #4 do the same for the UGA draw protocol discovery >> routines. >> >> Patch #5 fixes a latent bug in the UGA draw code. >> >> Patch #6 helps unused code paths to be optimized away on configurations >> that don't need them (32-bit only and 64-bit only) > > > I've given a kernel with these patches a quick spin running in mixed > mode on a Bay Trail based tablet: > > Tested-by: Hans de Goede <hdegoede@redhat.com> > Thanks Hans, much appreciated. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ard, > This series contains some fixes and cleanups for mixed-mode UEFI on x86. > > Patch #1 adds the missing locking in the runtime service wrapper code for mixed > mode. This was found by inspection rather than having been reported but could > be a candidate for -stable nonetheless. > > Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci > routine. > > Patches #3 and #4 do the same for the UGA draw protocol discovery routines. > > Patch #5 fixes a latent bug in the UGA draw code. > > Patch #6 helps unused code paths to be optimized away on configurations that > don't need them (32-bit only and 64-bit only) I have tested this patch set on qemu and I see mixed mode kernel not booting. My test setup is: Running qemu-system-x86_64 with 32 bit OVMF and x86_64 kernel built with Mixed mode enabled. I am using elilinux as bootloader. Upon further investigation, I found that the issue isn't related to this patch set. It's introduced with commit 2e6eb40ca5eb ("Fix incorrect invocation of PciIO->Attributes") Reverting the change does help in booting mixed mode kernel. Regards, Sai -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2018 at 03:59, Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com> wrote: > Hi Ard, > >> This series contains some fixes and cleanups for mixed-mode UEFI on x86. >> >> Patch #1 adds the missing locking in the runtime service wrapper code for mixed >> mode. This was found by inspection rather than having been reported but could >> be a candidate for -stable nonetheless. >> >> Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci >> routine. >> >> Patches #3 and #4 do the same for the UGA draw protocol discovery routines. >> >> Patch #5 fixes a latent bug in the UGA draw code. >> >> Patch #6 helps unused code paths to be optimized away on configurations that >> don't need them (32-bit only and 64-bit only) > > I have tested this patch set on qemu and I see mixed mode kernel not booting. > My test setup is: > Running qemu-system-x86_64 with 32 bit OVMF and x86_64 kernel built with Mixed mode enabled. > I am using elilinux as bootloader. > > Upon further investigation, I found that the issue isn't related to this patch set. > It's introduced with commit 2e6eb40ca5eb ("Fix incorrect invocation of PciIO->Attributes") > Reverting the change does help in booting mixed mode kernel. > Hello Sai, This is likely due to the fact that you are missing this patch https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=efi/urgent&id=e296701800f30d260a66f8aa1971b5b1bc3d2f81 Could you please double check? Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> Patch #6 helps unused code paths to be optimized away on > >> configurations that don't need them (32-bit only and 64-bit only) > > > > I have tested this patch set on qemu and I see mixed mode kernel not booting. > > My test setup is: > > Running qemu-system-x86_64 with 32 bit OVMF and x86_64 kernel built with > Mixed mode enabled. > > I am using elilinux as bootloader. > > > > Upon further investigation, I found that the issue isn't related to this patch set. > > It's introduced with commit 2e6eb40ca5eb ("Fix incorrect invocation of > > PciIO->Attributes") Reverting the change does help in booting mixed mode > kernel. > > > > Hello Sai, > > This is likely due to the fact that you are missing this patch > > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=efi/urgen > t&id=e296701800f30d260a66f8aa1971b5b1bc3d2f81 > > Could you please double check? Thanks. Thanks for updating me on this. I now see why my testing broke. I think, the above patch is only in TIP tree and didn't yet make to Linus's tree and is also not part of efi tree next branch (these were the trees I am testing on). Regards, Sai
On Thu, Jul 12, 2018 at 02:21:48PM +0200, Ard Biesheuvel wrote: > Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci > routine. > > Patches #3 and #4 do the same for the UGA draw protocol discovery routines. > > Patch #6 helps unused code paths to be optimized away on configurations > that don't need them (32-bit only and 64-bit only) Thanks a lot for doing this Ard, I meant to deduplicate that code further but didn't get around to it. FWIW I have a single unsubmitted deduplication patch which has been rotting on my development branch for more than a year now, I'm including it below in case it helps, needs a careful review though. -- >8 -- From 7fa51faec20c74eea2bb3ba96ecab1bc3e18e5a8 Mon Sep 17 00:00:00 2001 From: Lukas Wunner <lukas@wunner.de> Date: Sun, 7 May 2017 14:42:51 +0200 Subject: [PATCH] efi: Deduplicate efi_open_volume() 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 efi_call_proto() macro introduced by commit 3552fdf29f01 ("efi: Allow bitness-agnostic protocol calls"). To be able to dereference the device_handle attribute from the efi_loaded_image_t table in an arch- and bitness-agnostic manner, introduce the efi_table_attr() macro (which already exists for x86) to arm and arm64. No functional change intended. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- arch/arm/include/asm/efi.h | 3 ++ arch/arm64/include/asm/efi.h | 3 ++ arch/x86/boot/compressed/eboot.c | 61 -------------------------- drivers/firmware/efi/libstub/arm-stub.c | 25 ----------- drivers/firmware/efi/libstub/efi-stub-helper.c | 29 +++++++++++- drivers/firmware/efi/libstub/efistub.h | 3 -- include/linux/efi.h | 10 +++++ 7 files changed, 43 insertions(+), 91 deletions(-) diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h index 17f1f1a..38badaa 100644 --- a/arch/arm/include/asm/efi.h +++ b/arch/arm/include/asm/efi.h @@ -58,6 +58,9 @@ static inline void efi_set_pgd(struct mm_struct *mm) #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__) #define efi_is_64bit() (false) +#define efi_table_attr(table, attr, instance) \ + ((table##_t *)instance)->attr + #define efi_call_proto(protocol, f, instance, ...) \ ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__) diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index c4cd508..5a21037 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -85,6 +85,9 @@ static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base, #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__) #define efi_is_64bit() (true) +#define efi_table_attr(table, attr, instance) \ + ((table##_t *)instance)->attr + #define efi_call_proto(protocol, f, instance, ...) \ ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 9a5b6d3..2d2b599 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -41,67 +41,6 @@ __pure const struct efi_config *__efi_early(void) BOOT_SERVICES(32); BOOT_SERVICES(64); -static inline efi_status_t __open_volume32(void *__image, void **__fh) -{ - efi_file_io_interface_t *io; - efi_loaded_image_32_t *image = __image; - efi_file_handle_32_t *fh; - efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; - efi_status_t status; - void *handle = (void *)(unsigned long)image->device_handle; - unsigned long func; - - status = efi_call_early(handle_protocol, handle, - &fs_proto, (void **)&io); - if (status != EFI_SUCCESS) { - efi_printk(sys_table, "Failed to handle fs_proto\n"); - return status; - } - - func = (unsigned long)io->open_volume; - status = efi_early->call(func, io, &fh); - if (status != EFI_SUCCESS) - efi_printk(sys_table, "Failed to open volume\n"); - - *__fh = fh; - return status; -} - -static inline efi_status_t __open_volume64(void *__image, void **__fh) -{ - efi_file_io_interface_t *io; - efi_loaded_image_64_t *image = __image; - efi_file_handle_64_t *fh; - efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; - efi_status_t status; - void *handle = (void *)(unsigned long)image->device_handle; - unsigned long func; - - status = efi_call_early(handle_protocol, handle, - &fs_proto, (void **)&io); - if (status != EFI_SUCCESS) { - efi_printk(sys_table, "Failed to handle fs_proto\n"); - return status; - } - - func = (unsigned long)io->open_volume; - status = efi_early->call(func, io, &fh); - if (status != EFI_SUCCESS) - efi_printk(sys_table, "Failed to open volume\n"); - - *__fh = fh; - return status; -} - -efi_status_t -efi_open_volume(efi_system_table_t *sys_table, void *__image, void **__fh) -{ - if (efi_early->is64) - return __open_volume64(__image, __fh); - - return __open_volume32(__image, __fh); -} - void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str) { efi_call_proto(efi_simple_text_output_protocol, output_string, diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c index 01a9d78..e242fb9 100644 --- a/drivers/firmware/efi/libstub/arm-stub.c +++ b/drivers/firmware/efi/libstub/arm-stub.c @@ -40,31 +40,6 @@ static u64 virtmap_base = EFI_RT_VIRTUAL_BASE; -efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, - void *__image, void **__fh) -{ - efi_file_io_interface_t *io; - efi_loaded_image_t *image = __image; - efi_file_handle_t *fh; - efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; - efi_status_t status; - void *handle = (void *)(unsigned long)image->device_handle; - - status = sys_table_arg->boottime->handle_protocol(handle, - &fs_proto, (void **)&io); - if (status != EFI_SUCCESS) { - efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); - return status; - } - - status = io->open_volume(io, &fh); - if (status != EFI_SUCCESS) - efi_printk(sys_table_arg, "Failed to open volume\n"); - - *__fh = fh; - return status; -} - void efi_char16_printk(efi_system_table_t *sys_table_arg, efi_char16_t *str) { diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index 50a9cab..d399679 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -413,6 +413,32 @@ static efi_status_t efi_file_close(void *handle) return efi_call_proto(efi_file_handle, close, handle); } +static efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, + efi_loaded_image_t *image, + efi_file_handle_t **__fh) +{ + efi_file_io_interface_t *io; + efi_file_handle_t *fh; + efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; + efi_status_t status; + void *handle = + (void *)efi_table_attr(efi_loaded_image, device_handle, image); + + status = efi_call_early(handle_protocol, handle, + &fs_proto, (void **)&io); + if (status != EFI_SUCCESS) { + efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); + return status; + } + + status = efi_call_proto(efi_file_io_interface, open_volume, io, &fh); + if (status != EFI_SUCCESS) + efi_printk(sys_table_arg, "Failed to open volume\n"); + + *__fh = fh; + return status; +} + /* * Parse the ASCII string 'cmdline' for EFI options, denoted by the efi= * option, e.g. efi=nochunk. @@ -563,8 +589,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, /* Only open the volume once. */ if (!i) { - status = efi_open_volume(sys_table_arg, image, - (void **)&fh); + status = efi_open_volume(sys_table_arg, image, &fh); if (status != EFI_SUCCESS) goto free_files; } diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index f59564b..32799cf 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -36,9 +36,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); - unsigned long get_dram_base(efi_system_table_t *sys_table_arg); efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, diff --git a/include/linux/efi.h b/include/linux/efi.h index d0a6fea..d56b624 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -859,6 +859,16 @@ struct efi_fdt_params { void *flush; } efi_file_handle_t; +typedef struct { + u64 revision; + u32 open_volume; +} efi_file_io_interface_32_t; + +typedef struct { + u64 revision; + u64 open_volume; +} efi_file_io_interface_64_t; + typedef struct _efi_file_io_interface { u64 revision; int (*open_volume)(struct _efi_file_io_interface *, -- 2.18.0 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Lukas Wunner <lukas@wunner.de> wrote: > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -413,6 +413,32 @@ static efi_status_t efi_file_close(void *handle) > return efi_call_proto(efi_file_handle, close, handle); > } > > +static efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, > + efi_loaded_image_t *image, > + efi_file_handle_t **__fh) > +{ > + efi_file_io_interface_t *io; > + efi_file_handle_t *fh; > + efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; > + efi_status_t status; > + void *handle = > + (void *)efi_table_attr(efi_loaded_image, device_handle, image); > + > + status = efi_call_early(handle_protocol, handle, > + &fs_proto, (void **)&io); > + if (status != EFI_SUCCESS) { > + efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); > + return status; > + } > + > + status = efi_call_proto(efi_file_io_interface, open_volume, io, &fh); > + if (status != EFI_SUCCESS) > + efi_printk(sys_table_arg, "Failed to open volume\n"); > + > + *__fh = fh; > + return status; BTW., in the second failure branch is 'fh' guaranteed to be set by the EFI call? If not then we set *__fh to a potentially undefined value, from the kernels stack? (I realize that your refactoring just inherited this existing pattern, but it caught my attention.) Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2018 at 15:29, Ingo Molnar <mingo@kernel.org> wrote: > > * Lukas Wunner <lukas@wunner.de> wrote: > >> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c >> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c >> @@ -413,6 +413,32 @@ static efi_status_t efi_file_close(void *handle) >> return efi_call_proto(efi_file_handle, close, handle); >> } >> >> +static efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, >> + efi_loaded_image_t *image, >> + efi_file_handle_t **__fh) >> +{ >> + efi_file_io_interface_t *io; >> + efi_file_handle_t *fh; >> + efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; >> + efi_status_t status; >> + void *handle = >> + (void *)efi_table_attr(efi_loaded_image, device_handle, image); >> + >> + status = efi_call_early(handle_protocol, handle, >> + &fs_proto, (void **)&io); >> + if (status != EFI_SUCCESS) { >> + efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); >> + return status; >> + } >> + >> + status = efi_call_proto(efi_file_io_interface, open_volume, io, &fh); >> + if (status != EFI_SUCCESS) >> + efi_printk(sys_table_arg, "Failed to open volume\n"); >> + >> + *__fh = fh; >> + return status; > > BTW., in the second failure branch is 'fh' guaranteed to be set by the EFI call? > If not then we set *__fh to a potentially undefined value, from the kernels stack? > > (I realize that your refactoring just inherited this existing pattern, but it > caught my attention.) > No, only EFI_SUCCESS guarantees that fh will have been assigned. Since we propagate 'status' here, it does not seem to matter in practice, but it would indeed be better not to clobber *__fh with random junk when returning failure. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2018 at 11:57, Lukas Wunner <lukas@wunner.de> wrote: > On Thu, Jul 12, 2018 at 02:21:48PM +0200, Ard Biesheuvel wrote: >> Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci >> routine. >> >> Patches #3 and #4 do the same for the UGA draw protocol discovery routines. >> >> Patch #6 helps unused code paths to be optimized away on configurations >> that don't need them (32-bit only and 64-bit only) > > Thanks a lot for doing this Ard, I meant to deduplicate that code > further but didn't get around to it. FWIW I have a single unsubmitted > deduplication patch which has been rotting on my development branch for > more than a year now, I'm including it below in case it helps, needs a > careful review though. > Thanks Lukas. I will apply that (with Ingo's comment addressed). Did you have any thoughts about how we could at least detect situations where we are invoking protocols that are not mixed-mode safe (i.e., they pass 64-bit quantities by value, which our current thunking routine does not take into account) > -- >8 -- > From 7fa51faec20c74eea2bb3ba96ecab1bc3e18e5a8 Mon Sep 17 00:00:00 2001 > From: Lukas Wunner <lukas@wunner.de> > Date: Sun, 7 May 2017 14:42:51 +0200 > Subject: [PATCH] efi: Deduplicate efi_open_volume() > > 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 > efi_call_proto() macro introduced by commit 3552fdf29f01 ("efi: Allow > bitness-agnostic protocol calls"). > > To be able to dereference the device_handle attribute from the > efi_loaded_image_t table in an arch- and bitness-agnostic manner, > introduce the efi_table_attr() macro (which already exists for x86) > to arm and arm64. > > No functional change intended. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > arch/arm/include/asm/efi.h | 3 ++ > arch/arm64/include/asm/efi.h | 3 ++ > arch/x86/boot/compressed/eboot.c | 61 -------------------------- > drivers/firmware/efi/libstub/arm-stub.c | 25 ----------- > drivers/firmware/efi/libstub/efi-stub-helper.c | 29 +++++++++++- > drivers/firmware/efi/libstub/efistub.h | 3 -- > include/linux/efi.h | 10 +++++ > 7 files changed, 43 insertions(+), 91 deletions(-) > > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h > index 17f1f1a..38badaa 100644 > --- a/arch/arm/include/asm/efi.h > +++ b/arch/arm/include/asm/efi.h > @@ -58,6 +58,9 @@ static inline void efi_set_pgd(struct mm_struct *mm) > #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__) > #define efi_is_64bit() (false) > > +#define efi_table_attr(table, attr, instance) \ > + ((table##_t *)instance)->attr > + > #define efi_call_proto(protocol, f, instance, ...) \ > ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__) > > diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h > index c4cd508..5a21037 100644 > --- a/arch/arm64/include/asm/efi.h > +++ b/arch/arm64/include/asm/efi.h > @@ -85,6 +85,9 @@ static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base, > #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__) > #define efi_is_64bit() (true) > > +#define efi_table_attr(table, attr, instance) \ > + ((table##_t *)instance)->attr > + > #define efi_call_proto(protocol, f, instance, ...) \ > ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__) > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index 9a5b6d3..2d2b599 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -41,67 +41,6 @@ __pure const struct efi_config *__efi_early(void) > BOOT_SERVICES(32); > BOOT_SERVICES(64); > > -static inline efi_status_t __open_volume32(void *__image, void **__fh) > -{ > - efi_file_io_interface_t *io; > - efi_loaded_image_32_t *image = __image; > - efi_file_handle_32_t *fh; > - efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; > - efi_status_t status; > - void *handle = (void *)(unsigned long)image->device_handle; > - unsigned long func; > - > - status = efi_call_early(handle_protocol, handle, > - &fs_proto, (void **)&io); > - if (status != EFI_SUCCESS) { > - efi_printk(sys_table, "Failed to handle fs_proto\n"); > - return status; > - } > - > - func = (unsigned long)io->open_volume; > - status = efi_early->call(func, io, &fh); > - if (status != EFI_SUCCESS) > - efi_printk(sys_table, "Failed to open volume\n"); > - > - *__fh = fh; > - return status; > -} > - > -static inline efi_status_t __open_volume64(void *__image, void **__fh) > -{ > - efi_file_io_interface_t *io; > - efi_loaded_image_64_t *image = __image; > - efi_file_handle_64_t *fh; > - efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; > - efi_status_t status; > - void *handle = (void *)(unsigned long)image->device_handle; > - unsigned long func; > - > - status = efi_call_early(handle_protocol, handle, > - &fs_proto, (void **)&io); > - if (status != EFI_SUCCESS) { > - efi_printk(sys_table, "Failed to handle fs_proto\n"); > - return status; > - } > - > - func = (unsigned long)io->open_volume; > - status = efi_early->call(func, io, &fh); > - if (status != EFI_SUCCESS) > - efi_printk(sys_table, "Failed to open volume\n"); > - > - *__fh = fh; > - return status; > -} > - > -efi_status_t > -efi_open_volume(efi_system_table_t *sys_table, void *__image, void **__fh) > -{ > - if (efi_early->is64) > - return __open_volume64(__image, __fh); > - > - return __open_volume32(__image, __fh); > -} > - > void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str) > { > efi_call_proto(efi_simple_text_output_protocol, output_string, > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c > index 01a9d78..e242fb9 100644 > --- a/drivers/firmware/efi/libstub/arm-stub.c > +++ b/drivers/firmware/efi/libstub/arm-stub.c > @@ -40,31 +40,6 @@ > > static u64 virtmap_base = EFI_RT_VIRTUAL_BASE; > > -efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, > - void *__image, void **__fh) > -{ > - efi_file_io_interface_t *io; > - efi_loaded_image_t *image = __image; > - efi_file_handle_t *fh; > - efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; > - efi_status_t status; > - void *handle = (void *)(unsigned long)image->device_handle; > - > - status = sys_table_arg->boottime->handle_protocol(handle, > - &fs_proto, (void **)&io); > - if (status != EFI_SUCCESS) { > - efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); > - return status; > - } > - > - status = io->open_volume(io, &fh); > - if (status != EFI_SUCCESS) > - efi_printk(sys_table_arg, "Failed to open volume\n"); > - > - *__fh = fh; > - return status; > -} > - > void efi_char16_printk(efi_system_table_t *sys_table_arg, > efi_char16_t *str) > { > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index 50a9cab..d399679 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -413,6 +413,32 @@ static efi_status_t efi_file_close(void *handle) > return efi_call_proto(efi_file_handle, close, handle); > } > > +static efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, > + efi_loaded_image_t *image, > + efi_file_handle_t **__fh) > +{ > + efi_file_io_interface_t *io; > + efi_file_handle_t *fh; > + efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID; > + efi_status_t status; > + void *handle = > + (void *)efi_table_attr(efi_loaded_image, device_handle, image); > + > + status = efi_call_early(handle_protocol, handle, > + &fs_proto, (void **)&io); > + if (status != EFI_SUCCESS) { > + efi_printk(sys_table_arg, "Failed to handle fs_proto\n"); > + return status; > + } > + > + status = efi_call_proto(efi_file_io_interface, open_volume, io, &fh); > + if (status != EFI_SUCCESS) > + efi_printk(sys_table_arg, "Failed to open volume\n"); > + > + *__fh = fh; > + return status; > +} > + > /* > * Parse the ASCII string 'cmdline' for EFI options, denoted by the efi= > * option, e.g. efi=nochunk. > @@ -563,8 +589,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, > > /* Only open the volume once. */ > if (!i) { > - status = efi_open_volume(sys_table_arg, image, > - (void **)&fh); > + status = efi_open_volume(sys_table_arg, image, &fh); > if (status != EFI_SUCCESS) > goto free_files; > } > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index f59564b..32799cf 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -36,9 +36,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); > - > unsigned long get_dram_base(efi_system_table_t *sys_table_arg); > > efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, > diff --git a/include/linux/efi.h b/include/linux/efi.h > index d0a6fea..d56b624 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -859,6 +859,16 @@ struct efi_fdt_params { > void *flush; > } efi_file_handle_t; > > +typedef struct { > + u64 revision; > + u32 open_volume; > +} efi_file_io_interface_32_t; > + > +typedef struct { > + u64 revision; > + u64 open_volume; > +} efi_file_io_interface_64_t; > + > typedef struct _efi_file_io_interface { > u64 revision; > int (*open_volume)(struct _efi_file_io_interface *, > -- > 2.18.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 03:54:29PM +0200, Ard Biesheuvel wrote: > Did you have any thoughts about how we could at least detect > situations where we are invoking protocols that are not mixed-mode > safe (i.e., they pass 64-bit quantities by value, which our current > thunking routine does not take into account) efi_thunk_64.S only supports five arguments AFAICS, we'd have to check for each argument passed to efi_call_early() or efi_call_proto() if the corresponding argument in the *_32_t protocol struct has sizeof(u64), and BUILD_BUG_ON() if so. Is it possible to retrieve the n-th element of a struct without knowing its name? Basically offsetof() but with a number instead of a name? We have to check the size of the struct element, not the argument passed in to efi_call_early() / efi_call_proto() because we pass in 64-bit pointers (and possibly also integers) and fully expect that the upper 32-bit are disregarded. Difficult. :-( Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14 July 2018 at 13:41, Lukas Wunner <lukas@wunner.de> wrote: > On Fri, Jul 13, 2018 at 03:54:29PM +0200, Ard Biesheuvel wrote: >> Did you have any thoughts about how we could at least detect >> situations where we are invoking protocols that are not mixed-mode >> safe (i.e., they pass 64-bit quantities by value, which our current >> thunking routine does not take into account) > > efi_thunk_64.S only supports five arguments AFAICS, we'd have to check > for each argument passed to efi_call_early() or efi_call_proto() if > the corresponding argument in the *_32_t protocol struct has sizeof(u64), > and BUILD_BUG_ON() if so. > > Is it possible to retrieve the n-th element of a struct without knowing > its name? Basically offsetof() but with a number instead of a name? > We have to check the size of the struct element, not the argument passed > in to efi_call_early() / efi_call_proto() because we pass in 64-bit > pointers (and possibly also integers) and fully expect that the upper > 32-bit are disregarded. > > Difficult. :-( > Yeah. I think a GCC plugin is the only way to achieve this. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This series contains some fixes and cleanups for mixed-mode UEFI on x86. Patch #1 adds the missing locking in the runtime service wrapper code for mixed mode. This was found by inspection rather than having been reported but could be a candidate for -stable nonetheless. Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci routine. Patches #3 and #4 do the same for the UGA draw protocol discovery routines. Patch #5 fixes a latent bug in the UGA draw code. Patch #6 helps unused code paths to be optimized away on configurations that don't need them (32-bit only and 64-bit only) Ard Biesheuvel (6): efi/x86: prevent reentrant firmware calls in mixed mode efi/x86: merge setup_efi_pci32 and setup_efi_pci64 routines efi/x86: align efi_uga_draw_protocol typedef names to convention efi/x86: merge 32-bit and 64-bit UGA draw protocol setup routines efi/x86: add missing NULL initialization in UGA draw protocol discovery efi/x86: replace references to efi_early->is64 with efi_is_64bit() arch/x86/boot/compressed/eboot.c | 252 +++++--------------- arch/x86/boot/compressed/eboot.h | 12 +- arch/x86/platform/efi/efi_64.c | 101 +++++++- 3 files changed, 170 insertions(+), 195 deletions(-) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html