efi/x86: confine type unsafe casting to mixed mode

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 <[email protected]>

---

include/linux/efi.h +10 -0

+#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

Signature · 1 line

2.17.1

Quoted 24 lines · Ard Biesheuvel

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 <[email protected]>

---

include/linux/efi.h +10 -1

+#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
--

Quoted 2 lines

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++)

Quoted 26 lines · Arvind Sankar

Quoted 24 lines · Ard Biesheuvel

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 <[email protected]>

---

include/linux/efi.h +10 -1

+#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
--

Quoted 9 lines

Quoted 2 lines

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.

Quoted 11 lines

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)

Quoted 52 lines · Ard Biesheuvel

Quoted 26 lines · Arvind Sankar

Quoted 24 lines · Ard Biesheuvel

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 <[email protected]>

---

include/linux/efi.h +10 -1

+#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
--

Quoted 9 lines

Quoted 2 lines

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.

Quoted 11 lines

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++)

Quoted 3 lines

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++)

Quoted 26 lines · Arvind Sankar

Quoted 24 lines · Ard Biesheuvel

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 <[email protected]>

---

include/linux/efi.h +10 -1

+#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
--

Quoted 9 lines

Quoted 2 lines

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.

Quoted 11 lines

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)

Quoted 10 lines · Ard Biesheuvel

arch/x86/include/asm/efi.h +10 -0

+
+#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;		\
+	}								\

arch/x86/include/asm/efi.h static inline bool efi_is_64bit(void) · line 242

Quoted 2 lines

arch/x86/include/asm/efi.h +2 -0

+	__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?

Quoted 11 lines · Arvind Sankar

Quoted 10 lines · Ard Biesheuvel

arch/x86/include/asm/efi.h +10 -0

+
+#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;           \
+   }                                                               \

Quoted 8 lines

arch/x86/include/asm/efi.h static inline bool efi_is_64bit(void) · line 242

Quoted 2 lines

arch/x86/include/asm/efi.h +2 -0

+   __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?

Quoted 22 lines · Arvind Sankar

Quoted 19 lines · Arvind Sankar

arch/x86/include/asm/efi.h static inline bool efi_is_64bit(void) · line 242

Quoted 12 lines · Ard Biesheuvel

arch/x86/include/asm/efi.h +12 -0

+
+#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?

Quoted 3 lines

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?

Quoted 27 lines · Ard Biesheuvel

Quoted 25 lines · Arvind Sankar

Quoted 11 lines · Arvind Sankar

Quoted 10 lines · Ard Biesheuvel

arch/x86/include/asm/efi.h +10 -0

+
+#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;           \
+   }                                                               \

Quoted 8 lines

arch/x86/include/asm/efi.h static inline bool efi_is_64bit(void) · line 242

Quoted 2 lines

arch/x86/include/asm/efi.h +2 -0

+   __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?

Quoted 1 line

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.

Quoted 6 lines · Arvind Sankar

Quoted 4 lines · Ard Biesheuvel

Quoted 3 lines · Arvind Sankar

Quoted 1 line · Arvind Sankar

On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

arch/x86/include/asm/efi.h +5 -0

+
+#define efi_table_attr(table, attr, instance) ({                   \
+   __typeof__(((table##_t *)0)->attr) __ret;                       \
+   if (efi_is_native()) {                                          \
+           __ret = ((table##_t *)instance)->attr;                  \

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/x86/include/asm/efi.h static inline bool efi_is_64bit(void) · line 242

Quoted 7 lines

arch/x86/include/asm/efi.h +7 -0

+   } 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.

Quoted 20 lines · Ard Biesheuvel

Quoted 6 lines · Arvind Sankar

Quoted 4 lines · Ard Biesheuvel

Quoted 3 lines · Arvind Sankar

Quoted 1 line · Arvind Sankar

On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+#define efi_table_attr(table, attr, instance) ({                   \
+   __typeof__(((table##_t *)0)->attr) __ret;                       \
+   if (efi_is_native()) {                                          \
+           __ret = ((table##_t *)instance)->attr;                  \

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+   } else {                                                        \
+           __typeof__(((table##_32_t *)0)->attr) at;               \
+           at = (((table##_32_t *)(unsigned long)instance)->attr); \
+           __ret = (__typeof__(__ret))(unsigned long)at;           \
+   }                                                               \
+   __ret;                                                          \
+})

Quoted 4 lines

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.

Quoted 7 lines · Arvind Sankar

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,

Quoted 27 lines · Arvind Sankar

Quoted 20 lines · Ard Biesheuvel

Quoted 6 lines · Arvind Sankar

Quoted 4 lines · Ard Biesheuvel

Quoted 3 lines · Arvind Sankar

Quoted 1 line · Arvind Sankar

On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+#define efi_table_attr(table, attr, instance) ({                   \
+   __typeof__(((table##_t *)0)->attr) __ret;                       \
+   if (efi_is_native()) {                                          \
+           __ret = ((table##_t *)instance)->attr;                  \

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+   } else {                                                        \
+           __typeof__(((table##_32_t *)0)->attr) at;               \
+           at = (((table##_32_t *)(unsigned long)instance)->attr); \
+           __ret = (__typeof__(__ret))(unsigned long)at;           \
+   }                                                               \
+   __ret;                                                          \
+})

Quoted 4 lines

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.

Quoted 6 lines

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,

Quoted 36 lines · Ard Biesheuvel

Quoted 33 lines · Arvind Sankar

Quoted 20 lines · Ard Biesheuvel

Quoted 6 lines · Arvind Sankar

Quoted 4 lines · Ard Biesheuvel

Quoted 3 lines · Arvind Sankar

Quoted 1 line · Arvind Sankar

On Sat, Dec 14, 2019 at 06:57:30PM +0100, Ard Biesheuvel wrote:

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+#define efi_table_attr(table, attr, instance) ({                   \
+   __typeof__(((table##_t *)0)->attr) __ret;                       \
+   if (efi_is_native()) {                                          \
+           __ret = ((table##_t *)instance)->attr;                  \

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+   } else {                                                        \
+           __typeof__(((table##_32_t *)0)->attr) at;               \
+           at = (((table##_32_t *)(unsigned long)instance)->attr); \
+           __ret = (__typeof__(__ret))(unsigned long)at;           \
+   }                                                               \
+   __ret;                                                          \
+})

Quoted 4 lines

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.

Quoted 2 lines

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.

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 <[email protected]>

---
 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(-)

arch/x86/Kconfig +1 -0

index 5e8949953660..8ba81036a7ef 100644
@@ -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

arch/x86/boot/compressed/eboot.h +2 -2

index 318531f128c2..512d2143a94f 100644
@@ -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;

drivers/firmware/efi/libstub/random.c +4 -4

index 3b85883fb312..872d2de3eaaf 100644
@@ -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,

include/linux/efi.h +0 -0

index 7f1a99bd2c05..2ba13d901055 100644

arch/x86/include/asm/efi.h +8 -4

@@ -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, ...)					\

arch/x86/include/asm/efi.h static inline bool efi_is_native(void) · line 238

Quoted 10 lines · Ard Biesheuvel

arch/x86/include/asm/efi.h +1 -1

@@ -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?

Quoted 30 lines

arch/x86/include/asm/efi.h +12 -6

@@ -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__)
 

arch/x86/include/asm/efi.h static inline bool efi_is_native(void) · line 266

Quoted 2 lines

arch/x86/include/asm/efi.h +0 -0

 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.