From patchwork Wed May 18 13:01:39 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 68046 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp2622286qge; Wed, 18 May 2016 06:02:39 -0700 (PDT) X-Received: by 10.67.15.9 with SMTP id fk9mr10694752pad.77.1463576559249; Wed, 18 May 2016 06:02:39 -0700 (PDT) Return-Path: Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id bk5si12086108pac.52.2016.05.18.06.02.39 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 May 2016 06:02:39 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) client-ip=2001:1868:205::9; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) smtp.mailfrom=linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1b316E-0000XB-2w; Wed, 18 May 2016 13:01:18 +0000 Received: from mail-wm0-x236.google.com ([2a00:1450:400c:c09::236]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1b316A-0000Nc-Fd for linux-arm-kernel@lists.infradead.org; Wed, 18 May 2016 13:01:15 +0000 Received: by mail-wm0-x236.google.com with SMTP id n129so183833120wmn.1 for ; Wed, 18 May 2016 06:00:54 -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=ZG2bh6pYl5hMqkY4RErFH4d4VxseAoq5DbAN7dQIpo0=; b=P32A1dma6XD6KbKBBoKCq42D85WGeBXOVmaxJJuO0DUhWG1T/bwfnF7ZHl7JatnDXd c75evGpH7tZNWMkRVuiaFxMrR9IGCJU68pOqTsUUTnFn1K+Xaf5Lu2O810Q2YG0Wh0eV 49qJ3GEZdf/ud3z3mPpOPDC+9K1Na3t4dtvkM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=ZG2bh6pYl5hMqkY4RErFH4d4VxseAoq5DbAN7dQIpo0=; b=ZfCn8fzQtb/D96GMZ1kbjePHwN4hLRgI9NnqqB+xr+160P5ClWBlui3adIodwbwsxc AIU5+KuwKJihqXDKfejgPPI75ETS6P4fXVRgLGV2eJy8wksp9Yw5OgC7Kdi7UikFM5Ej idRagSoHjIUHSUJ1q7wij7EnYDpIaoJhKwUPf/jBxJhPK30Ih+jiOhFxqj4IzqqbKoSx B0yXTxYJrNZ/9ypRVHslFO2YL0/xn+5n7fDaL+lv5nEBdB6iH0J8dMtq0jme/Z6Yr+Yj BtHeFL5vyYFGFjadtF9NHvdQcR1M8mysQGxIH4AaarSzWVTwzodX8SjuH7YR2aV+HxSA cSAQ== X-Gm-Message-State: AOPr4FUPfxaYy4ihjEfA9AxZgk60ZAOot3ihvoYU1v/O4PMXBe1jSm1NQzSWMI7RCireTrZ2 X-Received: by 10.28.56.132 with SMTP id f126mr4480862wma.96.1463576452855; Wed, 18 May 2016 06:00:52 -0700 (PDT) Received: from localhost ([94.18.191.146]) by smtp.gmail.com with ESMTPSA id e8sm8581033wjm.23.2016.05.18.06.00.51 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 18 May 2016 06:00:52 -0700 (PDT) Date: Wed, 18 May 2016 15:01:39 +0200 From: Christoffer Dall To: Andre Przywara Subject: Re: [PATCH v4 27/56] KVM: arm/arm64: vgic-new: Add ACTIVE registers handlers Message-ID: <20160518130139.GG3827@cbox> References: <1463392481-26583-1-git-send-email-andre.przywara@arm.com> <1463392481-26583-28-git-send-email-andre.przywara@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1463392481-26583-28-git-send-email-andre.przywara@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160518_060114_814728_C2D8EC5F X-CRM114-Status: GOOD ( 30.85 ) X-Spam-Score: -2.7 (--) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-2.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2a00:1450:400c:c09:0:0:0:236 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Marc Zyngier , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, Eric Auger Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org On Mon, May 16, 2016 at 10:53:15AM +0100, Andre Przywara wrote: > The active register handlers are shared between the v2 and v3 > emulation, so their implementation goes into vgic-mmio.c, to be > easily referenced from the v3 emulation as well later. > Since activation/deactivation of an interrupt may happen entirely > in the guest without it ever exiting, we need some extra logic to > properly track the active state. -- cut -- > Putting it on an ap_list on activation is similar to the normal case > handled by vgic_queue_irq_unlock(), but differs in some details that > make a separate implementation worthwhile. -- cut -- this bit is no longer correct. > > Signed-off-by: Andre Przywara > --- > Changelog RFC..v1: > - handling queueing in write handler > - remove IRQ lock from read handler > > Changelog v1 .. v2: > - adapt to new MMIO framework > > Changelog v3 .. v4: > - specify accessor width > - use IRQ number accessor macro > - drop elaborate write_sactive handler and use new vgic_queue_irq_unlock() > - properly emulate clear active by halting the guest (Christoffer) > - replace extract_bytes() with simple return > > virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +- > virt/kvm/arm/vgic/vgic-mmio.c | 82 ++++++++++++++++++++++++++++++++++++++++ > virt/kvm/arm/vgic/vgic-mmio.h | 10 +++++ > 3 files changed, 94 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > index c13a708..12e101b 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > @@ -84,10 +84,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_pending, vgic_mmio_write_cpending, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET, > - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, > + vgic_mmio_read_active, vgic_mmio_write_sactive, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR, > - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, > + vgic_mmio_read_active, vgic_mmio_write_cactive, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI, > vgic_mmio_read_raz, vgic_mmio_write_wi, 8, > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > index d8dc8f6..74d140e 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio.c > +++ b/virt/kvm/arm/vgic/vgic-mmio.c > @@ -155,6 +155,88 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu, > } > } > > +unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > + u32 value = 0; > + int i; > + > + /* Loop over all IRQs affected by this read */ > + for (i = 0; i < len * 8; i++) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + if (irq->active) > + value |= (1U << i); > + } > + > + return value; > +} > + > +void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > + int i; > + > + kvm_arm_halt_guest(vcpu->kvm); > + for_each_set_bit(i, &val, len * 8) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + spin_lock(&irq->irq_lock); > + /* > + * If this virtual IRQ was written into a list register, we > + * have to make sure the CPU that runs the VCPU thread has > + * synced back LR state to the struct vgic_irq. We can only > + * know this for sure, when either this irq is not assigned to > + * anyone's AP list anymore, or the VCPU thread is not > + * running on any CPUs. > + * > + * In the opposite case, we know the VCPU thread may be on its > + * way back from the guest and still has to sync back this > + * IRQ, so we release and re-acquire the spin_lock to let the > + * other thread sync back the IRQ. > + */ > + while (irq->vcpu && /* IRQ may have state in an LR somewhere */ > + irq->vcpu->cpu != -1) /* VCPU thread is running */ > + cond_resched_lock(&irq->irq_lock); > + > + irq->active = false; > + spin_unlock(&irq->irq_lock); > + } > + kvm_arm_resume_guest(vcpu->kvm); > +} > + > +void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > + int i; > + > + for_each_set_bit(i, &val, len * 8) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + spin_lock(&irq->irq_lock); > + > + /* > + * If the IRQ was already active or it was on a VCPU before > + * or there is no target VCPU assigned at the moment, then > + * just proceed. > + */ > + if (irq->active || irq->vcpu || !irq->target_vcpu) { why is it that we don't care if this IRQ is on a LR, for example being just pending and not active, and we thereby loose this active state when the vcpu syncs back the state? We care for the case where we clear the active state, but not when we set it. Perhaps we discussed this in the past, but now I've forgotten, so that should at least be documented somehow. > + irq->active = true; > + > + spin_unlock(&irq->irq_lock); > + continue; > + } > + > + irq->active = true; > + vgic_queue_irq_unlock(vcpu->kvm, irq); this won't work just yet, but I think all that's needed to make it work is this patchlet: Thanks, -Christoffer _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c index c22f7e2..27204f22 100644 --- a/virt/kvm/arm/vgic/vgic.c +++ b/virt/kvm/arm/vgic/vgic.c @@ -81,7 +81,7 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq) /* If the interrupt is active, it must stay on the current vcpu */ if (irq->active) - return irq->vcpu; + return irq->vcpu ? : irq->target_vcpu; /* * If the IRQ is not active but enabled and pending, we should direct