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 <christoffer.dall@xxxxxxxxxx> > >> > >> 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 <marc.zyngier@xxxxxxx> > >> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx> > >> --- > >> 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 <linux/kvm.h> > >> #include <linux/kvm_host.h> > >> #include <kvm/iodev.h> > >> +#include <kvm/arm_arch_timer.h> > >> #include <kvm/arm_vgic.h> > >> > >> #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
