Re: [PATCH] ArmVirtPkg: implement KVM safe IoLib instance

[Laszlo Ersek <lersek@redhat.com>]

On 06/05/18 13:05, 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 instead, let's reimplement IoLib in a KVM safe manner.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Yet another approach for the KVM MMIO emulation issue. Note that this one
> (as well as the MdePkg) affect both AArch64 and ARM. This is deliberate,
> given that there is no reason AArch64 should be immune to this: we simply
> haven't triggered the issue yet.
> 
>  ArmVirtPkg/Library/ArmVirtIoLib/AArch64/ArmVirtMmio.S |  164 ++
>  ArmVirtPkg/Library/ArmVirtIoLib/Arm/ArmVirtMmio.S     |  154 ++
>  ArmVirtPkg/Library/ArmVirtIoLib/Arm/ArmVirtMmio.asm   |  165 ++
>  ArmVirtPkg/Library/ArmVirtIoLib/ArmVirtIoLib.c        |  589 +++++
>  ArmVirtPkg/Library/ArmVirtIoLib/ArmVirtIoLib.h        |  188 ++
>  ArmVirtPkg/Library/ArmVirtIoLib/ArmVirtIoLib.inf      |   49 +
>  ArmVirtPkg/Library/ArmVirtIoLib/IoHighLevel.c         | 2358 ++++++++++++++++++++
>  ArmVirtPkg/Library/ArmVirtIoLib/IoLibMmioBuffer.c     |  413 ++++
>  8 files changed, 4080 insertions(+)

(1) I believe the edk2 nomenclature for library instances would suggest
we call this "BaseIoLibArmVirt" or "BaseIoLibKvm". However, I notice a
great many of our library instances under ArmVirtPkg/Library don't
follow that already, whereas we have several instances starting with
"ArmVirt*". So I value the consistency here; OK.


(2) I've compared some files:

(2a) BaseIoLibIntrinsic.inf <-> ArmVirtIoLib.inf:

- blurb updated,
- copyright notice updated,
- text rewrapped,
- INF_VERSION good,
- BASE_NAME good,
- FILE_GUID unique,
- VALID_ARCHITECTURES good,
- [Sources] etc good

OK.

(2b) IoLibMmioBuffer.c <-> IoLibMmioBuffer.c:

- copyright notice updated,
- text rewrapped,
- some trailing whitespace stripped,
- ArmVirtIoLib.h included rather than BaseIoLibIntrinsicInternal.h,

OK.

(2c) IoHighLevel.c <-> IoHighLevel.c:

- same as (2b),
- plus the reference to "IoLib instances" in the blurb has been updated
  to "MdePkg IoLib instances".

OK.

(2d) BaseIoLibIntrinsicInternal.h <-> ArmVirtIoLib.h:

- kept the common includes (see (2b) and (2c) #includes)
- added function declarations for the assembly functions

OK.

(2e) IoLibArm.c <-> ArmVirtIoLib.c:

This replaces the volatile pointer de-references with calls to the
assembly functions, which seems fine. However, some ASSERT()s about
alignment appear removed (in "read" functions only); is that
intentional? See MmioRead16(), MmioRead32().

Curiously, MmioRead64() preserves the ASSERT().

Otherwise, OK.


(3) New files (the assembly files): Question: why does ARM provide DMB
vs. DSB instructions? Answer: so that ARM experts can have a good
discussion about them every time they show up in new code. :P

(I mean, what chance do mere mortals have to get them right?...)


(4) When we added SEV customizations to IoLib (see
"BaseIoLibIntrinsicSev.inf" / commit b6d11d7c4678), we added those to
MdePkg. Can you please *briefly* investigate whether similar to commit
b6d11d7c4678 would be possible here?

I.e., introduce a new INF file called BaseIoLibIntrinsicArmVirt.inf, and
add "IoLibArmVirt.c" alongside the preexistent "IoLibArm.c". We already
have subdirs for Ia32 and X64 *.nasm, we could create Arm and AArch64
dirs as siblings. This way we could reuse "IoLibMmioBuffer.c" and
"IoHighLevel.c", perhaps?

Anyway I'm not trying to slow down this development by suggesting we add
it to MdePkg. So, if it ends up being too complex for MdePkg, I'm fine
with this patch, Package-wise.


In summary:
- Are the alignment ASSERT() removals in MmioRead16() and MmioRead32()
intentional?
- Can we make one *quick* attempt to put this into MdePkg, if you think
that's feasible?

Thanks!
Laszlo