On 13 July 2018 at 11:57, Lukas Wunner <lukas@xxxxxxxxx> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
