| Message ID | 20210126110606.21741-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/intr: interrupt related fixes | expand |
On 26.01.2021 12:06, Roger Pau Monne wrote: > When force unbinding a MSI the used IRQ would get added to the domain > irq-pirq tree as -irq -pirq, I think it's -pirq at index irq, i.e. I don't think IRQ gets negated as far as the radix tree goes. info->arch.irq gets a negative value stored, yes. > thus preventing the same IRQ to be used by the domain. Iirc this (answering your post-commit-message question here) is for cleaning up _after_ the domain, i.e. there's no goal to allow re-use of this IRQ. The comment ahead of unmap_domain_pirq() validly says "The pirq should have been unbound before this call." The only time we can't make ourselves dependent upon this is when the guest is being cleaned up. During normal operation I think we actually _want_ to enforce correct behavior of the guest here. > It's not clear to me why we add the irq as -irq -pirq to the irq-pirq > domain tree on forced unbind, as allowing to bind the irq again should > just work regardless of whether it has been previously forcefully > unbound? To continue from the above, see pirq_guest_unbind() where we have if ( desc == NULL ) { irq = -pirq->arch.irq; BUG_ON(irq <= 0); desc = irq_to_desc(irq); spin_lock_irq(&desc->lock); clear_domain_irq_pirq(d, irq, pirq); } as the alternative to going the normal path through __pirq_guest_unbind(). Again iirc that's to cover for the unbind request arriving after the unmap one (i.e. having caused the unmap to force-unbind the IRQ). Jan
On Tue, Jan 26, 2021 at 03:52:54PM +0100, Jan Beulich wrote: > On 26.01.2021 12:06, Roger Pau Monne wrote: > > When force unbinding a MSI the used IRQ would get added to the domain > > irq-pirq tree as -irq -pirq, > > I think it's -pirq at index irq, i.e. I don't think IRQ gets > negated as far as the radix tree goes. info->arch.irq gets a > negative value stored, yes. Right, and this then prevents the IRQ to be used at all by the domain. Doiong a domain_irq_to_pirq with that IRQ will get -pirq, but that seems pretty arbitrary for MSI IRQs, that get allocated on demand. At the end of unmap_domain_pirq the IRQ will get freed if it was assigned to an MSI source, and hence it seem pointless to add irq -> -pirq to the domain irq tree. > > thus preventing the same IRQ to be used by the domain. > > Iirc this (answering your post-commit-message question here) > is for cleaning up _after_ the domain, i.e. there's no goal > to allow re-use of this IRQ. The comment ahead of > unmap_domain_pirq() validly says "The pirq should have been > unbound before this call." The only time we can't make > ourselves dependent upon this is when the guest is being > cleaned up. During normal operation I think we actually > _want_ to enforce correct behavior of the guest here. OK, so that might be fine for legacy PCI IRQs, that are fixed, but quite pointless for allocated on demand MSI IRQs that can change between allocations. > > It's not clear to me why we add the irq as -irq -pirq to the irq-pirq > > domain tree on forced unbind, as allowing to bind the irq again should > > just work regardless of whether it has been previously forcefully > > unbound? > > To continue from the above, see pirq_guest_unbind() where > we have > > if ( desc == NULL ) > { > irq = -pirq->arch.irq; > BUG_ON(irq <= 0); > desc = irq_to_desc(irq); > spin_lock_irq(&desc->lock); > clear_domain_irq_pirq(d, irq, pirq); > } > > as the alternative to going the normal path through > __pirq_guest_unbind(). Again iirc that's to cover for the > unbind request arriving after the unmap one (i.e. having > caused the unmap to force-unbind the IRQ). Oh, so that's the point. Do you think you could add some comments to explain the indented behaviour? I think I get it now, but it's hard to follow without some pointers. Thanks, Roger.
On 26.01.2021 17:21, Roger Pau Monné wrote: > On Tue, Jan 26, 2021 at 03:52:54PM +0100, Jan Beulich wrote: >> On 26.01.2021 12:06, Roger Pau Monne wrote: >>> When force unbinding a MSI the used IRQ would get added to the domain >>> irq-pirq tree as -irq -pirq, >> >> I think it's -pirq at index irq, i.e. I don't think IRQ gets >> negated as far as the radix tree goes. info->arch.irq gets a >> negative value stored, yes. > > Right, and this then prevents the IRQ to be used at all by the domain. > Doiong a domain_irq_to_pirq with that IRQ will get -pirq, but that > seems pretty arbitrary for MSI IRQs, that get allocated on demand. > > At the end of unmap_domain_pirq the IRQ will get freed if it was > assigned to an MSI source, This is a good point, but ... > and hence it seem pointless to add irq -> > -pirq to the domain irq tree. ... without this I think ... >>> thus preventing the same IRQ to be used by the domain. >> >> Iirc this (answering your post-commit-message question here) >> is for cleaning up _after_ the domain, i.e. there's no goal >> to allow re-use of this IRQ. The comment ahead of >> unmap_domain_pirq() validly says "The pirq should have been >> unbound before this call." The only time we can't make >> ourselves dependent upon this is when the guest is being >> cleaned up. During normal operation I think we actually >> _want_ to enforce correct behavior of the guest here. > > OK, so that might be fine for legacy PCI IRQs, that are fixed, but > quite pointless for allocated on demand MSI IRQs that can change > between allocations. > >>> It's not clear to me why we add the irq as -irq -pirq to the irq-pirq >>> domain tree on forced unbind, as allowing to bind the irq again should >>> just work regardless of whether it has been previously forcefully >>> unbound? >> >> To continue from the above, see pirq_guest_unbind() where >> we have >> >> if ( desc == NULL ) >> { >> irq = -pirq->arch.irq; >> BUG_ON(irq <= 0); ... this would then trigger (or other badness result) if the guest does unmap and unbind in the wrong order. >> desc = irq_to_desc(irq); >> spin_lock_irq(&desc->lock); >> clear_domain_irq_pirq(d, irq, pirq); >> } >> >> as the alternative to going the normal path through >> __pirq_guest_unbind(). Again iirc that's to cover for the >> unbind request arriving after the unmap one (i.e. having >> caused the unmap to force-unbind the IRQ). > > Oh, so that's the point. Do you think you could add some comments to > explain the indented behaviour? I think I get it now, but it's hard to > follow without some pointers. Let's first settle on what exactly to do here, because this may then affect what exactly those comments would want to say. (If the patch here ended up touching at least one of the two involved pieces of code, perhaps such comments could also get added while altering that code.) Jan
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 49849bd7d3..c8e5277eb1 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -2362,7 +2362,7 @@ int unmap_domain_pirq(struct domain *d, int pirq) { BUG_ON(irq != domain_pirq_to_irq(d, pirq + i)); - if ( !forced_unbind ) + if ( !forced_unbind || msi_desc ) clear_domain_irq_pirq(d, irq, info); else {
When force unbinding a MSI the used IRQ would get added to the domain irq-pirq tree as -irq -pirq, thus preventing the same IRQ to be used by the domain. Doing so for MSI allocated IRQs prevents the same IRQ to be used for any other device, since MSI IRQs are allocated and deallocated on demand unlike legacy PCI IRQs. Ignore forced unbinds for MSI IRQs and just cleanup the irq-pirq information as normal. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- It's not clear to me why we add the irq as -irq -pirq to the irq-pirq domain tree on forced unbind, as allowing to bind the irq again should just work regardless of whether it has been previously forcefully unbound? --- xen/arch/x86/irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)