From patchwork Mon Mar 20 18:55:11 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 9635205 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 413CA602D6 for ; Mon, 20 Mar 2017 18:55:22 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 352FD26224 for ; Mon, 20 Mar 2017 18:55:22 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 29EC626530; Mon, 20 Mar 2017 18:55:22 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 01EFC26224 for ; Mon, 20 Mar 2017 18:55:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932068AbdCTSzT (ORCPT ); Mon, 20 Mar 2017 14:55:19 -0400 Received: from mail-wr0-f181.google.com ([209.85.128.181]:34321 "EHLO mail-wr0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755882AbdCTSzR (ORCPT ); Mon, 20 Mar 2017 14:55:17 -0400 Received: by mail-wr0-f181.google.com with SMTP id l37so98669878wrc.1 for ; Mon, 20 Mar 2017 11:55:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=dV6DeEH+F/c4jfWjy6wScqKTkj4qNnY4iLDYLVzafyU=; b=ABXK4oDFeeuroPqWSrKy3opTq8uIyVNoA62aDhm12g0Y/KqMw8+oDaOCd6kPfvoo2o Y9yPb4PQnnLsj4NlTIscnxWzN/Lh7I/ysZPs+7E2jGWQ+qCAwqjBRddHv2Uy6+3YQCRw rr+5N0w3+T9UcJH3Yth17/CU9N1b7wS+PIms4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=dV6DeEH+F/c4jfWjy6wScqKTkj4qNnY4iLDYLVzafyU=; b=hNyANbiYVT+y3XSfQ5KTrVNN9JgFp+Zd7Spig2VmJFmfQkbifFbrSs14VzDsG9sMf0 565Bo+Yi9dAOmYYPy9FHPgxIo5WZEGVR5Kv48HmQctIjFtY6D0XBX2k/AJyVgrhm3fPy tR4fGkz7bWD23aBMXTfMAGrfZ7LlvvmxioN06X4HGoQBqBlBqJ2xJX+ARUFqSpJ0Zee9 AdW/gvS+WuTUBPuygirPp3spMdefr/m9/55jdk5VHUO5QspAzsjHwmXgSxbcPmbDC4cO nLWnUbaRybYUz5Pq8rqqJMXI/wWXeoNBHsuTQlyg1Jvg1mYTTzVBo8H8wWHUZ/ui7T5/ jJgg== X-Gm-Message-State: AFeK/H0u3rVX3JOQjy05JZKQSbRHqV6WRhsnaL1oEGdLviHwlgkg64nQqG/cVGdRAlgPSYIg X-Received: by 10.223.147.164 with SMTP id 33mr29340478wrp.42.1490036112868; Mon, 20 Mar 2017 11:55:12 -0700 (PDT) Received: from localhost (xd93ddc2d.cust.hiper.dk. [217.61.220.45]) by smtp.gmail.com with ESMTPSA id c35sm21800496wra.1.2017.03.20.11.55.11 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 20 Mar 2017 11:55:11 -0700 (PDT) Date: Mon, 20 Mar 2017 19:55:11 +0100 From: Christoffer Dall To: Marc Zyngier Cc: Christoffer Dall , Peter Maydell , Eric Auger , Andre Przywara , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org Subject: Re: [PATCH 1/2] KVM: arm/arm64: vgic-v2: Expose the correct GICC_PMR values to userspace Message-ID: <20170320185511.GB32242@cbox> References: <20170316114535.25233-1-marc.zyngier@arm.com> <20170316114535.25233-2-marc.zyngier@arm.com> <20170320142425.GB20711@cbox> <3fb0ff07-7191-b814-b7d6-5165de97a805@arm.com> <20170320183129.GA32242@cbox> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170320183129.GA32242@cbox> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Mar 20, 2017 at 07:31:29PM +0100, Christoffer Dall wrote: > On Mon, Mar 20, 2017 at 03:12:05PM +0000, Marc Zyngier wrote: > > On 20/03/17 14:24, Christoffer Dall wrote: > > > On Thu, Mar 16, 2017 at 11:45:34AM +0000, Marc Zyngier wrote: > > >> We allow userspace to save/restore the GICC_PMR values in order > > >> to allow migration. This value is extracted from GICH_PMCR, where > > >> it occupies a 5 bit field. But the canonical PMR is an 8 bit > > >> value and we fail to shift the virtual priority, resulting in > > >> a non-sensical value being reported to userspace. > > >> > > >> Fixing it once and for all would be ideal, but that would break > > >> migration of guest from old to new kernels. We thus introduce > > >> a new GICv2 attribute (KVM_DEV_ARM_VGIC_CTRL_CANONICAL_PMR) > > >> that allows userspace to register its interest for the one true > > >> representation of PMR. > > > > > > Thinking about this some more, I think we should just leave the ABI as > > > is without introducing the flag, because we do not loose any information > > > by doing so, and we can completely leave it up to userspace to work > > > around our funny ABI. > > > > Right. That's the other option. Do we have any use case where we'd like > > to expose the real thing to userspace? > > My stand here is that we *are* exposing the real thing - we just decided > to use a funny format. If anything relied on the format being exported > as reading the GICC_PMR directly, then their code would be already > broken, and I don't think we care about supporting an already-broken > non-functional userspace. The ABI is already what it is - not > beautiful - but functionally just fine. > > > > This would impact migration from > > KVM to "something else", but I'm not sure we ever want to consider this > > seriously. > > > > I don't think it really impacts anything. For example, KVM to TCG will > still work, it just requires userspace to do the wrangling of shifting > the PMR 3 bits left and right, but it knows all about the versions it's > dealing with etc. so that can be solved in userspace as well. > > And also, you're right, nobody is doing anything like this in userspace > in the moment, so let's just clarify our bad ABI and declare success ;) > > > > > > In the end, considering my comments on the next patch, the result would > > > be amusing, and look something like this patch instead: > > > > > > > > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt > > > index 76e61c8..b2f60ca 100644 > > > --- a/Documentation/virtual/kvm/devices/arm-vgic.txt > > > +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt > > > @@ -83,6 +83,12 @@ Groups: > > > > > > Bits for undefined preemption levels are RAZ/WI. > > > > > > + For historical reasons and to provide ABI compatibility with userspace we > > > + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask > > > + field in the lower 5 bits of a word, meaning that userspace must always > > > + use the lower 5 bits to communicate with the KVM device and must shift the > > > + value left by 3 places to obtain the actual priority mask level. > > > + > > > Limitations: > > > - Priorities are not implemented, and registers are RAZ/WI > > > - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. > > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > index a3ad7ff..7b7ecac 100644 > > > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > > > @@ -229,7 +229,14 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > > > val = vmcr.ctlr; > > > break; > > > case GIC_CPU_PRIMASK: > > > - val = vmcr.pmr; > > > + /* > > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > > + * GICC_PMR.Priority, so we expose the upper five bits of > > > + * priority mask to userspace using the lower bits in the > > > + * unsigned long. > > > + */ > > > + val = vmcr.pmr >> 3; > > > break; > > > case GIC_CPU_BINPOINT: > > > val = vmcr.bpr; > > > @@ -262,7 +269,14 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > > > vmcr.ctlr = val; > > > break; > > > case GIC_CPU_PRIMASK: > > > - vmcr.pmr = val; > > > + /* > > > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the > > > + * the PMR field as GICH_VMCR.VMPriMask rather than > > > + * GICC_PMR.Priority, so we expose the upper five bits of > > > + * priority mask to userspace using the lower bits in the > > > + * unsigned long. > > > + */ > > > + vmcr.pmr = val << 3; > > > > By just looking at the code, I understand that you have struct vmcr > > carrying PMR using its architectural representation? That's cunning indeed. > > > > Yeah, so that's the idea. My thought is that we either (a) don't use > the intermediate struct vmcr representation for PMR at all, or (b) > clearly define why we need to intermediate data structure and which > format it should be in (the architectural one). > > If there's a better case for (a), we can do that too, but I found this > one easily explainable with the comments I suggested. > Just for reference, this is what the other solution looks like (untested, of course). I prefer my first version, but I don't feel strongly about it: Thanks, -Christoffer diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index 76e61c8..b2f60ca 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -83,6 +83,12 @@ Groups: Bits for undefined preemption levels are RAZ/WI. + For historical reasons and to provide ABI compatibility with userspace we + export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask + field in the lower 5 bits of a word, meaning that userspace must always + use the lower 5 bits to communicate with the KVM device and must shift the + value left by 3 places to obtain the actual priority mask level. + Limitations: - Priorities are not implemented, and registers are RAZ/WI - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2. diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c index 79f37e3..5befc53 100644 --- a/arch/arm64/kvm/vgic-sys-reg-v3.c +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c @@ -95,14 +95,18 @@ static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - struct vgic_vmcr vmcr; + struct vgic_v3_cpu_if *vgic_v3 = &vcpu->arch.vgic_cpu.vgic_v3; + u32 pmr; - vgic_get_vmcr(vcpu, &vmcr); if (p->is_write) { - vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT; - vgic_set_vmcr(vcpu, &vmcr); + pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT; + vgic_v3->vgic_vmcr &= ~ICH_VMCR_PMR_MASK; + vgic_v3->vgic_vmcr |= (pmr << ICH_VMCR_PMR_SHIFT) + & ICH_VMCR_PMR_MASK; } else { - p->regval = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK; + pmr = (vgic_v3->vgic_vmcr & ICH_VMCR_PMR_MASK) >> + ICH_VMCR_PMR_SHIFT; + p->regval = (pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK; } return true; diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c index a3ad7ff..41c413b 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c @@ -219,6 +219,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len) { + struct vgic_v2_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v2; struct vgic_vmcr vmcr; u32 val; @@ -229,7 +230,15 @@ static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, val = vmcr.ctlr; break; case GIC_CPU_PRIMASK: - val = vmcr.pmr; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * 32-bit word provided by userspace. + */ + val = (cpuif->vgic_vmcr & GICH_VMCR_PRIMASK_MASK) >> + GICH_VMCR_PRIMASK_SHIFT; break; case GIC_CPU_BINPOINT: val = vmcr.bpr; @@ -253,6 +262,7 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len, unsigned long val) { + struct vgic_v2_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v2; struct vgic_vmcr vmcr; vgic_get_vmcr(vcpu, &vmcr); @@ -262,7 +272,16 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, vmcr.ctlr = val; break; case GIC_CPU_PRIMASK: - vmcr.pmr = val; + /* + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the + * the PMR field as GICH_VMCR.VMPriMask rather than + * GICC_PMR.Priority, so we expose the upper five bits of + * priority mask to userspace using the lower bits in the + * 32-bit word provided by userspace. + */ + cpuif->vgic_vmcr &= ~GICH_VMCR_PRIMASK_MASK; + cpuif->vgic_vmcr |= (val << GICH_VMCR_PRIMASK_SHIFT) & + GICH_VMCR_PRIMASK_MASK; break; case GIC_CPU_BINPOINT: vmcr.bpr = val; diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index b834ecd..cc85bbf 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -191,8 +191,6 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_MASK; vmcr |= (vmcrp->bpr << GICH_VMCR_BINPOINT_SHIFT) & GICH_VMCR_BINPOINT_MASK; - vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) & - GICH_VMCR_PRIMASK_MASK; vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr; } @@ -207,8 +205,6 @@ void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) GICH_VMCR_ALIAS_BINPOINT_SHIFT; vmcrp->bpr = (vmcr & GICH_VMCR_BINPOINT_MASK) >> GICH_VMCR_BINPOINT_SHIFT; - vmcrp->pmr = (vmcr & GICH_VMCR_PRIMASK_MASK) >> - GICH_VMCR_PRIMASK_SHIFT; } void vgic_v2_enable(struct kvm_vcpu *vcpu) diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index edc6ee2..bcc5434 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -184,7 +184,6 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcr |= (vmcrp->ctlr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK; vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK; vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK; - vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK; vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK; vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK; @@ -204,7 +203,6 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp) vmcrp->ctlr |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT; vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT; vmcrp->bpr = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT; - vmcrp->pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT; vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT; vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT; } diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index db28f7c..9886be3 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -85,7 +85,6 @@ struct vgic_vmcr { u32 ctlr; u32 abpr; u32 bpr; - u32 pmr; /* Below member variable are valid only for GICv3 */ u32 grpen0; u32 grpen1;