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

[Jan Beulich <jbeulich@xxxxxxxx>]

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