Re: [PATCH] MdePkg/BaseIoLibIntrinsic: make BaseIoLibIntrinsic safe for ArmVirt/KVM

[Laszlo Ersek <lersek@redhat.com>]

On 06/07/18 12:46, 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
> 
> The IoLib implementation provided by MdePkg/Library/BaseIoLibIntrinsic
> is based on C code, and when LTO is in effect, the MMIO accesses could
> be merged with, e.g., manipulations of the loop counter, producing
> opcodes that KVM does not support for emulated MMIO.
> 
> So let's add a special ArmVirt flavor of this library that implements
> that actual load/store operations in assembler, ensuring that the
> instructions involved can be emulated by KVM.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc                                      |   2 +-
>  MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S         | 164 +++++
>  MdePkg/Library/BaseIoLibIntrinsic/Arm/ArmVirtMmio.S             | 161 +++++
>  MdePkg/Library/BaseIoLibIntrinsic/Arm/ArmVirtMmio.asm           | 165 +++++
>  MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicArmVirt.inf |  50 ++
>  MdePkg/Library/BaseIoLibIntrinsic/IoLibArmVirt.c                | 641 ++++++++++++++++++++
>  6 files changed, 1182 insertions(+), 1 deletion(-)

Assuming Liming is OK with this patch for MdePkg, I have some comments
(specific to adding this package to MdePkg):

(1) ArmVirtPkg and MdePkg should likely not be modified in the same patch.

(2) The library instance should be added as a standalone component to
MdePkg.dsc (so it can be built without any client modules):

[Components.ARM, Components.AARCH64]
  MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
  MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicArmVirt.inf <-HERE

(3) Library instances under MdePkg are supposed to have
MODULE_UNI_FILEs. I think customizing "BaseIoLibIntrinsic.uni" shouldn't
be hard; it's basically a different format for re-stating the blurb of
the new INF file.


(Obviously, don't do anything about these until Liming follows up.)

The rest looks OK to me!

Thank you for this!
Laszlo