Re: [PATCH] x86/physdev: Call xsm_unmap_domain_irq earlier

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.03.2022 13:40, Roger Pau Monné wrote:
> On Fri, Mar 25, 2022 at 10:18:26AM -0400, Jason Andryuk wrote:
>> Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq.
>>
>> xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from
>> complete_domain_destroy as an RCU callback.  The source context was an
>> unexpected, random domain.  Since this is a xen-internal operation,
>> going through the XSM hook is inapproriate.
>>
>> Move the XSM hook up into physdev_unmap_pirq, which is the
>> guest-accessible path.  This requires moving some of the sanity check
>> upwards as well since the hook needs the additional data to make its
>> decision.  Since complete_domain_destroy still calls unmap_domain_pirq,
>> replace the moved runtime checking with assert.  Only valid pirqs should
>> make their way into unmap_domain_pirq from complete_domain_destroy.
>>
>> This is mostly code movement, but one style change is to pull `irq =
>> info->arch.irq` out of the if condition.

And why is this better? You now have an extra conditional expression
there.

>> Label done is now unused and removed.
>>
>> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
>> ---
>> unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and
>> vpci_msi_disable.  vioapic_hwdom_map_gsi is a cleanup path after going
>> through map_domain_pirq, and I don't think the vpci code is directly
>> guest-accessible.  So I think those are okay, but I not familiar with
>> that code.  Hence, I am highlighting it.
> 
> Hm, for vpci_msi_disable it's not technically correct to drop the XSM
> check. This is a guests accessible path, however the value of PIRQ is
> not settable by the guest, so it's kind of OK just for that reason.

Kind of - perhaps. But better to avoid if possible. Hence I would prefer
the ->is_dying alternative you suggest at the end.

Jan