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

[Julien Grall <julien@xxxxxxx>]

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