[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |