diff mbox series

[2/3] x86/irq: don't disable domain MSI IRQ on force unbind

Message ID 20210126110606.21741-3-roger.pau@citrix.com (mailing list archive)
State New
Headers show
Series x86/intr: interrupt related fixes | expand

Commit Message

Roger Pau Monné Jan. 26, 2021, 11:06 a.m. UTC
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(-)

Comments

Jan Beulich Jan. 26, 2021, 2:52 p.m. UTC | #1
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
Roger Pau Monné Jan. 26, 2021, 4:21 p.m. UTC | #2
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.
Jan Beulich Jan. 27, 2021, 8:59 a.m. UTC | #3
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 mbox series

Patch

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
         {