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: 1973641 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id B1DDFDF2E1 for ; Mon, 14 Jan 2013 21:14:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758796Ab3ANVOb (ORCPT ); Mon, 14 Jan 2013 16:14:31 -0500 Received: from mail-ie0-f182.google.com ([209.85.223.182]:54908 "EHLO mail-ie0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757809Ab3ANVOa (ORCPT ); Mon, 14 Jan 2013 16:14:30 -0500 Received: by mail-ie0-f182.google.com with SMTP id s9so5901322iec.13 for ; Mon, 14 Jan 2013 13:14:30 -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=bdCzkOez2LqMSNyAdt9UmXv+nhW1oTVH8XHmFCJLWDurypUfy6il1HlDb6NCZCnU5p eAivo1ZnPsJTbbpYpXx/7C4g6AvKB3io+8H2LH0/sWyeuq+LTx0CM3UJGUoigctAx4P4 mTaKFKd43nzv8yOqRAtCQeEJCcvni59//KnQ0M9d09k2sP4J9gx2eLoP93ipQQAch+6P ZMkdSMnS8mVLrYwk4pyPKL1ntqJzyBFfGBugbzT9YTPIYwDLWIAjtUEF54DKuxtjjXZX hqS9FLhlaRW1X2OGwPiItWqt0xFlsuhBdI8R38i8Iy3l/m6E6HaEiabsvaLwGxxpCJFN Thvg== 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 Cc: "kvm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "kvmarm@lists.cs.columbia.edu" , Marc Zyngier X-Gm-Message-State: ALoCoQmdZvD/TkPCGiH52dbU1ckd9oh6P8Hd8jS//jeEIZ3STcTWpwoERxST3MDQAl/9dMhabv2E Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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