[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs



Hi,

On 17/05/2024 07:03, Henry Wang wrote:


On 5/16/2024 6:03 PM, Henry Wang wrote:
From: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>

Currently, routing/removing physical interrupts are only allowed at
the domain creation/destroy time. For use cases such as dynamic device
tree overlay adding/removing, the routing/removing of physical IRQ to
running domains should be allowed.

Removing the above-mentioned domain creation/dying check. Since this
will introduce interrupt state unsync issues for cases when the
interrupt is active or pending in the guest, therefore for these cases
we simply reject the operation. Do it for both new and old vGIC
implementations.

Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
---
v2:
- Reject the case where the IRQ is active or pending in guest.
---
  xen/arch/arm/gic-vgic.c  |  8 ++++++--
  xen/arch/arm/gic.c       | 15 ---------------
  xen/arch/arm/vgic/vgic.c |  5 +++--
  3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 56490dbc43..d1608415f8 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
      {
          /* The VIRQ should not be already enabled by the guest */

This comment needs to be updated.

          if ( !p->desc &&
-             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
              p->desc = desc;
          else
              ret = -EBUSY;
      }
      else
      {
-        if ( desc && p->desc != desc )
+        if ( desc && p->desc != desc &&
+             (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+              test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )

This should be

+        if ( (desc && p->desc != desc) ||
+             test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+             test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
Looking at gic_set_lr(), we first check p->desc, before setting IRQ_GUEST_VISIBLE.

I can't find a common lock, so what would guarantee that p->desc is not going to be used or IRQ_GUEST_VISIBLE set afterwards?



@@ -887,7 +887,8 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
      }
      else                                /* remove a mapped IRQ */
      {
-        if ( desc && irq->hwintid != desc->irq )
+        if ( desc && irq->hwintid != desc->irq &&
+             (irq->active || irq->pending_latch) )

Same here, this should be

+        if ( (desc && irq->hwintid != desc->irq) ||
+             irq->active || irq->pending_latch )

I think the new vGIC doesn't have the same problem because it looks like the IRQ lock will be taken for any access to 'irq'.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.