[v4,11/21] efi_loader: Introduce ms abi vararg helpers

Message ID 20180618152315.34233-12-agraf@suse.de
State New
Headers show
Series
  • sandbox: efi_loader support
Related show

Commit Message

Alexander Graf June 18, 2018, 3:23 p.m.
Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
abi though, so we also need to make sure we use x86_64 varargs helpers.

This patch introduces generic efi vararg helpers that adhere to the
respective EFI ABI. That way we can deal with them properly from efi
loader code and properly interpret variable arguments.

This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
on x86_64 for me.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 include/efi.h                 |  8 ++++++++
 lib/efi_loader/efi_boottime.c | 36 ++++++++++++++++++------------------
 2 files changed, 26 insertions(+), 18 deletions(-)

Comments

Simon Glass June 21, 2018, 2:02 a.m. | #1
Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
> abi though, so we also need to make sure we use x86_64 varargs helpers.
>
> This patch introduces generic efi vararg helpers that adhere to the
> respective EFI ABI. That way we can deal with them properly from efi
> loader code and properly interpret variable arguments.
>
> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
> on x86_64 for me.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  include/efi.h                 |  8 ++++++++
>  lib/efi_loader/efi_boottime.c | 36 ++++++++++++++++++------------------
>  2 files changed, 26 insertions(+), 18 deletions(-)

I thought this was a bug in gcc. Should we include this workaround
only for older gcc versions?

Regards,
Simon
Alexander Graf June 21, 2018, 9:40 a.m. | #2
On 06/21/2018 04:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>
>> This patch introduces generic efi vararg helpers that adhere to the
>> respective EFI ABI. That way we can deal with them properly from efi
>> loader code and properly interpret variable arguments.
>>
>> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
>> on x86_64 for me.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   include/efi.h                 |  8 ++++++++
>>   lib/efi_loader/efi_boottime.c | 36 ++++++++++++++++++------------------
>>   2 files changed, 26 insertions(+), 18 deletions(-)
> I thought this was a bug in gcc. Should we include this workaround
> only for older gcc versions?

The bug is something different - it's about using unannotated helper 
functions on an ms_abi declared vararg list. This patch really just 
implements expected behavior regardless of that bug.


Alex
Alexander Graf June 21, 2018, 3:13 p.m. | #3
> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
> abi though, so we also need to make sure we use x86_64 varargs helpers.
> 
> This patch introduces generic efi vararg helpers that adhere to the
> respective EFI ABI. That way we can deal with them properly from efi
> loader code and properly interpret variable arguments.
> 
> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
> on x86_64 for me.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks, applied to efi-next

Alex
Bin Meng June 23, 2018, 8:37 a.m. | #4
Hi Alex,

On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>
>> This patch introduces generic efi vararg helpers that adhere to the
>> respective EFI ABI. That way we can deal with them properly from efi
>> loader code and properly interpret variable arguments.
>>
>> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
>> on x86_64 for me.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> Thanks, applied to efi-next
>

I applied this patch on my x86 tree and tested there, but
qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
am missing?

Regards,
Bin
Alexander Graf June 25, 2018, 4:47 p.m. | #5
On 06/23/2018 10:37 AM, Bin Meng wrote:
> Hi Alex,
>
> On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the ms
>>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>>
>>> This patch introduces generic efi vararg helpers that adhere to the
>>> respective EFI ABI. That way we can deal with them properly from efi
>>> loader code and properly interpret variable arguments.
>>>
>>> This fixes the InstallMultipleProtocolInterfaces tests in the efi selftests
>>> on x86_64 for me.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Thanks, applied to efi-next
>>
> I applied this patch on my x86 tree and tested there, but
> qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
> am missing?

Where does it fail? There is basically this pitfall and setjmp/longjmp 
that can easily go wrong.


Alex
Bin Meng June 26, 2018, 1:51 a.m. | #6
Hi Alex,

On Tue, Jun 26, 2018 at 12:47 AM, Alexander Graf <agraf@suse.de> wrote:
> On 06/23/2018 10:37 AM, Bin Meng wrote:
>>
>> Hi Alex,
>>
>> On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the
>>>> ms
>>>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>>>
>>>> This patch introduces generic efi vararg helpers that adhere to the
>>>> respective EFI ABI. That way we can deal with them properly from efi
>>>> loader code and properly interpret variable arguments.
>>>>
>>>> This fixes the InstallMultipleProtocolInterfaces tests in the efi
>>>> selftests
>>>> on x86_64 for me.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> Thanks, applied to efi-next
>>>
>> I applied this patch on my x86 tree and tested there, but
>> qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
>> am missing?
>
>
> Where does it fail? There is basically this pitfall and setjmp/longjmp that
> can easily go wrong.

