Re: [PATCH] ArmVirtPkg/ArmVirtQemu ARM: work around KVM limitations in LTO build

[Laszlo Ersek <lersek@redhat.com>]

On 06/04/18 17:30, Ard Biesheuvel wrote:
> On 4 June 2018 at 17:18, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 06/04/18 16:50, Ard Biesheuvel wrote:
>>> KVM on ARM refuses to decode load/store instructions used to perform
>>> I/O to emulated devices, and instead relies on the exception syndrome
>>> information to describe the operand register, access size, etc.
>>> This is only possible for instructions that have a single input/output
>>> register (as opposed to ones that increment the offset register, or
>>> load/store pair instructions, etc). Otherwise, QEMU crashes with the
>>> following error
>>>
>>>   error: kvm run failed Function not implemented
>>>   R00=01010101 R01=00000008 R02=00000048 R03=08000820
>>>   R04=00000120 R05=7faaa0e0 R06=7faaa0dc R07=7faaa0e8
>>>   R08=7faaa0ec R09=7faaa088 R10=000000ff R11=00000080
>>>   R12=ff000000 R13=7fccfe08 R14=7faa835f R15=7faa887c
>>>   PSR=800001f3 N--- T svc32
>>>   QEMU: Terminated
>>>
>>> and KVM produces a warning such as the following in the kernel log
>>>
>>>   kvm [17646]: load/store instruction decoding not implemented
>>>
>>> GCC with LTO enabled will emit such instructions for Mmio[Read|Write]
>>> invocations performed in a loop, so we need to disable LTO for the
>>> IoLib library to ensure that the emitted instructions are suitable for
>>> emulated I/O under KVM
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  ArmVirtPkg/ArmVirtQemu.dsc | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>> index d74feb709cd1..e6e3d82d6ca9 100644
>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>> @@ -414,3 +414,21 @@ [Components.AARCH64]
>>>      <LibraryClasses>
>>>        NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
>>>    }
>>> +
>>> +[Components.ARM]
>>> +  MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf {
>>> +    <BuildOptions>
>>> +      //
>>> +      // KVM on ARM refuses to decode load/store instructions used to perform
>>> +      // I/O to emulated devices, and instead relies on the exception syndrome
>>> +      // information to describe the operand register, access size, etc.
>>> +      // This is only possible for instructions that have a single input/output
>>> +      // register (as opposed to ones that increment the offset register, or
>>> +      // load/store pair instructions, etc).
>>> +      // GCC with LTO enabled will emit such instructions for Mmio[Read|Write]
>>> +      // invocations performed in a loop, so we need to disable LTO for this
>>> +      // library to ensure that the emitted instructions are suitable for
>>> +      // emulated I/O under KVM
>>> +      //
>>> +      GCC:*_GCC5_ARM_CC_FLAGS = -fno-lto
>>> +  }
>>>
>>
>> Heh :) See <https://bugzilla.redhat.com/show_bug.cgi?id=1576593>.
>>
>> - Is there perhaps a finer-grained GCC option for this? (This is a
>> rhetorical question; I know you must have checked.)
>>
> 
> Unfortunately not. That is why this is a workaround rather than a fix.
> 
>> - Is this only with gcc-8?
>>
> 
> I don't think so. Any GCC/GNU-ld combo that supports LTO is
> susceptible to this afaik. Even worse, I don't think this is limited
> to 32-bit ARM either, even if we've never managed to hit it.
> 
>> - Should we do the same for the ArmVirtXen and ArmVirtQemuKernel
>> platforms? In turn, patch "ArmVirt.dsc.inc" instead? (BTW I have no clue
>> about Xen's emulation of the instructions at hand.)
>>
> 
> This is a fix I would like to apply with moderation, so that we get a
> feeling for which platforms need it and which don't.
> 
> ArmVirtQemuKernel is primarily used in TCG mode, as far as I am aware,
> by the OP-TEE guys for instance, who use secure world emulation.
> 
> Xen doesn't really use I/O emulation on ARM (everything is
> paravirtualized) so I don't think it is affected.

Can you please work some of the above answers (as you see fit) into the
commit message, when you push the patch?

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

>> In general, I'm fine with the patch. According to [1] [2], this appears
>> to be the right syntax for the goal.
>>
> 
> Thanks,
>