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,
