On 5 June 2018 at 15:04, Laszlo Ersek <[email protected]> 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 <[email protected]> >> --- >> 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. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
