From patchwork Mon Jan 14 21:08:54 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 1973631 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork1.kernel.org (Postfix) with ESMTP id 6C3AD3FE33 for ; Mon, 14 Jan 2013 21:12:40 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TurH9-0002yN-K6; Mon, 14 Jan 2013 21:08:59 +0000 Received: from mail-ie0-f173.google.com ([209.85.223.173]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TurH6-0002xz-Hz for linux-arm-kernel@lists.infradead.org; Mon, 14 Jan 2013 21:08:57 +0000 Received: by mail-ie0-f173.google.com with SMTP id e13so5848024iej.4 for ; Mon, 14 Jan 2013 13:08:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-received:x-originating-ip:in-reply-to:references :date:message-id:subject:from:to:cc:content-type:x-gm-message-state; bh=6na9Y71G4NhbG+n+FLyjnbofwAdjsWndq//YIvXVVXs=; b=e3hbeCUDK0a7AM3/H3kVnVfGjunoR0mT/gZZipfA02IiIfLHfXi6t+ZNBBwJPdDqvr /al8lrWNaWsSMtWKMTcrx7rqPxk+weV5aPdRSrGLlhQ2rKxr0I/Dqfgg56dG9rZear5O 8oJ/ZC5mMkyu0Ba2ncx/ZNkM2QqNw6Ae4ttDa/rQYp1OBZQ3C55qDe33FIF5YbpICFpR nuPWZsLpesoRAFk/zYMyxydrC5LZOsL1k63G3lRm4lhpzdwKsbwN0ATEGJkBjkgH4V3w 7iir0R8BLEcpImR8oaJKCPjyp7dTCmD6zl9xIplro/0E09xT27Z0mIqCXN2JQVTirUt0 0ukg== MIME-Version: 1.0 X-Received: by 10.50.57.225 with SMTP id l1mr7712741igq.37.1358197734185; Mon, 14 Jan 2013 13:08:54 -0800 (PST) Received: by 10.64.37.70 with HTTP; Mon, 14 Jan 2013 13:08:54 -0800 (PST) X-Originating-IP: [72.80.83.148] In-Reply-To: <20130114153107.GE18935@mudshark.cambridge.arm.com> References: <20130108184116.46558.3558.stgit@ubuntu> <20130108184150.46558.35674.stgit@ubuntu> <20130114153107.GE18935@mudshark.cambridge.arm.com> Date: Mon, 14 Jan 2013 16:08:54 -0500 Message-ID: Subject: Re: [PATCH v5 04/12] ARM: KVM: Initial VGIC infrastructure code From: Christoffer Dall To: Will Deacon X-Gm-Message-State: ALoCoQkWM/SWpKYkYzmS6urM47W3L+juJoplcHtYCG7gCFuI2mgx545qU5KErk1zXDRe48vbXhJQ X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130114_160856_697123_6C9027EC X-CRM114-Status: GOOD ( 26.11 ) X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.223.173 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Marc Zyngier , "linux-arm-kernel@lists.infradead.org" , "kvm@vger.kernel.org" , "kvmarm@lists.cs.columbia.edu" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Mon, Jan 14, 2013 at 10:31 AM, Will Deacon wrote: > On Tue, Jan 08, 2013 at 06:41:51PM +0000, Christoffer Dall wrote: >> From: Marc Zyngier >> >> Wire the basic framework code for VGIC support and the initial in-kernel >> MMIO support code for the VGIC, used for the distributor emulation. > > [...] > >> +/** >> + * vgic_reg_access - access vgic register >> + * @mmio: pointer to the data describing the mmio access >> + * @reg: pointer to the virtual backing of vgic distributor data >> + * @offset: least significant 2 bits used for word offset >> + * @mode: ACCESS_ mode (see defines above) >> + * >> + * Helper to make vgic register access easier using one of the access >> + * modes defined for vgic register access >> + * (read,raz,write-ignored,setbit,clearbit,write) >> + */ >> +static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, >> + phys_addr_t offset, int mode) >> +{ >> + int shift = (offset & 3) * 8; >> + u32 mask; >> + u32 regval; >> + >> + /* >> + * Any alignment fault should have been delivered to the guest >> + * directly (ARM ARM B3.12.7 "Prioritization of aborts"). >> + */ >> + >> + mask = (~0U) >> shift; >> + if (reg) { >> + regval = *reg; >> + } else { >> + BUG_ON(mode != (ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED)); >> + regval = 0; >> + } >> + >> + if (mmio->is_write) { >> + u32 data = (*((u32 *)mmio->data) & mask) << shift; >> + switch (ACCESS_WRITE_MASK(mode)) { >> + case ACCESS_WRITE_IGNORED: >> + return; >> + >> + case ACCESS_WRITE_SETBIT: >> + regval |= data; >> + break; >> + >> + case ACCESS_WRITE_CLEARBIT: >> + regval &= ~data; >> + break; >> + >> + case ACCESS_WRITE_VALUE: >> + regval = (regval & ~(mask << shift)) | data; >> + break; >> + } >> + *reg = regval; >> + } else { >> + switch (ACCESS_READ_MASK(mode)) { >> + case ACCESS_READ_RAZ: >> + regval = 0; >> + /* fall through */ >> + >> + case ACCESS_READ_VALUE: >> + *((u32 *)mmio->data) = (regval >> shift) & mask; >> + } >> + } >> +} > > As I mentioned previously, I suspect that this doesn't work with big-endian > systems. Whilst that's reasonable for the moment, a comment would be useful > for the unlucky soul that decides to do that work in future (or add > accessors for mmio->data as I suggested before). > admittedly this really hurts my brain, but I think there's actually no problem with endianness: whatever comes in mmio->data will have native endianness and the vgic is always little-endian, so a guest would have to make sure to do its own endianness conversion before writing data, or did I get this backwards? (some nasty feeling about if the OS is compiled in another endianness than the hardware everything may break). Anyhow, I think there's another bug in this code though. Please take a look and see if you agree: commit 3cab2b93a6f6acd3c043e584f23b94ab8f1bbd66 Author: Christoffer Dall Date: Mon Jan 14 15:55:18 2013 -0500 KVM: ARM: Limit vgic read/writes to load/store length The vgic read/write operations did not consider ldrb/strb masks, and would therefore unintentionally overwrite parts of a register. Consider for example a store of a single byte to a word-aligned address of one of the priority registers, that would cause the 3 most significant bytes to be overwritten with zeros. Cc: Marc Zyniger Cc: Will Deacon Signed-off-by: Christoffer Dall * @mmio: pointer to the data describing the mmio access @@ -247,8 +257,8 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq) static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, phys_addr_t offset, int mode) { - int shift = (offset & 3) * 8; - u32 mask; + int word_offset = (offset & 3) * 8; + u32 mask = (1UL << (mmio->len * 8)) - 1; u32 regval; /* @@ -256,7 +266,6 @@ static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, * directly (ARM ARM B3.12.7 "Prioritization of aborts"). */ - mask = (~0U) >> shift; if (reg) { regval = *reg; } else { @@ -265,7 +274,7 @@ static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, } if (mmio->is_write) { - u32 data = (*((u32 *)mmio->data) & mask) << shift; + u32 data = mmio_data_read(mmio, mask) << word_offset; switch (ACCESS_WRITE_MASK(mode)) { case ACCESS_WRITE_IGNORED: return; @@ -279,7 +288,7 @@ static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, break; case ACCESS_WRITE_VALUE: - regval = (regval & ~(mask << shift)) | data; + regval = (regval & ~(mask << word_offset)) | data; break; } *reg = regval; @@ -290,7 +299,7 @@ static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, /* fall through */ case ACCESS_READ_VALUE: - *((u32 *)mmio->data) = (regval >> shift) & mask; + mmio_data_write(mmio, mask, regval >> word_offset); } } } @@ -702,6 +711,12 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, (mmio->phys_addr + mmio->len) > (base + KVM_VGIC_V2_DIST_SIZE)) return false; + /* We don't support ldrd / strd or ldm / stm to the emulated vgic */ + if (mmio->len > 4) { + kvm_inject_dabt(vcpu, mmio->phys_addr); + return true; + } + range = find_matching_range(vgic_ranges, mmio, base); if (unlikely(!range || !range->handle_mmio)) { pr_warn("Unhandled access %d %08llx %d\n", Acked-by: Marc Zyngier --- Thanks, -Christoffer diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c index 25daa07..5c1bcf5 100644 --- a/arch/arm/kvm/vgic.c +++ b/arch/arm/kvm/vgic.c @@ -233,6 +233,16 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq) vcpu->arch.vgic_cpu.pending_shared); } +static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask) +{ + return *((u32 *)mmio->data) & mask; +} + +static void mmio_data_write(struct kvm_exit_mmio *mmio, u32 mask, u32 value) +{ + *((u32 *)mmio->data) = value & mask; +} + /** * vgic_reg_access - access vgic register