[PATCH v6 6/8] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs

[Yury Norov]

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 at linaro.org>
> >>
> >> 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 at arm.com>
> >> Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>
> >> ---
> >>  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