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

[Henry Wang <xin.wang2@amd.com>]

On 5/16/2024 6:03 PM, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> 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@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> 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 */
>           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) )

> @@ -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 )

Kind regards,
Henry