* [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
@ 2016-12-01 15:28 Anthony PERARD
2016-12-01 15:28 ` [PATCH 1/4] OvmfPkg/XenHypercallLib: Add EFIAPI Anthony PERARD
` (4 more replies)
0 siblings, 5 replies; 29+ messages in thread
From: Anthony PERARD @ 2016-12-01 15:28 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Anthony PERARD
Hi,
That might be only with the Xen part of OVMF but now that the GCC5
toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
to boot in Xen guests.
Here is the result:
!!!! X64 Exception Type - 06(#UD - Invalid Opcode) CPU Apic ID - 00000000 !!!!
RIP - 000000001F26AF6B, CS - 0000000000000038, RFLAGS - 0000000000010202
RAX - 0000000000000001, RCX - 000000001F26AF51, RDX - 0000000000000004
RBX - 0000000000000000, RSP - 000000001F43C510, RBP - 000000001E583D18
RSI - 0000000000000003, RDI - 0000000000000001
R8 - 0000000000000000, R9 - 0000000000000000, R10 - 000000001E58DB98
R11 - 0000000000000002, R12 - 000000001E58D898, R13 - 0000000000000000
R14 - 000000001E58D8A0, R15 - 000000001F26D001
DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030
GS - 0000000000000030, SS - 0000000000000030
CR0 - 0000000080000033, CR2 - 0000000000000000, CR3 - 000000001F3DB000
CR4 - 0000000000000668, CR8 - 0000000000000000
DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
GDTR - 000000001F3C9A98 0000000000000047, LDTR - 0000000000000000
IDTR - 000000001EB0A018 0000000000000FFF, TR - 0000000000000000
FXSAVE_STATE - 000000001F43C170
!!!! Find PE image ./Build/OvmfX64/DEBUG_GCC5/X64/OvmfPkg/XenBusDxe/XenBusDxe/DEBUG/XenBusDxe.dll (ImageBase=000000001F266000, EntryPoint=000000001F2669D5) !!!!
Removing the gcc option -flto in only the XenBusDxe module makes OVMF
boot.
While trying to debug that, I've added some debug prints (in this module
and in XenPvBlkDxe), and the exception could change and become a "page
fault" instead, or even an assert failure in the PrintLib, that was the
ASSERT(Buffer != NULL) at I think
MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
Adding EFIAPI to internal functions in XenBusDxe makes things work
again. My guest is that gcc would bypass (optimise) an exported
functions and call directly an internal one but without reordering the
arguments (EFIAPI vs nothing).
Does that make sense?
Anthony PERARD (4):
OvmfPkg/XenHypercallLib: Add EFIAPI
OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify
OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions
OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant,End}Access
OvmfPkg/Include/Library/XenHypercallLib.h | 3 +++
OvmfPkg/Library/XenHypercallLib/XenHypercall.c | 3 +++
OvmfPkg/XenBusDxe/EventChannel.c | 1 +
OvmfPkg/XenBusDxe/EventChannel.h | 1 +
OvmfPkg/XenBusDxe/GrantTable.c | 2 ++
OvmfPkg/XenBusDxe/XenStore.c | 13 +++++++++++++
OvmfPkg/XenBusDxe/XenStore.h | 10 ++++++++++
7 files changed, 33 insertions(+)
--
Anthony PERARD
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/4] OvmfPkg/XenHypercallLib: Add EFIAPI
2016-12-01 15:28 [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Anthony PERARD
@ 2016-12-01 15:28 ` Anthony PERARD
2016-12-01 15:28 ` [PATCH 2/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify Anthony PERARD
` (3 subsequent siblings)
4 siblings, 0 replies; 29+ messages in thread
From: Anthony PERARD @ 2016-12-01 15:28 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Anthony PERARD
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/Include/Library/XenHypercallLib.h | 3 +++
OvmfPkg/Library/XenHypercallLib/XenHypercall.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/OvmfPkg/Include/Library/XenHypercallLib.h b/OvmfPkg/Include/Library/XenHypercallLib.h
index 509855d..36e3344 100644
--- a/OvmfPkg/Include/Library/XenHypercallLib.h
+++ b/OvmfPkg/Include/Library/XenHypercallLib.h
@@ -58,6 +58,7 @@ XenHypercall2 (
@return The value of the asked parameter or 0 in case of error.
**/
UINT64
+EFIAPI
XenHypercallHvmGetParam (
UINT32 Index
);
@@ -72,6 +73,7 @@ XenHypercallHvmGetParam (
otherwise, an error code.
**/
INTN
+EFIAPI
XenHypercallMemoryOp (
IN UINTN Operation,
IN OUT VOID *Arguments
@@ -87,6 +89,7 @@ XenHypercallMemoryOp (
otherwise, an error code.
**/
INTN
+EFIAPI
XenHypercallEventChannelOp (
IN INTN Operation,
IN OUT VOID *Arguments
diff --git a/OvmfPkg/Library/XenHypercallLib/XenHypercall.c b/OvmfPkg/Library/XenHypercallLib/XenHypercall.c
index 82cdbd9..5ea5c45 100644
--- a/OvmfPkg/Library/XenHypercallLib/XenHypercall.c
+++ b/OvmfPkg/Library/XenHypercallLib/XenHypercall.c
@@ -22,6 +22,7 @@
#include <Library/XenHypercallLib.h>
UINT64
+EFIAPI
XenHypercallHvmGetParam (
IN UINT32 Index
)
@@ -43,6 +44,7 @@ XenHypercallHvmGetParam (
}
INTN
+EFIAPI
XenHypercallMemoryOp (
IN UINTN Operation,
IN OUT VOID *Arguments
@@ -53,6 +55,7 @@ XenHypercallMemoryOp (
}
INTN
+EFIAPI
XenHypercallEventChannelOp (
IN INTN Operation,
IN OUT VOID *Arguments
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify
2016-12-01 15:28 [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Anthony PERARD
2016-12-01 15:28 ` [PATCH 1/4] OvmfPkg/XenHypercallLib: Add EFIAPI Anthony PERARD
@ 2016-12-01 15:28 ` Anthony PERARD
2016-12-01 15:28 ` [PATCH 3/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions Anthony PERARD
` (2 subsequent siblings)
4 siblings, 0 replies; 29+ messages in thread
From: Anthony PERARD @ 2016-12-01 15:28 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Anthony PERARD
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/EventChannel.c | 1 +
OvmfPkg/XenBusDxe/EventChannel.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/OvmfPkg/XenBusDxe/EventChannel.c b/OvmfPkg/XenBusDxe/EventChannel.c
index 490ca58..b9d919f 100644
--- a/OvmfPkg/XenBusDxe/EventChannel.c
+++ b/OvmfPkg/XenBusDxe/EventChannel.c
@@ -20,6 +20,7 @@
#include <Library/XenHypercallLib.h>
UINT32
+EFIAPI
XenEventChannelNotify (
IN XENBUS_DEVICE *Dev,
IN evtchn_port_t Port
diff --git a/OvmfPkg/XenBusDxe/EventChannel.h b/OvmfPkg/XenBusDxe/EventChannel.h
index 4dcc20f..24520e6 100644
--- a/OvmfPkg/XenBusDxe/EventChannel.h
+++ b/OvmfPkg/XenBusDxe/EventChannel.h
@@ -28,6 +28,7 @@
@return Return 0 on success, or return the errno code from the hypercall.
**/
UINT32
+EFIAPI
XenEventChannelNotify (
IN XENBUS_DEVICE *Dev,
IN evtchn_port_t Port
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 3/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions
2016-12-01 15:28 [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Anthony PERARD
2016-12-01 15:28 ` [PATCH 1/4] OvmfPkg/XenHypercallLib: Add EFIAPI Anthony PERARD
2016-12-01 15:28 ` [PATCH 2/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify Anthony PERARD
@ 2016-12-01 15:28 ` Anthony PERARD
2016-12-01 15:28 ` [PATCH 4/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant, End}Access Anthony PERARD
2016-12-01 18:43 ` [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Laszlo Ersek
4 siblings, 0 replies; 29+ messages in thread
From: Anthony PERARD @ 2016-12-01 15:28 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Anthony PERARD
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/XenStore.c | 13 +++++++++++++
OvmfPkg/XenBusDxe/XenStore.h | 10 ++++++++++
2 files changed, 23 insertions(+)
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 1666c4b..d69ed7d 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -791,6 +791,7 @@ XenStoreReadReply (
**/
STATIC
XENSTORE_STATUS
+EFIAPI
XenStoreTalkv (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN enum xsd_sockmsg_type RequestType,
@@ -874,6 +875,7 @@ Error:
**/
STATIC
XENSTORE_STATUS
+EFIAPI
XenStoreSingle (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN enum xsd_sockmsg_type RequestType,
@@ -949,6 +951,7 @@ XenStoreUnwatch (
STATIC
XENSTORE_STATUS
+EFIAPI
XenStoreWaitWatch (
VOID *Token
)
@@ -1162,6 +1165,7 @@ XenStoreDeinit (
//
XENSTORE_STATUS
+EFIAPI
XenStoreListDirectory (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -1189,6 +1193,7 @@ XenStoreListDirectory (
}
BOOLEAN
+EFIAPI
XenStorePathExists (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *Directory,
@@ -1209,6 +1214,7 @@ XenStorePathExists (
}
XENSTORE_STATUS
+EFIAPI
XenStoreRead (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -1233,6 +1239,7 @@ XenStoreRead (
}
XENSTORE_STATUS
+EFIAPI
XenStoreWrite (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -1258,6 +1265,7 @@ XenStoreWrite (
}
XENSTORE_STATUS
+EFIAPI
XenStoreRemove (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -1275,6 +1283,7 @@ XenStoreRemove (
}
XENSTORE_STATUS
+EFIAPI
XenStoreTransactionStart (
OUT XENSTORE_TRANSACTION *Transaction
)
@@ -1293,6 +1302,7 @@ XenStoreTransactionStart (
}
XENSTORE_STATUS
+EFIAPI
XenStoreTransactionEnd (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN BOOLEAN Abort
@@ -1307,6 +1317,7 @@ XenStoreTransactionEnd (
}
XENSTORE_STATUS
+EFIAPI
XenStoreVSPrint (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -1352,6 +1363,7 @@ XenStoreSPrint (
}
XENSTORE_STATUS
+EFIAPI
XenStoreRegisterWatch (
IN CONST CHAR8 *DirectoryPath,
IN CONST CHAR8 *Node,
@@ -1393,6 +1405,7 @@ XenStoreRegisterWatch (
}
VOID
+EFIAPI
XenStoreUnregisterWatch (
IN XENSTORE_WATCH *Watch
)
diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
index c9d4c65..c38fe37 100644
--- a/OvmfPkg/XenBusDxe/XenStore.h
+++ b/OvmfPkg/XenBusDxe/XenStore.h
@@ -53,6 +53,7 @@ typedef struct _XENSTORE_WATCH XENSTORE_WATCH;
caller.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreListDirectory (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -73,6 +74,7 @@ XenStoreListDirectory (
to make that determination.
**/
BOOLEAN
+EFIAPI
XenStorePathExists (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *Directory,
@@ -97,6 +99,7 @@ XenStorePathExists (
caller.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreRead (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -117,6 +120,7 @@ XenStoreRead (
indicating the type of failure.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreWrite (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -135,6 +139,7 @@ XenStoreWrite (
indicating the type of failure.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreRemove (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -154,6 +159,7 @@ XenStoreRemove (
indicating the type of failure.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreTransactionStart (
OUT XENSTORE_TRANSACTION *Transaction
);
@@ -169,6 +175,7 @@ XenStoreTransactionStart (
indicating the type of failure.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreTransactionEnd (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN BOOLEAN Abort
@@ -209,6 +216,7 @@ XenStoreSPrint (
indicating the type of write failure.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreVSPrint (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
@@ -233,6 +241,7 @@ XenStoreVSPrint (
xenbus_watch objects, to watch the same path in the XenStore.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreRegisterWatch (
IN CONST CHAR8 *DirectoryPath,
IN CONST CHAR8 *Node,
@@ -246,6 +255,7 @@ XenStoreRegisterWatch (
call to XenStoreRegisterWatch ().
**/
VOID
+EFIAPI
XenStoreUnregisterWatch (
IN XENSTORE_WATCH *Watch
);
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 4/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant, End}Access
2016-12-01 15:28 [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Anthony PERARD
` (2 preceding siblings ...)
2016-12-01 15:28 ` [PATCH 3/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions Anthony PERARD
@ 2016-12-01 15:28 ` Anthony PERARD
2016-12-01 18:43 ` [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Laszlo Ersek
4 siblings, 0 replies; 29+ messages in thread
From: Anthony PERARD @ 2016-12-01 15:28 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Anthony PERARD
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenBusDxe/GrantTable.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c
index 5c4b528..2448585 100644
--- a/OvmfPkg/XenBusDxe/GrantTable.c
+++ b/OvmfPkg/XenBusDxe/GrantTable.c
@@ -90,6 +90,7 @@ XenGrantTableGetFreeEntry (
STATIC
grant_ref_t
+EFIAPI
XenGrantTableGrantAccess (
IN domid_t DomainId,
IN UINTN Frame,
@@ -115,6 +116,7 @@ XenGrantTableGrantAccess (
STATIC
EFI_STATUS
+EFIAPI
XenGrantTableEndAccess (
grant_ref_t Ref
)
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-01 15:28 [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Anthony PERARD
` (3 preceding siblings ...)
2016-12-01 15:28 ` [PATCH 4/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant, End}Access Anthony PERARD
@ 2016-12-01 18:43 ` Laszlo Ersek
2016-12-01 20:06 ` Jordan Justen
` (2 more replies)
4 siblings, 3 replies; 29+ messages in thread
From: Laszlo Ersek @ 2016-12-01 18:43 UTC (permalink / raw)
To: Anthony PERARD, Jordan Justen, Gao, Liming, Zhu, Yonghong,
Ard Biesheuvel
Cc: edk2-devel
On 12/01/16 16:28, Anthony PERARD wrote:
> Hi,
>
> That might be only with the Xen part of OVMF but now that the GCC5
> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
> to boot in Xen guests.
>
> Here is the result:
> !!!! X64 Exception Type - 06(#UD - Invalid Opcode) CPU Apic ID - 00000000 !!!!
> RIP - 000000001F26AF6B, CS - 0000000000000038, RFLAGS - 0000000000010202
> RAX - 0000000000000001, RCX - 000000001F26AF51, RDX - 0000000000000004
> RBX - 0000000000000000, RSP - 000000001F43C510, RBP - 000000001E583D18
> RSI - 0000000000000003, RDI - 0000000000000001
> R8 - 0000000000000000, R9 - 0000000000000000, R10 - 000000001E58DB98
> R11 - 0000000000000002, R12 - 000000001E58D898, R13 - 0000000000000000
> R14 - 000000001E58D8A0, R15 - 000000001F26D001
> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030
> GS - 0000000000000030, SS - 0000000000000030
> CR0 - 0000000080000033, CR2 - 0000000000000000, CR3 - 000000001F3DB000
> CR4 - 0000000000000668, CR8 - 0000000000000000
> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> GDTR - 000000001F3C9A98 0000000000000047, LDTR - 0000000000000000
> IDTR - 000000001EB0A018 0000000000000FFF, TR - 0000000000000000
> FXSAVE_STATE - 000000001F43C170
> !!!! Find PE image ./Build/OvmfX64/DEBUG_GCC5/X64/OvmfPkg/XenBusDxe/XenBusDxe/DEBUG/XenBusDxe.dll (ImageBase=000000001F266000, EntryPoint=000000001F2669D5) !!!!
>
> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
> boot.
>
> While trying to debug that, I've added some debug prints (in this module
> and in XenPvBlkDxe), and the exception could change and become a "page
> fault" instead, or even an assert failure in the PrintLib, that was the
> ASSERT(Buffer != NULL) at I think
> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>
> Adding EFIAPI to internal functions in XenBusDxe makes things work
> again. My guest is that gcc would bypass (optimise) an exported
> functions and call directly an internal one but without reordering the
> arguments (EFIAPI vs nothing).
>
> Does that make sense?
Thank you for the investigation. It is strange that only the Xen modules
are affected, I'm unsure what that's the case.
Either way, it seems to be a gcc-6 bug, or an edk2 toolchain bug. You
should *not* need EFIAPI for functions with external linkage if the
calls to them straddle only files in the same module. I'm suspecting
gcc-6 (we've received no such reports with gcc-5). Maybe we need a GCC6
toolchain as well, for turning off some new features in gcc-6?
Jordan, Liming, Yonghong, Ard -- any ideas?
Anthony: while we all figure this out, please consider building OVMF
with the "-b NOOPT" switch. Support for the NOOPT build target has
recently been added to the GCC Ia32/X64 toolchains in BaseTools, and to
the OVMF DSC files as well. The build targets correspond to:
RELEASE -- compiler optimization enabled; DEBUG, ASSERT, and similar
DebugLib features compiled out
DEBUG -- compiler optimization enabled; DebugLib features preserved
NOOPT -- compiler optimization disabled; DebugLib features preserved
(Note that for ArmVirtPkg and the GCC AARCH64 toolchains in BaseTools,
there is no NOOPT, and DEBUG means actually NOOPT -- if I remember
correctly. Ard will correct me if I'm wrong :))
If "-b NOOPT" works for you, I'd prefer that as a temporary solution
(until the root cause is found and addressed) to the XenBusDxe patches.
Hrpmf, wait a second, I do see something interesting: in this series you
*are* modifying APIs declared in a library class header (namely
"OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
libraries) *are* required to specify EFIAPI.
What happens if you apply patch #1 only?
Thanks!
Laszlo
> Anthony PERARD (4):
> OvmfPkg/XenHypercallLib: Add EFIAPI
> OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify
> OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions
> OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant,End}Access
>
> OvmfPkg/Include/Library/XenHypercallLib.h | 3 +++
> OvmfPkg/Library/XenHypercallLib/XenHypercall.c | 3 +++
> OvmfPkg/XenBusDxe/EventChannel.c | 1 +
> OvmfPkg/XenBusDxe/EventChannel.h | 1 +
> OvmfPkg/XenBusDxe/GrantTable.c | 2 ++
> OvmfPkg/XenBusDxe/XenStore.c | 13 +++++++++++++
> OvmfPkg/XenBusDxe/XenStore.h | 10 ++++++++++
> 7 files changed, 33 insertions(+)
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-01 18:43 ` [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Laszlo Ersek
@ 2016-12-01 20:06 ` Jordan Justen
2016-12-01 20:54 ` Laszlo Ersek
2016-12-02 4:36 ` Gao, Liming
2016-12-02 16:02 ` Anthony PERARD
2 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2016-12-01 20:06 UTC (permalink / raw)
To: Laszlo Ersek, Anthony PERARD, Gao, Liming, Zhu, Yonghong,
Ard Biesheuvel
Cc: edk2-devel
On 2016-12-01 10:43:24, Laszlo Ersek wrote:
> On 12/01/16 16:28, Anthony PERARD wrote:
> > Hi,
> >
> > That might be only with the Xen part of OVMF but now that the GCC5
> > toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
> > to boot in Xen guests.
> >
> > Here is the result:
> > !!!! X64 Exception Type - 06(#UD - Invalid Opcode) CPU Apic ID - 00000000 !!!!
> > RIP - 000000001F26AF6B, CS - 0000000000000038, RFLAGS - 0000000000010202
> > RAX - 0000000000000001, RCX - 000000001F26AF51, RDX - 0000000000000004
> > RBX - 0000000000000000, RSP - 000000001F43C510, RBP - 000000001E583D18
> > RSI - 0000000000000003, RDI - 0000000000000001
> > R8 - 0000000000000000, R9 - 0000000000000000, R10 - 000000001E58DB98
> > R11 - 0000000000000002, R12 - 000000001E58D898, R13 - 0000000000000000
> > R14 - 000000001E58D8A0, R15 - 000000001F26D001
> > DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030
> > GS - 0000000000000030, SS - 0000000000000030
> > CR0 - 0000000080000033, CR2 - 0000000000000000, CR3 - 000000001F3DB000
> > CR4 - 0000000000000668, CR8 - 0000000000000000
> > DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> > DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> > GDTR - 000000001F3C9A98 0000000000000047, LDTR - 0000000000000000
> > IDTR - 000000001EB0A018 0000000000000FFF, TR - 0000000000000000
> > FXSAVE_STATE - 000000001F43C170
> > !!!! Find PE image ./Build/OvmfX64/DEBUG_GCC5/X64/OvmfPkg/XenBusDxe/XenBusDxe/DEBUG/XenBusDxe.dll (ImageBase=000000001F266000, EntryPoint=000000001F2669D5) !!!!
> >
> > Removing the gcc option -flto in only the XenBusDxe module makes OVMF
> > boot.
> >
> > While trying to debug that, I've added some debug prints (in this module
> > and in XenPvBlkDxe), and the exception could change and become a "page
> > fault" instead, or even an assert failure in the PrintLib, that was the
> > ASSERT(Buffer != NULL) at I think
> > MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
> >
> > Adding EFIAPI to internal functions in XenBusDxe makes things work
> > again. My guest is that gcc would bypass (optimise) an exported
> > functions and call directly an internal one but without reordering the
> > arguments (EFIAPI vs nothing).
> >
> > Does that make sense?
>
> Thank you for the investigation. It is strange that only the Xen modules
> are affected, I'm unsure what that's the case.
>
> Either way, it seems to be a gcc-6 bug, or an edk2 toolchain bug. You
> should *not* need EFIAPI for functions with external linkage if the
> calls to them straddle only files in the same module. I'm suspecting
> gcc-6 (we've received no such reports with gcc-5). Maybe we need a GCC6
> toolchain as well, for turning off some new features in gcc-6?
>
> Jordan, Liming, Yonghong, Ard -- any ideas?
>
> Anthony: while we all figure this out, please consider building OVMF
> with the "-b NOOPT" switch. Support for the NOOPT build target has
> recently been added to the GCC Ia32/X64 toolchains in BaseTools, and to
> the OVMF DSC files as well. The build targets correspond to:
>
> RELEASE -- compiler optimization enabled; DEBUG, ASSERT, and similar
> DebugLib features compiled out
> DEBUG -- compiler optimization enabled; DebugLib features preserved
> NOOPT -- compiler optimization disabled; DebugLib features preserved
>
> (Note that for ArmVirtPkg and the GCC AARCH64 toolchains in BaseTools,
> there is no NOOPT, and DEBUG means actually NOOPT -- if I remember
> correctly. Ard will correct me if I'm wrong :))
>
> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
> (until the root cause is found and addressed) to the XenBusDxe patches.
>
> Hrpmf, wait a second, I do see something interesting: in this series you
> *are* modifying APIs declared in a library class header (namely
> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
> libraries) *are* required to specify EFIAPI.
>
> What happens if you apply patch #1 only?
I agree that this should be fixed.
But, if it works, I'm concerned that it would just be hiding a bug. My
understanding was that the EFIAPI on libraries was needed so that a
library implementation could be assembly based if desired. In this
case C is used for the implementation, so the calling conventions
should align.
-Jordan
>
> > Anthony PERARD (4):
> > OvmfPkg/XenHypercallLib: Add EFIAPI
> > OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify
> > OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions
> > OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant,End}Access
> >
> > OvmfPkg/Include/Library/XenHypercallLib.h | 3 +++
> > OvmfPkg/Library/XenHypercallLib/XenHypercall.c | 3 +++
> > OvmfPkg/XenBusDxe/EventChannel.c | 1 +
> > OvmfPkg/XenBusDxe/EventChannel.h | 1 +
> > OvmfPkg/XenBusDxe/GrantTable.c | 2 ++
> > OvmfPkg/XenBusDxe/XenStore.c | 13 +++++++++++++
> > OvmfPkg/XenBusDxe/XenStore.h | 10 ++++++++++
> > 7 files changed, 33 insertions(+)
> >
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-01 20:06 ` Jordan Justen
@ 2016-12-01 20:54 ` Laszlo Ersek
2016-12-02 0:58 ` Jordan Justen
0 siblings, 1 reply; 29+ messages in thread
From: Laszlo Ersek @ 2016-12-01 20:54 UTC (permalink / raw)
To: Jordan Justen, Anthony PERARD, Gao, Liming, Zhu, Yonghong,
Ard Biesheuvel
Cc: edk2-devel
On 12/01/16 21:06, Jordan Justen wrote:
> On 2016-12-01 10:43:24, Laszlo Ersek wrote:
>> On 12/01/16 16:28, Anthony PERARD wrote:
>>> Hi,
>>>
>>> That might be only with the Xen part of OVMF but now that the GCC5
>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
>>> to boot in Xen guests.
>>>
>>> Here is the result:
>>> !!!! X64 Exception Type - 06(#UD - Invalid Opcode) CPU Apic ID - 00000000 !!!!
>>> RIP - 000000001F26AF6B, CS - 0000000000000038, RFLAGS - 0000000000010202
>>> RAX - 0000000000000001, RCX - 000000001F26AF51, RDX - 0000000000000004
>>> RBX - 0000000000000000, RSP - 000000001F43C510, RBP - 000000001E583D18
>>> RSI - 0000000000000003, RDI - 0000000000000001
>>> R8 - 0000000000000000, R9 - 0000000000000000, R10 - 000000001E58DB98
>>> R11 - 0000000000000002, R12 - 000000001E58D898, R13 - 0000000000000000
>>> R14 - 000000001E58D8A0, R15 - 000000001F26D001
>>> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030
>>> GS - 0000000000000030, SS - 0000000000000030
>>> CR0 - 0000000080000033, CR2 - 0000000000000000, CR3 - 000000001F3DB000
>>> CR4 - 0000000000000668, CR8 - 0000000000000000
>>> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
>>> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
>>> GDTR - 000000001F3C9A98 0000000000000047, LDTR - 0000000000000000
>>> IDTR - 000000001EB0A018 0000000000000FFF, TR - 0000000000000000
>>> FXSAVE_STATE - 000000001F43C170
>>> !!!! Find PE image ./Build/OvmfX64/DEBUG_GCC5/X64/OvmfPkg/XenBusDxe/XenBusDxe/DEBUG/XenBusDxe.dll (ImageBase=000000001F266000, EntryPoint=000000001F2669D5) !!!!
>>>
>>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
>>> boot.
>>>
>>> While trying to debug that, I've added some debug prints (in this module
>>> and in XenPvBlkDxe), and the exception could change and become a "page
>>> fault" instead, or even an assert failure in the PrintLib, that was the
>>> ASSERT(Buffer != NULL) at I think
>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>>
>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
>>> again. My guest is that gcc would bypass (optimise) an exported
>>> functions and call directly an internal one but without reordering the
>>> arguments (EFIAPI vs nothing).
>>>
>>> Does that make sense?
>>
>> Thank you for the investigation. It is strange that only the Xen modules
>> are affected, I'm unsure what that's the case.
>>
>> Either way, it seems to be a gcc-6 bug, or an edk2 toolchain bug. You
>> should *not* need EFIAPI for functions with external linkage if the
>> calls to them straddle only files in the same module. I'm suspecting
>> gcc-6 (we've received no such reports with gcc-5). Maybe we need a GCC6
>> toolchain as well, for turning off some new features in gcc-6?
>>
>> Jordan, Liming, Yonghong, Ard -- any ideas?
>>
>> Anthony: while we all figure this out, please consider building OVMF
>> with the "-b NOOPT" switch. Support for the NOOPT build target has
>> recently been added to the GCC Ia32/X64 toolchains in BaseTools, and to
>> the OVMF DSC files as well. The build targets correspond to:
>>
>> RELEASE -- compiler optimization enabled; DEBUG, ASSERT, and similar
>> DebugLib features compiled out
>> DEBUG -- compiler optimization enabled; DebugLib features preserved
>> NOOPT -- compiler optimization disabled; DebugLib features preserved
>>
>> (Note that for ArmVirtPkg and the GCC AARCH64 toolchains in BaseTools,
>> there is no NOOPT, and DEBUG means actually NOOPT -- if I remember
>> correctly. Ard will correct me if I'm wrong :))
>>
>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
>> (until the root cause is found and addressed) to the XenBusDxe patches.
>>
>> Hrpmf, wait a second, I do see something interesting: in this series you
>> *are* modifying APIs declared in a library class header (namely
>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>> libraries) *are* required to specify EFIAPI.
>>
>> What happens if you apply patch #1 only?
>
> I agree that this should be fixed.
>
> But, if it works, I'm concerned that it would just be hiding a bug. My
> understanding was that the EFIAPI on libraries was needed so that a
> library implementation could be assembly based if desired. In this
> case C is used for the implementation, so the calling conventions
> should align.
Never tried the following, so I'm unsure if edk2 intends to support it
explicitly, but what about binary-only library instances (the 2-clause
BSDL allows that)?
If the library class header doesn't state EFIAPI on the functions, the
library vendor builds the library instance with Visual Studio, and the
library user builds the client module with gcc (against the same library
class header), calls will fail.
(The way I imagine using binary-only library instances is that the
library comes as a binary object with a matching INF file, in a separate
subdirectory, and the user resolves the library class in his/her
platform DSC to that INF file. Not sure about the exact [section names]
in the library instance's INF file though.)
Thanks!
Laszlo
>
> -Jordan
>
>>
>>> Anthony PERARD (4):
>>> OvmfPkg/XenHypercallLib: Add EFIAPI
>>> OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify
>>> OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions
>>> OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant,End}Access
>>>
>>> OvmfPkg/Include/Library/XenHypercallLib.h | 3 +++
>>> OvmfPkg/Library/XenHypercallLib/XenHypercall.c | 3 +++
>>> OvmfPkg/XenBusDxe/EventChannel.c | 1 +
>>> OvmfPkg/XenBusDxe/EventChannel.h | 1 +
>>> OvmfPkg/XenBusDxe/GrantTable.c | 2 ++
>>> OvmfPkg/XenBusDxe/XenStore.c | 13 +++++++++++++
>>> OvmfPkg/XenBusDxe/XenStore.h | 10 ++++++++++
>>> 7 files changed, 33 insertions(+)
>>>
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-01 20:54 ` Laszlo Ersek
@ 2016-12-02 0:58 ` Jordan Justen
2016-12-02 9:45 ` Laszlo Ersek
0 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2016-12-02 0:58 UTC (permalink / raw)
To: Laszlo Ersek, Anthony PERARD, Gao, Liming, Zhu, Yonghong,
Ard Biesheuvel
Cc: edk2-devel
On 2016-12-01 12:54:34, Laszlo Ersek wrote:
> On 12/01/16 21:06, Jordan Justen wrote:
> > On 2016-12-01 10:43:24, Laszlo Ersek wrote:
> >> Hrpmf, wait a second, I do see something interesting: in this series you
> >> *are* modifying APIs declared in a library class header (namely
> >> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
> >> libraries) *are* required to specify EFIAPI.
> >>
> >> What happens if you apply patch #1 only?
> >
> > I agree that this should be fixed.
> >
> > But, if it works, I'm concerned that it would just be hiding a bug. My
> > understanding was that the EFIAPI on libraries was needed so that a
> > library implementation could be assembly based if desired. In this
> > case C is used for the implementation, so the calling conventions
> > should align.
>
> Never tried the following, so I'm unsure if edk2 intends to support it
> explicitly, but what about binary-only library instances (the 2-clause
> BSDL allows that)?
Good point. Once again, I agree that it is a bug that needs to be
fixed. The functions in the library interface should have EFIAPI.
I was just pointing out that I don't think it *ought* be the issue
here since both side are C, and being built by the same compiler.
Like you mentioned ... maybe it is a compiler bug, or a bug with our
build flags.
-Jordan
> If the library class header doesn't state EFIAPI on the functions, the
> library vendor builds the library instance with Visual Studio, and the
> library user builds the client module with gcc (against the same library
> class header), calls will fail.
>
> (The way I imagine using binary-only library instances is that the
> library comes as a binary object with a matching INF file, in a separate
> subdirectory, and the user resolves the library class in his/her
> platform DSC to that INF file. Not sure about the exact [section names]
> in the library instance's INF file though.)
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-01 18:43 ` [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Laszlo Ersek
2016-12-01 20:06 ` Jordan Justen
@ 2016-12-02 4:36 ` Gao, Liming
2016-12-02 10:00 ` Laszlo Ersek
2016-12-02 16:02 ` Anthony PERARD
2 siblings, 1 reply; 29+ messages in thread
From: Gao, Liming @ 2016-12-02 4:36 UTC (permalink / raw)
To: Laszlo Ersek, Anthony PERARD, Justen, Jordan L, Zhu, Yonghong,
Ard Biesheuvel
Cc: edk2-devel@ml01.01.org
I agree to root cause this issue with GCC6. Could you help submit this issue in bugzilia? Then, we will investigate it further.
Thanks
Liming
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Friday, December 02, 2016 2:43 AM
To: Anthony PERARD <anthony.perard@citrix.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel@ml01.01.org
Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
On 12/01/16 16:28, Anthony PERARD wrote:
> Hi,
>
> That might be only with the Xen part of OVMF but now that the GCC5
> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
> to boot in Xen guests.
>
> Here is the result:
> !!!! X64 Exception Type - 06(#UD - Invalid Opcode) CPU Apic ID - 00000000 !!!!
> RIP - 000000001F26AF6B, CS - 0000000000000038, RFLAGS -
> 0000000000010202 RAX - 0000000000000001, RCX - 000000001F26AF51, RDX
> - 0000000000000004 RBX - 0000000000000000, RSP - 000000001F43C510,
> RBP - 000000001E583D18 RSI - 0000000000000003, RDI - 0000000000000001
> R8 - 0000000000000000, R9 - 0000000000000000, R10 - 000000001E58DB98
> R11 - 0000000000000002, R12 - 000000001E58D898, R13 -
> 0000000000000000
> R14 - 000000001E58D8A0, R15 - 000000001F26D001
> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030
> GS - 0000000000000030, SS - 0000000000000030
> CR0 - 0000000080000033, CR2 - 0000000000000000, CR3 -
> 000000001F3DB000
> CR4 - 0000000000000668, CR8 - 0000000000000000
> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 -
> 0000000000000000
> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 -
> 0000000000000400 GDTR - 000000001F3C9A98 0000000000000047, LDTR - 0000000000000000
> IDTR - 000000001EB0A018 0000000000000FFF, TR - 0000000000000000
> FXSAVE_STATE - 000000001F43C170
> !!!! Find PE image ./Build/OvmfX64/DEBUG_GCC5/X64/OvmfPkg/XenBusDxe/XenBusDxe/DEBUG/XenBusDxe.dll (ImageBase=000000001F266000, EntryPoint=000000001F2669D5) !!!!
>
> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
> boot.
>
> While trying to debug that, I've added some debug prints (in this
> module and in XenPvBlkDxe), and the exception could change and become
> a "page fault" instead, or even an assert failure in the PrintLib,
> that was the ASSERT(Buffer != NULL) at I think
> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>
> Adding EFIAPI to internal functions in XenBusDxe makes things work
> again. My guest is that gcc would bypass (optimise) an exported
> functions and call directly an internal one but without reordering the
> arguments (EFIAPI vs nothing).
>
> Does that make sense?
Thank you for the investigation. It is strange that only the Xen modules are affected, I'm unsure what that's the case.
Either way, it seems to be a gcc-6 bug, or an edk2 toolchain bug. You should *not* need EFIAPI for functions with external linkage if the calls to them straddle only files in the same module. I'm suspecting
gcc-6 (we've received no such reports with gcc-5). Maybe we need a GCC6 toolchain as well, for turning off some new features in gcc-6?
Jordan, Liming, Yonghong, Ard -- any ideas?
Anthony: while we all figure this out, please consider building OVMF with the "-b NOOPT" switch. Support for the NOOPT build target has recently been added to the GCC Ia32/X64 toolchains in BaseTools, and to the OVMF DSC files as well. The build targets correspond to:
RELEASE -- compiler optimization enabled; DEBUG, ASSERT, and similar
DebugLib features compiled out
DEBUG -- compiler optimization enabled; DebugLib features preserved
NOOPT -- compiler optimization disabled; DebugLib features preserved
(Note that for ArmVirtPkg and the GCC AARCH64 toolchains in BaseTools, there is no NOOPT, and DEBUG means actually NOOPT -- if I remember correctly. Ard will correct me if I'm wrong :))
If "-b NOOPT" works for you, I'd prefer that as a temporary solution (until the root cause is found and addressed) to the XenBusDxe patches.
Hrpmf, wait a second, I do see something interesting: in this series you
*are* modifying APIs declared in a library class header (namely "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
libraries) *are* required to specify EFIAPI.
What happens if you apply patch #1 only?
Thanks!
Laszlo
> Anthony PERARD (4):
> OvmfPkg/XenHypercallLib: Add EFIAPI
> OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify
> OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions
> OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant,End}Access
>
> OvmfPkg/Include/Library/XenHypercallLib.h | 3 +++
> OvmfPkg/Library/XenHypercallLib/XenHypercall.c | 3 +++
> OvmfPkg/XenBusDxe/EventChannel.c | 1 +
> OvmfPkg/XenBusDxe/EventChannel.h | 1 +
> OvmfPkg/XenBusDxe/GrantTable.c | 2 ++
> OvmfPkg/XenBusDxe/XenStore.c | 13 +++++++++++++
> OvmfPkg/XenBusDxe/XenStore.h | 10 ++++++++++
> 7 files changed, 33 insertions(+)
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-02 0:58 ` Jordan Justen
@ 2016-12-02 9:45 ` Laszlo Ersek
0 siblings, 0 replies; 29+ messages in thread
From: Laszlo Ersek @ 2016-12-02 9:45 UTC (permalink / raw)
To: Jordan Justen, Anthony PERARD, Gao, Liming, Zhu, Yonghong,
Ard Biesheuvel
Cc: edk2-devel
On 12/02/16 01:58, Jordan Justen wrote:
> On 2016-12-01 12:54:34, Laszlo Ersek wrote:
>> On 12/01/16 21:06, Jordan Justen wrote:
>>> On 2016-12-01 10:43:24, Laszlo Ersek wrote:
>>>> Hrpmf, wait a second, I do see something interesting: in this series you
>>>> *are* modifying APIs declared in a library class header (namely
>>>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>>>> libraries) *are* required to specify EFIAPI.
>>>>
>>>> What happens if you apply patch #1 only?
>>>
>>> I agree that this should be fixed.
>>>
>>> But, if it works, I'm concerned that it would just be hiding a bug. My
>>> understanding was that the EFIAPI on libraries was needed so that a
>>> library implementation could be assembly based if desired. In this
>>> case C is used for the implementation, so the calling conventions
>>> should align.
>>
>> Never tried the following, so I'm unsure if edk2 intends to support it
>> explicitly, but what about binary-only library instances (the 2-clause
>> BSDL allows that)?
>
> Good point. Once again, I agree that it is a bug that needs to be
> fixed. The functions in the library interface should have EFIAPI.
>
> I was just pointing out that I don't think it *ought* be the issue
> here since both side are C, and being built by the same compiler.
Got it now. :)
Thanks!
Laszlo
> Like you mentioned ... maybe it is a compiler bug, or a bug with our
> build flags.
>
> -Jordan
>
>> If the library class header doesn't state EFIAPI on the functions, the
>> library vendor builds the library instance with Visual Studio, and the
>> library user builds the client module with gcc (against the same library
>> class header), calls will fail.
>>
>> (The way I imagine using binary-only library instances is that the
>> library comes as a binary object with a matching INF file, in a separate
>> subdirectory, and the user resolves the library class in his/her
>> platform DSC to that INF file. Not sure about the exact [section names]
>> in the library instance's INF file though.)
>>
>> Thanks!
>> Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-02 4:36 ` Gao, Liming
@ 2016-12-02 10:00 ` Laszlo Ersek
0 siblings, 0 replies; 29+ messages in thread
From: Laszlo Ersek @ 2016-12-02 10:00 UTC (permalink / raw)
To: Gao, Liming, Anthony PERARD, Justen, Jordan L, Zhu, Yonghong,
Ard Biesheuvel
Cc: edk2-devel@ml01.01.org
On 12/02/16 05:36, Gao, Liming wrote:
> I agree to root cause this issue with GCC6. Could you help submit this issue in bugzilia? Then, we will investigate it further.
Thanks, Liming, I've now filed
<https://bugzilla.tianocore.org/show_bug.cgi?id=281>.
Cheers
Laszlo
> Thanks
> Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Friday, December 02, 2016 2:43 AM
> To: Anthony PERARD <anthony.perard@citrix.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@ml01.01.org
> Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
>
> On 12/01/16 16:28, Anthony PERARD wrote:
>> Hi,
>>
>> That might be only with the Xen part of OVMF but now that the GCC5
>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
>> to boot in Xen guests.
>>
>> Here is the result:
>> !!!! X64 Exception Type - 06(#UD - Invalid Opcode) CPU Apic ID - 00000000 !!!!
>> RIP - 000000001F26AF6B, CS - 0000000000000038, RFLAGS -
>> 0000000000010202 RAX - 0000000000000001, RCX - 000000001F26AF51, RDX
>> - 0000000000000004 RBX - 0000000000000000, RSP - 000000001F43C510,
>> RBP - 000000001E583D18 RSI - 0000000000000003, RDI - 0000000000000001
>> R8 - 0000000000000000, R9 - 0000000000000000, R10 - 000000001E58DB98
>> R11 - 0000000000000002, R12 - 000000001E58D898, R13 -
>> 0000000000000000
>> R14 - 000000001E58D8A0, R15 - 000000001F26D001
>> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030
>> GS - 0000000000000030, SS - 0000000000000030
>> CR0 - 0000000080000033, CR2 - 0000000000000000, CR3 -
>> 000000001F3DB000
>> CR4 - 0000000000000668, CR8 - 0000000000000000
>> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 -
>> 0000000000000000
>> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 -
>> 0000000000000400 GDTR - 000000001F3C9A98 0000000000000047, LDTR - 0000000000000000
>> IDTR - 000000001EB0A018 0000000000000FFF, TR - 0000000000000000
>> FXSAVE_STATE - 000000001F43C170
>> !!!! Find PE image ./Build/OvmfX64/DEBUG_GCC5/X64/OvmfPkg/XenBusDxe/XenBusDxe/DEBUG/XenBusDxe.dll (ImageBase=000000001F266000, EntryPoint=000000001F2669D5) !!!!
>>
>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
>> boot.
>>
>> While trying to debug that, I've added some debug prints (in this
>> module and in XenPvBlkDxe), and the exception could change and become
>> a "page fault" instead, or even an assert failure in the PrintLib,
>> that was the ASSERT(Buffer != NULL) at I think
>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>
>> Adding EFIAPI to internal functions in XenBusDxe makes things work
>> again. My guest is that gcc would bypass (optimise) an exported
>> functions and call directly an internal one but without reordering the
>> arguments (EFIAPI vs nothing).
>>
>> Does that make sense?
>
> Thank you for the investigation. It is strange that only the Xen modules are affected, I'm unsure what that's the case.
>
> Either way, it seems to be a gcc-6 bug, or an edk2 toolchain bug. You should *not* need EFIAPI for functions with external linkage if the calls to them straddle only files in the same module. I'm suspecting
> gcc-6 (we've received no such reports with gcc-5). Maybe we need a GCC6 toolchain as well, for turning off some new features in gcc-6?
>
> Jordan, Liming, Yonghong, Ard -- any ideas?
>
> Anthony: while we all figure this out, please consider building OVMF with the "-b NOOPT" switch. Support for the NOOPT build target has recently been added to the GCC Ia32/X64 toolchains in BaseTools, and to the OVMF DSC files as well. The build targets correspond to:
>
> RELEASE -- compiler optimization enabled; DEBUG, ASSERT, and similar
> DebugLib features compiled out
> DEBUG -- compiler optimization enabled; DebugLib features preserved
> NOOPT -- compiler optimization disabled; DebugLib features preserved
>
> (Note that for ArmVirtPkg and the GCC AARCH64 toolchains in BaseTools, there is no NOOPT, and DEBUG means actually NOOPT -- if I remember correctly. Ard will correct me if I'm wrong :))
>
> If "-b NOOPT" works for you, I'd prefer that as a temporary solution (until the root cause is found and addressed) to the XenBusDxe patches.
>
> Hrpmf, wait a second, I do see something interesting: in this series you
> *are* modifying APIs declared in a library class header (namely "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
> libraries) *are* required to specify EFIAPI.
>
> What happens if you apply patch #1 only?
>
> Thanks!
> Laszlo
>
>> Anthony PERARD (4):
>> OvmfPkg/XenHypercallLib: Add EFIAPI
>> OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify
>> OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions
>> OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant,End}Access
>>
>> OvmfPkg/Include/Library/XenHypercallLib.h | 3 +++
>> OvmfPkg/Library/XenHypercallLib/XenHypercall.c | 3 +++
>> OvmfPkg/XenBusDxe/EventChannel.c | 1 +
>> OvmfPkg/XenBusDxe/EventChannel.h | 1 +
>> OvmfPkg/XenBusDxe/GrantTable.c | 2 ++
>> OvmfPkg/XenBusDxe/XenStore.c | 13 +++++++++++++
>> OvmfPkg/XenBusDxe/XenStore.h | 10 ++++++++++
>> 7 files changed, 33 insertions(+)
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-01 18:43 ` [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Laszlo Ersek
2016-12-01 20:06 ` Jordan Justen
2016-12-02 4:36 ` Gao, Liming
@ 2016-12-02 16:02 ` Anthony PERARD
2016-12-02 19:26 ` Laszlo Ersek
2 siblings, 1 reply; 29+ messages in thread
From: Anthony PERARD @ 2016-12-02 16:02 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Jordan Justen, Gao, Liming, Zhu, Yonghong, Ard Biesheuvel,
edk2-devel
On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
> On 12/01/16 16:28, Anthony PERARD wrote:
> > Hi,
> >
> > That might be only with the Xen part of OVMF but now that the GCC5
> > toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
> > to boot in Xen guests.
> >
[...]
> >
> > Removing the gcc option -flto in only the XenBusDxe module makes OVMF
> > boot.
> >
> > While trying to debug that, I've added some debug prints (in this module
> > and in XenPvBlkDxe), and the exception could change and become a "page
> > fault" instead, or even an assert failure in the PrintLib, that was the
> > ASSERT(Buffer != NULL) at I think
> > MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
> >
> > Adding EFIAPI to internal functions in XenBusDxe makes things work
> > again. My guest is that gcc would bypass (optimise) an exported
> > functions and call directly an internal one but without reordering the
> > arguments (EFIAPI vs nothing).
> >
> > Does that make sense?
>
> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
> (until the root cause is found and addressed) to the XenBusDxe patches.
That works, using GCC49 (with gcc 6.2.1) works as well.
> Hrpmf, wait a second, I do see something interesting: in this series you
> *are* modifying APIs declared in a library class header (namely
> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
> libraries) *are* required to specify EFIAPI.
>
> What happens if you apply patch #1 only?
With only XenHypercallLib changes, the error is the same.
But I did find the minimum change needed, it envolve a function with a
VA_LIST as argument.
With only the following patch, OVMF works again.
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 1666c4b..85b0956 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (
}
XENSTORE_STATUS
+EFIAPI
XenStoreVSPrint (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
index c9d4c65..33bb647 100644
--- a/OvmfPkg/XenBusDxe/XenStore.h
+++ b/OvmfPkg/XenBusDxe/XenStore.h
@@ -209,6 +209,7 @@ XenStoreSPrint (
indicating the type of write failure.
**/
XENSTORE_STATUS
+EFIAPI
XenStoreVSPrint (
IN CONST XENSTORE_TRANSACTION *Transaction,
IN CONST CHAR8 *DirectoryPath,
IN CONST CHAR8 *Node,
IN CONST CHAR8 *FormatString,
IN VA_LIST Marker
);
I think the exception happen when this function is called via
XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
OvmfPkg/XenPvBlkDxe/BlockFront.c
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-02 16:02 ` Anthony PERARD
@ 2016-12-02 19:26 ` Laszlo Ersek
2016-12-03 17:59 ` Laszlo Ersek
0 siblings, 1 reply; 29+ messages in thread
From: Laszlo Ersek @ 2016-12-02 19:26 UTC (permalink / raw)
To: Anthony PERARD
Cc: Jordan Justen, Gao, Liming, Zhu, Yonghong, Ard Biesheuvel,
edk2-devel
On 12/02/16 17:02, Anthony PERARD wrote:
> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
>> On 12/01/16 16:28, Anthony PERARD wrote:
>>> Hi,
>>>
>>> That might be only with the Xen part of OVMF but now that the GCC5
>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
>>> to boot in Xen guests.
>>>
> [...]
>>>
>>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
>>> boot.
>>>
>>> While trying to debug that, I've added some debug prints (in this module
>>> and in XenPvBlkDxe), and the exception could change and become a "page
>>> fault" instead, or even an assert failure in the PrintLib, that was the
>>> ASSERT(Buffer != NULL) at I think
>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>>
>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
>>> again. My guest is that gcc would bypass (optimise) an exported
>>> functions and call directly an internal one but without reordering the
>>> arguments (EFIAPI vs nothing).
>>>
>>> Does that make sense?
>>
>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
>> (until the root cause is found and addressed) to the XenBusDxe patches.
>
> That works, using GCC49 (with gcc 6.2.1) works as well.
>
>> Hrpmf, wait a second, I do see something interesting: in this series you
>> *are* modifying APIs declared in a library class header (namely
>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>> libraries) *are* required to specify EFIAPI.
>>
>> What happens if you apply patch #1 only?
>
> With only XenHypercallLib changes, the error is the same.
>
> But I did find the minimum change needed, it envolve a function with a
> VA_LIST as argument.
>
> With only the following patch, OVMF works again.
>
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 1666c4b..85b0956 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (
> }
>
> XENSTORE_STATUS
> +EFIAPI
> XenStoreVSPrint (
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *DirectoryPath,
> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
> index c9d4c65..33bb647 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.h
> +++ b/OvmfPkg/XenBusDxe/XenStore.h
> @@ -209,6 +209,7 @@ XenStoreSPrint (
> indicating the type of write failure.
> **/
> XENSTORE_STATUS
> +EFIAPI
> XenStoreVSPrint (
> IN CONST XENSTORE_TRANSACTION *Transaction,
> IN CONST CHAR8 *DirectoryPath,
> IN CONST CHAR8 *Node,
> IN CONST CHAR8 *FormatString,
> IN VA_LIST Marker
> );
>
>
> I think the exception happen when this function is called via
> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
> OvmfPkg/XenPvBlkDxe/BlockFront.c
>
It used to be a known requirement / limitation that all functions with
variable argument lists had to be EFIAPI, regardless of cross-module
use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed
that, and varargs should "just work" now. I suspect this is a
__builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
It might make sense to report a bug in the upstream gcc tracker.
... Oh wow, this is a known gcc bug! See:
https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html
Upstream gcc BZ <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955> was
apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
So we'll either need a GCC6 toolchain in BaseTools that drops -flto, in
order to work around this gcc issue, or we'll have to ask gcc-6 users to
use at least gcc-6.3.
Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools
workaround then.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-02 19:26 ` Laszlo Ersek
@ 2016-12-03 17:59 ` Laszlo Ersek
2016-12-05 2:55 ` Gao, Liming
2017-02-21 16:39 ` Anthony PERARD
0 siblings, 2 replies; 29+ messages in thread
From: Laszlo Ersek @ 2016-12-03 17:59 UTC (permalink / raw)
To: Anthony PERARD
Cc: Jordan Justen, Gao, Liming, Zhu, Yonghong, Ard Biesheuvel,
edk2-devel
On 12/02/16 20:26, Laszlo Ersek wrote:
> On 12/02/16 17:02, Anthony PERARD wrote:
>> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
>>> On 12/01/16 16:28, Anthony PERARD wrote:
>>>> Hi,
>>>>
>>>> That might be only with the Xen part of OVMF but now that the GCC5
>>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
>>>> to boot in Xen guests.
>>>>
>> [...]
>>>>
>>>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
>>>> boot.
>>>>
>>>> While trying to debug that, I've added some debug prints (in this module
>>>> and in XenPvBlkDxe), and the exception could change and become a "page
>>>> fault" instead, or even an assert failure in the PrintLib, that was the
>>>> ASSERT(Buffer != NULL) at I think
>>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>>>
>>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
>>>> again. My guest is that gcc would bypass (optimise) an exported
>>>> functions and call directly an internal one but without reordering the
>>>> arguments (EFIAPI vs nothing).
>>>>
>>>> Does that make sense?
>>>
>>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
>>> (until the root cause is found and addressed) to the XenBusDxe patches.
>>
>> That works, using GCC49 (with gcc 6.2.1) works as well.
>>
>>> Hrpmf, wait a second, I do see something interesting: in this series you
>>> *are* modifying APIs declared in a library class header (namely
>>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>>> libraries) *are* required to specify EFIAPI.
>>>
>>> What happens if you apply patch #1 only?
>>
>> With only XenHypercallLib changes, the error is the same.
>>
>> But I did find the minimum change needed, it envolve a function with a
>> VA_LIST as argument.
>>
>> With only the following patch, OVMF works again.
>>
>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
>> index 1666c4b..85b0956 100644
>> --- a/OvmfPkg/XenBusDxe/XenStore.c
>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
>> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (
>> }
>>
>> XENSTORE_STATUS
>> +EFIAPI
>> XenStoreVSPrint (
>> IN CONST XENSTORE_TRANSACTION *Transaction,
>> IN CONST CHAR8 *DirectoryPath,
>> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
>> index c9d4c65..33bb647 100644
>> --- a/OvmfPkg/XenBusDxe/XenStore.h
>> +++ b/OvmfPkg/XenBusDxe/XenStore.h
>> @@ -209,6 +209,7 @@ XenStoreSPrint (
>> indicating the type of write failure.
>> **/
>> XENSTORE_STATUS
>> +EFIAPI
>> XenStoreVSPrint (
>> IN CONST XENSTORE_TRANSACTION *Transaction,
>> IN CONST CHAR8 *DirectoryPath,
>> IN CONST CHAR8 *Node,
>> IN CONST CHAR8 *FormatString,
>> IN VA_LIST Marker
>> );
>>
>>
>> I think the exception happen when this function is called via
>> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
>> OvmfPkg/XenPvBlkDxe/BlockFront.c
>>
>
> It used to be a known requirement / limitation that all functions with
> variable argument lists had to be EFIAPI, regardless of cross-module
> use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed
> that, and varargs should "just work" now. I suspect this is a
> __builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
> It might make sense to report a bug in the upstream gcc tracker.
>
> ... Oh wow, this is a known gcc bug! See:
>
> https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html
>
> Upstream gcc BZ <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955> was
> apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
> So we'll either need a GCC6 toolchain in BaseTools that drops -flto, in
> order to work around this gcc issue, or we'll have to ask gcc-6 users to
> use at least gcc-6.3.
>
> Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools
> workaround then.
I think I got confused in parts of the above; I got some details wrong.
Namely, commit 48d5f9a551a9 did not remove the requirement/limitation
that all varargs functions have to be EFIAPI. Said commit only changed
how the VA_*() macros would be implemented.
The two caller functions of XenStoreVSPrint(), namely XenStoreSPrint()
and XenBusXenStoreSPrint(), are varargs functions, but they are already
EFIAPI. So the requirement/limitation (which was unaffected by
48d5f9a551a9) is actually satisfied / considered in XenBusDxe.
The XenStoreVSPrint() function, which you identified as the breaking
part, is *not* a varargs function itself, so it needn't be EFIAPI. It
simply receives a VA_LIST parameter (which is "__builtin_ms_va_list",
from commit 48d5f9a551a9), and (a) copies it with VA_COPY() for passing
the copy to SPrintLengthAsciiFormat(), (b) passes the original parameter
to AsciiVSPrint(). In turn both of those functions call the common
BasePrintLibSPrintMarker() function.
Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c6> says,
> This is bug report that the specialized
> __builtin_ms_va_{list,start,end,copy} builtins have stopped working
> when -flto is used. They worked until gcc 5.3, both with or without
> -flto. In gcc 6.1 with -flto, the canonical iterator __builtin_va_arg
> ignores them and works on a sysv_va_list. To be precise, it's
> __builtin_va_arg in the context of -flto that's broken, not the
> specialized builtins. __builtin_va_arg has always been a polymorphic
> builtin that changes its behavior based on the type of va_list it's
> given as an argument. Without this polymorphic behavior, there's no
> way to iterate over an ms_va_list.
Apparently, when BasePrintLibSPrintMarker() finally calls VA_ARG() (==
__builtin_va_arg(), from commit 48d5f9a551a9) on Marker / Marker2, with
LTO enabled, __builtin_va_arg() fails to notice what context
VaListMarker comes from:
- __builtin_ms_va_start() in XenStoreSPrint() and XenBusXenStoreSPrint(), or
- __builtin_ms_va_copy() in XenStoreVSPrint().
So I think we *are* being hit by gcc BZ#70955, and making
XenStoreVSPrint() EFIAPI only masks the issue. Comment
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c7> seems relevant:
> The change with GCC 6 is that the builtins are now lowered during
> link-time optimization rather than at compile-time. Thus the abi
> selection bits are possibly not transfered correctly (type merging?).
> I remember the business was quite ugly, but eventually we just miss to
> properly transfer the function attribute.
The end result for edk2 remains the same (= BaseTools should work around
this gcc issue with a new GCC6 toolchain that drops -flto, unless
gcc-6.3 is about to become available to users real quick). I just wanted
to point out that my earlier statement "commit 48d5f9a551a9 had removed
the need for varargs functions to be EFIAPI" was incorrect -- varargs
functions still must be EFIAPI (and XenBusDxe conforms, see
XenStoreSPrint() and XenBusXenStoreSPrint()).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-03 17:59 ` Laszlo Ersek
@ 2016-12-05 2:55 ` Gao, Liming
2016-12-05 10:09 ` Laszlo Ersek
2017-02-21 16:39 ` Anthony PERARD
1 sibling, 1 reply; 29+ messages in thread
From: Gao, Liming @ 2016-12-05 2:55 UTC (permalink / raw)
To: Laszlo Ersek, Anthony PERARD
Cc: Justen, Jordan L, edk2-devel@ml01.01.org, Ard Biesheuvel
Laszlo:
Thanks for your investigation. In tools_def.txt, there are two chains: GCC49 and GCC5. GCC49 enables Os without lto, GCC5 enables Os and lto. If GCC version supports lto well, it can use GCC5 tool chain. Otherwise, it can use GCC49 tool chain. I suggest to add comments in GCC5 tool chain to document the known workable GCC version. From below comments, only GCC5.3 and GCC5.4 can work with GCC5 tool chain with lto enable.
Thanks
Liming
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Sunday, December 04, 2016 1:59 AM
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
On 12/02/16 20:26, Laszlo Ersek wrote:
> On 12/02/16 17:02, Anthony PERARD wrote:
>> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
>>> On 12/01/16 16:28, Anthony PERARD wrote:
>>>> Hi,
>>>>
>>>> That might be only with the Xen part of OVMF but now that the GCC5
>>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF
>>>> fail to boot in Xen guests.
>>>>
>> [...]
>>>>
>>>> Removing the gcc option -flto in only the XenBusDxe module makes
>>>> OVMF boot.
>>>>
>>>> While trying to debug that, I've added some debug prints (in this
>>>> module and in XenPvBlkDxe), and the exception could change and
>>>> become a "page fault" instead, or even an assert failure in the
>>>> PrintLib, that was the ASSERT(Buffer != NULL) at I think
>>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>>>
>>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
>>>> again. My guest is that gcc would bypass (optimise) an exported
>>>> functions and call directly an internal one but without reordering
>>>> the arguments (EFIAPI vs nothing).
>>>>
>>>> Does that make sense?
>>>
>>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
>>> (until the root cause is found and addressed) to the XenBusDxe patches.
>>
>> That works, using GCC49 (with gcc 6.2.1) works as well.
>>
>>> Hrpmf, wait a second, I do see something interesting: in this series
>>> you
>>> *are* modifying APIs declared in a library class header (namely
>>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>>> libraries) *are* required to specify EFIAPI.
>>>
>>> What happens if you apply patch #1 only?
>>
>> With only XenHypercallLib changes, the error is the same.
>>
>> But I did find the minimum change needed, it envolve a function with
>> a VA_LIST as argument.
>>
>> With only the following patch, OVMF works again.
>>
>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c
>> b/OvmfPkg/XenBusDxe/XenStore.c index 1666c4b..85b0956 100644
>> --- a/OvmfPkg/XenBusDxe/XenStore.c
>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
>> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd ( }
>>
>> XENSTORE_STATUS
>> +EFIAPI
>> XenStoreVSPrint (
>> IN CONST XENSTORE_TRANSACTION *Transaction,
>> IN CONST CHAR8 *DirectoryPath,
>> diff --git a/OvmfPkg/XenBusDxe/XenStore.h
>> b/OvmfPkg/XenBusDxe/XenStore.h index c9d4c65..33bb647 100644
>> --- a/OvmfPkg/XenBusDxe/XenStore.h
>> +++ b/OvmfPkg/XenBusDxe/XenStore.h
>> @@ -209,6 +209,7 @@ XenStoreSPrint (
>> indicating the type of write failure.
>> **/
>> XENSTORE_STATUS
>> +EFIAPI
>> XenStoreVSPrint (
>> IN CONST XENSTORE_TRANSACTION *Transaction,
>> IN CONST CHAR8 *DirectoryPath,
>> IN CONST CHAR8 *Node,
>> IN CONST CHAR8 *FormatString,
>> IN VA_LIST Marker
>> );
>>
>>
>> I think the exception happen when this function is called via
>> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
>> OvmfPkg/XenPvBlkDxe/BlockFront.c
>>
>
> It used to be a known requirement / limitation that all functions with
> variable argument lists had to be EFIAPI, regardless of cross-module
> use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed
> that, and varargs should "just work" now. I suspect this is a
> __builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
> It might make sense to report a bug in the upstream gcc tracker.
>
> ... Oh wow, this is a known gcc bug! See:
>
> https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html
>
> Upstream gcc BZ <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955>
> was apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
> So we'll either need a GCC6 toolchain in BaseTools that drops -flto,
> in order to work around this gcc issue, or we'll have to ask gcc-6
> users to use at least gcc-6.3.
>
> Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools
> workaround then.
I think I got confused in parts of the above; I got some details wrong.
Namely, commit 48d5f9a551a9 did not remove the requirement/limitation that all varargs functions have to be EFIAPI. Said commit only changed how the VA_*() macros would be implemented.
The two caller functions of XenStoreVSPrint(), namely XenStoreSPrint() and XenBusXenStoreSPrint(), are varargs functions, but they are already EFIAPI. So the requirement/limitation (which was unaffected by
48d5f9a551a9) is actually satisfied / considered in XenBusDxe.
The XenStoreVSPrint() function, which you identified as the breaking part, is *not* a varargs function itself, so it needn't be EFIAPI. It simply receives a VA_LIST parameter (which is "__builtin_ms_va_list", from commit 48d5f9a551a9), and (a) copies it with VA_COPY() for passing the copy to SPrintLengthAsciiFormat(), (b) passes the original parameter to AsciiVSPrint(). In turn both of those functions call the common
BasePrintLibSPrintMarker() function.
Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c6> says,
> This is bug report that the specialized
> __builtin_ms_va_{list,start,end,copy} builtins have stopped working
> when -flto is used. They worked until gcc 5.3, both with or without
> -flto. In gcc 6.1 with -flto, the canonical iterator __builtin_va_arg
> ignores them and works on a sysv_va_list. To be precise, it's
> __builtin_va_arg in the context of -flto that's broken, not the
> specialized builtins. __builtin_va_arg has always been a polymorphic
> builtin that changes its behavior based on the type of va_list it's
> given as an argument. Without this polymorphic behavior, there's no
> way to iterate over an ms_va_list.
Apparently, when BasePrintLibSPrintMarker() finally calls VA_ARG() (== __builtin_va_arg(), from commit 48d5f9a551a9) on Marker / Marker2, with LTO enabled, __builtin_va_arg() fails to notice what context VaListMarker comes from:
- __builtin_ms_va_start() in XenStoreSPrint() and XenBusXenStoreSPrint(), or
- __builtin_ms_va_copy() in XenStoreVSPrint().
So I think we *are* being hit by gcc BZ#70955, and making
XenStoreVSPrint() EFIAPI only masks the issue. Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c7> seems relevant:
> The change with GCC 6 is that the builtins are now lowered during
> link-time optimization rather than at compile-time. Thus the abi
> selection bits are possibly not transfered correctly (type merging?).
> I remember the business was quite ugly, but eventually we just miss to
> properly transfer the function attribute.
The end result for edk2 remains the same (= BaseTools should work around this gcc issue with a new GCC6 toolchain that drops -flto, unless
gcc-6.3 is about to become available to users real quick). I just wanted to point out that my earlier statement "commit 48d5f9a551a9 had removed the need for varargs functions to be EFIAPI" was incorrect -- varargs functions still must be EFIAPI (and XenBusDxe conforms, see
XenStoreSPrint() and XenBusXenStoreSPrint()).
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-05 2:55 ` Gao, Liming
@ 2016-12-05 10:09 ` Laszlo Ersek
0 siblings, 0 replies; 29+ messages in thread
From: Laszlo Ersek @ 2016-12-05 10:09 UTC (permalink / raw)
To: Anthony PERARD, Justen, Jordan L
Cc: Gao, Liming, edk2-devel@ml01.01.org, Ard Biesheuvel
On 12/05/16 03:55, Gao, Liming wrote:
> Laszlo:
> Thanks for your investigation. In tools_def.txt, there are two
> chains: GCC49 and GCC5. GCC49 enables Os without lto, GCC5 enables Os
> and lto. If GCC version supports lto well, it can use GCC5 tool
> chain. Otherwise, it can use GCC49 tool chain. I suggest to add
> comments in GCC5 tool chain to document the known workable GCC
> version. From below comments, only GCC5.3 and GCC5.4 can work with
> GCC5 tool chain with lto enable.
Anthony, I suggest that you please send a new patch series, with two
patches:
- The first patch should be the same as your current patch #1, but it
should have a non-empty commit message. Please state that EFIAPI is
necessary for functions declared in library class header files.
- The second patch should be for "OvmfPkg/build.sh". gcc versions 6.0,
6.1, and 6.2 should be mapped to the GCC49 toolchain:
4.9.*|6.[0-2].*)
TARGET_TOOLS=GCC49
Jordan, do you agree?
Thanks
Laszlo
>
> Thanks
> Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Sunday, December 04, 2016 1:59 AM
> To: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
>
> On 12/02/16 20:26, Laszlo Ersek wrote:
>> On 12/02/16 17:02, Anthony PERARD wrote:
>>> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
>>>> On 12/01/16 16:28, Anthony PERARD wrote:
>>>>> Hi,
>>>>>
>>>>> That might be only with the Xen part of OVMF but now that the GCC5
>>>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF
>>>>> fail to boot in Xen guests.
>>>>>
>>> [...]
>>>>>
>>>>> Removing the gcc option -flto in only the XenBusDxe module makes
>>>>> OVMF boot.
>>>>>
>>>>> While trying to debug that, I've added some debug prints (in this
>>>>> module and in XenPvBlkDxe), and the exception could change and
>>>>> become a "page fault" instead, or even an assert failure in the
>>>>> PrintLib, that was the ASSERT(Buffer != NULL) at I think
>>>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>>>>
>>>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
>>>>> again. My guest is that gcc would bypass (optimise) an exported
>>>>> functions and call directly an internal one but without reordering
>>>>> the arguments (EFIAPI vs nothing).
>>>>>
>>>>> Does that make sense?
>>>>
>>>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
>>>> (until the root cause is found and addressed) to the XenBusDxe patches.
>>>
>>> That works, using GCC49 (with gcc 6.2.1) works as well.
>>>
>>>> Hrpmf, wait a second, I do see something interesting: in this series
>>>> you
>>>> *are* modifying APIs declared in a library class header (namely
>>>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>>>> libraries) *are* required to specify EFIAPI.
>>>>
>>>> What happens if you apply patch #1 only?
>>>
>>> With only XenHypercallLib changes, the error is the same.
>>>
>>> But I did find the minimum change needed, it envolve a function with
>>> a VA_LIST as argument.
>>>
>>> With only the following patch, OVMF works again.
>>>
>>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c
>>> b/OvmfPkg/XenBusDxe/XenStore.c index 1666c4b..85b0956 100644
>>> --- a/OvmfPkg/XenBusDxe/XenStore.c
>>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
>>> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd ( }
>>>
>>> XENSTORE_STATUS
>>> +EFIAPI
>>> XenStoreVSPrint (
>>> IN CONST XENSTORE_TRANSACTION *Transaction,
>>> IN CONST CHAR8 *DirectoryPath,
>>> diff --git a/OvmfPkg/XenBusDxe/XenStore.h
>>> b/OvmfPkg/XenBusDxe/XenStore.h index c9d4c65..33bb647 100644
>>> --- a/OvmfPkg/XenBusDxe/XenStore.h
>>> +++ b/OvmfPkg/XenBusDxe/XenStore.h
>>> @@ -209,6 +209,7 @@ XenStoreSPrint (
>>> indicating the type of write failure.
>>> **/
>>> XENSTORE_STATUS
>>> +EFIAPI
>>> XenStoreVSPrint (
>>> IN CONST XENSTORE_TRANSACTION *Transaction,
>>> IN CONST CHAR8 *DirectoryPath,
>>> IN CONST CHAR8 *Node,
>>> IN CONST CHAR8 *FormatString,
>>> IN VA_LIST Marker
>>> );
>>>
>>>
>>> I think the exception happen when this function is called via
>>> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
>>> OvmfPkg/XenPvBlkDxe/BlockFront.c
>>>
>>
>> It used to be a known requirement / limitation that all functions with
>> variable argument lists had to be EFIAPI, regardless of cross-module
>> use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed
>> that, and varargs should "just work" now. I suspect this is a
>> __builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
>> It might make sense to report a bug in the upstream gcc tracker.
>>
>> ... Oh wow, this is a known gcc bug! See:
>>
>> https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html
>>
>> Upstream gcc BZ <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955>
>> was apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
>> So we'll either need a GCC6 toolchain in BaseTools that drops -flto,
>> in order to work around this gcc issue, or we'll have to ask gcc-6
>> users to use at least gcc-6.3.
>>
>> Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools
>> workaround then.
>
> I think I got confused in parts of the above; I got some details wrong.
> Namely, commit 48d5f9a551a9 did not remove the requirement/limitation that all varargs functions have to be EFIAPI. Said commit only changed how the VA_*() macros would be implemented.
>
> The two caller functions of XenStoreVSPrint(), namely XenStoreSPrint() and XenBusXenStoreSPrint(), are varargs functions, but they are already EFIAPI. So the requirement/limitation (which was unaffected by
> 48d5f9a551a9) is actually satisfied / considered in XenBusDxe.
>
> The XenStoreVSPrint() function, which you identified as the breaking part, is *not* a varargs function itself, so it needn't be EFIAPI. It simply receives a VA_LIST parameter (which is "__builtin_ms_va_list", from commit 48d5f9a551a9), and (a) copies it with VA_COPY() for passing the copy to SPrintLengthAsciiFormat(), (b) passes the original parameter to AsciiVSPrint(). In turn both of those functions call the common
> BasePrintLibSPrintMarker() function.
>
> Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c6> says,
>
>> This is bug report that the specialized
>> __builtin_ms_va_{list,start,end,copy} builtins have stopped working
>> when -flto is used. They worked until gcc 5.3, both with or without
>> -flto. In gcc 6.1 with -flto, the canonical iterator __builtin_va_arg
>> ignores them and works on a sysv_va_list. To be precise, it's
>> __builtin_va_arg in the context of -flto that's broken, not the
>> specialized builtins. __builtin_va_arg has always been a polymorphic
>> builtin that changes its behavior based on the type of va_list it's
>> given as an argument. Without this polymorphic behavior, there's no
>> way to iterate over an ms_va_list.
>
> Apparently, when BasePrintLibSPrintMarker() finally calls VA_ARG() (== __builtin_va_arg(), from commit 48d5f9a551a9) on Marker / Marker2, with LTO enabled, __builtin_va_arg() fails to notice what context VaListMarker comes from:
> - __builtin_ms_va_start() in XenStoreSPrint() and XenBusXenStoreSPrint(), or
> - __builtin_ms_va_copy() in XenStoreVSPrint().
>
> So I think we *are* being hit by gcc BZ#70955, and making
> XenStoreVSPrint() EFIAPI only masks the issue. Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c7> seems relevant:
>
>> The change with GCC 6 is that the builtins are now lowered during
>> link-time optimization rather than at compile-time. Thus the abi
>> selection bits are possibly not transfered correctly (type merging?).
>> I remember the business was quite ugly, but eventually we just miss to
>> properly transfer the function attribute.
>
> The end result for edk2 remains the same (= BaseTools should work around this gcc issue with a new GCC6 toolchain that drops -flto, unless
> gcc-6.3 is about to become available to users real quick). I just wanted to point out that my earlier statement "commit 48d5f9a551a9 had removed the need for varargs functions to be EFIAPI" was incorrect -- varargs functions still must be EFIAPI (and XenBusDxe conforms, see
> XenStoreSPrint() and XenBusXenStoreSPrint()).
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2016-12-03 17:59 ` Laszlo Ersek
2016-12-05 2:55 ` Gao, Liming
@ 2017-02-21 16:39 ` Anthony PERARD
2017-02-21 17:07 ` Laszlo Ersek
1 sibling, 1 reply; 29+ messages in thread
From: Anthony PERARD @ 2017-02-21 16:39 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Jordan Justen, Gao, Liming, Zhu, Yonghong, Ard Biesheuvel,
edk2-devel
On Sat, Dec 03, 2016 at 06:59:28PM +0100, Laszlo Ersek wrote:
> On 12/02/16 20:26, Laszlo Ersek wrote:
> > On 12/02/16 17:02, Anthony PERARD wrote:
> >> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
> >>> On 12/01/16 16:28, Anthony PERARD wrote:
> >>>> Hi,
> >>>>
> >>>> That might be only with the Xen part of OVMF but now that the GCC5
> >>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
> >>>> to boot in Xen guests.
> >>>>
> >> [...]
> >>>>
> >>>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
> >>>> boot.
> >>>>
> >>>> While trying to debug that, I've added some debug prints (in this module
> >>>> and in XenPvBlkDxe), and the exception could change and become a "page
> >>>> fault" instead, or even an assert failure in the PrintLib, that was the
> >>>> ASSERT(Buffer != NULL) at I think
> >>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
> >>>>
> >>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
> >>>> again. My guest is that gcc would bypass (optimise) an exported
> >>>> functions and call directly an internal one but without reordering the
> >>>> arguments (EFIAPI vs nothing).
> >>>>
> >>>> Does that make sense?
> >>>
> >>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
> >>> (until the root cause is found and addressed) to the XenBusDxe patches.
> >>
> >> That works, using GCC49 (with gcc 6.2.1) works as well.
> >>
> >>> Hrpmf, wait a second, I do see something interesting: in this series you
> >>> *are* modifying APIs declared in a library class header (namely
> >>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
> >>> libraries) *are* required to specify EFIAPI.
> >>>
> >>> What happens if you apply patch #1 only?
> >>
> >> With only XenHypercallLib changes, the error is the same.
> >>
> >> But I did find the minimum change needed, it envolve a function with a
> >> VA_LIST as argument.
> >>
> >> With only the following patch, OVMF works again.
> >>
> >> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> >> index 1666c4b..85b0956 100644
> >> --- a/OvmfPkg/XenBusDxe/XenStore.c
> >> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> >> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (
> >> }
> >>
> >> XENSTORE_STATUS
> >> +EFIAPI
> >> XenStoreVSPrint (
> >> IN CONST XENSTORE_TRANSACTION *Transaction,
> >> IN CONST CHAR8 *DirectoryPath,
> >> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
> >> index c9d4c65..33bb647 100644
> >> --- a/OvmfPkg/XenBusDxe/XenStore.h
> >> +++ b/OvmfPkg/XenBusDxe/XenStore.h
> >> @@ -209,6 +209,7 @@ XenStoreSPrint (
> >> indicating the type of write failure.
> >> **/
> >> XENSTORE_STATUS
> >> +EFIAPI
> >> XenStoreVSPrint (
> >> IN CONST XENSTORE_TRANSACTION *Transaction,
> >> IN CONST CHAR8 *DirectoryPath,
> >> IN CONST CHAR8 *Node,
> >> IN CONST CHAR8 *FormatString,
> >> IN VA_LIST Marker
> >> );
> >>
> >>
> >> I think the exception happen when this function is called via
> >> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
> >> OvmfPkg/XenPvBlkDxe/BlockFront.c
> >>
> >
> > It used to be a known requirement / limitation that all functions with
> > variable argument lists had to be EFIAPI, regardless of cross-module
> > use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed
> > that, and varargs should "just work" now. I suspect this is a
> > __builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
> > It might make sense to report a bug in the upstream gcc tracker.
> >
> > ... Oh wow, this is a known gcc bug! See:
> >
> > https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html
> >
> > Upstream gcc BZ <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955> was
> > apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
> > So we'll either need a GCC6 toolchain in BaseTools that drops -flto, in
> > order to work around this gcc issue, or we'll have to ask gcc-6 users to
> > use at least gcc-6.3.
> >
> > Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools
> > workaround then.
>
> I think I got confused in parts of the above; I got some details wrong.
> Namely, commit 48d5f9a551a9 did not remove the requirement/limitation
> that all varargs functions have to be EFIAPI. Said commit only changed
> how the VA_*() macros would be implemented.
>
> The two caller functions of XenStoreVSPrint(), namely XenStoreSPrint()
> and XenBusXenStoreSPrint(), are varargs functions, but they are already
> EFIAPI. So the requirement/limitation (which was unaffected by
> 48d5f9a551a9) is actually satisfied / considered in XenBusDxe.
>
> The XenStoreVSPrint() function, which you identified as the breaking
> part, is *not* a varargs function itself, so it needn't be EFIAPI. It
> simply receives a VA_LIST parameter (which is "__builtin_ms_va_list",
> from commit 48d5f9a551a9), and (a) copies it with VA_COPY() for passing
> the copy to SPrintLengthAsciiFormat(), (b) passes the original parameter
> to AsciiVSPrint(). In turn both of those functions call the common
> BasePrintLibSPrintMarker() function.
>
> Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c6> says,
>
> > This is bug report that the specialized
> > __builtin_ms_va_{list,start,end,copy} builtins have stopped working
> > when -flto is used. They worked until gcc 5.3, both with or without
> > -flto. In gcc 6.1 with -flto, the canonical iterator __builtin_va_arg
> > ignores them and works on a sysv_va_list. To be precise, it's
> > __builtin_va_arg in the context of -flto that's broken, not the
> > specialized builtins. __builtin_va_arg has always been a polymorphic
> > builtin that changes its behavior based on the type of va_list it's
> > given as an argument. Without this polymorphic behavior, there's no
> > way to iterate over an ms_va_list.
>
> Apparently, when BasePrintLibSPrintMarker() finally calls VA_ARG() (==
> __builtin_va_arg(), from commit 48d5f9a551a9) on Marker / Marker2, with
> LTO enabled, __builtin_va_arg() fails to notice what context
> VaListMarker comes from:
> - __builtin_ms_va_start() in XenStoreSPrint() and XenBusXenStoreSPrint(), or
> - __builtin_ms_va_copy() in XenStoreVSPrint().
>
> So I think we *are* being hit by gcc BZ#70955, and making
> XenStoreVSPrint() EFIAPI only masks the issue. Comment
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c7> seems relevant:
>
> > The change with GCC 6 is that the builtins are now lowered during
> > link-time optimization rather than at compile-time. Thus the abi
> > selection bits are possibly not transfered correctly (type merging?).
> > I remember the business was quite ugly, but eventually we just miss to
> > properly transfer the function attribute.
>
> The end result for edk2 remains the same (= BaseTools should work around
> this gcc issue with a new GCC6 toolchain that drops -flto, unless
> gcc-6.3 is about to become available to users real quick). I just wanted
> to point out that my earlier statement "commit 48d5f9a551a9 had removed
> the need for varargs functions to be EFIAPI" was incorrect -- varargs
> functions still must be EFIAPI (and XenBusDxe conforms, see
> XenStoreSPrint() and XenBusXenStoreSPrint()).
Hi Laszlo,
Now that gcc 6.3 is out, the bug described in the thread strikes again.
Building OVMF with -flto result in Page-Fault or General Protection
fault, due to the way va_args are used in XenStoreVSPrint().
Also, now I've tried to build OVMF with gcc 5.4, same result, using
-flto result in a firmware that does not work.
I've tried to create a small programme that use the va_args in the same
way, and compiled-test it with different gcc (gcc 4.9.2, gcc 5.4, gcc
6.3), then depending on the options use, it does not work or it works:
Don't work (prog segv or wrong output):
gcc -o prog va_main.c va_test.c
gcc -o prog -flto va_main.c va_test.c
gcc -Os -o prog -flto va_main.c va_test.c
Work:
gcc -Os -o prog va_main.c va_test.c
I'll attach va_main.c and va_test.c.
So, should I add EFIAPI to XenStoreVSPrint, as it is using VA_COPY?
--
Anthony PERARD
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-21 16:39 ` Anthony PERARD
@ 2017-02-21 17:07 ` Laszlo Ersek
2017-02-21 17:53 ` Anthony PERARD
0 siblings, 1 reply; 29+ messages in thread
From: Laszlo Ersek @ 2017-02-21 17:07 UTC (permalink / raw)
To: Anthony PERARD
Cc: Jordan Justen, Gao, Liming, Zhu, Yonghong, Ard Biesheuvel,
edk2-devel, Rebecca Cran, Konrad Rzeszutek Wilk
CC Rebecca & Konrad
On 02/21/17 17:39, Anthony PERARD wrote:
> On Sat, Dec 03, 2016 at 06:59:28PM +0100, Laszlo Ersek wrote:
>> On 12/02/16 20:26, Laszlo Ersek wrote:
>>> On 12/02/16 17:02, Anthony PERARD wrote:
>>>> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
>>>>> On 12/01/16 16:28, Anthony PERARD wrote:
>>>>>> Hi,
>>>>>>
>>>>>> That might be only with the Xen part of OVMF but now that the GCC5
>>>>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
>>>>>> to boot in Xen guests.
>>>>>>
>>>> [...]
>>>>>>
>>>>>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
>>>>>> boot.
>>>>>>
>>>>>> While trying to debug that, I've added some debug prints (in this module
>>>>>> and in XenPvBlkDxe), and the exception could change and become a "page
>>>>>> fault" instead, or even an assert failure in the PrintLib, that was the
>>>>>> ASSERT(Buffer != NULL) at I think
>>>>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>>>>>
>>>>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
>>>>>> again. My guest is that gcc would bypass (optimise) an exported
>>>>>> functions and call directly an internal one but without reordering the
>>>>>> arguments (EFIAPI vs nothing).
>>>>>>
>>>>>> Does that make sense?
>>>>>
>>>>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
>>>>> (until the root cause is found and addressed) to the XenBusDxe patches.
>>>>
>>>> That works, using GCC49 (with gcc 6.2.1) works as well.
>>>>
>>>>> Hrpmf, wait a second, I do see something interesting: in this series you
>>>>> *are* modifying APIs declared in a library class header (namely
>>>>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>>>>> libraries) *are* required to specify EFIAPI.
>>>>>
>>>>> What happens if you apply patch #1 only?
>>>>
>>>> With only XenHypercallLib changes, the error is the same.
>>>>
>>>> But I did find the minimum change needed, it envolve a function with a
>>>> VA_LIST as argument.
>>>>
>>>> With only the following patch, OVMF works again.
>>>>
>>>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
>>>> index 1666c4b..85b0956 100644
>>>> --- a/OvmfPkg/XenBusDxe/XenStore.c
>>>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
>>>> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (
>>>> }
>>>>
>>>> XENSTORE_STATUS
>>>> +EFIAPI
>>>> XenStoreVSPrint (
>>>> IN CONST XENSTORE_TRANSACTION *Transaction,
>>>> IN CONST CHAR8 *DirectoryPath,
>>>> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
>>>> index c9d4c65..33bb647 100644
>>>> --- a/OvmfPkg/XenBusDxe/XenStore.h
>>>> +++ b/OvmfPkg/XenBusDxe/XenStore.h
>>>> @@ -209,6 +209,7 @@ XenStoreSPrint (
>>>> indicating the type of write failure.
>>>> **/
>>>> XENSTORE_STATUS
>>>> +EFIAPI
>>>> XenStoreVSPrint (
>>>> IN CONST XENSTORE_TRANSACTION *Transaction,
>>>> IN CONST CHAR8 *DirectoryPath,
>>>> IN CONST CHAR8 *Node,
>>>> IN CONST CHAR8 *FormatString,
>>>> IN VA_LIST Marker
>>>> );
>>>>
>>>>
>>>> I think the exception happen when this function is called via
>>>> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
>>>> OvmfPkg/XenPvBlkDxe/BlockFront.c
>>>>
>>>
>>> It used to be a known requirement / limitation that all functions with
>>> variable argument lists had to be EFIAPI, regardless of cross-module
>>> use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed
>>> that, and varargs should "just work" now. I suspect this is a
>>> __builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
>>> It might make sense to report a bug in the upstream gcc tracker.
>>>
>>> ... Oh wow, this is a known gcc bug! See:
>>>
>>> https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html
>>>
>>> Upstream gcc BZ <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955> was
>>> apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
>>> So we'll either need a GCC6 toolchain in BaseTools that drops -flto, in
>>> order to work around this gcc issue, or we'll have to ask gcc-6 users to
>>> use at least gcc-6.3.
>>>
>>> Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools
>>> workaround then.
>>
>> I think I got confused in parts of the above; I got some details wrong.
>> Namely, commit 48d5f9a551a9 did not remove the requirement/limitation
>> that all varargs functions have to be EFIAPI. Said commit only changed
>> how the VA_*() macros would be implemented.
>>
>> The two caller functions of XenStoreVSPrint(), namely XenStoreSPrint()
>> and XenBusXenStoreSPrint(), are varargs functions, but they are already
>> EFIAPI. So the requirement/limitation (which was unaffected by
>> 48d5f9a551a9) is actually satisfied / considered in XenBusDxe.
>>
>> The XenStoreVSPrint() function, which you identified as the breaking
>> part, is *not* a varargs function itself, so it needn't be EFIAPI. It
>> simply receives a VA_LIST parameter (which is "__builtin_ms_va_list",
>> from commit 48d5f9a551a9), and (a) copies it with VA_COPY() for passing
>> the copy to SPrintLengthAsciiFormat(), (b) passes the original parameter
>> to AsciiVSPrint(). In turn both of those functions call the common
>> BasePrintLibSPrintMarker() function.
>>
>> Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c6> says,
>>
>>> This is bug report that the specialized
>>> __builtin_ms_va_{list,start,end,copy} builtins have stopped working
>>> when -flto is used. They worked until gcc 5.3, both with or without
>>> -flto. In gcc 6.1 with -flto, the canonical iterator __builtin_va_arg
>>> ignores them and works on a sysv_va_list. To be precise, it's
>>> __builtin_va_arg in the context of -flto that's broken, not the
>>> specialized builtins. __builtin_va_arg has always been a polymorphic
>>> builtin that changes its behavior based on the type of va_list it's
>>> given as an argument. Without this polymorphic behavior, there's no
>>> way to iterate over an ms_va_list.
>>
>> Apparently, when BasePrintLibSPrintMarker() finally calls VA_ARG() (==
>> __builtin_va_arg(), from commit 48d5f9a551a9) on Marker / Marker2, with
>> LTO enabled, __builtin_va_arg() fails to notice what context
>> VaListMarker comes from:
>> - __builtin_ms_va_start() in XenStoreSPrint() and XenBusXenStoreSPrint(), or
>> - __builtin_ms_va_copy() in XenStoreVSPrint().
>>
>> So I think we *are* being hit by gcc BZ#70955, and making
>> XenStoreVSPrint() EFIAPI only masks the issue. Comment
>> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c7> seems relevant:
>>
>>> The change with GCC 6 is that the builtins are now lowered during
>>> link-time optimization rather than at compile-time. Thus the abi
>>> selection bits are possibly not transfered correctly (type merging?).
>>> I remember the business was quite ugly, but eventually we just miss to
>>> properly transfer the function attribute.
>>
>> The end result for edk2 remains the same (= BaseTools should work around
>> this gcc issue with a new GCC6 toolchain that drops -flto, unless
>> gcc-6.3 is about to become available to users real quick). I just wanted
>> to point out that my earlier statement "commit 48d5f9a551a9 had removed
>> the need for varargs functions to be EFIAPI" was incorrect -- varargs
>> functions still must be EFIAPI (and XenBusDxe conforms, see
>> XenStoreSPrint() and XenBusXenStoreSPrint()).
>
> Hi Laszlo,
>
> Now that gcc 6.3 is out, the bug described in the thread strikes again.
> Building OVMF with -flto result in Page-Fault or General Protection
> fault, due to the way va_args are used in XenStoreVSPrint().
>
> Also, now I've tried to build OVMF with gcc 5.4, same result, using
> -flto result in a firmware that does not work.
>
>
> I've tried to create a small programme that use the va_args in the same
> way, and compiled-test it with different gcc (gcc 4.9.2, gcc 5.4, gcc
> 6.3), then depending on the options use, it does not work or it works:
>
> Don't work (prog segv or wrong output):
> gcc -o prog va_main.c va_test.c
> gcc -o prog -flto va_main.c va_test.c
> gcc -Os -o prog -flto va_main.c va_test.c
>
> Work:
> gcc -Os -o prog va_main.c va_test.c
>
> I'll attach va_main.c and va_test.c.
>
> So, should I add EFIAPI to XenStoreVSPrint, as it is using VA_COPY?
>
Hm, please help me jog my memory...
If I remember correctly, this is still a GCC bug, one that we suppressed for gcc-6.2 with your patch as follows:
> commit 432f1d83f77acf92d52ef18d2cee6dbf7c5b9b86
> Author: Anthony PERARD <anthony.perard@citrix.com>
> Date: Tue Dec 6 12:03:25 2016 +0000
>
> OvmfPkg/build.sh: Use GCC49 toolchains with GCC 6.[0-2]
>
> The goal of the patch is to avoid using -flto with GCC 6.0 to 6.2.
>
> This is to workaround a GCC bug:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
> index 95fe8fb07647..b6e936056ca0 100755
> --- a/OvmfPkg/build.sh
> +++ b/OvmfPkg/build.sh
> @@ -102,7 +102,7 @@ case `uname` in
> 4.8.*)
> TARGET_TOOLS=GCC48
> ;;
> - 4.9.*)
> + 4.9.*|6.[0-2].*)
> TARGET_TOOLS=GCC49
> ;;
> *)
Do I understand correctly that the gcc bug has not been fixed in gcc-6.3, and -- because we don't suppress it for gcc-6.3 as the above expression does not match -- it causes problems again?
You also mention gcc-5.4 as problematic. I think we haven't received such reports about gcc-5 versions up to and including gcc-5.3 (that's why GCC5 is the default selection in "OvmfPkg/build.sh"). Do you mean that the gcc bug has now been "backported" from the gcc-6 series to the gcc-5 series (starting with gcc-5.4)?
If that's the case, then I suggest flipping "OvmfPkg/build.sh" from black-listing gcc versions for -flto to white-listing. In other words, assume that -flto is generally broken with GCC, except for a few known versions: 5.0 through 5.3 inclusive. Those versions should trigger the use of the GCC5 toolchain, and everything else (5.4+, 6.*, 4.9.*) should use GCC49.
I don't feel comfortable about adding EFIAPI to XenStoreVSPrint just because it takes a VA_LIST parameter -- note: it is *not* a varargs function itself! --; the same issue might hit elsewhere in the edk2 tree at any time, outside of OvmfPkg too.
Would the gcc white-listing work for you?
Note that the white-listing would practically undo Konrad's commit 2667ad40919a ("OvmfPkg/build.sh: Make GCC5 the default toolchain, catch GCC43 and earlier", 2016-11-23), but given the recent gcc developments (gcc-6.3 has not fixed the gcc bug, and the bug has even surfaced in gcc-5.4), I think it would be justified.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-21 17:07 ` Laszlo Ersek
@ 2017-02-21 17:53 ` Anthony PERARD
2017-02-21 19:02 ` Laszlo Ersek
0 siblings, 1 reply; 29+ messages in thread
From: Anthony PERARD @ 2017-02-21 17:53 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Jordan Justen, Gao, Liming, Zhu, Yonghong, Ard Biesheuvel,
edk2-devel, Rebecca Cran, Konrad Rzeszutek Wilk
On Tue, Feb 21, 2017 at 06:07:15PM +0100, Laszlo Ersek wrote:
> CC Rebecca & Konrad
>
> On 02/21/17 17:39, Anthony PERARD wrote:
> > On Sat, Dec 03, 2016 at 06:59:28PM +0100, Laszlo Ersek wrote:
> >> On 12/02/16 20:26, Laszlo Ersek wrote:
> >>> On 12/02/16 17:02, Anthony PERARD wrote:
> >>>> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
> >>>>> On 12/01/16 16:28, Anthony PERARD wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> That might be only with the Xen part of OVMF but now that the GCC5
> >>>>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
> >>>>>> to boot in Xen guests.
> >>>>>>
> >>>> [...]
> >>>>>>
> >>>>>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
> >>>>>> boot.
> >>>>>>
> >>>>>> While trying to debug that, I've added some debug prints (in this module
> >>>>>> and in XenPvBlkDxe), and the exception could change and become a "page
> >>>>>> fault" instead, or even an assert failure in the PrintLib, that was the
> >>>>>> ASSERT(Buffer != NULL) at I think
> >>>>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
> >>>>>>
> >>>>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
> >>>>>> again. My guest is that gcc would bypass (optimise) an exported
> >>>>>> functions and call directly an internal one but without reordering the
> >>>>>> arguments (EFIAPI vs nothing).
> >>>>>>
> >>>>>> Does that make sense?
> >>>>>
> >>>>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
> >>>>> (until the root cause is found and addressed) to the XenBusDxe patches.
> >>>>
> >>>> That works, using GCC49 (with gcc 6.2.1) works as well.
> >>>>
> >>>>> Hrpmf, wait a second, I do see something interesting: in this series you
> >>>>> *are* modifying APIs declared in a library class header (namely
> >>>>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
> >>>>> libraries) *are* required to specify EFIAPI.
> >>>>>
> >>>>> What happens if you apply patch #1 only?
> >>>>
> >>>> With only XenHypercallLib changes, the error is the same.
> >>>>
> >>>> But I did find the minimum change needed, it envolve a function with a
> >>>> VA_LIST as argument.
> >>>>
> >>>> With only the following patch, OVMF works again.
> >>>>
> >>>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> >>>> index 1666c4b..85b0956 100644
> >>>> --- a/OvmfPkg/XenBusDxe/XenStore.c
> >>>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> >>>> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (
> >>>> }
> >>>>
> >>>> XENSTORE_STATUS
> >>>> +EFIAPI
> >>>> XenStoreVSPrint (
> >>>> IN CONST XENSTORE_TRANSACTION *Transaction,
> >>>> IN CONST CHAR8 *DirectoryPath,
> >>>> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
> >>>> index c9d4c65..33bb647 100644
> >>>> --- a/OvmfPkg/XenBusDxe/XenStore.h
> >>>> +++ b/OvmfPkg/XenBusDxe/XenStore.h
> >>>> @@ -209,6 +209,7 @@ XenStoreSPrint (
> >>>> indicating the type of write failure.
> >>>> **/
> >>>> XENSTORE_STATUS
> >>>> +EFIAPI
> >>>> XenStoreVSPrint (
> >>>> IN CONST XENSTORE_TRANSACTION *Transaction,
> >>>> IN CONST CHAR8 *DirectoryPath,
> >>>> IN CONST CHAR8 *Node,
> >>>> IN CONST CHAR8 *FormatString,
> >>>> IN VA_LIST Marker
> >>>> );
> >>>>
> >>>>
> >>>> I think the exception happen when this function is called via
> >>>> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
> >>>> OvmfPkg/XenPvBlkDxe/BlockFront.c
> >>>>
> >>>
> >>> It used to be a known requirement / limitation that all functions with
> >>> variable argument lists had to be EFIAPI, regardless of cross-module
> >>> use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed
> >>> that, and varargs should "just work" now. I suspect this is a
> >>> __builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
> >>> It might make sense to report a bug in the upstream gcc tracker.
> >>>
> >>> ... Oh wow, this is a known gcc bug! See:
> >>>
> >>> https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html
> >>>
> >>> Upstream gcc BZ <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955> was
> >>> apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
> >>> So we'll either need a GCC6 toolchain in BaseTools that drops -flto, in
> >>> order to work around this gcc issue, or we'll have to ask gcc-6 users to
> >>> use at least gcc-6.3.
> >>>
> >>> Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools
> >>> workaround then.
> >>
> >> I think I got confused in parts of the above; I got some details wrong.
> >> Namely, commit 48d5f9a551a9 did not remove the requirement/limitation
> >> that all varargs functions have to be EFIAPI. Said commit only changed
> >> how the VA_*() macros would be implemented.
> >>
> >> The two caller functions of XenStoreVSPrint(), namely XenStoreSPrint()
> >> and XenBusXenStoreSPrint(), are varargs functions, but they are already
> >> EFIAPI. So the requirement/limitation (which was unaffected by
> >> 48d5f9a551a9) is actually satisfied / considered in XenBusDxe.
> >>
> >> The XenStoreVSPrint() function, which you identified as the breaking
> >> part, is *not* a varargs function itself, so it needn't be EFIAPI. It
> >> simply receives a VA_LIST parameter (which is "__builtin_ms_va_list",
> >> from commit 48d5f9a551a9), and (a) copies it with VA_COPY() for passing
> >> the copy to SPrintLengthAsciiFormat(), (b) passes the original parameter
> >> to AsciiVSPrint(). In turn both of those functions call the common
> >> BasePrintLibSPrintMarker() function.
> >>
> >> Comment <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c6> says,
> >>
> >>> This is bug report that the specialized
> >>> __builtin_ms_va_{list,start,end,copy} builtins have stopped working
> >>> when -flto is used. They worked until gcc 5.3, both with or without
> >>> -flto. In gcc 6.1 with -flto, the canonical iterator __builtin_va_arg
> >>> ignores them and works on a sysv_va_list. To be precise, it's
> >>> __builtin_va_arg in the context of -flto that's broken, not the
> >>> specialized builtins. __builtin_va_arg has always been a polymorphic
> >>> builtin that changes its behavior based on the type of va_list it's
> >>> given as an argument. Without this polymorphic behavior, there's no
> >>> way to iterate over an ms_va_list.
> >>
> >> Apparently, when BasePrintLibSPrintMarker() finally calls VA_ARG() (==
> >> __builtin_va_arg(), from commit 48d5f9a551a9) on Marker / Marker2, with
> >> LTO enabled, __builtin_va_arg() fails to notice what context
> >> VaListMarker comes from:
> >> - __builtin_ms_va_start() in XenStoreSPrint() and XenBusXenStoreSPrint(), or
> >> - __builtin_ms_va_copy() in XenStoreVSPrint().
> >>
> >> So I think we *are* being hit by gcc BZ#70955, and making
> >> XenStoreVSPrint() EFIAPI only masks the issue. Comment
> >> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c7> seems relevant:
> >>
> >>> The change with GCC 6 is that the builtins are now lowered during
> >>> link-time optimization rather than at compile-time. Thus the abi
> >>> selection bits are possibly not transfered correctly (type merging?).
> >>> I remember the business was quite ugly, but eventually we just miss to
> >>> properly transfer the function attribute.
> >>
> >> The end result for edk2 remains the same (= BaseTools should work around
> >> this gcc issue with a new GCC6 toolchain that drops -flto, unless
> >> gcc-6.3 is about to become available to users real quick). I just wanted
> >> to point out that my earlier statement "commit 48d5f9a551a9 had removed
> >> the need for varargs functions to be EFIAPI" was incorrect -- varargs
> >> functions still must be EFIAPI (and XenBusDxe conforms, see
> >> XenStoreSPrint() and XenBusXenStoreSPrint()).
> >
> > Hi Laszlo,
> >
> > Now that gcc 6.3 is out, the bug described in the thread strikes again.
> > Building OVMF with -flto result in Page-Fault or General Protection
> > fault, due to the way va_args are used in XenStoreVSPrint().
> >
> > Also, now I've tried to build OVMF with gcc 5.4, same result, using
> > -flto result in a firmware that does not work.
> >
> >
> > I've tried to create a small programme that use the va_args in the same
> > way, and compiled-test it with different gcc (gcc 4.9.2, gcc 5.4, gcc
> > 6.3), then depending on the options use, it does not work or it works:
> >
> > Don't work (prog segv or wrong output):
> > gcc -o prog va_main.c va_test.c
> > gcc -o prog -flto va_main.c va_test.c
> > gcc -Os -o prog -flto va_main.c va_test.c
> >
> > Work:
> > gcc -Os -o prog va_main.c va_test.c
> >
> > I'll attach va_main.c and va_test.c.
> >
> > So, should I add EFIAPI to XenStoreVSPrint, as it is using VA_COPY?
> >
>
> Hm, please help me jog my memory...
>
> If I remember correctly, this is still a GCC bug, one that we suppressed for gcc-6.2 with your patch as follows:
Yes.
> > commit 432f1d83f77acf92d52ef18d2cee6dbf7c5b9b86
> > Author: Anthony PERARD <anthony.perard@citrix.com>
> > Date: Tue Dec 6 12:03:25 2016 +0000
> >
> > OvmfPkg/build.sh: Use GCC49 toolchains with GCC 6.[0-2]
> >
> > The goal of the patch is to avoid using -flto with GCC 6.0 to 6.2.
> >
> > This is to workaround a GCC bug:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> >
> > diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
> > index 95fe8fb07647..b6e936056ca0 100755
> > --- a/OvmfPkg/build.sh
> > +++ b/OvmfPkg/build.sh
> > @@ -102,7 +102,7 @@ case `uname` in
> > 4.8.*)
> > TARGET_TOOLS=GCC48
> > ;;
> > - 4.9.*)
> > + 4.9.*|6.[0-2].*)
> > TARGET_TOOLS=GCC49
> > ;;
> > *)
>
> Do I understand correctly that the gcc bug has not been fixed in gcc-6.3, and -- because we don't suppress it for gcc-6.3 as the above expression does not match -- it causes problems again?
The bug describe in the GCC bugzilla is probably fix, but the test-case
does not make use of __builtin_ms_va_copy.
> You also mention gcc-5.4 as problematic. I think we haven't received such reports about gcc-5 versions up to and including gcc-5.3 (that's why GCC5 is the default selection in "OvmfPkg/build.sh"). Do you mean that the gcc bug has now been "backported" from the gcc-6 series to the gcc-5 series (starting with gcc-5.4)?
I don't know the state of gcc-5.0 to gcc-5.3, I have never tested -flto
with gcc-5.x (until now), I would say they are also problematic until
proven otherwise.
> If that's the case, then I suggest flipping "OvmfPkg/build.sh" from black-listing gcc versions for -flto to white-listing. In other words, assume that -flto is generally broken with GCC, except for a few known versions: 5.0 through 5.3 inclusive. Those versions should trigger the use of the GCC5 toolchain, and everything else (5.4+, 6.*, 4.9.*) should use GCC49.
>
> I don't feel comfortable about adding EFIAPI to XenStoreVSPrint just because it takes a VA_LIST parameter -- note: it is *not* a varargs function itself! --; the same issue might hit elsewhere in the edk2 tree at any time, outside of OvmfPkg too.
>From the different tests I've done, I feel more like VA_COPY might be
the issue, but I don't know how __builtin_ms_va_* are supposed to be
used.
> Would the gcc white-listing work for you?
>
> Note that the white-listing would practically undo Konrad's commit 2667ad40919a ("OvmfPkg/build.sh: Make GCC5 the default toolchain, catch GCC43 and earlier", 2016-11-23), but given the recent gcc developments (gcc-6.3 has not fixed the gcc bug, and the bug has even surfaced in gcc-5.4), I think it would be justified.
Do be honnest, I don't think the toolchain GCC5 has ever been tested
with gcc-5.x and the module XenBusDxe. I think most people that want to
start OVMF under Xen are likely to build it with gcc-4.9 or already had
gcc-6.x when OVMF switch to the GCC5 toolchain by default.
--
Anthony PERARD
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-21 17:53 ` Anthony PERARD
@ 2017-02-21 19:02 ` Laszlo Ersek
2017-02-21 19:08 ` Rebecca Cran
2017-02-22 8:54 ` Gao, Liming
0 siblings, 2 replies; 29+ messages in thread
From: Laszlo Ersek @ 2017-02-21 19:02 UTC (permalink / raw)
To: Anthony PERARD
Cc: Jordan Justen, Gao, Liming, Zhu, Yonghong, Ard Biesheuvel,
edk2-devel, Rebecca Cran, Konrad Rzeszutek Wilk
On 02/21/17 18:53, Anthony PERARD wrote:
> On Tue, Feb 21, 2017 at 06:07:15PM +0100, Laszlo Ersek wrote:
>> CC Rebecca & Konrad
>>
>> On 02/21/17 17:39, Anthony PERARD wrote:
[snip]
>>> So, should I add EFIAPI to XenStoreVSPrint, as it is using VA_COPY?
>>>
>>
>> Hm, please help me jog my memory...
>>
>> If I remember correctly, this is still a GCC bug, one that we suppressed for gcc-6.2 with your patch as follows:
>
> Yes.
>
>>> commit 432f1d83f77acf92d52ef18d2cee6dbf7c5b9b86
>>> Author: Anthony PERARD <anthony.perard@citrix.com>
>>> Date: Tue Dec 6 12:03:25 2016 +0000
>>>
>>> OvmfPkg/build.sh: Use GCC49 toolchains with GCC 6.[0-2]
>>>
>>> The goal of the patch is to avoid using -flto with GCC 6.0 to 6.2.
>>>
>>> This is to workaround a GCC bug:
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
>>> index 95fe8fb07647..b6e936056ca0 100755
>>> --- a/OvmfPkg/build.sh
>>> +++ b/OvmfPkg/build.sh
>>> @@ -102,7 +102,7 @@ case `uname` in
>>> 4.8.*)
>>> TARGET_TOOLS=GCC48
>>> ;;
>>> - 4.9.*)
>>> + 4.9.*|6.[0-2].*)
>>> TARGET_TOOLS=GCC49
>>> ;;
>>> *)
>>
>> Do I understand correctly that the gcc bug has not been fixed in
>> gcc-6.3, and -- because we don't suppress it for gcc-6.3 as the
>> above expression does not match -- it causes problems again?
>
> The bug describe in the GCC bugzilla is probably fix, but the
> test-case does not make use of __builtin_ms_va_copy.
:/
>
>> You also mention gcc-5.4 as problematic. I think we haven't
>> received such reports about gcc-5 versions up to and including
>> gcc-5.3 (that's why GCC5 is the default selection in
>> "OvmfPkg/build.sh"). Do you mean that the gcc bug has now been
>> "backported" from the gcc-6 series to the gcc-5 series (starting
>> with gcc-5.4)?
>
> I don't know the state of gcc-5.0 to gcc-5.3, I have never tested -flto
> with gcc-5.x (until now), I would say they are also problematic until
> proven otherwise.
When we enabled GCC5, it definitely worked for at least one gcc release,
with -flto. (-flto is the default for DEBUG and RELEASE builds with
GCC5; NOOPT disables -Os and -flto.)
>
>> If that's the case, then I suggest flipping "OvmfPkg/build.sh" from
>> black-listing gcc versions for -flto to white-listing. In other
>> words, assume that -flto is generally broken with GCC, except for a
>> few known versions: 5.0 through 5.3 inclusive. Those versions
>> should trigger the use of the GCC5 toolchain, and everything else
>> (5.4+, 6.*, 4.9.*) should use GCC49.
>>
>> I don't feel comfortable about adding EFIAPI to XenStoreVSPrint
>> just because it takes a VA_LIST parameter -- note: it is *not* a
>> varargs function itself! --; the same issue might hit elsewhere in
>> the edk2 tree at any time, outside of OvmfPkg too.
>
> From the different tests I've done, I feel more like VA_COPY might be
> the issue, but I don't know how __builtin_ms_va_* are supposed to be
> used.
If I recall correctly, from the upstream GCC bug, the problem is that
__builtin_va_list does not track internally whether it was created in an
msabi or sysvabi function, and therefore the va_* functions cannot be
used transparently on it. Instead, when va_list is accessed, the
accessor builtins seem to apply the currently executing function's
calling convetion to va_list. (Even if the creation context of va_list
was different.)
>
>> Would the gcc white-listing work for you?
>>
>> Note that the white-listing would practically undo Konrad's commit
>> 2667ad40919a ("OvmfPkg/build.sh: Make GCC5 the default toolchain,
>> catch GCC43 and earlier", 2016-11-23), but given the recent gcc
>> developments (gcc-6.3 has not fixed the gcc bug, and the bug has
>> even surfaced in gcc-5.4), I think it would be justified.
>
> Do be honnest, I don't think the toolchain GCC5 has ever been tested
> with gcc-5.x and the module XenBusDxe. I think most people that want to
> start OVMF under Xen are likely to build it with gcc-4.9 or already had
> gcc-6.x when OVMF switch to the GCC5 toolchain by default.
>
Okay... I'm equally fine if we just say "given that GCC is broken like
this, we hereby require all functions that take a variable argument
list, *or* a VA_LIST parameter, to be EFIAPI". (The first part of the
requirement already exists.)
But in this case, the full edk2 codebase has to be grepped for
VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if
they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems
incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.)
Also, in this case, your commit 432f1d83f77a should likely be reverted.
(Because we are ultimately giving in to the gcc bug.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-21 19:02 ` Laszlo Ersek
@ 2017-02-21 19:08 ` Rebecca Cran
2017-02-21 22:45 ` Jordan Justen
2017-02-22 8:54 ` Gao, Liming
1 sibling, 1 reply; 29+ messages in thread
From: Rebecca Cran @ 2017-02-21 19:08 UTC (permalink / raw)
To: Laszlo Ersek, Anthony PERARD
Cc: Jordan Justen, Gao, Liming, Zhu, Yonghong, Ard Biesheuvel,
edk2-devel, Konrad Rzeszutek Wilk
On 2/21/2017 12:02 PM, Laszlo Ersek wrote:
> But in this case, the full edk2 codebase has to be grepped for
> VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if
> they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems
> incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.)
I think this was discussed previously but I can't remember: is there a
reason for not just compiling everything with -mabi=ms ?
--
Rebecca
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-21 19:08 ` Rebecca Cran
@ 2017-02-21 22:45 ` Jordan Justen
2017-02-21 23:59 ` Laszlo Ersek
0 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2017-02-21 22:45 UTC (permalink / raw)
To: Anthony PERARD, Laszlo Ersek, Rebecca Cran
Cc: Gao, Liming, Zhu, Yonghong, Ard Biesheuvel, edk2-devel,
Konrad Rzeszutek Wilk
On 2017-02-21 11:08:24, Rebecca Cran wrote:
> On 2/21/2017 12:02 PM, Laszlo Ersek wrote:
>
> > But in this case, the full edk2 codebase has to be grepped for
> > VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if
> > they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems
> > incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.)
>
> I think this was discussed previously but I can't remember: is there a
> reason for not just compiling everything with -mabi=ms ?
>
Originally GCC didn't support -mabi=ms. Once it gained support, it
then produced larger executables. Nowadays (and for quite some time),
I think it generally results in smaller executables.
A benefit of not using -mabi=ms is that we are able to catch some
cases of misused EFIAPI with a compiler warning, or unfortunately in
some cases with crashes or misbehaving code.
I think the benefit of helping keep EFIAPI clean means that we should
continue to not use -mabi=ms for DEBUG builds. But, I think it would
be good to get the size advantages of -mabi=ms by enabling it for
RELEASE builds.
-Jordan
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-21 22:45 ` Jordan Justen
@ 2017-02-21 23:59 ` Laszlo Ersek
2017-02-22 14:16 ` Gao, Liming
0 siblings, 1 reply; 29+ messages in thread
From: Laszlo Ersek @ 2017-02-21 23:59 UTC (permalink / raw)
To: Jordan Justen, Anthony PERARD, Rebecca Cran
Cc: Gao, Liming, Zhu, Yonghong, Ard Biesheuvel, edk2-devel,
Konrad Rzeszutek Wilk
On 02/21/17 23:45, Jordan Justen wrote:
> On 2017-02-21 11:08:24, Rebecca Cran wrote:
>> On 2/21/2017 12:02 PM, Laszlo Ersek wrote:
>>
>>> But in this case, the full edk2 codebase has to be grepped for
>>> VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if
>>> they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems
>>> incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.)
>>
>> I think this was discussed previously but I can't remember: is there a
>> reason for not just compiling everything with -mabi=ms ?
>>
>
> Originally GCC didn't support -mabi=ms. Once it gained support, it
> then produced larger executables. Nowadays (and for quite some time),
> I think it generally results in smaller executables.
>
> A benefit of not using -mabi=ms is that we are able to catch some
> cases of misused EFIAPI
I agree.
> with a compiler warning, or unfortunately in
> some cases with crashes or misbehaving code.
>
> I think the benefit of helping keep EFIAPI clean means that we should
> continue to not use -mabi=ms for DEBUG builds.
I agree (also for NOOPT builds).
> But, I think it would
> be good to get the size advantages of -mabi=ms by enabling it for
> RELEASE builds.
That sounds useful too (even though it wouldn't make the current problem go away).
As one caveat, I believe -mabi=ms wouldn't be allowed for building OpensslLib even for RELEASE. See "-DNO_MSABI_VA_FUNCS" in "OpensslLib.inf":
commit b2dc04a87fab89307240dc0f30b9a23bb5726c81
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Sun Jul 17 11:57:45 2016 +0200
CryptoPkg: set new define to avoid MS ABI VA_LIST on GCC/X64
Set the #define NO_MSABI_VA_FUNCS that will be introduced in a subsequent
patch to avoid the use of the MS ABI in variadic functions. In EDK2, such
functions normally require the EFIAPI modifier to be used, but for external
libraries such as OpenSSL, which lack these annotations, it is easier to
simply revert to the default SysV style VA_LIST ABI.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Tested-By: Liming Gao <liming.gao@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-21 19:02 ` Laszlo Ersek
2017-02-21 19:08 ` Rebecca Cran
@ 2017-02-22 8:54 ` Gao, Liming
2017-02-23 10:19 ` Laszlo Ersek
1 sibling, 1 reply; 29+ messages in thread
From: Gao, Liming @ 2017-02-22 8:54 UTC (permalink / raw)
To: Laszlo Ersek, Anthony PERARD
Cc: Ard Biesheuvel, Justen, Jordan L, edk2-devel@ml01.01.org
Laszlo:
In edk2, I find the several functions with VA_LIST have no EFIAPI. They may use VA_ARG() or call other functions, but they don't use VA_COPY(). In Base.h, VA_ARG() is defined as __builtin_va_arg(), which is same to native one. VA_COPY() is defined as __builtin_ms_va_copy(). So, I also think this is MS ABI request. That means only if the function implementation uses VA_START(),VA_END() or VA_COPY(), it must be declared with EFIAPI.
MdePkg\Library\BasePrintLib\PrintLibInternal.c BasePrintLibSPrintMarker()
ShellPkg\Library\UefiShellLib\UefiShellLib.c InternalShellPrintWorker()
Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Wednesday, February 22, 2017 3:03 AM
>To: Anthony PERARD <anthony.perard@citrix.com>
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
><jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Gao, Liming
><liming.gao@intel.com>
>Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when
>compiled with GCC5
>
>On 02/21/17 18:53, Anthony PERARD wrote:
>> On Tue, Feb 21, 2017 at 06:07:15PM +0100, Laszlo Ersek wrote:
>>> CC Rebecca & Konrad
>>>
>>> On 02/21/17 17:39, Anthony PERARD wrote:
>
>[snip]
>
>>>> So, should I add EFIAPI to XenStoreVSPrint, as it is using VA_COPY?
>>>>
>>>
>>> Hm, please help me jog my memory...
>>>
>>> If I remember correctly, this is still a GCC bug, one that we suppressed for
>gcc-6.2 with your patch as follows:
>>
>> Yes.
>>
>>>> commit 432f1d83f77acf92d52ef18d2cee6dbf7c5b9b86
>>>> Author: Anthony PERARD <anthony.perard@citrix.com>
>>>> Date: Tue Dec 6 12:03:25 2016 +0000
>>>>
>>>> OvmfPkg/build.sh: Use GCC49 toolchains with GCC 6.[0-2]
>>>>
>>>> The goal of the patch is to avoid using -flto with GCC 6.0 to 6.2.
>>>>
>>>> This is to workaround a GCC bug:
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>> diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
>>>> index 95fe8fb07647..b6e936056ca0 100755
>>>> --- a/OvmfPkg/build.sh
>>>> +++ b/OvmfPkg/build.sh
>>>> @@ -102,7 +102,7 @@ case `uname` in
>>>> 4.8.*)
>>>> TARGET_TOOLS=GCC48
>>>> ;;
>>>> - 4.9.*)
>>>> + 4.9.*|6.[0-2].*)
>>>> TARGET_TOOLS=GCC49
>>>> ;;
>>>> *)
>>>
>>> Do I understand correctly that the gcc bug has not been fixed in
>>> gcc-6.3, and -- because we don't suppress it for gcc-6.3 as the
>>> above expression does not match -- it causes problems again?
>>
>> The bug describe in the GCC bugzilla is probably fix, but the
>> test-case does not make use of __builtin_ms_va_copy.
>
>:/
>
>>
>>> You also mention gcc-5.4 as problematic. I think we haven't
>>> received such reports about gcc-5 versions up to and including
>>> gcc-5.3 (that's why GCC5 is the default selection in
>>> "OvmfPkg/build.sh"). Do you mean that the gcc bug has now been
>>> "backported" from the gcc-6 series to the gcc-5 series (starting
>>> with gcc-5.4)?
>
>>
>> I don't know the state of gcc-5.0 to gcc-5.3, I have never tested -flto
>> with gcc-5.x (until now), I would say they are also problematic until
>> proven otherwise.
>
>When we enabled GCC5, it definitely worked for at least one gcc release,
>with -flto. (-flto is the default for DEBUG and RELEASE builds with
>GCC5; NOOPT disables -Os and -flto.)
>
>>
>>> If that's the case, then I suggest flipping "OvmfPkg/build.sh" from
>>> black-listing gcc versions for -flto to white-listing. In other
>>> words, assume that -flto is generally broken with GCC, except for a
>>> few known versions: 5.0 through 5.3 inclusive. Those versions
>>> should trigger the use of the GCC5 toolchain, and everything else
>>> (5.4+, 6.*, 4.9.*) should use GCC49.
>>>
>>> I don't feel comfortable about adding EFIAPI to XenStoreVSPrint
>>> just because it takes a VA_LIST parameter -- note: it is *not* a
>>> varargs function itself! --; the same issue might hit elsewhere in
>>> the edk2 tree at any time, outside of OvmfPkg too.
>>
>> From the different tests I've done, I feel more like VA_COPY might be
>> the issue, but I don't know how __builtin_ms_va_* are supposed to be
>> used.
>
>If I recall correctly, from the upstream GCC bug, the problem is that
>__builtin_va_list does not track internally whether it was created in an
>msabi or sysvabi function, and therefore the va_* functions cannot be
>used transparently on it. Instead, when va_list is accessed, the
>accessor builtins seem to apply the currently executing function's
>calling convetion to va_list. (Even if the creation context of va_list
>was different.)
>
>>
>>> Would the gcc white-listing work for you?
>>>
>>> Note that the white-listing would practically undo Konrad's commit
>>> 2667ad40919a ("OvmfPkg/build.sh: Make GCC5 the default toolchain,
>>> catch GCC43 and earlier", 2016-11-23), but given the recent gcc
>>> developments (gcc-6.3 has not fixed the gcc bug, and the bug has
>>> even surfaced in gcc-5.4), I think it would be justified.
>>
>> Do be honnest, I don't think the toolchain GCC5 has ever been tested
>> with gcc-5.x and the module XenBusDxe. I think most people that want to
>> start OVMF under Xen are likely to build it with gcc-4.9 or already had
>> gcc-6.x when OVMF switch to the GCC5 toolchain by default.
>>
>
>Okay... I'm equally fine if we just say "given that GCC is broken like
>this, we hereby require all functions that take a variable argument
>list, *or* a VA_LIST parameter, to be EFIAPI". (The first part of the
>requirement already exists.)
>
>But in this case, the full edk2 codebase has to be grepped for
>VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if
>they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems
>incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.)
>
>Also, in this case, your commit 432f1d83f77a should likely be reverted.
>(Because we are ultimately giving in to the gcc bug.)
>
>Thanks
>Laszlo
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-21 23:59 ` Laszlo Ersek
@ 2017-02-22 14:16 ` Gao, Liming
0 siblings, 0 replies; 29+ messages in thread
From: Gao, Liming @ 2017-02-22 14:16 UTC (permalink / raw)
To: 'Laszlo Ersek', Justen, Jordan L, Anthony PERARD,
Rebecca Cran
Cc: Zhu, Yonghong, Ard Biesheuvel, edk2-devel@ml01.01.org,
Konrad Rzeszutek Wilk
Jordan:
If enable -mabi=ms, EFIAPI and ms_var_list will not be required to be defined, right? Do you verify it on X64 arch?
Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Wednesday, February 22, 2017 8:00 AM
>To: Justen, Jordan L <jordan.l.justen@intel.com>; Anthony PERARD
><anthony.perard@citrix.com>; Rebecca Cran <rebecca@bluestop.org>
>Cc: Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong
><yonghong.zhu@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
>edk2-devel@ml01.01.org; Konrad Rzeszutek Wilk <konrad@kernel.org>
>Subject: Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with
>GCC5
>
>On 02/21/17 23:45, Jordan Justen wrote:
>> On 2017-02-21 11:08:24, Rebecca Cran wrote:
>>> On 2/21/2017 12:02 PM, Laszlo Ersek wrote:
>>>
>>>> But in this case, the full edk2 codebase has to be grepped for
>>>> VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if
>>>> they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems
>>>> incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.)
>>>
>>> I think this was discussed previously but I can't remember: is there a
>>> reason for not just compiling everything with -mabi=ms ?
>>>
>>
>> Originally GCC didn't support -mabi=ms. Once it gained support, it
>> then produced larger executables. Nowadays (and for quite some time),
>> I think it generally results in smaller executables.
>>
>> A benefit of not using -mabi=ms is that we are able to catch some
>> cases of misused EFIAPI
>
>I agree.
>
>> with a compiler warning, or unfortunately in
>> some cases with crashes or misbehaving code.
>>
>> I think the benefit of helping keep EFIAPI clean means that we should
>> continue to not use -mabi=ms for DEBUG builds.
>
>I agree (also for NOOPT builds).
>
>> But, I think it would
>> be good to get the size advantages of -mabi=ms by enabling it for
>> RELEASE builds.
>
>That sounds useful too (even though it wouldn't make the current problem go
>away).
>
>As one caveat, I believe -mabi=ms wouldn't be allowed for building OpensslLib
>even for RELEASE. See "-DNO_MSABI_VA_FUNCS" in "OpensslLib.inf":
>
>commit b2dc04a87fab89307240dc0f30b9a23bb5726c81
>Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Date: Sun Jul 17 11:57:45 2016 +0200
>
> CryptoPkg: set new define to avoid MS ABI VA_LIST on GCC/X64
>
> Set the #define NO_MSABI_VA_FUNCS that will be introduced in a
>subsequent
> patch to avoid the use of the MS ABI in variadic functions. In EDK2, such
> functions normally require the EFIAPI modifier to be used, but for external
> libraries such as OpenSSL, which lack these annotations, it is easier to
> simply revert to the default SysV style VA_LIST ABI.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> Tested-By: Liming Gao <liming.gao@intel.com>
> Reviewed-by: Liming Gao <liming.gao@intel.com>
>
>Thanks!
>Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-22 8:54 ` Gao, Liming
@ 2017-02-23 10:19 ` Laszlo Ersek
2017-02-23 12:43 ` Anthony PERARD
2017-02-23 13:00 ` Gao, Liming
0 siblings, 2 replies; 29+ messages in thread
From: Laszlo Ersek @ 2017-02-23 10:19 UTC (permalink / raw)
To: Gao, Liming, Anthony PERARD, Chao Zhang, David Wei, Guo, Mang
Cc: Ard Biesheuvel, Justen, Jordan L, edk2-devel@ml01.01.org
On 02/22/17 09:54, Gao, Liming wrote:
> Laszlo:
> In edk2, I find the several functions with VA_LIST have no EFIAPI.
> They may use VA_ARG() or call other functions, but they don't use
> VA_COPY(). In Base.h, VA_ARG() is defined as __builtin_va_arg(),
> which is same to native one.
You are right; apparently __builtin_va_arg() works with __builtin_ms_va_list and __builtin_va_list alike.
However, as you say,
> VA_COPY() is defined as
> __builtin_ms_va_copy(). So, I also think this is MS ABI request. That
> means only if the function implementation uses VA_START(),VA_END() or
> VA_COPY(), it must be declared with EFIAPI.
- __builtin_va_start / __builtin_ms_va_start,
- __builtin_va_end / __builtin_ms_va_end,
- __builtin_va_copy / __builtin_ms_va_copy
must be matched to __builtin_va_list vs. __builtin_ms_va_list.
>
> MdePkg\Library\BasePrintLib\PrintLibInternal.c BasePrintLibSPrintMarker()
> ShellPkg\Library\UefiShellLib\UefiShellLib.c InternalShellPrintWorker()
Yes, you are right -- when looking for functions that should be made EFIAPI, we shouldn't search for VA_LIST, but VA_START|VA_END|VA_COPY.
Thanks for the correction.
The following command is a good start:
git grep -E -n '\<(VA_START|VA_END|VA_COPY)\>|^[A-Za-z0-9_]' \
| grep -E -B 3 '\<(VA_START|VA_END|VA_COPY)\>'
I just went over the output (it was gruesome), and -- outside of EdkCompatibilityPkg -- I indeed found only a handful of affected functions:
- XenStoreVSPrint() [OvmfPkg/XenBusDxe/XenStore.c]
- VariableGetBestLanguage() [SecurityPkg/VariableAuthenticated/EsalVariableDxeSal/Variable.c]
- SmmBootScriptWrite() [Vlv2TbltDevicePkg/PlatformSmm/SmmScriptSave.c]
Anthony, can you please submit the patch for XenStoreVSPrint()?
Chao Zhang, can you please submit a patch for VariableGetBestLanguage()?
David Wei or Mang Guo, can one of you guys please submit a patch for SmmBootScriptWrite()?
Thanks,
Laszlo
>
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, February 22, 2017 3:03 AM
>> To: Anthony PERARD <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Gao, Liming
>> <liming.gao@intel.com>
>> Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when
>> compiled with GCC5
>>
>> On 02/21/17 18:53, Anthony PERARD wrote:
>>> On Tue, Feb 21, 2017 at 06:07:15PM +0100, Laszlo Ersek wrote:
>>>> CC Rebecca & Konrad
>>>>
>>>> On 02/21/17 17:39, Anthony PERARD wrote:
>>
>> [snip]
>>
>>>>> So, should I add EFIAPI to XenStoreVSPrint, as it is using VA_COPY?
>>>>>
>>>>
>>>> Hm, please help me jog my memory...
>>>>
>>>> If I remember correctly, this is still a GCC bug, one that we suppressed for
>> gcc-6.2 with your patch as follows:
>>>
>>> Yes.
>>>
>>>>> commit 432f1d83f77acf92d52ef18d2cee6dbf7c5b9b86
>>>>> Author: Anthony PERARD <anthony.perard@citrix.com>
>>>>> Date: Tue Dec 6 12:03:25 2016 +0000
>>>>>
>>>>> OvmfPkg/build.sh: Use GCC49 toolchains with GCC 6.[0-2]
>>>>>
>>>>> The goal of the patch is to avoid using -flto with GCC 6.0 to 6.2.
>>>>>
>>>>> This is to workaround a GCC bug:
>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>>> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
>>>>>
>>>>> diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
>>>>> index 95fe8fb07647..b6e936056ca0 100755
>>>>> --- a/OvmfPkg/build.sh
>>>>> +++ b/OvmfPkg/build.sh
>>>>> @@ -102,7 +102,7 @@ case `uname` in
>>>>> 4.8.*)
>>>>> TARGET_TOOLS=GCC48
>>>>> ;;
>>>>> - 4.9.*)
>>>>> + 4.9.*|6.[0-2].*)
>>>>> TARGET_TOOLS=GCC49
>>>>> ;;
>>>>> *)
>>>>
>>>> Do I understand correctly that the gcc bug has not been fixed in
>>>> gcc-6.3, and -- because we don't suppress it for gcc-6.3 as the
>>>> above expression does not match -- it causes problems again?
>>>
>>> The bug describe in the GCC bugzilla is probably fix, but the
>>> test-case does not make use of __builtin_ms_va_copy.
>>
>> :/
>>
>>>
>>>> You also mention gcc-5.4 as problematic. I think we haven't
>>>> received such reports about gcc-5 versions up to and including
>>>> gcc-5.3 (that's why GCC5 is the default selection in
>>>> "OvmfPkg/build.sh"). Do you mean that the gcc bug has now been
>>>> "backported" from the gcc-6 series to the gcc-5 series (starting
>>>> with gcc-5.4)?
>>
>>>
>>> I don't know the state of gcc-5.0 to gcc-5.3, I have never tested -flto
>>> with gcc-5.x (until now), I would say they are also problematic until
>>> proven otherwise.
>>
>> When we enabled GCC5, it definitely worked for at least one gcc release,
>> with -flto. (-flto is the default for DEBUG and RELEASE builds with
>> GCC5; NOOPT disables -Os and -flto.)
>>
>>>
>>>> If that's the case, then I suggest flipping "OvmfPkg/build.sh" from
>>>> black-listing gcc versions for -flto to white-listing. In other
>>>> words, assume that -flto is generally broken with GCC, except for a
>>>> few known versions: 5.0 through 5.3 inclusive. Those versions
>>>> should trigger the use of the GCC5 toolchain, and everything else
>>>> (5.4+, 6.*, 4.9.*) should use GCC49.
>>>>
>>>> I don't feel comfortable about adding EFIAPI to XenStoreVSPrint
>>>> just because it takes a VA_LIST parameter -- note: it is *not* a
>>>> varargs function itself! --; the same issue might hit elsewhere in
>>>> the edk2 tree at any time, outside of OvmfPkg too.
>>>
>>> From the different tests I've done, I feel more like VA_COPY might be
>>> the issue, but I don't know how __builtin_ms_va_* are supposed to be
>>> used.
>>
>> If I recall correctly, from the upstream GCC bug, the problem is that
>> __builtin_va_list does not track internally whether it was created in an
>> msabi or sysvabi function, and therefore the va_* functions cannot be
>> used transparently on it. Instead, when va_list is accessed, the
>> accessor builtins seem to apply the currently executing function's
>> calling convetion to va_list. (Even if the creation context of va_list
>> was different.)
>>
>>>
>>>> Would the gcc white-listing work for you?
>>>>
>>>> Note that the white-listing would practically undo Konrad's commit
>>>> 2667ad40919a ("OvmfPkg/build.sh: Make GCC5 the default toolchain,
>>>> catch GCC43 and earlier", 2016-11-23), but given the recent gcc
>>>> developments (gcc-6.3 has not fixed the gcc bug, and the bug has
>>>> even surfaced in gcc-5.4), I think it would be justified.
>>>
>>> Do be honnest, I don't think the toolchain GCC5 has ever been tested
>>> with gcc-5.x and the module XenBusDxe. I think most people that want to
>>> start OVMF under Xen are likely to build it with gcc-4.9 or already had
>>> gcc-6.x when OVMF switch to the GCC5 toolchain by default.
>>>
>>
>> Okay... I'm equally fine if we just say "given that GCC is broken like
>> this, we hereby require all functions that take a variable argument
>> list, *or* a VA_LIST parameter, to be EFIAPI". (The first part of the
>> requirement already exists.)
>>
>> But in this case, the full edk2 codebase has to be grepped for
>> VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if
>> they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems
>> incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.)
>>
>> Also, in this case, your commit 432f1d83f77a should likely be reverted.
>> (Because we are ultimately giving in to the gcc bug.)
>>
>> Thanks
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-23 10:19 ` Laszlo Ersek
@ 2017-02-23 12:43 ` Anthony PERARD
2017-02-23 13:00 ` Gao, Liming
1 sibling, 0 replies; 29+ messages in thread
From: Anthony PERARD @ 2017-02-23 12:43 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Gao, Liming, Chao Zhang, David Wei, Guo, Mang, Ard Biesheuvel,
Justen, Jordan L, edk2-devel@ml01.01.org
On Thu, Feb 23, 2017 at 11:19:03AM +0100, Laszlo Ersek wrote:
> On 02/22/17 09:54, Gao, Liming wrote:
> > Laszlo:
> > In edk2, I find the several functions with VA_LIST have no EFIAPI.
> > They may use VA_ARG() or call other functions, but they don't use
> > VA_COPY(). In Base.h, VA_ARG() is defined as __builtin_va_arg(),
> > which is same to native one.
>
> You are right; apparently __builtin_va_arg() works with __builtin_ms_va_list and __builtin_va_list alike.
>
> However, as you say,
>
> > VA_COPY() is defined as
> > __builtin_ms_va_copy(). So, I also think this is MS ABI request. That
> > means only if the function implementation uses VA_START(),VA_END() or
> > VA_COPY(), it must be declared with EFIAPI.
>
> - __builtin_va_start / __builtin_ms_va_start,
> - __builtin_va_end / __builtin_ms_va_end,
> - __builtin_va_copy / __builtin_ms_va_copy
>
> must be matched to __builtin_va_list vs. __builtin_ms_va_list.
>
> >
> > MdePkg\Library\BasePrintLib\PrintLibInternal.c BasePrintLibSPrintMarker()
> > ShellPkg\Library\UefiShellLib\UefiShellLib.c InternalShellPrintWorker()
>
> Yes, you are right -- when looking for functions that should be made EFIAPI, we shouldn't search for VA_LIST, but VA_START|VA_END|VA_COPY.
>
> Thanks for the correction.
>
> The following command is a good start:
>
> git grep -E -n '\<(VA_START|VA_END|VA_COPY)\>|^[A-Za-z0-9_]' \
> | grep -E -B 3 '\<(VA_START|VA_END|VA_COPY)\>'
>
> I just went over the output (it was gruesome), and -- outside of EdkCompatibilityPkg -- I indeed found only a handful of affected functions:
>
> - XenStoreVSPrint() [OvmfPkg/XenBusDxe/XenStore.c]
> - VariableGetBestLanguage() [SecurityPkg/VariableAuthenticated/EsalVariableDxeSal/Variable.c]
> - SmmBootScriptWrite() [Vlv2TbltDevicePkg/PlatformSmm/SmmScriptSave.c]
>
> Anthony, can you please submit the patch for XenStoreVSPrint()?
Done.
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
2017-02-23 10:19 ` Laszlo Ersek
2017-02-23 12:43 ` Anthony PERARD
@ 2017-02-23 13:00 ` Gao, Liming
1 sibling, 0 replies; 29+ messages in thread
From: Gao, Liming @ 2017-02-23 13:00 UTC (permalink / raw)
To: Laszlo Ersek, Anthony PERARD, Zhang, Chao B, Wei, David,
Guo, Mang
Cc: Justen, Jordan L, edk2-devel@ml01.01.org, Ard Biesheuvel
Laszlo:
- VariableGetBestLanguage() [SecurityPkg/VariableAuthenticated/EsalVariableDxeSal/Variable.c]
[Liming] It is for IPF arch only. We don't support IPF any longer. So, keep it as-is
- SmmBootScriptWrite() [Vlv2TbltDevicePkg/PlatformSmm/SmmScriptSave.c]
[Liming] I am not sure MinnowMax platform supports GCC build or not. If it supports GCC build, I agree to fix it.
Thanks
Liming
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, February 23, 2017 6:19 PM
To: Gao, Liming <liming.gao@intel.com>; Anthony PERARD <anthony.perard@citrix.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Wei, David <david.wei@intel.com>; Guo, Mang <mang.guo@intel.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5
On 02/22/17 09:54, Gao, Liming wrote:
> Laszlo:
> In edk2, I find the several functions with VA_LIST have no EFIAPI.
> They may use VA_ARG() or call other functions, but they don't use
> VA_COPY(). In Base.h, VA_ARG() is defined as __builtin_va_arg(),
> which is same to native one.
You are right; apparently __builtin_va_arg() works with __builtin_ms_va_list and __builtin_va_list alike.
However, as you say,
> VA_COPY() is defined as
> __builtin_ms_va_copy(). So, I also think this is MS ABI request. That
> means only if the function implementation uses VA_START(),VA_END() or
> VA_COPY(), it must be declared with EFIAPI.
- __builtin_va_start / __builtin_ms_va_start,
- __builtin_va_end / __builtin_ms_va_end,
- __builtin_va_copy / __builtin_ms_va_copy
must be matched to __builtin_va_list vs. __builtin_ms_va_list.
>
> MdePkg\Library\BasePrintLib\PrintLibInternal.c BasePrintLibSPrintMarker()
> ShellPkg\Library\UefiShellLib\UefiShellLib.c InternalShellPrintWorker()
Yes, you are right -- when looking for functions that should be made EFIAPI, we shouldn't search for VA_LIST, but VA_START|VA_END|VA_COPY.
Thanks for the correction.
The following command is a good start:
git grep -E -n '\<(VA_START|VA_END|VA_COPY)\>|^[A-Za-z0-9_]' \
| grep -E -B 3 '\<(VA_START|VA_END|VA_COPY)\>'
I just went over the output (it was gruesome), and -- outside of EdkCompatibilityPkg -- I indeed found only a handful of affected functions:
- XenStoreVSPrint() [OvmfPkg/XenBusDxe/XenStore.c]
- VariableGetBestLanguage() [SecurityPkg/VariableAuthenticated/EsalVariableDxeSal/Variable.c]
- SmmBootScriptWrite() [Vlv2TbltDevicePkg/PlatformSmm/SmmScriptSave.c]
Anthony, can you please submit the patch for XenStoreVSPrint()?
Chao Zhang, can you please submit a patch for VariableGetBestLanguage()?
David Wei or Mang Guo, can one of you guys please submit a patch for SmmBootScriptWrite()?
Thanks,
Laszlo
>
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, February 22, 2017 3:03 AM
>> To: Anthony PERARD <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; edk2-devel@ml01.01.org; Gao, Liming
>> <liming.gao@intel.com>
>> Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when
>> compiled with GCC5
>>
>> On 02/21/17 18:53, Anthony PERARD wrote:
>>> On Tue, Feb 21, 2017 at 06:07:15PM +0100, Laszlo Ersek wrote:
>>>> CC Rebecca & Konrad
>>>>
>>>> On 02/21/17 17:39, Anthony PERARD wrote:
>>
>> [snip]
>>
>>>>> So, should I add EFIAPI to XenStoreVSPrint, as it is using VA_COPY?
>>>>>
>>>>
>>>> Hm, please help me jog my memory...
>>>>
>>>> If I remember correctly, this is still a GCC bug, one that we suppressed for
>> gcc-6.2 with your patch as follows:
>>>
>>> Yes.
>>>
>>>>> commit 432f1d83f77acf92d52ef18d2cee6dbf7c5b9b86
>>>>> Author: Anthony PERARD <anthony.perard@citrix.com>
>>>>> Date: Tue Dec 6 12:03:25 2016 +0000
>>>>>
>>>>> OvmfPkg/build.sh: Use GCC49 toolchains with GCC 6.[0-2]
>>>>>
>>>>> The goal of the patch is to avoid using -flto with GCC 6.0 to 6.2.
>>>>>
>>>>> This is to workaround a GCC bug:
>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>>> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
>>>>>
>>>>> diff --git a/OvmfPkg/build.sh b/OvmfPkg/build.sh
>>>>> index 95fe8fb07647..b6e936056ca0 100755
>>>>> --- a/OvmfPkg/build.sh
>>>>> +++ b/OvmfPkg/build.sh
>>>>> @@ -102,7 +102,7 @@ case `uname` in
>>>>> 4.8.*)
>>>>> TARGET_TOOLS=GCC48
>>>>> ;;
>>>>> - 4.9.*)
>>>>> + 4.9.*|6.[0-2].*)
>>>>> TARGET_TOOLS=GCC49
>>>>> ;;
>>>>> *)
>>>>
>>>> Do I understand correctly that the gcc bug has not been fixed in
>>>> gcc-6.3, and -- because we don't suppress it for gcc-6.3 as the
>>>> above expression does not match -- it causes problems again?
>>>
>>> The bug describe in the GCC bugzilla is probably fix, but the
>>> test-case does not make use of __builtin_ms_va_copy.
>>
>> :/
>>
>>>
>>>> You also mention gcc-5.4 as problematic. I think we haven't
>>>> received such reports about gcc-5 versions up to and including
>>>> gcc-5.3 (that's why GCC5 is the default selection in
>>>> "OvmfPkg/build.sh"). Do you mean that the gcc bug has now been
>>>> "backported" from the gcc-6 series to the gcc-5 series (starting
>>>> with gcc-5.4)?
>>
>>>
>>> I don't know the state of gcc-5.0 to gcc-5.3, I have never tested -flto
>>> with gcc-5.x (until now), I would say they are also problematic until
>>> proven otherwise.
>>
>> When we enabled GCC5, it definitely worked for at least one gcc release,
>> with -flto. (-flto is the default for DEBUG and RELEASE builds with
>> GCC5; NOOPT disables -Os and -flto.)
>>
>>>
>>>> If that's the case, then I suggest flipping "OvmfPkg/build.sh" from
>>>> black-listing gcc versions for -flto to white-listing. In other
>>>> words, assume that -flto is generally broken with GCC, except for a
>>>> few known versions: 5.0 through 5.3 inclusive. Those versions
>>>> should trigger the use of the GCC5 toolchain, and everything else
>>>> (5.4+, 6.*, 4.9.*) should use GCC49.
>>>>
>>>> I don't feel comfortable about adding EFIAPI to XenStoreVSPrint
>>>> just because it takes a VA_LIST parameter -- note: it is *not* a
>>>> varargs function itself! --; the same issue might hit elsewhere in
>>>> the edk2 tree at any time, outside of OvmfPkg too.
>>>
>>> From the different tests I've done, I feel more like VA_COPY might be
>>> the issue, but I don't know how __builtin_ms_va_* are supposed to be
>>> used.
>>
>> If I recall correctly, from the upstream GCC bug, the problem is that
>> __builtin_va_list does not track internally whether it was created in an
>> msabi or sysvabi function, and therefore the va_* functions cannot be
>> used transparently on it. Instead, when va_list is accessed, the
>> accessor builtins seem to apply the currently executing function's
>> calling convetion to va_list. (Even if the creation context of va_list
>> was different.)
>>
>>>
>>>> Would the gcc white-listing work for you?
>>>>
>>>> Note that the white-listing would practically undo Konrad's commit
>>>> 2667ad40919a ("OvmfPkg/build.sh: Make GCC5 the default toolchain,
>>>> catch GCC43 and earlier", 2016-11-23), but given the recent gcc
>>>> developments (gcc-6.3 has not fixed the gcc bug, and the bug has
>>>> even surfaced in gcc-5.4), I think it would be justified.
>>>
>>> Do be honnest, I don't think the toolchain GCC5 has ever been tested
>>> with gcc-5.x and the module XenBusDxe. I think most people that want to
>>> start OVMF under Xen are likely to build it with gcc-4.9 or already had
>>> gcc-6.x when OVMF switch to the GCC5 toolchain by default.
>>>
>>
>> Okay... I'm equally fine if we just say "given that GCC is broken like
>> this, we hereby require all functions that take a variable argument
>> list, *or* a VA_LIST parameter, to be EFIAPI". (The first part of the
>> requirement already exists.)
>>
>> But in this case, the full edk2 codebase has to be grepped for
>> VA_LIST-taking functions, and all of them must be flipped to EFIAPI, if
>> they currently aren't EFIAPI. Covering just XenStoreVSPrint() seems
>> incomplete. (Note: CryptoPkg/Library/OpensslLib is an exception.)
>>
>> Also, in this case, your commit 432f1d83f77a should likely be reverted.
>> (Because we are ultimately giving in to the gcc bug.)
>>
>> Thanks
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2017-02-23 13:00 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-01 15:28 [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Anthony PERARD
2016-12-01 15:28 ` [PATCH 1/4] OvmfPkg/XenHypercallLib: Add EFIAPI Anthony PERARD
2016-12-01 15:28 ` [PATCH 2/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify Anthony PERARD
2016-12-01 15:28 ` [PATCH 3/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenStore functions Anthony PERARD
2016-12-01 15:28 ` [PATCH 4/4] OvmfPkg/XenBusDxe: Add EFIAPI to XenGrantTable{Grant, End}Access Anthony PERARD
2016-12-01 18:43 ` [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5 Laszlo Ersek
2016-12-01 20:06 ` Jordan Justen
2016-12-01 20:54 ` Laszlo Ersek
2016-12-02 0:58 ` Jordan Justen
2016-12-02 9:45 ` Laszlo Ersek
2016-12-02 4:36 ` Gao, Liming
2016-12-02 10:00 ` Laszlo Ersek
2016-12-02 16:02 ` Anthony PERARD
2016-12-02 19:26 ` Laszlo Ersek
2016-12-03 17:59 ` Laszlo Ersek
2016-12-05 2:55 ` Gao, Liming
2016-12-05 10:09 ` Laszlo Ersek
2017-02-21 16:39 ` Anthony PERARD
2017-02-21 17:07 ` Laszlo Ersek
2017-02-21 17:53 ` Anthony PERARD
2017-02-21 19:02 ` Laszlo Ersek
2017-02-21 19:08 ` Rebecca Cran
2017-02-21 22:45 ` Jordan Justen
2017-02-21 23:59 ` Laszlo Ersek
2017-02-22 14:16 ` Gao, Liming
2017-02-22 8:54 ` Gao, Liming
2017-02-23 10:19 ` Laszlo Ersek
2017-02-23 12:43 ` Anthony PERARD
2017-02-23 13:00 ` Gao, Liming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox