Re: [PATCH] x86/pass-through: avoid double IRQ unbind during domain cleanup

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2020 18:14, Roger Pau Monné wrote:
> On Tue, Apr 28, 2020 at 02:21:48PM +0200, Jan Beulich wrote:
>> XEN_DOMCTL_destroydomain creates a continuation if domain_kill -ERESTARTs.
>> In that scenario, it is possible to receive multiple _pirq_guest_unbind
>> calls for the same pirq from domain_kill, if the pirq has not yet been
>> removed from the domain's pirq_tree, as:
>>   domain_kill()
>>     -> domain_relinquish_resources()
>>       -> pci_release_devices()
>>         -> pci_clean_dpci_irq()
>>           -> pirq_guest_unbind()
>>             -> __pirq_guest_unbind()
>>
>> Avoid recurring invocations of pirq_guest_unbind() by removing the pIRQ
>> from the tree being iterated after the first call there. In case such a
>> removed entry still has a softirq outstanding, record it and re-check
>> upon re-invocation.
>>
>> Reported-by: Varad Gautam <vrd@xxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Tested-by: Varad Gautam <vrd@xxxxxxxxx>
>>
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -1323,7 +1323,7 @@ void (pirq_cleanup_check)(struct pirq *p
>>      }
>>  
>>      if ( radix_tree_delete(&d->pirq_tree, pirq->pirq) != pirq )
>> -        BUG();
>> +        BUG_ON(!d->is_dying);
> 
> I think to keep the previous behavior this should be:
> 
> BUG_ON(!is_hvm_domain(d) || !d->is_dying);
> 
> Since the pirqs will only be removed elsewhere if the domain is HVM?

pirq_cleanup_check() is a generic hook, and hence I consider it more
correct to not have it behave differently in this regard for different
types of guests. IOW while it _may_ (didn't check) not be the case
today that this can be called multiple times even for PV guests, I'd
view this as legitimate behavior.

Jan