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

[Jordan Justen <jordan.l.justen@intel.com>]

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