On 29.04.2020 10:26, Roger Pau Monné wrote: > On Wed, Apr 29, 2020 at 09:37:11AM +0200, Jan Beulich wrote: >> 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 <v...@amazon.de> >>>> Signed-off-by: Jan Beulich <jbeul...@suse.com> >>>> Tested-by: Varad Gautam <v...@amazon.de> >>>> >>>> --- 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. > > Previous to this patch pirq_cleanup_check couldn't be called multiple > times, as it would result in the BUG triggering, that was true for > both PV and HVM. Now that the removal of PIRQs from the tree is done > elsewhere for HVM when the domain is dying the check needs to be > relaxed for HVM at least. I would prefer if it was kept as-is for PV > (since there's been no change in behavior for PV that could allow for > multiple calls to pirq_cleanup_check), or else a small comment in the > commit message would help clarify this is done on purpose.
I've added "Note that pirq_cleanup_check() gets relaxed beyond what's strictly needed here, to avoid introducing an asymmetry there between HVM and PV guests." Does this sound suitable to you? Jan
