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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 5 June 2018 at 15:04, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

No, that is an oversight. The whole pointed of wrapping the asm
functions in C functions was so that the ASSERT()s could be preserved.

>
> (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?...)
>

This is about ordering vs. side effects. DMB manages the order in
which memory accesses are issued, DSB ensures that they have actually
completed.

>
> (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?
>

I'll send out another patch.