It just reboots without printing any error message. You can reproduce
this in the latest u-boot/master.

Regards,
Bin
Alexander Graf June 26, 2018, 11:18 a.m. | #7
On 06/26/2018 03:51 AM, Bin Meng wrote:
> Hi Alex,
>
> On Tue, Jun 26, 2018 at 12:47 AM, Alexander Graf <agraf@suse.de> wrote:
>> On 06/23/2018 10:37 AM, Bin Meng wrote:
>>> Hi Alex,
>>>
>>> On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>> Varargs differ between sysv and ms abi. On x86_64 we have to follow the
>>>>> ms
>>>>> abi though, so we also need to make sure we use x86_64 varargs helpers.
>>>>>
>>>>> This patch introduces generic efi vararg helpers that adhere to the
>>>>> respective EFI ABI. That way we can deal with them properly from efi
>>>>> loader code and properly interpret variable arguments.
>>>>>
>>>>> This fixes the InstallMultipleProtocolInterfaces tests in the efi
>>>>> selftests
>>>>> on x86_64 for me.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> Thanks, applied to efi-next
>>>>
>>> I applied this patch on my x86 tree and tested there, but
>>> qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
>>> am missing?
>>
>> Where does it fail? There is basically this pitfall and setjmp/longjmp that
>> can easily go wrong.
> It just reboots without printing any error message. You can reproduce
> this in the latest u-boot/master.

How do you run this? I tried the qemu x86_64 target, but it complains 
about a missing serial driver. I guess I need to pass some DT in?

Alex

$ qemu-system-x86_64 -nographic -pflash u-boot.rom -m 1G
WARNING: Image format was not specified for 'u-boot.rom' and probing 
guessed raw.
          Automatically detecting the format is dangerous for raw 
images, write operations on block 0 will be restricted.
          Specify the 'raw' format explicitly to remove the restrictions.

U-Boot SPL 2018.07-rc2-00066-gebea1f8367 (Jun 26 2018 - 13:13:34 +0200)
CPU: x86_64, vendor AMD, device 663h
Trying to boot from SPI
Jumping to 64-bit U-Boot: Note many features are missing
  No serial driver found
QEMU: Terminated
Bin Meng June 27, 2018, 2:59 a.m. | #8
Hi Alex,

On Tue, Jun 26, 2018 at 7:18 PM, Alexander Graf <agraf@suse.de> wrote:
> On 06/26/2018 03:51 AM, Bin Meng wrote:
>>
>> Hi Alex,
>>
>> On Tue, Jun 26, 2018 at 12:47 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06/23/2018 10:37 AM, Bin Meng wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On Thu, Jun 21, 2018 at 11:13 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>> Varargs differ between sysv and ms abi. On x86_64 we have to follow
>>>>>> the
>>>>>> ms
>>>>>> abi though, so we also need to make sure we use x86_64 varargs
>>>>>> helpers.
>>>>>>
>>>>>> This patch introduces generic efi vararg helpers that adhere to the
>>>>>> respective EFI ABI. That way we can deal with them properly from efi
>>>>>> loader code and properly interpret variable arguments.
>>>>>>
>>>>>> This fixes the InstallMultipleProtocolInterfaces tests in the efi
>>>>>> selftests
>>>>>> on x86_64 for me.
>>>>>>
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>
>>>>> Thanks, applied to efi-next
>>>>>
>>>> I applied this patch on my x86 tree and tested there, but
>>>> qemu-x86_64_defconfig still fails when 'bootefi selftest'. Anything I
>>>> am missing?
>>>
>>>
>>> Where does it fail? There is basically this pitfall and setjmp/longjmp
>>> that
>>> can easily go wrong.
>>
>> It just reboots without printing any error message. You can reproduce
>> this in the latest u-boot/master.
>
>
> How do you run this? I tried the qemu x86_64 target, but it complains about
> a missing serial driver. I guess I need to pass some DT in?
>

That's weird. I never saw such boot logs before. There is no need to pass DT in.

> Alex
>
> $ qemu-system-x86_64 -nographic -pflash u-boot.rom -m 1G

I was using '-bios' instead of '-pflash', but I just tested '-pflash'
and it worked too. I am using QEMU 2.5.0 (the one shipped on Ubuntu
16.04)

> WARNING: Image format was not specified for 'u-boot.rom' and probing guessed
> raw.
>          Automatically detecting the format is dangerous for raw images,
> write operations on block 0 will be restricted.
>          Specify the 'raw' format explicitly to remove the restrictions.
>
> U-Boot SPL 2018.07-rc2-00066-gebea1f8367 (Jun 26 2018 - 13:13:34 +0200)
> CPU: x86_64, vendor AMD, device 663h
> Trying to boot from SPI
> Jumping to 64-bit U-Boot: Note many features are missing
>  No serial driver found
> QEMU: Terminated
>

