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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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




> > What about the other flags? Is this going to be a concern if we don't reset
> > them?
> 
> I don't think so, if I am not mistaken, no LR will be allocated with other
> flags set.
> 
> Kind regards,
> Henry