Re: [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

[Anthony PERARD <anthony.perard@citrix.com>]

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