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

[Christoffer Dall <cdall@kernel.org>]

On Wed, Nov 29, 2017 at 04:13:14PM +0100, Andrew Jones wrote:
> On Mon, Nov 20, 2017 at 08:16:47PM +0100, Christoffer Dall wrote:
> > 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@arm.com>
> > Signed-off-by: Christoffer Dall <christoffer.dall@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 6113cf850f47..294e949ece24 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"
> > @@ -142,10 +143,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
> >  	return vcpu;
> >  }
> >  
> > +/* Must be called with irq->irq_lock held */
> > +static void vgic_hw_irq_spending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> > +				 bool is_uaccess)
> > +{
> > +	if (!is_uaccess)
> > +		irq->pending_latch = true;
> > +
> > +	if (!is_uaccess)
> > +		vgic_irq_set_phys_active(irq, true);
> 
> I see this whole patch has this two 'if (!is_uaccess)' checks pattern.
> Is that meant to convey something? 

Yeah, that I'm a fool.

> Or is the first condition not supposed
> to have the '!'? (I'm lost with regards to the state tracking differences
> between the guest and userspace and just reviewing superficially...)
> 

This became weird becaus the first clause used to be "if (!is_uaccess ||
is_timer)", but now when the timer is interrupt driven (the timer
optimization series), we don't have that concept anymore, and I just
blindly removes the "|| is_timer" part.

I reworked this so that the functions now have

	if (is_uaccess)
		return;


Thanks,
-Christoffer