[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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 <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.
> 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

Does this sound suitable to you?




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.