From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yury Norov Subject: Re: [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs Date: Wed, 6 Dec 2017 01:39:00 +0300 Message-ID: <20171205223900.znmruf3pfpg3pvqa@yury-thinkpad> References: <20171204200506.3224-1-cdall@kernel.org> <20171204200506.3224-7-cdall@kernel.org> <20171205150328.gcwnt5jzmoarvetd@yury-thinkpad> <3cd51419-cd43-998d-f801-407b805eb9a2@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Christoffer Dall , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, Andre Przywara , Eric Auger , Christoffer Dall To: Marc Zyngier Return-path: Received: from mail-bl2nam02on0075.outbound.protection.outlook.com ([104.47.38.75]:27040 "EHLO NAM02-BL2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751847AbdLEWjS (ORCPT ); Tue, 5 Dec 2017 17:39:18 -0500 Content-Disposition: inline In-Reply-To: <3cd51419-cd43-998d-f801-407b805eb9a2@arm.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Dec 05, 2017 at 04:47:46PM +0000, Marc Zyngier wrote: > On 05/12/17 15:03, Yury Norov wrote: > > On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote: > >> From: Christoffer Dall > >> > >> For mapped IRQs (with the HW bit set in the LR) we have to follow some > >> rules of the architecture. One of these rules is that VM must not be > >> allowed to deactivate a virtual interrupt with the HW bit set unless the > >> physical interrupt is also active. > >> > >> This works fine when injecting mapped interrupts, because we leave it up > >> to the injector to either set EOImode==1 or manually set the active > >> state of the physical interrupt. > >> > >> However, the guest can set virtual interrupt to be pending or active by > >> writing to the virtual distributor, which could lead to deactivating a > >> virtual interrupt with the HW bit set without the physical interrupt > >> being active. > >> > >> We could set the physical interrupt to active whenever we are about to > >> enter the VM with a HW interrupt either pending or active, but that > >> would be really slow, especially on GICv2. So we take the long way > >> around and do the hard work when needed, which is expected to be > >> extremely rare. > >> > >> When the VM sets the pending state for a HW interrupt on the virtual > >> distributor we set the active state on the physical distributor, because > >> the virtual interrupt can become active and then the guest can > >> deactivate it. > >> > >> When the VM clears the pending state we also clear it on the physical > >> side, because the injector might otherwise raise the interrupt. We also > >> clear the physical active state when the virtual interrupt is not > >> active, since otherwise a SPEND/CPEND sequence from the guest would > >> prevent signaling of future interrupts. > >> > >> Changing the state of mapped interrupts from userspace is not supported, > >> and it's expected that userspace unmaps devices from VFIO before > >> attempting to set the interrupt state, because the interrupt state is > >> driven by hardware. > >> > >> Reviewed-by: Marc Zyngier > >> Signed-off-by: Christoffer Dall > >> --- > >> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++---- > >> virt/kvm/arm/vgic/vgic.c | 7 +++++ > >> virt/kvm/arm/vgic/vgic.h | 1 + > >> 3 files changed, 73 insertions(+), 6 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > >> index 747b0a3b4784..8d173d99a7a4 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c > >> @@ -16,6 +16,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> #include > >> > >> #include "vgic.h" > >> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void) > >> return vcpu; > >> } > >> > >> +/* Must be called with irq->irq_lock held */ > > > > Instead of enforcing this rule in comment, you can enforce it in code: > > > > BUG_ON(!spin_is_locked(irq->irq_lock)) > > Are we going to litter the kernel with such assertions? I don't think > that's a valuable thing to do. That's what I agree - BUG-ifiyng is somewhat debugging technique and should be avoided in release code. But as I can see, in kvm code BUG()s are widely used: $ find . -name "*.c" | xargs grep -w 'BUG\|BUG_ON' | grep kvm | wc -l 155 So I tuned my littering detector. :) In this patchset new BUG()s are added in patches 4 and 6. In patch 6 BUG() has meaning of TODO: + if (vintid == vcpu_vtimer(vcpu)->irq.irq) + timer = vcpu_vtimer(vcpu); + else + BUG(); /* We only map the vtimer so far */ + Which is far from original purpose of BUG(). If you think that BUG() is not OK in this case (and I agree with it), I think they should be also removed from patches 4 and 6. 6 - for sure. Yury From mboxrd@z Thu Jan 1 00:00:00 1970 From: ynorov@caviumnetworks.com (Yury Norov) Date: Wed, 6 Dec 2017 01:39:00 +0300 Subject: [PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs In-Reply-To: <3cd51419-cd43-998d-f801-407b805eb9a2@arm.com> References: <20171204200506.3224-1-cdall@kernel.org> <20171204200506.3224-7-cdall@kernel.org> <20171205150328.gcwnt5jzmoarvetd@yury-thinkpad> <3cd51419-cd43-998d-f801-407b805eb9a2@arm.com> Message-ID: <20171205223900.znmruf3pfpg3pvqa@yury-thinkpad> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Dec 05, 2017 at 04:47:46PM +0000, Marc Zyngier wrote: > On 05/12/17 15:03, Yury Norov wrote: > > On Mon, Dec 04, 2017 at 09:05:04PM +0100, Christoffer Dall wrote: > >> From: Christoffer Dall > >> > >> For mapped IRQs (with the HW bit set in the LR) we have to follow some > >> rules of the architecture. One of these rules is that VM must not be > >> allowed to deactivate a virtual interrupt with the HW bit set unless the > >> physical interrupt is also active. > >> > >> This works fine when injecting mapped interrupts, because we leave it up > >> to the injector to either set EOImode==1 or manually set the active > >> state of the physical interrupt. > >> > >> However, the guest can set virtual interrupt to be pending or active by > >> writing to the virtual distributor, which could lead to deactivating a > >> virtual interrupt with the HW bit set without the physical interrupt > >> being active. > >> > >> We could set the physical interrupt to active whenever we are about to > >> enter the VM with a HW interrupt either pending or active, but that > >> would be really slow, especially on GICv2. So we take the long way > >> around and do the hard work when needed, which is expected to be > >> extremely rare. > >> > >> When the VM sets the pending state for a HW interrupt on the virtual > >> distributor we set the active state on the physical distributor, because > >> the virtual interrupt can become active and then the guest can > >> deactivate it. > >> > >> When the VM clears the pending state we also clear it on the physical > >> side, because the injector might otherwise raise the interrupt. We also > >> clear the physical active state when the virtual interrupt is not > >> active, since otherwise a SPEND/CPEND sequence from the guest would > >> prevent signaling of future interrupts. > >> > >> Changing the state of mapped interrupts from userspace is not supported, > >> and it's expected that userspace unmaps devices from VFIO before > >> attempting to set the interrupt state, because the interrupt state is > >> driven by hardware. > >> > >> Reviewed-by: Marc Zyngier > >> Signed-off-by: Christoffer Dall > >> --- > >> virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++---- > >> virt/kvm/arm/vgic/vgic.c | 7 +++++ > >> virt/kvm/arm/vgic/vgic.h | 1 + > >> 3 files changed, 73 insertions(+), 6 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > >> index 747b0a3b4784..8d173d99a7a4 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c > >> @@ -16,6 +16,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> #include > >> > >> #include "vgic.h" > >> @@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void) > >> return vcpu; > >> } > >> > >> +/* Must be called with irq->irq_lock held */ > > > > Instead of enforcing this rule in comment, you can enforce it in code: > > > > BUG_ON(!spin_is_locked(irq->irq_lock)) > > Are we going to litter the kernel with such assertions? I don't think > that's a valuable thing to do. That's what I agree - BUG-ifiyng is somewhat debugging technique and should be avoided in release code. But as I can see, in kvm code BUG()s are widely used: $ find . -name "*.c" | xargs grep -w 'BUG\|BUG_ON' | grep kvm | wc -l 155 So I tuned my littering detector. :) In this patchset new BUG()s are added in patches 4 and 6. In patch 6 BUG() has meaning of TODO: + if (vintid == vcpu_vtimer(vcpu)->irq.irq) + timer = vcpu_vtimer(vcpu); + else + BUG(); /* We only map the vtimer so far */ + Which is far from original purpose of BUG(). If you think that BUG() is not OK in this case (and I agree with it), I think they should be also removed from patches 4 and 6. 6 - for sure. Yury