From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6BA212110D51E for ; Tue, 5 Jun 2018 06:04:32 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9F3CF857A4; Tue, 5 Jun 2018 13:04:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-104.rdu2.redhat.com [10.10.120.104]) by smtp.corp.redhat.com (Postfix) with ESMTP id C91142026DEF; Tue, 5 Jun 2018 13:04:30 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: leif.lindholm@linaro.org References: <20180605110543.17663-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Tue, 5 Jun 2018 15:04:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180605110543.17663-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Tue, 05 Jun 2018 13:04:31 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Tue, 05 Jun 2018 13:04:31 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] ArmVirtPkg: implement KVM safe IoLib instance X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Jun 2018 13:04:33 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > --- > 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