Regards,
Bin

Patch

diff --git a/include/efi.h b/include/efi.h
index 826d484977..7be7798d8d 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -22,8 +22,16 @@ 
 /* EFI on x86_64 uses the Microsoft ABI which is not the default for GCC */
 #ifdef __x86_64__
 #define EFIAPI __attribute__((ms_abi))
+#define efi_va_list __builtin_ms_va_list
+#define efi_va_start __builtin_ms_va_start
+#define efi_va_arg __builtin_va_arg
+#define efi_va_end __builtin_ms_va_end
 #else
 #define EFIAPI asmlinkage
+#define efi_va_list va_list
+#define efi_va_start va_start
+#define efi_va_arg va_arg
+#define efi_va_end va_end
 #endif /* __x86_64__ */
 
 struct efi_device_path;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 50d311548e..404743fe01 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2273,7 +2273,7 @@  static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 {
 	EFI_ENTRY("%p", handle);
 
-	va_list argptr;
+	efi_va_list argptr;
 	const efi_guid_t *protocol;
 	void *protocol_interface;
 	efi_status_t r = EFI_SUCCESS;
@@ -2282,12 +2282,12 @@  static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 	if (!handle)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
-	va_start(argptr, handle);
+	efi_va_start(argptr, handle);
 	for (;;) {
-		protocol = va_arg(argptr, efi_guid_t*);
+		protocol = efi_va_arg(argptr, efi_guid_t*);
 		if (!protocol)
 			break;
-		protocol_interface = va_arg(argptr, void*);
+		protocol_interface = efi_va_arg(argptr, void*);
 		r = EFI_CALL(efi_install_protocol_interface(
 						handle, protocol,
 						EFI_NATIVE_INTERFACE,
@@ -2296,19 +2296,19 @@  static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces(
 			break;
 		i++;
 	}
-	va_end(argptr);
+	efi_va_end(argptr);
 	if (r == EFI_SUCCESS)
 		return EFI_EXIT(r);
 
 	/* If an error occurred undo all changes. */
-	va_start(argptr, handle);
+	efi_va_start(argptr, handle);
 	for (; i; --i) {
-		protocol = va_arg(argptr, efi_guid_t*);
-		protocol_interface = va_arg(argptr, void*);
+		protocol = efi_va_arg(argptr, efi_guid_t*);
+		protocol_interface = efi_va_arg(argptr, void*);
 		EFI_CALL(efi_uninstall_protocol_interface(handle, protocol,
 							  protocol_interface));
 	}
-	va_end(argptr);
+	efi_va_end(argptr);
 
 	return EFI_EXIT(r);
 }
@@ -2332,7 +2332,7 @@  static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
 {
 	EFI_ENTRY("%p", handle);
 
-	va_list argptr;
+	efi_va_list argptr;
 	const efi_guid_t *protocol;
 	void *protocol_interface;
 	efi_status_t r = EFI_SUCCESS;
@@ -2341,12 +2341,12 @@  static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
 	if (!handle)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
-	va_start(argptr, handle);
+	efi_va_start(argptr, handle);
 	for (;;) {
-		protocol = va_arg(argptr, efi_guid_t*);
+		protocol = efi_va_arg(argptr, efi_guid_t*);
 		if (!protocol)
 			break;
-		protocol_interface = va_arg(argptr, void*);
+		protocol_interface = efi_va_arg(argptr, void*);
 		r = EFI_CALL(efi_uninstall_protocol_interface(
 						handle, protocol,
 						protocol_interface));
@@ -2354,20 +2354,20 @@  static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
 			break;
 		i++;
 	}
-	va_end(argptr);
+	efi_va_end(argptr);
 	if (r == EFI_SUCCESS)
 		return EFI_EXIT(r);
 
 	/* If an error occurred undo all changes. */
-	va_start(argptr, handle);
+	efi_va_start(argptr, handle);
 	for (; i; --i) {
-		protocol = va_arg(argptr, efi_guid_t*);
-		protocol_interface = va_arg(argptr, void*);
+		protocol = efi_va_arg(argptr, efi_guid_t*);
+		protocol_interface = efi_va_arg(argptr, void*);
 		EFI_CALL(efi_install_protocol_interface(&handle, protocol,
 							EFI_NATIVE_INTERFACE,
 							protocol_interface));
 	}
-	va_end(argptr);
+	efi_va_end(argptr);
 
 	return EFI_EXIT(r);
 }