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

[Laszlo Ersek <lersek@redhat.com>]

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