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

[Christoffer Dall]

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 
> > 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 6113cf850f47..294e949ece24 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"
> > @@ -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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Andrew Jones]

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 
> 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 6113cf850f47..294e949ece24 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"
> @@ -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? 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...)

> +}
> +
>  void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
>  {
> + bool is_uaccess = !vgic_get_mmio_requester_vcpu();
>   u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>   int i;
>   unsigned long flags;
> @@ -154,17 +167,45 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   spin_lock_irqsave(>irq_lock, flags);
> - irq->pending_latch = true;
> -
> + if (irq->hw)
> + vgic_hw_irq_spending(vcpu, irq, is_uaccess);
> + else
> + irq->pending_latch = true;
>   vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
>   vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> +/* Must be called with irq->irq_lock held */
> +static void vgic_hw_irq_cpending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> +  bool is_uaccess)
> +{
> + if (!is_uaccess)
> + irq->pending_latch = false;
> +
> + if (!is_uaccess) {
> + /*
> +  * We don't want the guest to effectively mask the physical
> +  * interrupt by doing a write to SPENDR followed by a write to
> +  * CPENDR for HW interrupts, so we clear the active state on
> +  * the physical side if the virtual interrupt is not active.
> +  * This may lead to taking an additional interrupt on the
> +  * host, but that should not be a problem as the worst that
> +  * can happen is an additional vgic injection.  We also clear
> +  * the pending 

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

[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 6113cf850f47..294e949ece24 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"
@@ -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);
+}
+
 void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
  gpa_t addr, unsigned int len,
  unsigned long val)
 {
+   bool is_uaccess = !vgic_get_mmio_requester_vcpu();
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
int i;
unsigned long flags;
@@ -154,17 +167,45 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
 
spin_lock_irqsave(>irq_lock, flags);
-   irq->pending_latch = true;
-
+   if (irq->hw)
+   vgic_hw_irq_spending(vcpu, irq, is_uaccess);
+   else
+   irq->pending_latch = true;
vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
vgic_put_irq(vcpu->kvm, irq);
}
 }
 
+/* Must be called with irq->irq_lock held */
+static void vgic_hw_irq_cpending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+bool is_uaccess)
+{
+   if (!is_uaccess)
+   irq->pending_latch = false;
+
+   if (!is_uaccess) {
+   /*
+* We don't want the guest to effectively mask the physical
+* interrupt by doing a write to SPENDR followed by a write to
+* CPENDR for HW interrupts, so we clear the active state on
+* the physical side if the virtual interrupt is not active.
+* This may lead to taking an additional interrupt on the
+* host, but that should not be a problem as the worst that
+* can happen is an additional vgic injection.  We also clear
+* the pending state to maintain proper semantics for edge HW
+* interrupts.
+*/
+   vgic_irq_set_phys_pending(irq, false);
+   if (!irq->active)
+   vgic_irq_set_phys_active(irq, false);
+   }
+}
+
 void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
  gpa_t addr, unsigned int len,
  unsigned long val)
 {
+   bool is_uaccess = !vgic_get_mmio_requester_vcpu();
u32 intid =