From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::243; helo=mail-it0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DECD4210F4747 for ; Thu, 7 Jun 2018 03:44:03 -0700 (PDT) Received: by mail-it0-x243.google.com with SMTP id u4-v6so12222846itg.0 for ; Thu, 07 Jun 2018 03:44:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=LzBId3R/GaYzCzAnxKVtmlwGRkB9by+vPG1YaTaugT4=; b=MPvZSPukISotr861D4M07nnVVWiHwyHcu7K71GFzAeQp3aXnSXnHkI/l+TNSX/2kZH Mp0td6R8ZhURki5dp5hAs+Izj7zaKf0ZNoFpGGLhVHZtXiuqaZY09B1H23ShwwVmu8Yc bKoM4u64N7CdtIlj/FxKGtJSHPrku5Nh97DvI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=LzBId3R/GaYzCzAnxKVtmlwGRkB9by+vPG1YaTaugT4=; b=pmcqQ1QS6nJS742ifNBPH8TVYHQmKh/TZsm6FhSh819QVGFqMxn6HFrTEK8rmSK2wb HH3upN2BufwOKK0lKnY+c1s8yA7cvR1H4bNGNGhmLUq12WUvibvK9HMpiqsxmzeEYoiP 0SocN5D2sVbsk+AkLbpqSMsxQkCADZnjd9ucUBicDWbp4V7TWkb/xEb08dbF/xpbIIac FfZYm5EIM55EP3gOfO/ndO1NccQyafLNrLyfcRcN1oz5xTjrCyvDJKZZInx/upbuDhfD GX6g6PhENgk1Qbq/FxprIMCG2nyXyOkP2CltVg2k/IioCnIoL28QOPZF4Ot5uLfTiwZx ElQQ== X-Gm-Message-State: APt69E3oLfUh5LAml/3UqkOdsvTfGb6kybM6HIz9aR+tu2FfJgO0hXvD YBLzoZGvMKF8+VcvOH0xyIQbjwm4K/CE7He3KwQ/3Uv7 X-Google-Smtp-Source: ADUXVKIJLpLwnu3jRBzv/OyPyB9dpnVtMIkUbKiS3WPV9J6q1Tvl0t1wXt6y5S+au3uwuizq1ywZz7L0U/mw+z0wjdM= X-Received: by 2002:a24:534e:: with SMTP id n75-v6mr1350863itb.138.1528368243144; Thu, 07 Jun 2018 03:44:03 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 7 Jun 2018 03:44:02 -0700 (PDT) In-Reply-To: References: <20180605110543.17663-1-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Thu, 7 Jun 2018 12:44:02 +0200 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm 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: Thu, 07 Jun 2018 10:44:04 -0000 Content-Type: text/plain; charset="UTF-8" On 5 June 2018 at 15:04, Laszlo Ersek 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 >> --- >> 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.