| Message ID | 20240516100330.1433265-6-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remaining patches for dynamic node programming using overlay dtbo | expand |
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
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@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 */ 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,
Hi Julien, On 5/19/2024 7:08 PM, Julien Grall wrote: > Hi, > > On 17/05/2024 07:03, Henry Wang wrote: >>> @@ -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. Yes, sorry. I will update this and the one in the new vGIC in v3. >>> 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? I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock taken, at least the current two callers (gic_raise_guest_irq() and gic_restore_pending_irqs()) are doing it this way. Would this address your concern? Thanks. Kind regards, Henry
Hi Henry, On 20/05/2024 02:01, Henry Wang wrote: > Hi Julien, > > On 5/19/2024 7:08 PM, Julien Grall wrote: >> Hi, >> >> On 17/05/2024 07:03, Henry Wang wrote: >>>> @@ -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. > > Yes, sorry. I will update this and the one in the new vGIC in v3. > >>>> 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? > > I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock > taken, at least the current two callers (gic_raise_guest_irq() and > gic_restore_pending_irqs()) are doing it this way. Would this address > your concern? Thanks. I don't think it would address my concern. AFAICT, the lock is not taken by vgic_connect_hw_irq(). I also haven't touched the vGIC for quite a while and didn't have much time to dig into the code. Hence why I didn't propose a fix. The vGIC code was mainly written by Stefano, so maybe he will have an idea how this could be fixed. Cheers,
On Mon, 20 May 2024, Julien Grall wrote: > Hi Henry, > > On 20/05/2024 02:01, Henry Wang wrote: > > Hi Julien, > > > > On 5/19/2024 7:08 PM, Julien Grall wrote: > > > Hi, > > > > > > On 17/05/2024 07:03, Henry Wang wrote: > > > > > @@ -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. > > > > Yes, sorry. I will update this and the one in the new vGIC in v3. > > > > > > > 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? > > > > I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock > > taken, at least the current two callers (gic_raise_guest_irq() and > > gic_restore_pending_irqs()) are doing it this way. Would this address your > > concern? Thanks. > > I don't think it would address my concern. AFAICT, the lock is not taken by > vgic_connect_hw_irq(). > > I also haven't touched the vGIC for quite a while and didn't have much time to > dig into the code. Hence why I didn't propose a fix. > > The vGIC code was mainly written by Stefano, so maybe he will have an idea how > this could be fixed. I think we need to take the v->arch.vgic.lock just after the rank lock in vgic_connect_hw_irq(): vgic_lock_rank(v_target, rank, flags); spin_lock(&v_target->arch.vgic.lock);
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)) ) ret = -EINVAL; else p->desc = NULL; diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 44c40e86de..3ebd89940a 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -135,14 +135,6 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, ASSERT(virq < vgic_num_irqs(d)); ASSERT(!is_lpi(virq)); - /* - * When routing an IRQ to guest, the virtual state is not synced - * back to the physical IRQ. To prevent get unsync, restrict the - * routing to when the Domain is been created. - */ - if ( d->creation_finished ) - return -EBUSY; - ret = vgic_connect_hw_irq(d, NULL, virq, desc, true); if ( ret ) return ret; @@ -167,13 +159,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, ASSERT(test_bit(_IRQ_GUEST, &desc->status)); ASSERT(!is_lpi(virq)); - /* - * Removing an interrupt while the domain is running may have - * undesirable effect on the vGIC emulation. - */ - if ( !d->is_dying ) - return -EBUSY; - desc->handler->shutdown(desc); /* EOI the IRQ if it has not been done by the guest */ diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c index b9463a5f27..785ef2b192 100644 --- a/xen/arch/arm/vgic/vgic.c +++ b/xen/arch/arm/vgic/vgic.c @@ -877,7 +877,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu, if ( connect ) /* assign a mapped IRQ */ { /* The VIRQ should not be already enabled by the guest */ - if ( !irq->hw && !irq->enabled ) + if ( !irq->hw && !irq->enabled && !irq->active && !irq->pending_latch ) { irq->hw = true; irq->hwintid = desc->irq; @@ -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) ) { ret = -EINVAL; }