* [PATCH 01/10] efi/libstub: remove unused __efi_call_early() macro
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 02/10] efi/x86: rename efi_is_native() to efi_is_mixed() Ard Biesheuvel
` (8 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
The macro __efi_call_early() is defined by various architectures but
never used. Let's get rid of it.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/arm/include/asm/efi.h | 1 -
arch/arm64/include/asm/efi.h | 1 -
arch/x86/include/asm/efi.h | 3 ---
3 files changed, 5 deletions(-)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 7667826b93f1..2306ed783ceb 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -51,7 +51,6 @@ void efi_virtmap_unload(void);
/* arch specific definitions used by the stub code */
#define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
-#define __efi_call_early(f, ...) f(__VA_ARGS__)
#define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
#define efi_is_64bit() (false)
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index b54d3a86c444..7cfac5e0e310 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -94,7 +94,6 @@ static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
}
#define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
-#define __efi_call_early(f, ...) f(__VA_ARGS__)
#define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
#define efi_is_64bit() (true)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index d028e9acdf1c..59c19e0b6027 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -233,9 +233,6 @@ static inline bool efi_is_64bit(void)
__efi_early()->call(efi_table_attr(efi_boot_services, f, \
__efi_early()->boot_services), __VA_ARGS__)
-#define __efi_call_early(f, ...) \
- __efi_early()->call((unsigned long)f, __VA_ARGS__);
-
#define efi_call_runtime(f, ...) \
__efi_early()->call(efi_table_attr(efi_runtime_services, f, \
__efi_early()->runtime_services), __VA_ARGS__)
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 02/10] efi/x86: rename efi_is_native() to efi_is_mixed()
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 01/10] efi/libstub: remove unused __efi_call_early() macro Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array Ard Biesheuvel
` (7 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
The ARM architecture does not permit combining 32-bit and 64-bit code
at the same privilege level, and so EFI mixed mode is strictly a x86
concept.
In preparation of turning the 32/64 bit distinction in shared stub
code to a native vs mixed one, refactor x86's current use of the
helper function efi_is_native() into efi_is_mixed().
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/include/asm/efi.h | 10 ++++++----
arch/x86/platform/efi/efi.c | 8 ++++----
arch/x86/platform/efi/efi_64.c | 4 ++--
arch/x86/platform/efi/quirks.c | 2 +-
4 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 59c19e0b6027..6094e7f49a99 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -153,17 +153,19 @@ extern u64 efi_setup;
#ifdef CONFIG_EFI
-static inline bool efi_is_native(void)
+static inline bool efi_is_mixed(void)
{
- return IS_ENABLED(CONFIG_X86_64) == efi_enabled(EFI_64BIT);
+ if (!IS_ENABLED(CONFIG_EFI_MIXED))
+ return false;
+ return IS_ENABLED(CONFIG_X86_64) && !efi_enabled(EFI_64BIT);
}
static inline bool efi_runtime_supported(void)
{
- if (efi_is_native())
+ if (!efi_is_mixed())
return true;
- if (IS_ENABLED(CONFIG_EFI_MIXED) && !efi_enabled(EFI_OLD_MEMMAP))
+ if (!efi_enabled(EFI_OLD_MEMMAP))
return true;
return false;
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index bb8e37a723d6..1493e964c267 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -848,7 +848,7 @@ static bool should_map_region(efi_memory_desc_t *md)
* Map all of RAM so that we can access arguments in the 1:1
* mapping when making EFI runtime calls.
*/
- if (IS_ENABLED(CONFIG_EFI_MIXED) && !efi_is_native()) {
+ if (efi_is_mixed()) {
if (md->type == EFI_CONVENTIONAL_MEMORY ||
md->type == EFI_LOADER_DATA ||
md->type == EFI_LOADER_CODE)
@@ -923,7 +923,7 @@ static void __init kexec_enter_virtual_mode(void)
* kexec kernel because in the initial boot something else might
* have been mapped at these virtual addresses.
*/
- if (!efi_is_native() || efi_enabled(EFI_OLD_MEMMAP)) {
+ if (efi_is_mixed() || efi_enabled(EFI_OLD_MEMMAP)) {
efi_memmap_unmap();
clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
return;
@@ -1060,7 +1060,7 @@ static void __init __efi_enter_virtual_mode(void)
efi_sync_low_kernel_mappings();
- if (efi_is_native()) {
+ if (!efi_is_mixed()) {
status = phys_efi_set_virtual_address_map(
efi.memmap.desc_size * count,
efi.memmap.desc_size,
@@ -1091,7 +1091,7 @@ static void __init __efi_enter_virtual_mode(void)
*/
efi.runtime_version = efi_systab.hdr.revision;
- if (efi_is_native())
+ if (!efi_is_mixed())
efi_native_runtime_setup();
else
efi_thunk_runtime_setup();
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 08ce8177c3af..885e50a707a6 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -388,7 +388,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
* text and allocate a new stack because we can't rely on the
* stack pointer being < 4GB.
*/
- if (!IS_ENABLED(CONFIG_EFI_MIXED) || efi_is_native())
+ if (!efi_is_mixed())
return 0;
page = alloc_page(GFP_KERNEL|__GFP_DMA32);
@@ -449,7 +449,7 @@ void __init efi_map_region(efi_memory_desc_t *md)
* booting in EFI mixed mode, because even though we may be
* running a 64-bit kernel, the firmware may only be 32-bit.
*/
- if (!efi_is_native () && IS_ENABLED(CONFIG_EFI_MIXED)) {
+ if (efi_is_mixed()) {
md->virt_addr = md->phys_addr;
return;
}
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 7675cf754d90..84d7176983d2 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -397,7 +397,7 @@ static void __init efi_unmap_pages(efi_memory_desc_t *md)
* EFI runtime calls, hence don't unmap EFI boot services code/data
* regions.
*/
- if (!efi_is_native())
+ if (efi_is_mixed())
return;
if (kernel_unmap_pages_in_pgd(pgd, pa, md->num_pages))
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 01/10] efi/libstub: remove unused __efi_call_early() macro Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 02/10] efi/x86: rename efi_is_native() to efi_is_mixed() Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-14 20:32 ` Arvind Sankar
2019-12-14 17:57 ` [PATCH 04/10] efi/libstub: add missing apple_properties_protocol_t definition Ard Biesheuvel
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
Iterating over a EFI handle array is a bit finicky, since we have
to take mixed mode into account, where handles are only 32-bit
while the native efi_handle_t type is 64-bit.
So introduce a helper, and replace the various occurrences of
this pattern.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/boot/compressed/eboot.c | 14 +++++---------
drivers/firmware/efi/libstub/gop.c | 9 ++-------
drivers/firmware/efi/libstub/pci.c | 9 +++------
include/linux/efi.h | 10 ++++++++++
4 files changed, 20 insertions(+), 22 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 0f1edd2c49fc..34962f2f3337 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -135,6 +135,7 @@ static void setup_efi_pci(struct boot_params *params)
unsigned long size = 0;
unsigned long nr_pci;
struct setup_data *data;
+ efi_handle_t h;
int i;
efi_pci_disable_bridge_busmaster(sys_table);
@@ -166,14 +167,11 @@ static void setup_efi_pci(struct boot_params *params)
while (data && data->next)
data = (struct setup_data *)(unsigned long)data->next;
- nr_pci = size / (efi_is_64bit() ? sizeof(u64) : sizeof(u32));
- for (i = 0; i < nr_pci; i++) {
+ for_each_efi_handle(h, pci_handle, size, i) {
efi_pci_io_protocol_t *pci = NULL;
struct pci_setup_rom *rom;
- status = efi_call_early(handle_protocol,
- efi_is_64bit() ? ((u64 *)pci_handle)[i]
- : ((u32 *)pci_handle)[i],
+ status = efi_call_early(handle_protocol, h,
&pci_proto, (void **)&pci);
if (status != EFI_SUCCESS || !pci)
continue;
@@ -268,6 +266,7 @@ setup_uga(struct screen_info *si, efi_guid_t *uga_proto, unsigned long size)
void **uga_handle = NULL;
efi_uga_draw_protocol_t *uga = NULL, *first_uga;
unsigned long nr_ugas;
+ efi_handle_t handle;
int i;
status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
@@ -285,13 +284,10 @@ setup_uga(struct screen_info *si, efi_guid_t *uga_proto, unsigned long size)
width = 0;
first_uga = NULL;
- nr_ugas = size / (efi_is_64bit() ? sizeof(u64) : sizeof(u32));
- for (i = 0; i < nr_ugas; i++) {
+ for_each_efi_handle(handle, uga_handle, size, i) {
efi_guid_t pciio_proto = EFI_PCI_IO_PROTOCOL_GUID;
u32 w, h, depth, refresh;
void *pciio;
- unsigned long handle = efi_is_64bit() ? ((u64 *)uga_handle)[i]
- : ((u32 *)uga_handle)[i];
status = efi_call_early(handle_protocol, handle,
uga_proto, (void **)&uga);
diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 94045ab7dd3d..5f4fbc2ac687 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -91,7 +91,6 @@ setup_gop(efi_system_table_t *sys_table_arg, struct screen_info *si,
efi_guid_t *proto, unsigned long size, void **handles)
{
efi_graphics_output_protocol_t *gop, *first_gop;
- unsigned long nr_gops;
u16 width, height;
u32 pixels_per_scan_line;
u32 ext_lfb_base;
@@ -99,22 +98,18 @@ setup_gop(efi_system_table_t *sys_table_arg, struct screen_info *si,
efi_pixel_bitmask_t pixel_info;
int pixel_format;
efi_status_t status;
+ efi_handle_t h;
int i;
- bool is64 = efi_is_64bit();
first_gop = NULL;
gop = NULL;
- nr_gops = size / (is64 ? sizeof(u64) : sizeof(u32));
- for (i = 0; i < nr_gops; i++) {
+ for_each_efi_handle(h, handles, size, i) {
efi_graphics_output_protocol_mode_t *mode;
efi_graphics_output_mode_info_t *info = NULL;
efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
bool conout_found = false;
void *dummy = NULL;
- efi_handle_t h = (efi_handle_t)(unsigned long)
- (is64 ? ((u64 *)handles)[i]
- : ((u32 *)handles)[i]);
efi_physical_addr_t current_fb_base;
status = efi_call_early(handle_protocol, h,
diff --git a/drivers/firmware/efi/libstub/pci.c b/drivers/firmware/efi/libstub/pci.c
index f792b241383d..d144551e36eb 100644
--- a/drivers/firmware/efi/libstub/pci.c
+++ b/drivers/firmware/efi/libstub/pci.c
@@ -19,8 +19,8 @@ void efi_pci_disable_bridge_busmaster(efi_system_table_t *sys_table_arg)
void **pci_handle = NULL;
efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
unsigned long size = 0;
- unsigned long nr_pci;
u16 class, command;
+ efi_handle_t handle;
int i;
if (!nobusmaster())
@@ -49,13 +49,10 @@ void efi_pci_disable_bridge_busmaster(efi_system_table_t *sys_table_arg)
if (status != EFI_SUCCESS)
goto free_handle;
- nr_pci = size / (efi_is_64bit() ? sizeof(u64) : sizeof(u32));
- for (i = 0; i < nr_pci; i++) {
+ for_each_efi_handle(handle, pci_handle, size, i) {
efi_pci_io_protocol_t *pci = NULL;
- unsigned long handle = efi_is_64bit() ? ((u64 *)pci_handle)[i]
- : ((u32 *)pci_handle)[i];
- status = efi_call_early(handle_protocol, (efi_handle_t)handle,
+ status = efi_call_early(handle_protocol, handle,
&pci_proto, (void **)&pci);
if (status != EFI_SUCCESS || !pci)
continue;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index de2087149089..d7ca0b85b2b5 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -48,6 +48,16 @@ typedef u16 efi_char16_t; /* UNICODE character */
typedef u64 efi_physical_addr_t;
typedef void *efi_handle_t;
+#define for_each_efi_handle(handle, array, size, i) \
+ for (i = 1, handle = efi_is_64bit() \
+ ? (efi_handle_t)(unsigned long)((u64 *)(array))[0] \
+ : (efi_handle_t)(unsigned long)((u32 *)(array))[0]; \
+ i++ <= (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
+ : sizeof(u32)); \
+ handle = efi_is_64bit() \
+ ? (efi_handle_t)(unsigned long)((u64 *)(array))[i] \
+ : (efi_handle_t)(unsigned long)((u32 *)(array))[i])
+
/*
* The UEFI spec and EDK2 reference implementation both define EFI_GUID as
* struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array
2019-12-14 17:57 ` [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array Ard Biesheuvel
@ 2019-12-14 20:32 ` Arvind Sankar
2019-12-14 20:40 ` Ard Biesheuvel
0 siblings, 1 reply; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 20:32 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel, linux-efi, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
On Sat, Dec 14, 2019 at 06:57:28PM +0100, Ard Biesheuvel wrote:
> Iterating over a EFI handle array is a bit finicky, since we have
> to take mixed mode into account, where handles are only 32-bit
> while the native efi_handle_t type is 64-bit.
>
> So introduce a helper, and replace the various occurrences of
> this pattern.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>
> +#define for_each_efi_handle(handle, array, size, i) \
> + for (i = 1, handle = efi_is_64bit() \
> + ? (efi_handle_t)(unsigned long)((u64 *)(array))[0] \
> + : (efi_handle_t)(unsigned long)((u32 *)(array))[0]; \
> + i++ <= (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> + : sizeof(u32)); \
> + handle = efi_is_64bit() \
> + ? (efi_handle_t)(unsigned long)((u64 *)(array))[i] \
> + : (efi_handle_t)(unsigned long)((u32 *)(array))[i])
> +
> /*
> * The UEFI spec and EDK2 reference implementation both define EFI_GUID as
> * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
> --
> 2.17.1
>
This would access one past the array, no? Eg if the array has one
handle, i is incremented to 2 the first time the condition is checked,
then the loop increment will access array[2] before the condition is
checked again. There seem to be at least a couple of other for_each
macros that might have similar issues.
How about the below instead?
#define for_each_efi_handle(handle, array, size, i) \
for (i = 0; \
(i < (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
: sizeof(u32))) && \
((handle = efi_is_64bit() \
? ((efi_handle_t *)(array))[i] \
: (efi_handle_t)(unsigned long)((u32 *)(array))[i]), 1);\
i++)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array
2019-12-14 20:32 ` Arvind Sankar
@ 2019-12-14 20:40 ` Ard Biesheuvel
2019-12-14 21:04 ` Ard Biesheuvel
2019-12-14 21:07 ` Arvind Sankar
0 siblings, 2 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 20:40 UTC (permalink / raw)
To: Arvind Sankar
Cc: Ard Biesheuvel, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Matthew Garrett, Ingo Molnar, Andy Lutomirski,
Thomas Gleixner
On Sat, 14 Dec 2019 at 21:33, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Sat, Dec 14, 2019 at 06:57:28PM +0100, Ard Biesheuvel wrote:
> > Iterating over a EFI handle array is a bit finicky, since we have
> > to take mixed mode into account, where handles are only 32-bit
> > while the native efi_handle_t type is 64-bit.
> >
> > So introduce a helper, and replace the various occurrences of
> > this pattern.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >
> > +#define for_each_efi_handle(handle, array, size, i) \
> > + for (i = 1, handle = efi_is_64bit() \
> > + ? (efi_handle_t)(unsigned long)((u64 *)(array))[0] \
> > + : (efi_handle_t)(unsigned long)((u32 *)(array))[0]; \
> > + i++ <= (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> > + : sizeof(u32)); \
> > + handle = efi_is_64bit() \
> > + ? (efi_handle_t)(unsigned long)((u64 *)(array))[i] \
> > + : (efi_handle_t)(unsigned long)((u32 *)(array))[i])
> > +
> > /*
> > * The UEFI spec and EDK2 reference implementation both define EFI_GUID as
> > * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
> > --
> > 2.17.1
> >
>
> This would access one past the array, no? Eg if the array has one
> handle, i is incremented to 2 the first time the condition is checked,
> then the loop increment will access array[2] before the condition is
> checked again. There seem to be at least a couple of other for_each
> macros that might have similar issues.
>
Indeed.
> How about the below instead?
>
> #define for_each_efi_handle(handle, array, size, i) \
> for (i = 0; \
> (i < (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> : sizeof(u32))) && \
> ((handle = efi_is_64bit() \
> ? ((efi_handle_t *)(array))[i] \
> : (efi_handle_t)(unsigned long)((u32 *)(array))[i]), 1);\
> i++)
>
Yeah, that looks correct to me, but perhaps we can come up with
something slightly more readable? :-)
(Not saying my code was better in that respect)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array
2019-12-14 20:40 ` Ard Biesheuvel
@ 2019-12-14 21:04 ` Ard Biesheuvel
2019-12-14 21:11 ` Arvind Sankar
2019-12-14 21:07 ` Arvind Sankar
1 sibling, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 21:04 UTC (permalink / raw)
To: Arvind Sankar
Cc: Ard Biesheuvel, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Matthew Garrett, Ingo Molnar, Andy Lutomirski,
Thomas Gleixner
On Sat, 14 Dec 2019 at 21:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Sat, 14 Dec 2019 at 21:33, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Sat, Dec 14, 2019 at 06:57:28PM +0100, Ard Biesheuvel wrote:
> > > Iterating over a EFI handle array is a bit finicky, since we have
> > > to take mixed mode into account, where handles are only 32-bit
> > > while the native efi_handle_t type is 64-bit.
> > >
> > > So introduce a helper, and replace the various occurrences of
> > > this pattern.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >
> > > +#define for_each_efi_handle(handle, array, size, i) \
> > > + for (i = 1, handle = efi_is_64bit() \
> > > + ? (efi_handle_t)(unsigned long)((u64 *)(array))[0] \
> > > + : (efi_handle_t)(unsigned long)((u32 *)(array))[0]; \
> > > + i++ <= (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> > > + : sizeof(u32)); \
> > > + handle = efi_is_64bit() \
> > > + ? (efi_handle_t)(unsigned long)((u64 *)(array))[i] \
> > > + : (efi_handle_t)(unsigned long)((u32 *)(array))[i])
> > > +
> > > /*
> > > * The UEFI spec and EDK2 reference implementation both define EFI_GUID as
> > > * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
> > > --
> > > 2.17.1
> > >
> >
> > This would access one past the array, no? Eg if the array has one
> > handle, i is incremented to 2 the first time the condition is checked,
> > then the loop increment will access array[2] before the condition is
> > checked again. There seem to be at least a couple of other for_each
> > macros that might have similar issues.
> >
>
> Indeed.
>
> > How about the below instead?
> >
> > #define for_each_efi_handle(handle, array, size, i) \
> > for (i = 0; \
> > (i < (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> > : sizeof(u32))) && \
> > ((handle = efi_is_64bit() \
> > ? ((efi_handle_t *)(array))[i] \
> > : (efi_handle_t)(unsigned long)((u32 *)(array))[i]), 1);\
> > i++)
> >
>
> Yeah, that looks correct to me, but perhaps we can come up with
> something slightly more readable? :-)
> (Not saying my code was better in that respect)
How about
#define efi_get_handle_at(array, idx) \
(efi_is_64bit() ? (efi_handle_t)(unsigned long)((u64 *)(array))[idx] \
: (efi_handle_t)(unsigned long)((u32 *)(array))[i])
#define efi_get_handle_num(size) \
((size) / (efi_is_64bit() ? sizeof(u64) : sizeof(u32)))
#define for_each_efi_handle(handle, array, size, i) \
for (i = 0; \
i < efi_get_handle_num(size) && \
((handle = efi_get_handle_at((array), i)) || true); \
i++)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array
2019-12-14 21:04 ` Ard Biesheuvel
@ 2019-12-14 21:11 ` Arvind Sankar
0 siblings, 0 replies; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 21:11 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arvind Sankar, Ard Biesheuvel, Linux Kernel Mailing List,
linux-efi, Hans de Goede, Matthew Garrett, Ingo Molnar,
Andy Lutomirski, Thomas Gleixner
On Sat, Dec 14, 2019 at 09:04:10PM +0000, Ard Biesheuvel wrote:
> On Sat, 14 Dec 2019 at 21:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Sat, 14 Dec 2019 at 21:33, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Sat, Dec 14, 2019 at 06:57:28PM +0100, Ard Biesheuvel wrote:
> > > > Iterating over a EFI handle array is a bit finicky, since we have
> > > > to take mixed mode into account, where handles are only 32-bit
> > > > while the native efi_handle_t type is 64-bit.
> > > >
> > > > So introduce a helper, and replace the various occurrences of
> > > > this pattern.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > ---
> > > >
> > > > +#define for_each_efi_handle(handle, array, size, i) \
> > > > + for (i = 1, handle = efi_is_64bit() \
> > > > + ? (efi_handle_t)(unsigned long)((u64 *)(array))[0] \
> > > > + : (efi_handle_t)(unsigned long)((u32 *)(array))[0]; \
> > > > + i++ <= (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> > > > + : sizeof(u32)); \
> > > > + handle = efi_is_64bit() \
> > > > + ? (efi_handle_t)(unsigned long)((u64 *)(array))[i] \
> > > > + : (efi_handle_t)(unsigned long)((u32 *)(array))[i])
> > > > +
> > > > /*
> > > > * The UEFI spec and EDK2 reference implementation both define EFI_GUID as
> > > > * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
> > > > --
> > > > 2.17.1
> > > >
> > >
> > > This would access one past the array, no? Eg if the array has one
> > > handle, i is incremented to 2 the first time the condition is checked,
> > > then the loop increment will access array[2] before the condition is
> > > checked again. There seem to be at least a couple of other for_each
> > > macros that might have similar issues.
> > >
> >
> > Indeed.
> >
> > > How about the below instead?
> > >
> > > #define for_each_efi_handle(handle, array, size, i) \
> > > for (i = 0; \
> > > (i < (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> > > : sizeof(u32))) && \
> > > ((handle = efi_is_64bit() \
> > > ? ((efi_handle_t *)(array))[i] \
> > > : (efi_handle_t)(unsigned long)((u32 *)(array))[i]), 1);\
> > > i++)
> > >
> >
> > Yeah, that looks correct to me, but perhaps we can come up with
> > something slightly more readable? :-)
> > (Not saying my code was better in that respect)
>
> How about
>
> #define efi_get_handle_at(array, idx) \
> (efi_is_64bit() ? (efi_handle_t)(unsigned long)((u64 *)(array))[idx] \
> : (efi_handle_t)(unsigned long)((u32 *)(array))[i])
>
>
> #define efi_get_handle_num(size) \
> ((size) / (efi_is_64bit() ? sizeof(u64) : sizeof(u32)))
>
> #define for_each_efi_handle(handle, array, size, i) \
> for (i = 0; \
> i < efi_get_handle_num(size) && \
> ((handle = efi_get_handle_at((array), i)) || true); \
> i++)
Heh, I came up with almost the same thing, but yours is slightly better,
You have a typo in efi_get_handle_at (i instead of idx in the second
line).
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array
2019-12-14 20:40 ` Ard Biesheuvel
2019-12-14 21:04 ` Ard Biesheuvel
@ 2019-12-14 21:07 ` Arvind Sankar
1 sibling, 0 replies; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 21:07 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arvind Sankar, Ard Biesheuvel, Linux Kernel Mailing List,
linux-efi, Hans de Goede, Matthew Garrett, Ingo Molnar,
Andy Lutomirski, Thomas Gleixner
On Sat, Dec 14, 2019 at 08:40:57PM +0000, Ard Biesheuvel wrote:
> On Sat, 14 Dec 2019 at 21:33, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Sat, Dec 14, 2019 at 06:57:28PM +0100, Ard Biesheuvel wrote:
> > > Iterating over a EFI handle array is a bit finicky, since we have
> > > to take mixed mode into account, where handles are only 32-bit
> > > while the native efi_handle_t type is 64-bit.
> > >
> > > So introduce a helper, and replace the various occurrences of
> > > this pattern.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >
> > > +#define for_each_efi_handle(handle, array, size, i) \
> > > + for (i = 1, handle = efi_is_64bit() \
> > > + ? (efi_handle_t)(unsigned long)((u64 *)(array))[0] \
> > > + : (efi_handle_t)(unsigned long)((u32 *)(array))[0]; \
> > > + i++ <= (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> > > + : sizeof(u32)); \
> > > + handle = efi_is_64bit() \
> > > + ? (efi_handle_t)(unsigned long)((u64 *)(array))[i] \
> > > + : (efi_handle_t)(unsigned long)((u32 *)(array))[i])
> > > +
> > > /*
> > > * The UEFI spec and EDK2 reference implementation both define EFI_GUID as
> > > * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
> > > --
> > > 2.17.1
> > >
> >
> > This would access one past the array, no? Eg if the array has one
> > handle, i is incremented to 2 the first time the condition is checked,
> > then the loop increment will access array[2] before the condition is
> > checked again. There seem to be at least a couple of other for_each
> > macros that might have similar issues.
> >
>
> Indeed.
>
> > How about the below instead?
> >
> > #define for_each_efi_handle(handle, array, size, i) \
> > for (i = 0; \
> > (i < (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
> > : sizeof(u32))) && \
> > ((handle = efi_is_64bit() \
> > ? ((efi_handle_t *)(array))[i] \
> > : (efi_handle_t)(unsigned long)((u32 *)(array))[i]), 1);\
> > i++)
> >
>
> Yeah, that looks correct to me, but perhaps we can come up with
> something slightly more readable? :-)
> (Not saying my code was better in that respect)
:) The idiom of the && with , operator is copied from for_each_bvec in bvec.h.
Perhaps more readable with helper macros:
#define __efi_handle_size (efi_is_64bit() ? sizeof(efi_handle_t) \
: sizeof(u32))
#define __efi_handle_elem(array, i) \
(efi_is_64bit() ? ((efi_handle_t *)(array))[i] \
: (efi_handle_t)(uintptr_t)((u32 *)(array))[i])
#define for_each_efi_handle(handle, array, size, i) \
for (i = 0; i < (size) / __efi_handle_size && \
(handle = __efi_handle_elem(array, i), 1); i++)
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 04/10] efi/libstub: add missing apple_properties_protocol_t definition
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
` (2 preceding siblings ...)
2019-12-14 17:57 ` [PATCH 03/10] efi/libstub: use a helper to iterate over a EFI handle array Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit Ard Biesheuvel
` (5 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
As apple_properties_protocol_t is only used on x86, we never bothered
to define the native apple_properties_protocol_t struct, but only added
the explicit 32-bit and 64-bit ones. We'll need the native one for the
next patch so let's add it, based on the prototypes that can be found
in commit 58c5475aba67706b31d9237808d5d3d54074e5ea.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
include/linux/efi.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index d7ca0b85b2b5..735388ea7012 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -518,6 +518,22 @@ typedef struct {
u64 get_all;
} apple_properties_protocol_64_t;
+struct efi_dev_path;
+
+typedef struct apple_properties_protocol {
+ unsigned long version;
+ efi_status_t (*get)(struct apple_properties_protocol *,
+ struct efi_dev_path *, efi_char16_t *,
+ void *, u32 *);
+ efi_status_t (*set)(struct apple_properties_protocol *,
+ struct efi_dev_path *, efi_char16_t *,
+ void *, u32);
+ efi_status_t (*del)(struct apple_properties_protocol *,
+ struct efi_dev_path *, efi_char16_t *);
+ efi_status_t (*get_all)(struct apple_properties_protocol *,
+ void *buffer, u32 *);
+} apple_properties_protocol_t;
+
typedef struct {
u32 get_capability;
u32 get_event_log;
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
` (3 preceding siblings ...)
2019-12-14 17:57 ` [PATCH 04/10] efi/libstub: add missing apple_properties_protocol_t definition Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-14 19:46 ` Arvind Sankar
2019-12-14 17:57 ` [PATCH 06/10] efi/libstub/x86: use mixed mode helpers to populate efi_config Ard Biesheuvel
` (4 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
Currently, we support mixed mode by casting all boot time firmware
calls to 64-bit explicitly on native 64-bit systems, and to 32-bit
on 32-bit systems or 64-bit systems running with 32-bit firmware.
Due to this explicit awareness of the bitness in the code, we do a
lot of casting even on generic code that is shared with other
architectures, where mixed mode does not even exist. This casting
leads to loss of coverage of type checking by the compiler, which
we should try to avoid.
So instead of distinguishing between 32-bit vs 64-bit, distinguish
between native vs mixed, and limit all the nasty casting and
pointer mangling to the code that actually deals with mixed mode.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/arm/include/asm/efi.h | 2 +-
arch/arm64/include/asm/efi.h | 2 +-
arch/x86/include/asm/efi.h | 31 +++++++++++----
drivers/firmware/efi/libstub/efi-stub-helper.c | 41 +++++++-------------
include/linux/efi.h | 10 ++---
5 files changed, 44 insertions(+), 42 deletions(-)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 2306ed783ceb..9b0c64c28bff 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -52,7 +52,7 @@ void efi_virtmap_unload(void);
#define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
#define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
-#define efi_is_64bit() (false)
+#define efi_is_native() (true)
#define efi_table_attr(table, attr, instance) \
((table##_t *)instance)->attr
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 7cfac5e0e310..189082c44c28 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -95,7 +95,7 @@ static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
#define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
#define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
-#define efi_is_64bit() (true)
+#define efi_is_native() (true)
#define efi_table_attr(table, attr, instance) \
((table##_t *)instance)->attr
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 6094e7f49a99..37364c43296e 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -222,21 +222,38 @@ static inline bool efi_is_64bit(void)
return __efi_early()->is64;
}
-#define efi_table_attr(table, attr, instance) \
- (efi_is_64bit() ? \
- ((table##_64_t *)(unsigned long)instance)->attr : \
- ((table##_32_t *)(unsigned long)instance)->attr)
+static inline bool efi_is_native(void)
+{
+ if (!IS_ENABLED(CONFIG_X86_64))
+ return true;
+ return efi_is_64bit();
+}
+
+#define efi_table_attr(table, attr, instance) ({ \
+ __typeof__(((table##_t *)0)->attr) __ret; \
+ if (efi_is_native()) { \
+ __ret = ((table##_t *)instance)->attr; \
+ } else { \
+ __typeof__(((table##_32_t *)0)->attr) at; \
+ at = (((table##_32_t *)(unsigned long)instance)->attr); \
+ __ret = (__typeof__(__ret))(unsigned long)at; \
+ } \
+ __ret; \
+})
#define efi_call_proto(protocol, f, instance, ...) \
- __efi_early()->call(efi_table_attr(protocol, f, instance), \
+ __efi_early()->call((unsigned long) \
+ efi_table_attr(protocol, f, instance), \
instance, ##__VA_ARGS__)
#define efi_call_early(f, ...) \
- __efi_early()->call(efi_table_attr(efi_boot_services, f, \
+ __efi_early()->call((unsigned long) \
+ efi_table_attr(efi_boot_services, f, \
__efi_early()->boot_services), __VA_ARGS__)
#define efi_call_runtime(f, ...) \
- __efi_early()->call(efi_table_attr(efi_runtime_services, f, \
+ __efi_early()->call((unsigned long) \
+ efi_table_attr(efi_runtime_services, f, \
__efi_early()->runtime_services), __VA_ARGS__)
extern bool efi_reboot_required(void);
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 7e39f443ca30..ff3266b9f673 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -981,33 +981,20 @@ efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table_arg,
return status;
}
-#define GET_EFI_CONFIG_TABLE(bits) \
-static void *get_efi_config_table##bits(efi_system_table_t *_sys_table, \
- efi_guid_t guid) \
-{ \
- efi_system_table_##bits##_t *sys_table; \
- efi_config_table_##bits##_t *tables; \
- int i; \
- \
- sys_table = (typeof(sys_table))_sys_table; \
- tables = (typeof(tables))(unsigned long)sys_table->tables; \
- \
- for (i = 0; i < sys_table->nr_tables; i++) { \
- if (efi_guidcmp(tables[i].guid, guid) != 0) \
- continue; \
- \
- return (void *)(unsigned long)tables[i].table; \
- } \
- \
- return NULL; \
-}
-GET_EFI_CONFIG_TABLE(32)
-GET_EFI_CONFIG_TABLE(64)
-
void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid)
{
- if (efi_is_64bit())
- return get_efi_config_table64(sys_table, guid);
- else
- return get_efi_config_table32(sys_table, guid);
+ unsigned long tables = efi_table_attr(efi_system_table, tables, sys_table);
+ int nr_tables = efi_table_attr(efi_system_table, nr_tables, sys_table);
+ int i;
+
+ for (i = 0; i < nr_tables; i++) {
+ efi_config_table_t *t = (void *)tables;
+
+ if (efi_guidcmp(t->guid, guid) == 0)
+ return efi_table_attr(efi_config_table, table, t);
+
+ tables += efi_is_native() ? sizeof(efi_config_table_t)
+ : sizeof(efi_config_table_32_t);
+ }
+ return NULL;
}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 735388ea7012..7d7ea32a9990 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -49,13 +49,11 @@ typedef u64 efi_physical_addr_t;
typedef void *efi_handle_t;
#define for_each_efi_handle(handle, array, size, i) \
- for (i = 1, handle = efi_is_64bit() \
- ? (efi_handle_t)(unsigned long)((u64 *)(array))[0] \
+ for (i = 1, handle = efi_is_native() ? (array)[0] \
: (efi_handle_t)(unsigned long)((u32 *)(array))[0]; \
- i++ <= (size) / (efi_is_64bit() ? sizeof(efi_handle_t) \
+ i++ <= (size) / (efi_is_native() ? sizeof(efi_handle_t) \
: sizeof(u32)); \
- handle = efi_is_64bit() \
- ? (efi_handle_t)(unsigned long)((u64 *)(array))[i] \
+ handle = efi_is_native() ? (array)[i] \
: (efi_handle_t)(unsigned long)((u32 *)(array))[i])
/*
@@ -753,7 +751,7 @@ typedef struct {
typedef struct {
efi_guid_t guid;
- unsigned long table;
+ void *table;
} efi_config_table_t;
typedef struct {
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 17:57 ` [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit Ard Biesheuvel
@ 2019-12-14 19:46 ` Arvind Sankar
2019-12-14 19:49 ` Arvind Sankar
0 siblings, 1 reply; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 19:46 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel, linux-efi, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> +
> +#define efi_table_attr(table, attr, instance) ({ \
> + __typeof__(((table##_t *)0)->attr) __ret; \
> + if (efi_is_native()) { \
> + __ret = ((table##_t *)instance)->attr; \
> + } else { \
> + __typeof__(((table##_32_t *)0)->attr) at; \
> + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> + __ret = (__typeof__(__ret))(unsigned long)at; \
> + } \
> + __ret; \
> +})
The casting of `at' is appropriate if the attr is a pointer type which
needs to be zero-extended to 64-bit, but for other fields it is
unnecessary at best and possibly dangerous. There are probably no
instances currently where it is called for a non-pointer field, but is
it possible to detect if the type is pointer and avoid the cast if not?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 19:46 ` Arvind Sankar
@ 2019-12-14 19:49 ` Arvind Sankar
2019-12-14 19:54 ` Ard Biesheuvel
0 siblings, 1 reply; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 19:49 UTC (permalink / raw)
To: Arvind Sankar
Cc: Ard Biesheuvel, linux-kernel, linux-efi, Hans de Goede,
Matthew Garrett, Ingo Molnar, Andy Lutomirski, Thomas Gleixner
On Sat, Dec 14, 2019 at 02:46:27PM -0500, Arvind Sankar wrote:
> On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> > +
> > +#define efi_table_attr(table, attr, instance) ({ \
> > + __typeof__(((table##_t *)0)->attr) __ret; \
> > + if (efi_is_native()) { \
> > + __ret = ((table##_t *)instance)->attr; \
> > + } else { \
> > + __typeof__(((table##_32_t *)0)->attr) at; \
> > + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> > + __ret = (__typeof__(__ret))(unsigned long)at; \
> > + } \
> > + __ret; \
> > +})
>
> The casting of `at' is appropriate if the attr is a pointer type which
> needs to be zero-extended to 64-bit, but for other fields it is
> unnecessary at best and possibly dangerous. There are probably no
> instances currently where it is called for a non-pointer field, but is
> it possible to detect if the type is pointer and avoid the cast if not?
To clarify, I mean the casting via `unsigned long' -- casting to type of
__ret should be ok. We could also use uintptr_t for cleanliness when the
cast is required?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 19:49 ` Arvind Sankar
@ 2019-12-14 19:54 ` Ard Biesheuvel
2019-12-14 20:13 ` Arvind Sankar
0 siblings, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 19:54 UTC (permalink / raw)
To: Arvind Sankar
Cc: Ard Biesheuvel, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Matthew Garrett, Ingo Molnar, Andy Lutomirski,
Thomas Gleixner
On Sat, 14 Dec 2019 at 20:49, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Sat, Dec 14, 2019 at 02:46:27PM -0500, Arvind Sankar wrote:
> > On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> > > +
> > > +#define efi_table_attr(table, attr, instance) ({ \
> > > + __typeof__(((table##_t *)0)->attr) __ret; \
> > > + if (efi_is_native()) { \
> > > + __ret = ((table##_t *)instance)->attr; \
> > > + } else { \
> > > + __typeof__(((table##_32_t *)0)->attr) at; \
> > > + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> > > + __ret = (__typeof__(__ret))(unsigned long)at; \
> > > + } \
> > > + __ret; \
> > > +})
> >
> > The casting of `at' is appropriate if the attr is a pointer type which
> > needs to be zero-extended to 64-bit, but for other fields it is
> > unnecessary at best and possibly dangerous. There are probably no
> > instances currently where it is called for a non-pointer field, but is
> > it possible to detect if the type is pointer and avoid the cast if not?
>
> To clarify, I mean the casting via `unsigned long' -- casting to type of
> __ret should be ok. We could also use uintptr_t for cleanliness when the
> cast is required?
Could you give an example of how it could break?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 19:54 ` Ard Biesheuvel
@ 2019-12-14 20:13 ` Arvind Sankar
2019-12-14 20:27 ` Ard Biesheuvel
0 siblings, 1 reply; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 20:13 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arvind Sankar, Ard Biesheuvel, Linux Kernel Mailing List,
linux-efi, Hans de Goede, Matthew Garrett, Ingo Molnar,
Andy Lutomirski, Thomas Gleixner
On Sat, Dec 14, 2019 at 07:54:25PM +0000, Ard Biesheuvel wrote:
> On Sat, 14 Dec 2019 at 20:49, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Sat, Dec 14, 2019 at 02:46:27PM -0500, Arvind Sankar wrote:
> > > On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> > > > +
> > > > +#define efi_table_attr(table, attr, instance) ({ \
> > > > + __typeof__(((table##_t *)0)->attr) __ret; \
> > > > + if (efi_is_native()) { \
> > > > + __ret = ((table##_t *)instance)->attr; \
> > > > + } else { \
> > > > + __typeof__(((table##_32_t *)0)->attr) at; \
> > > > + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> > > > + __ret = (__typeof__(__ret))(unsigned long)at; \
> > > > + } \
> > > > + __ret; \
> > > > +})
> > >
> > > The casting of `at' is appropriate if the attr is a pointer type which
> > > needs to be zero-extended to 64-bit, but for other fields it is
> > > unnecessary at best and possibly dangerous. There are probably no
> > > instances currently where it is called for a non-pointer field, but is
> > > it possible to detect if the type is pointer and avoid the cast if not?
> >
> > To clarify, I mean the casting via `unsigned long' -- casting to type of
> > __ret should be ok. We could also use uintptr_t for cleanliness when the
> > cast is required?
>
> Could you give an example of how it could break?
eg, if the field is actually a structure. Nobody seems to do this
currently, but say for
efi_table_attr(efi_boot_services, hdr, instance)
you shouldn't cast hdr to unsigned long.
There's also the case of nested 32/64-bit structures that breaks, but
that might be too hard to try to handle:
efi_table_attr(efi_pci_io_protocol, io, instance)
where io is a structure of two pointers which would need to be
individually casted. It's properly accessible as io.read/io.write
though.
In general though, everything is either a pointer or a u64, so if it's
too messy to detect pointer type, the cast is still probably safe in
practice.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 20:13 ` Arvind Sankar
@ 2019-12-14 20:27 ` Ard Biesheuvel
2019-12-14 21:17 ` Arvind Sankar
0 siblings, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 20:27 UTC (permalink / raw)
To: Arvind Sankar
Cc: Ard Biesheuvel, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Matthew Garrett, Ingo Molnar, Andy Lutomirski,
Thomas Gleixner
On Sat, 14 Dec 2019 at 21:13, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Sat, Dec 14, 2019 at 07:54:25PM +0000, Ard Biesheuvel wrote:
> > On Sat, 14 Dec 2019 at 20:49, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Sat, Dec 14, 2019 at 02:46:27PM -0500, Arvind Sankar wrote:
> > > > On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> > > > > +
> > > > > +#define efi_table_attr(table, attr, instance) ({ \
> > > > > + __typeof__(((table##_t *)0)->attr) __ret; \
> > > > > + if (efi_is_native()) { \
> > > > > + __ret = ((table##_t *)instance)->attr; \
> > > > > + } else { \
> > > > > + __typeof__(((table##_32_t *)0)->attr) at; \
> > > > > + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> > > > > + __ret = (__typeof__(__ret))(unsigned long)at; \
> > > > > + } \
> > > > > + __ret; \
> > > > > +})
> > > >
> > > > The casting of `at' is appropriate if the attr is a pointer type which
> > > > needs to be zero-extended to 64-bit, but for other fields it is
> > > > unnecessary at best and possibly dangerous. There are probably no
> > > > instances currently where it is called for a non-pointer field, but is
> > > > it possible to detect if the type is pointer and avoid the cast if not?
> > >
> > > To clarify, I mean the casting via `unsigned long' -- casting to type of
> > > __ret should be ok. We could also use uintptr_t for cleanliness when the
> > > cast is required?
> >
> > Could you give an example of how it could break?
>
> eg, if the field is actually a structure. Nobody seems to do this
> currently, but say for
> efi_table_attr(efi_boot_services, hdr, instance)
> you shouldn't cast hdr to unsigned long.
>
Yes. the efi_guid_t in efi_config_table_t is another example - I had
to work around that in get_efi_config_table().
> There's also the case of nested 32/64-bit structures that breaks, but
> that might be too hard to try to handle:
> efi_table_attr(efi_pci_io_protocol, io, instance)
> where io is a structure of two pointers which would need to be
> individually casted. It's properly accessible as io.read/io.write
> though.
>
Yes. We already have calls to pci.read that use this.
> In general though, everything is either a pointer or a u64, so if it's
> too messy to detect pointer type, the cast is still probably safe in
> practice.
Yes. I'm open to suggestions on how to improve this, but mixed mode is
somewhat of a maintenance burden, so if new future functionality needs
to leave mixed mode behind, I'm not too bothered.
Thanks,
Ard.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 20:27 ` Ard Biesheuvel
@ 2019-12-14 21:17 ` Arvind Sankar
2019-12-14 21:30 ` Ard Biesheuvel
0 siblings, 1 reply; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 21:17 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arvind Sankar, Ard Biesheuvel, Linux Kernel Mailing List,
linux-efi, Hans de Goede, Matthew Garrett, Ingo Molnar,
Andy Lutomirski, Thomas Gleixner
On Sat, Dec 14, 2019 at 08:27:50PM +0000, Ard Biesheuvel wrote:
> On Sat, 14 Dec 2019 at 21:13, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Sat, Dec 14, 2019 at 07:54:25PM +0000, Ard Biesheuvel wrote:
> > > On Sat, 14 Dec 2019 at 20:49, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On Sat, Dec 14, 2019 at 02:46:27PM -0500, Arvind Sankar wrote:
> > > > > On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> > > > > > +
> > > > > > +#define efi_table_attr(table, attr, instance) ({ \
> > > > > > + __typeof__(((table##_t *)0)->attr) __ret; \
> > > > > > + if (efi_is_native()) { \
> > > > > > + __ret = ((table##_t *)instance)->attr; \
> > > > > > + } else { \
> > > > > > + __typeof__(((table##_32_t *)0)->attr) at; \
> > > > > > + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> > > > > > + __ret = (__typeof__(__ret))(unsigned long)at; \
> > > > > > + } \
> > > > > > + __ret; \
> > > > > > +})
> > > > >
> Yes. I'm open to suggestions on how to improve this, but mixed mode is
> somewhat of a maintenance burden, so if new future functionality needs
> to leave mixed mode behind, I'm not too bothered.
>
Maybe just do
if (sizeof(at) < sizeof(__ret))
__ret = (__typeof__(__ret))(uintptr_t)at;
else
__ret = (__typeof__(__ret))at;
That should cover most of the cases.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 21:17 ` Arvind Sankar
@ 2019-12-14 21:30 ` Ard Biesheuvel
2019-12-14 22:14 ` Arvind Sankar
2019-12-14 22:14 ` Ard Biesheuvel
0 siblings, 2 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 21:30 UTC (permalink / raw)
To: Arvind Sankar
Cc: Ard Biesheuvel, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Matthew Garrett, Ingo Molnar, Andy Lutomirski,
Thomas Gleixner
On Sat, 14 Dec 2019 at 22:17, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Sat, Dec 14, 2019 at 08:27:50PM +0000, Ard Biesheuvel wrote:
> > On Sat, 14 Dec 2019 at 21:13, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Sat, Dec 14, 2019 at 07:54:25PM +0000, Ard Biesheuvel wrote:
> > > > On Sat, 14 Dec 2019 at 20:49, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > >
> > > > > On Sat, Dec 14, 2019 at 02:46:27PM -0500, Arvind Sankar wrote:
> > > > > > On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> > > > > > > +
> > > > > > > +#define efi_table_attr(table, attr, instance) ({ \
> > > > > > > + __typeof__(((table##_t *)0)->attr) __ret; \
> > > > > > > + if (efi_is_native()) { \
> > > > > > > + __ret = ((table##_t *)instance)->attr; \
> > > > > > > + } else { \
> > > > > > > + __typeof__(((table##_32_t *)0)->attr) at; \
> > > > > > > + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> > > > > > > + __ret = (__typeof__(__ret))(unsigned long)at; \
> > > > > > > + } \
> > > > > > > + __ret; \
> > > > > > > +})
> > > > > >
> > Yes. I'm open to suggestions on how to improve this, but mixed mode is
> > somewhat of a maintenance burden, so if new future functionality needs
> > to leave mixed mode behind, I'm not too bothered.
> >
>
> Maybe just do
> if (sizeof(at) < sizeof(__ret))
> __ret = (__typeof__(__ret))(uintptr_t)at;
> else
> __ret = (__typeof__(__ret))at;
> That should cover most of the cases.
But the compiler will still be unhappy about the else clause if __ret
is a pointer type, since we'll be casting an u32 to a pointer,
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 21:30 ` Ard Biesheuvel
@ 2019-12-14 22:14 ` Arvind Sankar
2019-12-14 22:14 ` Ard Biesheuvel
1 sibling, 0 replies; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 22:14 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arvind Sankar, Ard Biesheuvel, Linux Kernel Mailing List,
linux-efi, Hans de Goede, Matthew Garrett, Ingo Molnar,
Andy Lutomirski, Thomas Gleixner
On Sat, Dec 14, 2019 at 09:30:08PM +0000, Ard Biesheuvel wrote:
> On Sat, 14 Dec 2019 at 22:17, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > Maybe just do
> > if (sizeof(at) < sizeof(__ret))
> > __ret = (__typeof__(__ret))(uintptr_t)at;
> > else
> > __ret = (__typeof__(__ret))at;
> > That should cover most of the cases.
>
> But the compiler will still be unhappy about the else clause if __ret
> is a pointer type, since we'll be casting an u32 to a pointer,
Ugh, yeah it complains even though it can tell at compile time that that
branch isn't taken.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 21:30 ` Ard Biesheuvel
2019-12-14 22:14 ` Arvind Sankar
@ 2019-12-14 22:14 ` Ard Biesheuvel
2019-12-14 23:02 ` Arvind Sankar
1 sibling, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 22:14 UTC (permalink / raw)
To: Arvind Sankar
Cc: Ard Biesheuvel, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Matthew Garrett, Ingo Molnar, Andy Lutomirski,
Thomas Gleixner
On Sat, 14 Dec 2019 at 22:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Sat, 14 Dec 2019 at 22:17, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Sat, Dec 14, 2019 at 08:27:50PM +0000, Ard Biesheuvel wrote:
> > > On Sat, 14 Dec 2019 at 21:13, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > On Sat, Dec 14, 2019 at 07:54:25PM +0000, Ard Biesheuvel wrote:
> > > > > On Sat, 14 Dec 2019 at 20:49, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > >
> > > > > > On Sat, Dec 14, 2019 at 02:46:27PM -0500, Arvind Sankar wrote:
> > > > > > > On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> > > > > > > > +
> > > > > > > > +#define efi_table_attr(table, attr, instance) ({ \
> > > > > > > > + __typeof__(((table##_t *)0)->attr) __ret; \
> > > > > > > > + if (efi_is_native()) { \
> > > > > > > > + __ret = ((table##_t *)instance)->attr; \
> > > > > > > > + } else { \
> > > > > > > > + __typeof__(((table##_32_t *)0)->attr) at; \
> > > > > > > > + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> > > > > > > > + __ret = (__typeof__(__ret))(unsigned long)at; \
> > > > > > > > + } \
> > > > > > > > + __ret; \
> > > > > > > > +})
> > > > > > >
> > > Yes. I'm open to suggestions on how to improve this, but mixed mode is
> > > somewhat of a maintenance burden, so if new future functionality needs
> > > to leave mixed mode behind, I'm not too bothered.
> > >
> >
> > Maybe just do
> > if (sizeof(at) < sizeof(__ret))
> > __ret = (__typeof__(__ret))(uintptr_t)at;
> > else
> > __ret = (__typeof__(__ret))at;
> > That should cover most of the cases.
>
> But the compiler will still be unhappy about the else clause if __ret
> is a pointer type, since we'll be casting an u32 to a pointer,
I think the answer is to have efi_table_ptr() for pointers and
efi_table_attr() for other types.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit
2019-12-14 22:14 ` Ard Biesheuvel
@ 2019-12-14 23:02 ` Arvind Sankar
0 siblings, 0 replies; 29+ messages in thread
From: Arvind Sankar @ 2019-12-14 23:02 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arvind Sankar, Ard Biesheuvel, Linux Kernel Mailing List,
linux-efi, Hans de Goede, Matthew Garrett, Ingo Molnar,
Andy Lutomirski, Thomas Gleixner
On Sat, Dec 14, 2019 at 10:14:38PM +0000, Ard Biesheuvel wrote:
> On Sat, 14 Dec 2019 at 22:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Sat, 14 Dec 2019 at 22:17, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Sat, Dec 14, 2019 at 08:27:50PM +0000, Ard Biesheuvel wrote:
> > > > On Sat, 14 Dec 2019 at 21:13, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > >
> > > > > On Sat, Dec 14, 2019 at 07:54:25PM +0000, Ard Biesheuvel wrote:
> > > > > > On Sat, 14 Dec 2019 at 20:49, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > > > > >
> > > > > > > On Sat, Dec 14, 2019 at 02:46:27PM -0500, Arvind Sankar wrote:
> > > > > > > > On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:
> > > > > > > > > +
> > > > > > > > > +#define efi_table_attr(table, attr, instance) ({ \
> > > > > > > > > + __typeof__(((table##_t *)0)->attr) __ret; \
> > > > > > > > > + if (efi_is_native()) { \
> > > > > > > > > + __ret = ((table##_t *)instance)->attr; \
> > > > > > > > > + } else { \
> > > > > > > > > + __typeof__(((table##_32_t *)0)->attr) at; \
> > > > > > > > > + at = (((table##_32_t *)(unsigned long)instance)->attr); \
> > > > > > > > > + __ret = (__typeof__(__ret))(unsigned long)at; \
> > > > > > > > > + } \
> > > > > > > > > + __ret; \
> > > > > > > > > +})
> > > > > > > >
> > > > Yes. I'm open to suggestions on how to improve this, but mixed mode is
> > > > somewhat of a maintenance burden, so if new future functionality needs
> > > > to leave mixed mode behind, I'm not too bothered.
> > > >
> > >
> > > Maybe just do
> > > if (sizeof(at) < sizeof(__ret))
> > > __ret = (__typeof__(__ret))(uintptr_t)at;
> > > else
> > > __ret = (__typeof__(__ret))at;
> > > That should cover most of the cases.
> >
> > But the compiler will still be unhappy about the else clause if __ret
> > is a pointer type, since we'll be casting an u32 to a pointer,
>
> I think the answer is to have efi_table_ptr() for pointers and
> efi_table_attr() for other types.
Using __builtin_choose_expr avoids the warning:
__ret = (__typeof__(__ret))
__builtin_choose_expr(sizeof(at) < sizeof(ret),
(uintptr_t)at, at);
But having different efi_table_ macros sounds cleaner.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 06/10] efi/libstub/x86: use mixed mode helpers to populate efi_config
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
` (4 preceding siblings ...)
2019-12-14 17:57 ` [PATCH 05/10] efi/libstub: distinguish between native/mixed not 32/64 bit Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 07/10] efi/libstub: drop explicit 64-bit protocol definitions Ard Biesheuvel
` (3 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
The efi_config struct returned by __efi_early() contains a couple
of pointers that are obtained from the EFI system table, which
could be 32-bit on a 64-bit system. For this reason, there are
two versions of the setup_boot_services() routine, one for 32-bit
and one for 64-bit.
We have helpers now that hide all this nastiness, so let's use
those instead.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/boot/compressed/eboot.c | 27 +++++---------------
arch/x86/boot/compressed/head_32.S | 6 ++---
arch/x86/include/asm/efi.h | 6 ++---
arch/x86/platform/efi/efi.c | 4 +--
include/linux/efi.h | 6 ++---
5 files changed, 18 insertions(+), 31 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 34962f2f3337..6d833f93a878 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -27,19 +27,12 @@ __pure const struct efi_config *__efi_early(void)
return efi_early;
}
-#define BOOT_SERVICES(bits) \
-static void setup_boot_services##bits(struct efi_config *c) \
-{ \
- efi_system_table_##bits##_t *table; \
- \
- table = (typeof(table))sys_table; \
- \
- c->runtime_services = table->runtime; \
- c->boot_services = table->boottime; \
- c->text_output = table->con_out; \
+static void setup_boot_services(struct efi_config *c)
+{
+ c->runtime_services = efi_table_attr(efi_system_table, runtime, sys_table);
+ c->boot_services = efi_table_attr(efi_system_table, boottime, sys_table);
+ c->text_output = efi_table_attr(efi_system_table, con_out, sys_table);
}
-BOOT_SERVICES(32);
-BOOT_SERVICES(64);
void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str)
{
@@ -399,10 +392,7 @@ struct boot_params *make_boot_params(struct efi_config *c)
if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
return NULL;
- if (efi_is_64bit())
- setup_boot_services64(efi_early);
- else
- setup_boot_services32(efi_early);
+ setup_boot_services(efi_early);
status = efi_call_early(handle_protocol, handle,
&proto, (void *)&image);
@@ -761,10 +751,7 @@ efi_main(struct efi_config *c, struct boot_params *boot_params)
if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
goto fail;
- if (efi_is_64bit())
- setup_boot_services64(efi_early);
- else
- setup_boot_services32(efi_early);
+ setup_boot_services(efi_early);
/*
* make_boot_params() may have been called before efi_main(), in which
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index f2dfd6d083ef..40468ab49b9b 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -163,7 +163,7 @@ SYM_FUNC_START(efi_pe_entry)
/* Relocate efi_config->call() */
leal efi32_config(%esi), %eax
- add %esi, 40(%eax)
+ add %esi, 28(%eax)
pushl %eax
call make_boot_params
@@ -190,7 +190,7 @@ SYM_FUNC_START(efi32_stub_entry)
/* Relocate efi_config->call() */
leal efi32_config(%esi), %eax
- add %esi, 40(%eax)
+ add %esi, 28(%eax)
pushl %eax
2:
call efi_main
@@ -265,7 +265,7 @@ SYM_FUNC_END(.Lrelocated)
#ifdef CONFIG_EFI_STUB
.data
efi32_config:
- .fill 5,8,0
+ .fill 7,4,0
.long efi_call_phys
.long 0
.byte 0
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 37364c43296e..2244232108a0 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -202,9 +202,9 @@ static inline efi_status_t efi_thunk_set_virtual_address_map(
struct efi_config {
u64 image_handle;
u64 table;
- u64 runtime_services;
- u64 boot_services;
- u64 text_output;
+ efi_runtime_services_t *runtime_services;
+ efi_boot_services_t *boot_services;
+ efi_simple_text_output_protocol_t *text_output;
efi_status_t (*call)(unsigned long, ...);
bool is64;
} __packed;
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 1493e964c267..27700268ed4a 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -388,7 +388,7 @@ static int __init efi_systab_init(void *phys)
tmp |= systab64->con_in;
efi_systab.con_out_handle = systab64->con_out_handle;
tmp |= systab64->con_out_handle;
- efi_systab.con_out = systab64->con_out;
+ efi_systab.con_out = (void *)(unsigned long)systab64->con_out;
tmp |= systab64->con_out;
efi_systab.stderr_handle = systab64->stderr_handle;
tmp |= systab64->stderr_handle;
@@ -430,7 +430,7 @@ static int __init efi_systab_init(void *phys)
efi_systab.con_in_handle = systab32->con_in_handle;
efi_systab.con_in = systab32->con_in;
efi_systab.con_out_handle = systab32->con_out_handle;
- efi_systab.con_out = systab32->con_out;
+ efi_systab.con_out = (void *)(unsigned long)systab32->con_out;
efi_systab.stderr_handle = systab32->stderr_handle;
efi_systab.stderr = systab32->stderr;
efi_systab.runtime = (void *)(unsigned long)systab32->runtime;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7d7ea32a9990..e17c16d8d523 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -810,7 +810,7 @@ typedef struct {
unsigned long con_in_handle;
unsigned long con_in;
unsigned long con_out_handle;
- unsigned long con_out;
+ struct efi_simple_text_output_protocol *con_out;
unsigned long stderr_handle;
unsigned long stderr;
efi_runtime_services_t *runtime;
@@ -1445,11 +1445,11 @@ typedef struct {
u64 test_string;
} efi_simple_text_output_protocol_64_t;
-struct efi_simple_text_output_protocol {
+typedef struct efi_simple_text_output_protocol {
void *reset;
efi_status_t (*output_string)(void *, void *);
void *test_string;
-};
+} efi_simple_text_output_protocol_t;
#define PIXEL_RGB_RESERVED_8BIT_PER_COLOR 0
#define PIXEL_BGR_RESERVED_8BIT_PER_COLOR 1
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 07/10] efi/libstub: drop explicit 64-bit protocol definitions
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
` (5 preceding siblings ...)
2019-12-14 17:57 ` [PATCH 06/10] efi/libstub/x86: use mixed mode helpers to populate efi_config Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 08/10] efi/libstub: use stricter typing for firmware function pointers Ard Biesheuvel
` (2 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
Mixed mode involves casting 64-bit native calls to 32-bit EFI calls,
which requires explicit 32-bit protocol definitions. Native calls
are always done using the protocol definitions that use native types,
and so the explicit 64-bit ones are redundant. Remove them.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/boot/compressed/eboot.h | 6 -
drivers/firmware/efi/libstub/random.c | 5 -
include/linux/efi.h | 142 --------------------
3 files changed, 153 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h
index 8297387c4676..1cf384ba1b0e 100644
--- a/arch/x86/boot/compressed/eboot.h
+++ b/arch/x86/boot/compressed/eboot.h
@@ -18,12 +18,6 @@ typedef struct {
u32 blt;
} efi_uga_draw_protocol_32_t;
-typedef struct {
- u64 get_mode;
- u64 set_mode;
- u64 blt;
-} efi_uga_draw_protocol_64_t;
-
typedef struct {
void *get_mode;
void *set_mode;
diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index 97378cf96a2e..3b85883fb312 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -16,11 +16,6 @@ typedef struct {
u32 get_rng;
} efi_rng_protocol_32_t;
-typedef struct {
- u64 get_info;
- u64 get_rng;
-} efi_rng_protocol_64_t;
-
struct efi_rng_protocol {
efi_status_t (*get_info)(struct efi_rng_protocol *,
unsigned long *, efi_guid_t *);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e17c16d8d523..87c2537b4543 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -259,54 +259,6 @@ typedef struct {
u32 create_event_ex;
} __packed efi_boot_services_32_t;
-typedef struct {
- efi_table_hdr_t hdr;
- u64 raise_tpl;
- u64 restore_tpl;
- u64 allocate_pages;
- u64 free_pages;
- u64 get_memory_map;
- u64 allocate_pool;
- u64 free_pool;
- u64 create_event;
- u64 set_timer;
- u64 wait_for_event;
- u64 signal_event;
- u64 close_event;
- u64 check_event;
- u64 install_protocol_interface;
- u64 reinstall_protocol_interface;
- u64 uninstall_protocol_interface;
- u64 handle_protocol;
- u64 __reserved;
- u64 register_protocol_notify;
- u64 locate_handle;
- u64 locate_device_path;
- u64 install_configuration_table;
- u64 load_image;
- u64 start_image;
- u64 exit;
- u64 unload_image;
- u64 exit_boot_services;
- u64 get_next_monotonic_count;
- u64 stall;
- u64 set_watchdog_timer;
- u64 connect_controller;
- u64 disconnect_controller;
- u64 open_protocol;
- u64 close_protocol;
- u64 open_protocol_information;
- u64 protocols_per_handle;
- u64 locate_handle_buffer;
- u64 locate_protocol;
- u64 install_multiple_protocol_interfaces;
- u64 uninstall_multiple_protocol_interfaces;
- u64 calculate_crc32;
- u64 copy_mem;
- u64 set_mem;
- u64 create_event_ex;
-} __packed efi_boot_services_64_t;
-
/*
* EFI Boot Services table
*/
@@ -391,11 +343,6 @@ typedef struct {
u32 write;
} efi_pci_io_protocol_access_32_t;
-typedef struct {
- u64 read;
- u64 write;
-} efi_pci_io_protocol_access_64_t;
-
typedef struct efi_pci_io_protocol efi_pci_io_protocol_t;
typedef
@@ -440,26 +387,6 @@ typedef struct {
u32 romimage;
} efi_pci_io_protocol_32_t;
-typedef struct {
- u64 poll_mem;
- u64 poll_io;
- efi_pci_io_protocol_access_64_t mem;
- efi_pci_io_protocol_access_64_t io;
- efi_pci_io_protocol_access_64_t pci;
- u64 copy_mem;
- u64 map;
- u64 unmap;
- u64 allocate_buffer;
- u64 free_buffer;
- u64 flush;
- u64 get_location;
- u64 attributes;
- u64 get_bar_attributes;
- u64 set_bar_attributes;
- u64 romsize;
- u64 romimage;
-} efi_pci_io_protocol_64_t;
-
struct efi_pci_io_protocol {
void *poll_mem;
void *poll_io;
@@ -508,14 +435,6 @@ typedef struct {
u32 get_all;
} apple_properties_protocol_32_t;
-typedef struct {
- u64 version;
- u64 get;
- u64 set;
- u64 del;
- u64 get_all;
-} apple_properties_protocol_64_t;
-
struct efi_dev_path;
typedef struct apple_properties_protocol {
@@ -542,16 +461,6 @@ typedef struct {
u32 get_result_of_set_active_pcr_banks;
} efi_tcg2_protocol_32_t;
-typedef struct {
- u64 get_capability;
- u64 get_event_log;
- u64 hash_log_extend_event;
- u64 submit_command;
- u64 get_active_pcr_banks;
- u64 set_active_pcr_banks;
- u64 get_result_of_set_active_pcr_banks;
-} efi_tcg2_protocol_64_t;
-
typedef u32 efi_tcg2_event_log_format;
typedef struct {
@@ -870,22 +779,6 @@ typedef struct {
u32 unload;
} efi_loaded_image_32_t;
-typedef struct {
- u32 revision;
- u64 parent_handle;
- u64 system_table;
- u64 device_handle;
- u64 file_path;
- u64 reserved;
- u32 load_options_size;
- u64 load_options;
- u64 image_base;
- __aligned_u64 image_size;
- unsigned int image_code_type;
- unsigned int image_data_type;
- u64 unload;
-} efi_loaded_image_64_t;
-
typedef struct {
u32 revision;
efi_handle_t parent_handle;
@@ -928,20 +821,6 @@ typedef struct {
u32 flush;
} efi_file_handle_32_t;
-typedef struct {
- u64 revision;
- u64 open;
- u64 close;
- u64 delete;
- u64 read;
- u64 write;
- u64 get_position;
- u64 set_position;
- u64 get_info;
- u64 set_info;
- u64 flush;
-} efi_file_handle_64_t;
-
typedef struct _efi_file_handle {
u64 revision;
efi_status_t (*open)(struct _efi_file_handle *,
@@ -965,11 +844,6 @@ typedef struct {
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 *,
@@ -1482,15 +1356,6 @@ typedef struct {
u32 frame_buffer_size;
} efi_graphics_output_protocol_mode_32_t;
-typedef struct {
- u32 max_mode;
- u32 mode;
- u64 info;
- u64 size_of_info;
- u64 frame_buffer_base;
- u64 frame_buffer_size;
-} efi_graphics_output_protocol_mode_64_t;
-
typedef struct {
u32 max_mode;
u32 mode;
@@ -1507,13 +1372,6 @@ typedef struct {
u32 mode;
} efi_graphics_output_protocol_32_t;
-typedef struct {
- u64 query_mode;
- u64 set_mode;
- u64 blt;
- u64 mode;
-} efi_graphics_output_protocol_64_t;
-
typedef struct {
void *query_mode;
void *set_mode;
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 08/10] efi/libstub: use stricter typing for firmware function pointers
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
` (6 preceding siblings ...)
2019-12-14 17:57 ` [PATCH 07/10] efi/libstub: drop explicit 64-bit protocol definitions Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 09/10] efi/libstub: annotate firmware routines as __efiapi Ard Biesheuvel
2019-12-14 17:57 ` [PATCH 10/10] efi/libstub/x86: avoid thunking for native firmware calls Ard Biesheuvel
9 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
We will soon remove another level of pointer casting, so let's make
sure all type handling involving firmware calls at boot time is correct.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/boot/compressed/eboot.c | 10 ++++++----
arch/x86/boot/compressed/eboot.h | 5 +++--
drivers/firmware/efi/libstub/efi-stub-helper.c | 5 +++--
drivers/firmware/efi/libstub/tpm.c | 4 ++--
include/linux/efi.h | 6 +++++-
5 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 6d833f93a878..052852eaa076 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -64,7 +64,8 @@ preserve_pci_rom_image(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
size = romsize + sizeof(*rom);
- status = efi_call_early(allocate_pool, EFI_LOADER_DATA, size, &rom);
+ status = efi_call_early(allocate_pool, EFI_LOADER_DATA, size,
+ (void **)&rom);
if (status != EFI_SUCCESS) {
efi_printk(sys_table, "Failed to allocate memory for 'rom'\n");
return status;
@@ -191,9 +192,9 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
struct setup_data *data, *new;
efi_status_t status;
u32 size = 0;
- void *p;
+ apple_properties_protocol_t *p;
- status = efi_call_early(locate_protocol, &guid, NULL, &p);
+ status = efi_call_early(locate_protocol, &guid, NULL, (void **)&p);
if (status != EFI_SUCCESS)
return;
@@ -208,7 +209,8 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
do {
status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
- size + sizeof(struct setup_data), &new);
+ size + sizeof(struct setup_data),
+ (void **)&new);
if (status != EFI_SUCCESS) {
efi_printk(sys_table, "Failed to allocate memory for 'properties'\n");
return;
diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h
index 1cf384ba1b0e..318531f128c2 100644
--- a/arch/x86/boot/compressed/eboot.h
+++ b/arch/x86/boot/compressed/eboot.h
@@ -18,8 +18,9 @@ typedef struct {
u32 blt;
} efi_uga_draw_protocol_32_t;
-typedef struct {
- void *get_mode;
+typedef struct efi_uga_draw_protocol {
+ efi_status_t (*get_mode)(struct efi_uga_draw_protocol *,
+ u32*, u32*, u32*, u32*);
void *set_mode;
void *blt;
} efi_uga_draw_protocol_t;
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index ff3266b9f673..5bb1715cb154 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -423,12 +423,13 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh,
return status;
}
-static efi_status_t efi_file_read(void *handle, unsigned long *size, void *addr)
+static efi_status_t efi_file_read(efi_file_handle_t *handle,
+ unsigned long *size, void *addr)
{
return efi_call_proto(efi_file_handle, read, handle, size, addr);
}
-static efi_status_t efi_file_close(void *handle)
+static efi_status_t efi_file_close(efi_file_handle_t *handle)
{
return efi_call_proto(efi_file_handle, close, handle);
}
diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 0354d35ebaa7..83cf204bfe8f 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -59,11 +59,11 @@ void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
size_t log_size, last_entry_size;
efi_bool_t truncated;
int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
- void *tcg2_protocol = NULL;
+ efi_tcg2_protocol_t *tcg2_protocol = NULL;
int final_events_size = 0;
status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
- &tcg2_protocol);
+ (void **)&tcg2_protocol);
if (status != EFI_SUCCESS)
return;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 87c2537b4543..7f1a99bd2c05 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -399,7 +399,11 @@ struct efi_pci_io_protocol {
void *allocate_buffer;
void *free_buffer;
void *flush;
- void *get_location;
+ efi_status_t (*get_location)(struct efi_pci_io_protocol *,
+ unsigned long *segment_nr,
+ unsigned long *bus_nr,
+ unsigned long *device_nr,
+ unsigned long *function_nr);
void *attributes;
void *get_bar_attributes;
void *set_bar_attributes;
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 09/10] efi/libstub: annotate firmware routines as __efiapi
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
` (7 preceding siblings ...)
2019-12-14 17:57 ` [PATCH 08/10] efi/libstub: use stricter typing for firmware function pointers Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-17 15:01 ` Brian Gerst
2019-12-14 17:57 ` [PATCH 10/10] efi/libstub/x86: avoid thunking for native firmware calls Ard Biesheuvel
9 siblings, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
Annotate all the firmware routines (boot services, runtime services and
protocol methods) called in the boot context as __efiapi, and make
it expand to __attribute__((ms_abi)) on 64-bit x86. This allows us
to use the compiler to generate the calls into firmware that use the
MS calling convention instead of the SysV one.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/Kconfig | 1 +
arch/x86/boot/compressed/eboot.h | 4 +-
drivers/firmware/efi/libstub/random.c | 8 +-
include/linux/efi.h | 130 +++++++++++---------
4 files changed, 79 insertions(+), 64 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 5e8949953660..8ba81036a7ef 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1993,6 +1993,7 @@ config EFI
config EFI_STUB
bool "EFI stub support"
depends on EFI && !X86_USE_3DNOW
+ depends on $(cc-option,-mabi=ms)
select RELOCATABLE
---help---
This kernel feature allows a bzImage to be loaded directly
diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h
index 318531f128c2..512d2143a94f 100644
--- a/arch/x86/boot/compressed/eboot.h
+++ b/arch/x86/boot/compressed/eboot.h
@@ -19,8 +19,8 @@ typedef struct {
} efi_uga_draw_protocol_32_t;
typedef struct efi_uga_draw_protocol {
- efi_status_t (*get_mode)(struct efi_uga_draw_protocol *,
- u32*, u32*, u32*, u32*);
+ efi_status_t (__efiapi *get_mode)(struct efi_uga_draw_protocol *,
+ u32*, u32*, u32*, u32*);
void *set_mode;
void *blt;
} efi_uga_draw_protocol_t;
diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index 3b85883fb312..872d2de3eaaf 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -17,10 +17,10 @@ typedef struct {
} efi_rng_protocol_32_t;
struct efi_rng_protocol {
- efi_status_t (*get_info)(struct efi_rng_protocol *,
- unsigned long *, efi_guid_t *);
- efi_status_t (*get_rng)(struct efi_rng_protocol *,
- efi_guid_t *, unsigned long, u8 *out);
+ efi_status_t (__efiapi *get_info)(struct efi_rng_protocol *,
+ unsigned long *, efi_guid_t *);
+ efi_status_t (__efiapi *get_rng)(struct efi_rng_protocol *,
+ efi_guid_t *, unsigned long, u8 *out);
};
efi_status_t efi_get_random_bytes(efi_system_table_t *sys_table_arg,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7f1a99bd2c05..2ba13d901055 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -48,6 +48,12 @@ typedef u16 efi_char16_t; /* UNICODE character */
typedef u64 efi_physical_addr_t;
typedef void *efi_handle_t;
+#ifdef CONFIG_X86_64
+#define __efiapi __attribute__((ms_abi))
+#else
+#define __efiapi
+#endif
+
#define for_each_efi_handle(handle, array, size, i) \
for (i = 1, handle = efi_is_native() ? (array)[0] \
: (efi_handle_t)(unsigned long)((u32 *)(array))[0]; \
@@ -266,13 +272,14 @@ typedef struct {
efi_table_hdr_t hdr;
void *raise_tpl;
void *restore_tpl;
- efi_status_t (*allocate_pages)(int, int, unsigned long,
- efi_physical_addr_t *);
- efi_status_t (*free_pages)(efi_physical_addr_t, unsigned long);
- efi_status_t (*get_memory_map)(unsigned long *, void *, unsigned long *,
- unsigned long *, u32 *);
- efi_status_t (*allocate_pool)(int, unsigned long, void **);
- efi_status_t (*free_pool)(void *);
+ efi_status_t (__efiapi *allocate_pages)(int, int, unsigned long,
+ efi_physical_addr_t *);
+ efi_status_t (__efiapi *free_pages)(efi_physical_addr_t, unsigned long);
+ efi_status_t (__efiapi *get_memory_map)(unsigned long *, void *,
+ unsigned long *,
+ unsigned long *, u32 *);
+ efi_status_t (__efiapi *allocate_pool)(int, unsigned long, void **);
+ efi_status_t (__efiapi *free_pool)(void *);
void *create_event;
void *set_timer;
void *wait_for_event;
@@ -282,18 +289,21 @@ typedef struct {
void *install_protocol_interface;
void *reinstall_protocol_interface;
void *uninstall_protocol_interface;
- efi_status_t (*handle_protocol)(efi_handle_t, efi_guid_t *, void **);
+ efi_status_t (__efiapi *handle_protocol)(efi_handle_t, efi_guid_t *,
+ void **);
void *__reserved;
void *register_protocol_notify;
- efi_status_t (*locate_handle)(int, efi_guid_t *, void *,
+ efi_status_t (__efiapi *locate_handle)(int, efi_guid_t *, void *,
unsigned long *, efi_handle_t *);
void *locate_device_path;
- efi_status_t (*install_configuration_table)(efi_guid_t *, void *);
+ efi_status_t (__efiapi *install_configuration_table)(efi_guid_t *,
+ void *);
void *load_image;
void *start_image;
void *exit;
void *unload_image;
- efi_status_t (*exit_boot_services)(efi_handle_t, unsigned long);
+ efi_status_t (__efiapi *exit_boot_services)(efi_handle_t,
+ unsigned long);
void *get_next_monotonic_count;
void *stall;
void *set_watchdog_timer;
@@ -304,7 +314,7 @@ typedef struct {
void *open_protocol_information;
void *protocols_per_handle;
void *locate_handle_buffer;
- efi_status_t (*locate_protocol)(efi_guid_t *, void *, void **);
+ efi_status_t (__efiapi *locate_protocol)(efi_guid_t *, void *, void **);
void *install_multiple_protocol_interfaces;
void *uninstall_multiple_protocol_interfaces;
void *calculate_crc32;
@@ -346,13 +356,13 @@ typedef struct {
typedef struct efi_pci_io_protocol efi_pci_io_protocol_t;
typedef
-efi_status_t (*efi_pci_io_protocol_mem_t)(struct efi_pci_io_protocol *,
+efi_status_t (__efiapi *efi_pci_io_protocol_mem_t)(struct efi_pci_io_protocol *,
EFI_PCI_IO_PROTOCOL_WIDTH,
u8 bar_index, u64 offset,
unsigned long count, void *buffer);
typedef
-efi_status_t (*efi_pci_io_protocol_cfg_t)(struct efi_pci_io_protocol *,
+efi_status_t (__efiapi *efi_pci_io_protocol_cfg_t)(struct efi_pci_io_protocol *,
EFI_PCI_IO_PROTOCOL_WIDTH,
u32 offset, unsigned long count,
void *buffer);
@@ -399,11 +409,11 @@ struct efi_pci_io_protocol {
void *allocate_buffer;
void *free_buffer;
void *flush;
- efi_status_t (*get_location)(struct efi_pci_io_protocol *,
- unsigned long *segment_nr,
- unsigned long *bus_nr,
- unsigned long *device_nr,
- unsigned long *function_nr);
+ efi_status_t (__efiapi *get_location)(struct efi_pci_io_protocol *,
+ unsigned long *segment_nr,
+ unsigned long *bus_nr,
+ unsigned long *device_nr,
+ unsigned long *function_nr);
void *attributes;
void *get_bar_attributes;
void *set_bar_attributes;
@@ -443,16 +453,16 @@ struct efi_dev_path;
typedef struct apple_properties_protocol {
unsigned long version;
- efi_status_t (*get)(struct apple_properties_protocol *,
- struct efi_dev_path *, efi_char16_t *,
- void *, u32 *);
- efi_status_t (*set)(struct apple_properties_protocol *,
- struct efi_dev_path *, efi_char16_t *,
- void *, u32);
- efi_status_t (*del)(struct apple_properties_protocol *,
- struct efi_dev_path *, efi_char16_t *);
- efi_status_t (*get_all)(struct apple_properties_protocol *,
- void *buffer, u32 *);
+ efi_status_t (__efiapi *get)(struct apple_properties_protocol *,
+ struct efi_dev_path *, efi_char16_t *,
+ void *, u32 *);
+ efi_status_t (__efiapi *set)(struct apple_properties_protocol *,
+ struct efi_dev_path *, efi_char16_t *,
+ void *, u32);
+ efi_status_t (__efiapi *del)(struct apple_properties_protocol *,
+ struct efi_dev_path *, efi_char16_t *);
+ efi_status_t (__efiapi *get_all)(struct apple_properties_protocol *,
+ void *buffer, u32 *);
} apple_properties_protocol_t;
typedef struct {
@@ -469,8 +479,11 @@ typedef u32 efi_tcg2_event_log_format;
typedef struct {
void *get_capability;
- efi_status_t (*get_event_log)(efi_handle_t, efi_tcg2_event_log_format,
- efi_physical_addr_t *, efi_physical_addr_t *, efi_bool_t *);
+ efi_status_t (__efiapi *get_event_log)(efi_handle_t,
+ efi_tcg2_event_log_format,
+ efi_physical_addr_t *,
+ efi_physical_addr_t *,
+ efi_bool_t *);
void *hash_log_extend_event;
void *submit_command;
void *get_active_pcr_banks;
@@ -562,21 +575,21 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes,
bool nonblocking);
typedef struct {
- efi_table_hdr_t hdr;
- efi_get_time_t *get_time;
- efi_set_time_t *set_time;
- efi_get_wakeup_time_t *get_wakeup_time;
- efi_set_wakeup_time_t *set_wakeup_time;
- efi_set_virtual_address_map_t *set_virtual_address_map;
- void *convert_pointer;
- efi_get_variable_t *get_variable;
- efi_get_next_variable_t *get_next_variable;
- efi_set_variable_t *set_variable;
- efi_get_next_high_mono_count_t *get_next_high_mono_count;
- efi_reset_system_t *reset_system;
- efi_update_capsule_t *update_capsule;
- efi_query_capsule_caps_t *query_capsule_caps;
- efi_query_variable_info_t *query_variable_info;
+ efi_table_hdr_t hdr;
+ efi_get_time_t __efiapi *get_time;
+ efi_set_time_t __efiapi *set_time;
+ efi_get_wakeup_time_t __efiapi *get_wakeup_time;
+ efi_set_wakeup_time_t __efiapi *set_wakeup_time;
+ efi_set_virtual_address_map_t __efiapi *set_virtual_address_map;
+ void *convert_pointer;
+ efi_get_variable_t __efiapi *get_variable;
+ efi_get_next_variable_t __efiapi *get_next_variable;
+ efi_set_variable_t __efiapi *set_variable;
+ efi_get_next_high_mono_count_t __efiapi *get_next_high_mono_count;
+ efi_reset_system_t __efiapi *reset_system;
+ efi_update_capsule_t __efiapi *update_capsule;
+ efi_query_capsule_caps_t __efiapi *query_capsule_caps;
+ efi_query_variable_info_t __efiapi *query_variable_info;
} efi_runtime_services_t;
void efi_native_runtime_setup(void);
@@ -796,7 +809,7 @@ typedef struct {
__aligned_u64 image_size;
unsigned int image_code_type;
unsigned int image_data_type;
- efi_status_t (*unload)(efi_handle_t image_handle);
+ efi_status_t (__efiapi *unload)(efi_handle_t image_handle);
} efi_loaded_image_t;
@@ -827,18 +840,19 @@ typedef struct {
typedef struct _efi_file_handle {
u64 revision;
- efi_status_t (*open)(struct _efi_file_handle *,
- struct _efi_file_handle **,
- efi_char16_t *, u64, u64);
- efi_status_t (*close)(struct _efi_file_handle *);
+ efi_status_t (__efiapi *open)(struct _efi_file_handle *,
+ struct _efi_file_handle **,
+ efi_char16_t *, u64, u64);
+ efi_status_t (__efiapi *close)(struct _efi_file_handle *);
void *delete;
- efi_status_t (*read)(struct _efi_file_handle *, unsigned long *,
- void *);
+ efi_status_t (__efiapi *read)(struct _efi_file_handle *,
+ unsigned long *, void *);
void *write;
void *get_position;
void *set_position;
- efi_status_t (*get_info)(struct _efi_file_handle *, efi_guid_t *,
- unsigned long *, void *);
+ efi_status_t (__efiapi *get_info)(struct _efi_file_handle *,
+ efi_guid_t *, unsigned long *,
+ void *);
void *set_info;
void *flush;
} efi_file_handle_t;
@@ -850,8 +864,8 @@ typedef struct {
typedef struct _efi_file_io_interface {
u64 revision;
- int (*open_volume)(struct _efi_file_io_interface *,
- efi_file_handle_t **);
+ int (__efiapi *open_volume)(struct _efi_file_io_interface *,
+ efi_file_handle_t **);
} efi_file_io_interface_t;
#define EFI_FILE_MODE_READ 0x0000000000000001
@@ -1325,7 +1339,7 @@ typedef struct {
typedef struct efi_simple_text_output_protocol {
void *reset;
- efi_status_t (*output_string)(void *, void *);
+ efi_status_t (__efiapi *output_string)(void *, void *);
void *test_string;
} efi_simple_text_output_protocol_t;
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 09/10] efi/libstub: annotate firmware routines as __efiapi
2019-12-14 17:57 ` [PATCH 09/10] efi/libstub: annotate firmware routines as __efiapi Ard Biesheuvel
@ 2019-12-17 15:01 ` Brian Gerst
0 siblings, 0 replies; 29+ messages in thread
From: Brian Gerst @ 2019-12-17 15:01 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux Kernel Mailing List, linux-efi, Hans de Goede,
Matthew Garrett, Ingo Molnar, Andy Lutomirski, Thomas Gleixner,
Arvind Sankar
On Sat, Dec 14, 2019 at 1:00 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Annotate all the firmware routines (boot services, runtime services and
> protocol methods) called in the boot context as __efiapi, and make
> it expand to __attribute__((ms_abi)) on 64-bit x86. This allows us
> to use the compiler to generate the calls into firmware that use the
> MS calling convention instead of the SysV one.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> arch/x86/Kconfig | 1 +
> arch/x86/boot/compressed/eboot.h | 4 +-
> drivers/firmware/efi/libstub/random.c | 8 +-
> include/linux/efi.h | 130 +++++++++++---------
> 4 files changed, 79 insertions(+), 64 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 5e8949953660..8ba81036a7ef 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1993,6 +1993,7 @@ config EFI
> config EFI_STUB
> bool "EFI stub support"
> depends on EFI && !X86_USE_3DNOW
> + depends on $(cc-option,-mabi=ms)
> select RELOCATABLE
> ---help---
> This kernel feature allows a bzImage to be loaded directly
> diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h
> index 318531f128c2..512d2143a94f 100644
> --- a/arch/x86/boot/compressed/eboot.h
> +++ b/arch/x86/boot/compressed/eboot.h
> @@ -19,8 +19,8 @@ typedef struct {
> } efi_uga_draw_protocol_32_t;
>
> typedef struct efi_uga_draw_protocol {
> - efi_status_t (*get_mode)(struct efi_uga_draw_protocol *,
> - u32*, u32*, u32*, u32*);
> + efi_status_t (__efiapi *get_mode)(struct efi_uga_draw_protocol *,
> + u32*, u32*, u32*, u32*);
> void *set_mode;
> void *blt;
> } efi_uga_draw_protocol_t;
> diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
> index 3b85883fb312..872d2de3eaaf 100644
> --- a/drivers/firmware/efi/libstub/random.c
> +++ b/drivers/firmware/efi/libstub/random.c
> @@ -17,10 +17,10 @@ typedef struct {
> } efi_rng_protocol_32_t;
>
> struct efi_rng_protocol {
> - efi_status_t (*get_info)(struct efi_rng_protocol *,
> - unsigned long *, efi_guid_t *);
> - efi_status_t (*get_rng)(struct efi_rng_protocol *,
> - efi_guid_t *, unsigned long, u8 *out);
> + efi_status_t (__efiapi *get_info)(struct efi_rng_protocol *,
> + unsigned long *, efi_guid_t *);
> + efi_status_t (__efiapi *get_rng)(struct efi_rng_protocol *,
> + efi_guid_t *, unsigned long, u8 *out);
> };
>
> efi_status_t efi_get_random_bytes(efi_system_table_t *sys_table_arg,
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 7f1a99bd2c05..2ba13d901055 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -48,6 +48,12 @@ typedef u16 efi_char16_t; /* UNICODE character */
> typedef u64 efi_physical_addr_t;
> typedef void *efi_handle_t;
>
> +#ifdef CONFIG_X86_64
> +#define __efiapi __attribute__((ms_abi))
> +#else
> +#define __efiapi
I think it would be better to explicitly use the regparm(0) attribute
for 32-bit, even though that is currently the default.
--
Brian Gerst
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 10/10] efi/libstub/x86: avoid thunking for native firmware calls
2019-12-14 17:57 [PATCH 00/10] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
` (8 preceding siblings ...)
2019-12-14 17:57 ` [PATCH 09/10] efi/libstub: annotate firmware routines as __efiapi Ard Biesheuvel
@ 2019-12-14 17:57 ` Ard Biesheuvel
2019-12-15 19:30 ` Arvind Sankar
9 siblings, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-14 17:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-efi, Ard Biesheuvel, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
We use special wrapper routines to invoke firmware services in the
native case as well as the mixed mode case. For mixed mode, the need
is obvious, but for the native cases, we can simply rely on the
compiler to generate the indirect call, given that GCC now has
support for the MS calling convention (and has had it for quite some
time now). Note that on i386, the decompressor and the EFI stub are not
built with -mregparm=3 like the rest of the i386 kernel, so we can
safely allow the compiler to emit the indirect calls here as well.
So drop all the wrappers and indirection, and switch to either native
calls, or direct calls into the thunk routine for mixed mode.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/boot/compressed/Makefile | 2 +-
arch/x86/boot/compressed/efi_stub_32.S | 87 --------------------
arch/x86/boot/compressed/efi_stub_64.S | 5 --
arch/x86/boot/compressed/head_32.S | 6 --
arch/x86/boot/compressed/head_64.S | 12 ---
arch/x86/include/asm/efi.h | 22 +++--
arch/x86/platform/efi/efi_64.c | 2 -
7 files changed, 15 insertions(+), 121 deletions(-)
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index aa976adb7094..a20f55c59753 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -89,7 +89,7 @@ vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
$(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
-vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
+vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o \
$(objtree)/drivers/firmware/efi/libstub/lib.a
vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
diff --git a/arch/x86/boot/compressed/efi_stub_32.S b/arch/x86/boot/compressed/efi_stub_32.S
deleted file mode 100644
index ed6c351d34ed..000000000000
--- a/arch/x86/boot/compressed/efi_stub_32.S
+++ /dev/null
@@ -1,87 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * EFI call stub for IA32.
- *
- * This stub allows us to make EFI calls in physical mode with interrupts
- * turned off. Note that this implementation is different from the one in
- * arch/x86/platform/efi/efi_stub_32.S because we're _already_ in physical
- * mode at this point.
- */
-
-#include <linux/linkage.h>
-#include <asm/page_types.h>
-
-/*
- * efi_call_phys(void *, ...) is a function with variable parameters.
- * All the callers of this function assure that all the parameters are 4-bytes.
- */
-
-/*
- * In gcc calling convention, EBX, ESP, EBP, ESI and EDI are all callee save.
- * So we'd better save all of them at the beginning of this function and restore
- * at the end no matter how many we use, because we can not assure EFI runtime
- * service functions will comply with gcc calling convention, too.
- */
-
-.text
-SYM_FUNC_START(efi_call_phys)
- /*
- * 0. The function can only be called in Linux kernel. So CS has been
- * set to 0x0010, DS and SS have been set to 0x0018. In EFI, I found
- * the values of these registers are the same. And, the corresponding
- * GDT entries are identical. So I will do nothing about segment reg
- * and GDT, but change GDT base register in prelog and epilog.
- */
-
- /*
- * 1. Because we haven't been relocated by this point we need to
- * use relative addressing.
- */
- call 1f
-1: popl %edx
- subl $1b, %edx
-
- /*
- * 2. Now on the top of stack is the return
- * address in the caller of efi_call_phys(), then parameter 1,
- * parameter 2, ..., param n. To make things easy, we save the return
- * address of efi_call_phys in a global variable.
- */
- popl %ecx
- movl %ecx, saved_return_addr(%edx)
- /* get the function pointer into ECX*/
- popl %ecx
- movl %ecx, efi_rt_function_ptr(%edx)
-
- /*
- * 3. Call the physical function.
- */
- call *%ecx
-
- /*
- * 4. Balance the stack. And because EAX contain the return value,
- * we'd better not clobber it. We need to calculate our address
- * again because %ecx and %edx are not preserved across EFI function
- * calls.
- */
- call 1f
-1: popl %edx
- subl $1b, %edx
-
- movl efi_rt_function_ptr(%edx), %ecx
- pushl %ecx
-
- /*
- * 10. Push the saved return address onto the stack and return.
- */
- movl saved_return_addr(%edx), %ecx
- pushl %ecx
- ret
-SYM_FUNC_END(efi_call_phys)
-.previous
-
-.data
-saved_return_addr:
- .long 0
-efi_rt_function_ptr:
- .long 0
diff --git a/arch/x86/boot/compressed/efi_stub_64.S b/arch/x86/boot/compressed/efi_stub_64.S
deleted file mode 100644
index 99494dff2113..000000000000
--- a/arch/x86/boot/compressed/efi_stub_64.S
+++ /dev/null
@@ -1,5 +0,0 @@
-#include <asm/segment.h>
-#include <asm/msr.h>
-#include <asm/processor-flags.h>
-
-#include "../../platform/efi/efi_stub_64.S"
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 40468ab49b9b..7da4dfc53df6 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -161,9 +161,7 @@ SYM_FUNC_START(efi_pe_entry)
popl %ecx
movl %ecx, efi32_config+8(%esi) /* EFI System table pointer */
- /* Relocate efi_config->call() */
leal efi32_config(%esi), %eax
- add %esi, 28(%eax)
pushl %eax
call make_boot_params
@@ -188,9 +186,7 @@ SYM_FUNC_START(efi32_stub_entry)
movl %ecx, efi32_config(%esi) /* Handle */
movl %edx, efi32_config+8(%esi) /* EFI System table pointer */
- /* Relocate efi_config->call() */
leal efi32_config(%esi), %eax
- add %esi, 28(%eax)
pushl %eax
2:
call efi_main
@@ -266,8 +262,6 @@ SYM_FUNC_END(.Lrelocated)
.data
efi32_config:
.fill 7,4,0
- .long efi_call_phys
- .long 0
.byte 0
#endif
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 58a512e33d8d..6dc6a7ebb9e1 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -458,11 +458,6 @@ SYM_FUNC_START(efi_pe_entry)
1: popq %rbp
subq $1b, %rbp
- /*
- * Relocate efi_config->call().
- */
- addq %rbp, efi64_config+40(%rip)
-
movq %rax, %rdi
call make_boot_params
cmpq $0,%rax
@@ -477,11 +472,6 @@ handover_entry:
1: popq %rbp
subq $1b, %rbp
- /*
- * Relocate efi_config->call().
- */
- movq efi_config(%rip), %rax
- addq %rbp, 40(%rax)
2:
movq efi_config(%rip), %rdi
call efi_main
@@ -683,14 +673,12 @@ SYM_DATA_LOCAL(efi_config, .quad 0)
#ifdef CONFIG_EFI_MIXED
SYM_DATA_START(efi32_config)
.fill 5,8,0
- .quad efi64_thunk
.byte 0
SYM_DATA_END(efi32_config)
#endif
SYM_DATA_START(efi64_config)
.fill 5,8,0
- .quad efi_call
.byte 1
SYM_DATA_END(efi64_config)
#endif /* CONFIG_EFI_STUB */
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 2244232108a0..14a24bb01776 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -152,6 +152,7 @@ struct efi_setup_data {
extern u64 efi_setup;
#ifdef CONFIG_EFI
+extern efi_status_t efi64_thunk(u32, ...);
static inline bool efi_is_mixed(void)
{
@@ -205,7 +206,6 @@ struct efi_config {
efi_runtime_services_t *runtime_services;
efi_boot_services_t *boot_services;
efi_simple_text_output_protocol_t *text_output;
- efi_status_t (*call)(unsigned long, ...);
bool is64;
} __packed;
@@ -232,7 +232,7 @@ static inline bool efi_is_native(void)
#define efi_table_attr(table, attr, instance) ({ \
__typeof__(((table##_t *)0)->attr) __ret; \
if (efi_is_native()) { \
- __ret = ((table##_t *)instance)->attr; \
+ __ret = instance->attr; \
} else { \
__typeof__(((table##_32_t *)0)->attr) at; \
at = (((table##_32_t *)(unsigned long)instance)->attr); \
@@ -242,19 +242,25 @@ static inline bool efi_is_native(void)
})
#define efi_call_proto(protocol, f, instance, ...) \
- __efi_early()->call((unsigned long) \
+ efi_is_native() \
+ ? instance->f(instance, ##__VA_ARGS__) \
+ : efi64_thunk((unsigned long) \
efi_table_attr(protocol, f, instance), \
- instance, ##__VA_ARGS__)
+ instance, ##__VA_ARGS__)
#define efi_call_early(f, ...) \
- __efi_early()->call((unsigned long) \
+ efi_is_native() \
+ ? __efi_early()->boot_services->f(__VA_ARGS__) \
+ : efi64_thunk((unsigned long) \
efi_table_attr(efi_boot_services, f, \
- __efi_early()->boot_services), __VA_ARGS__)
+ __efi_early()->boot_services), __VA_ARGS__)
#define efi_call_runtime(f, ...) \
- __efi_early()->call((unsigned long) \
+ efi_is_native() \
+ ? __efi_early()->runtime_services->f(__VA_ARGS__) \
+ : efi64_thunk((unsigned long) \
efi_table_attr(efi_runtime_services, f, \
- __efi_early()->runtime_services), __VA_ARGS__)
+ __efi_early()->runtime_services), __VA_ARGS__)
extern bool efi_reboot_required(void);
extern bool efi_is_table_address(unsigned long phys_addr);
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 885e50a707a6..03c2ed3c645c 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -635,8 +635,6 @@ void efi_switch_mm(struct mm_struct *mm)
}
#ifdef CONFIG_EFI_MIXED
-extern efi_status_t efi64_thunk(u32, ...);
-
static DEFINE_SPINLOCK(efi_runtime_lock);
#define runtime_service32(func) \
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 10/10] efi/libstub/x86: avoid thunking for native firmware calls
2019-12-14 17:57 ` [PATCH 10/10] efi/libstub/x86: avoid thunking for native firmware calls Ard Biesheuvel
@ 2019-12-15 19:30 ` Arvind Sankar
2019-12-17 8:32 ` Ard Biesheuvel
0 siblings, 1 reply; 29+ messages in thread
From: Arvind Sankar @ 2019-12-15 19:30 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel, linux-efi, Hans de Goede, Matthew Garrett,
Ingo Molnar, Andy Lutomirski, Thomas Gleixner, Arvind Sankar
On Sat, Dec 14, 2019 at 06:57:35PM +0100, Ard Biesheuvel wrote:
>
> @@ -232,7 +232,7 @@ static inline bool efi_is_native(void)
> #define efi_table_attr(table, attr, instance) ({ \
> __typeof__(((table##_t *)0)->attr) __ret; \
> if (efi_is_native()) { \
> - __ret = ((table##_t *)instance)->attr; \
> + __ret = instance->attr; \
> } else { \
> __typeof__(((table##_32_t *)0)->attr) at; \
> at = (((table##_32_t *)(unsigned long)instance)->attr); \
Is there a reason we didn't remove this cast for native-mode earlier in
the series?
> @@ -242,19 +242,25 @@ static inline bool efi_is_native(void)
> })
>
> #define efi_call_proto(protocol, f, instance, ...) \
> - __efi_early()->call((unsigned long) \
> + efi_is_native() \
> + ? instance->f(instance, ##__VA_ARGS__) \
> + : efi64_thunk((unsigned long) \
> efi_table_attr(protocol, f, instance), \
> - instance, ##__VA_ARGS__)
> + instance, ##__VA_ARGS__)
>
> #define efi_call_early(f, ...) \
> - __efi_early()->call((unsigned long) \
> + efi_is_native() \
> + ? __efi_early()->boot_services->f(__VA_ARGS__) \
> + : efi64_thunk((unsigned long) \
> efi_table_attr(efi_boot_services, f, \
> - __efi_early()->boot_services), __VA_ARGS__)
> + __efi_early()->boot_services), __VA_ARGS__)
>
> #define efi_call_runtime(f, ...) \
> - __efi_early()->call((unsigned long) \
> + efi_is_native() \
> + ? __efi_early()->runtime_services->f(__VA_ARGS__) \
> + : efi64_thunk((unsigned long) \
> efi_table_attr(efi_runtime_services, f, \
> - __efi_early()->runtime_services), __VA_ARGS__)
> + __efi_early()->runtime_services), __VA_ARGS__)
>
> extern bool efi_reboot_required(void);
> extern bool efi_is_table_address(unsigned long phys_addr);
For the efi_call macros, their definition should be enclosed in
parentheses now that it's a ternary operator.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 10/10] efi/libstub/x86: avoid thunking for native firmware calls
2019-12-15 19:30 ` Arvind Sankar
@ 2019-12-17 8:32 ` Ard Biesheuvel
0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-12-17 8:32 UTC (permalink / raw)
To: Arvind Sankar
Cc: Ard Biesheuvel, Linux Kernel Mailing List, linux-efi,
Hans de Goede, Matthew Garrett, Ingo Molnar, Andy Lutomirski,
Thomas Gleixner
On Sun, 15 Dec 2019 at 21:31, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Sat, Dec 14, 2019 at 06:57:35PM +0100, Ard Biesheuvel wrote:
> >
> > @@ -232,7 +232,7 @@ static inline bool efi_is_native(void)
> > #define efi_table_attr(table, attr, instance) ({ \
> > __typeof__(((table##_t *)0)->attr) __ret; \
> > if (efi_is_native()) { \
> > - __ret = ((table##_t *)instance)->attr; \
> > + __ret = instance->attr; \
> > } else { \
> > __typeof__(((table##_32_t *)0)->attr) at; \
> > at = (((table##_32_t *)(unsigned long)instance)->attr); \
>
> Is there a reason we didn't remove this cast for native-mode earlier in
> the series?
>
Yes. In patch 9/10, I fix a couple of occurrences where the protocol
pointer is a void*, so without the cast, things break.
> > @@ -242,19 +242,25 @@ static inline bool efi_is_native(void)
> > })
> >
> > #define efi_call_proto(protocol, f, instance, ...) \
> > - __efi_early()->call((unsigned long) \
> > + efi_is_native() \
> > + ? instance->f(instance, ##__VA_ARGS__) \
> > + : efi64_thunk((unsigned long) \
> > efi_table_attr(protocol, f, instance), \
> > - instance, ##__VA_ARGS__)
> > + instance, ##__VA_ARGS__)
> >
> > #define efi_call_early(f, ...) \
> > - __efi_early()->call((unsigned long) \
> > + efi_is_native() \
> > + ? __efi_early()->boot_services->f(__VA_ARGS__) \
> > + : efi64_thunk((unsigned long) \
> > efi_table_attr(efi_boot_services, f, \
> > - __efi_early()->boot_services), __VA_ARGS__)
> > + __efi_early()->boot_services), __VA_ARGS__)
> >
> > #define efi_call_runtime(f, ...) \
> > - __efi_early()->call((unsigned long) \
> > + efi_is_native() \
> > + ? __efi_early()->runtime_services->f(__VA_ARGS__) \
> > + : efi64_thunk((unsigned long) \
> > efi_table_attr(efi_runtime_services, f, \
> > - __efi_early()->runtime_services), __VA_ARGS__)
> > + __efi_early()->runtime_services), __VA_ARGS__)
> >
> > extern bool efi_reboot_required(void);
> > extern bool efi_is_table_address(unsigned long phys_addr);
>
> For the efi_call macros, their definition should be enclosed in
> parentheses now that it's a ternary operator.
Ack
^ permalink raw reply [flat|nested] 29+ messages in thread