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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Stefano,

On 5/22/2024 9:16 AM, Stefano Stabellini wrote:
> On Wed, 22 May 2024, Henry Wang wrote:
> > Hi Julien,
> >
> > On 5/21/2024 8:30 PM, Julien Grall wrote:
> > > Hi,
> > >
> > > On 21/05/2024 05:35, Henry Wang wrote:
> > > > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> > > > index 56490dbc43..956c11ba13 100644
> > > > --- a/xen/arch/arm/gic-vgic.c
> > > > +++ b/xen/arch/arm/gic-vgic.c
> > > > @@ -439,24 +439,33 @@ int vgic_connect_hw_irq(struct domain *d, struct
> > > > vcpu *v, unsigned int virq,
> > > >          /* We are taking to rank lock to prevent parallel connections. */
> > > >        vgic_lock_rank(v_target, rank, flags);
> > > > +    spin_lock(&v_target->arch.vgic.lock);
> > > I know this is what Stefano suggested, but v_target would point to the
> > > current affinity whereas the interrupt may be pending/active on the
> > > "previous" vCPU. So it is a little unclear whether v_target is the correct
> > > lock. Do you have more pointer to show this is correct?
> > No I think you are correct, we have discussed this in the initial version of
> > this patch. Sorry.
> >
> > I followed the way from that discussion to note down the vcpu ID and retrieve
> > here, below is the diff, would this make sense to you?
> >
> > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> > index 956c11ba13..134ed4e107 100644
> > --- a/xen/arch/arm/gic-vgic.c
> > +++ b/xen/arch/arm/gic-vgic.c
> > @@ -439,7 +439,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v,
> > unsigned int virq,
> >
> >       /* We are taking to rank lock to prevent parallel connections. */
> >       vgic_lock_rank(v_target, rank, flags);
> > -    spin_lock(&v_target->arch.vgic.lock);
> > + spin_lock(&d->vcpu[p->spi_vcpu_id]->arch.vgic.lock);
> >
> >       if ( connect )
> >       {
> > @@ -465,7 +465,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v,
> > unsigned int virq,
> >               p->desc = NULL;
> >       }
> >
> > -    spin_unlock(&v_target->arch.vgic.lock);
> > + spin_unlock(&d->vcpu[p->spi_vcpu_id]->arch.vgic.lock);
> >       vgic_unlock_rank(v_target, rank, flags);
> >
> >       return ret;
> > diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> > index 79b73a0dbb..f4075d3e75 100644
> > --- a/xen/arch/arm/include/asm/vgic.h
> > +++ b/xen/arch/arm/include/asm/vgic.h
> > @@ -85,6 +85,7 @@ struct pending_irq
> >       uint8_t priority;
> >       uint8_t lpi_priority;       /* Caches the priority if this is an LPI. */
> >       uint8_t lpi_vcpu_id;        /* The VCPU for an LPI. */
> > +    uint8_t spi_vcpu_id;        /* The VCPU for an SPI. */
> >       /* inflight is used to append instances of pending_irq to
> >        * vgic.inflight_irqs */
> >       struct list_head inflight;
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index c04fc4f83f..e852479f13 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -632,6 +632,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v,
> > unsigned int virq,
> >       }
> >       list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
> >   out:
> > +    n->spi_vcpu_id = v->vcpu_id;
> >       spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >
> >       /* we have a new higher priority irq, inject it into the guest */
> >       vcpu_kick(v);
> >
> >
> > > Also, while looking at the locking, I noticed that we are not doing anything
> > > with GIC_IRQ_GUEST_MIGRATING. In gic_update_one_lr(), we seem to assume that
> > > if the flag is set, then p->desc cannot be NULL.
> > >
> > > Can we reach vgic_connect_hw_irq() with the flag set?
> > I think even from the perspective of making the code extra safe, we should
> > also check GIC_IRQ_GUEST_MIGRATING as the LR is allocated for this case. I
> > will also add the check of GIC_IRQ_GUEST_MIGRATING here.
> Yes. I think it might be easier to check for GIC_IRQ_GUEST_MIGRATING
> early and return error immediately in that case. Otherwise, we can
> continue and take spin_lock(&v_target->arch.vgic.lock) because no
> migration is in progress
Ok, this makes sense to me, I will add

    if( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
    {
        vgic_unlock_rank(v_target, rank, flags);
        return -EBUSY;
    }

right after taking the vgic rank lock.

Kind regards,
Henry