Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Julien,

On 5/9/2024 4:46 AM, Julien Grall wrote:
> Hi Henry,
> [...]
>
> > > > we have 3 possible states which can be read from LR for this case : 
> > > > active, pending, pending and active.
> > > > - I don't think we can do anything about the active state, so we 
> > > > should return -EBUSY and reject the whole operation of removing the 
> > > > IRQ from running guest, and user can always retry this operation.
> > > This would mean a malicious/buggy guest would be able to prevent a 
> > > device to be de-assigned. This is not a good idea in particular when 
> > > the domain is dying.
> > > That said, I think you can handle this case. The LR has a bit to 
> > > indicate whether the pIRQ needs to be EOIed. You can clear it and 
> > > this would prevent the guest to touch the pIRQ. There might be other 
> > > clean-up to do in the vGIC datastructure.
> > I probably misunderstood this sentence, do you mean the EOI bit in 
> > the pINTID field? I think this bit is only available when the HW bit 
> > of LR is 0, but in our case the HW is supposed to be 1 (as indicated 
> > as your previous comment). Would you mind clarifying a bit more? Thanks!
> You are right, ICH_LR.HW will be 1 for physical IRQ routed to a guest. 
> What I was trying to explain is this bit could be cleared (with 
> ICH_LR.pINTD adjusted).
Thank you for all the discussions. Based on that, would below diff make 
sense to you? I did a test of the dynamic dtbo adding/removing with a 
ethernet device with this patch applied. Test steps are:
(1) Use xl dt-overlay to add the ethernet device to Xen device tree and 
assign it to dom0.
(2) Create a domU.
(3) Use xl dt-overlay to de-assign the device from dom0 and assign it to 
domU.
(4) Destroy the domU.

The ethernet device is functional in the domain respectively when it is 
attached to a domain and I don't see errors when I destroy domU. But 
honestly I think the case we talked about is a quite unusual case so I 
am not sure if it was hit during my test.
```
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index a775f886ed..d3f9cd2299 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -135,16 +135,6 @@ int gic_route_irq_to_guest(struct domain *d, 
unsigned int virq,
     ASSERT(virq < vgic_num_irqs(d));
     ASSERT(!is_lpi(virq));

-    /*
-     * When routing an IRQ to guest, the virtual state is not synced
-     * back to the physical IRQ. To prevent get unsync, restrict the
-     * routing to when the Domain is been created.
-     */
-#ifndef CONFIG_OVERLAY_DTB
-    if ( d->creation_finished )
-        return -EBUSY;
-#endif
-
     ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
     if ( ret )
         return ret;
@@ -169,20 +159,40 @@ int gic_remove_irq_from_guest(struct domain *d, 
unsigned int virq,
     ASSERT(test_bit(_IRQ_GUEST, &desc->status));
     ASSERT(!is_lpi(virq));

-    /*
-     * Removing an interrupt while the domain is running may have
-     * undesirable effect on the vGIC emulation.
-     */
-#ifndef CONFIG_OVERLAY_DTB
-    if ( !d->is_dying )
-        return -EBUSY;
-#endif
-
     desc->handler->shutdown(desc);

     /* EOI the IRQ if it has not been done by the guest */
     if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
+    {
+        /*
+         * Handle the LR where the physical interrupt is de-assigned 
from the
+         * guest before it was EOIed
+         */
+        struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
+        struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
+        struct pending_irq *p = irq_to_pending(v_target, virq);
+        unsigned long flags;
+
+        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+        /* LR allocated for the IRQ */
+        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) &&
+             test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+        {
+            gic_hw_ops->clear_lr(p->lr);
+            clear_bit(p->lr, &v_target->arch.lr_mask);
+
+            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+            clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
+            p->lr = GIC_INVALID_LR;
+        }
+        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+
+        vgic_lock_rank(v_target, rank, flags);
+        vgic_disable_irqs(v_target, (~rank->ienable) & rank->ienable, 
rank->index);
+        vgic_unlock_rank(v_target, rank, flags);
+
         gic_hw_ops->deactivate_irq(desc);
+    }
     clear_bit(_IRQ_INPROGRESS, &desc->status);

     ret = vgic_connect_hw_irq(d, NULL, virq, desc, false);
```

Kind regards,
Henry

> Cheers,