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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 29, 2020 at 10:35:24AM +0200, Jan Beulich wrote:
> 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
>  guests."
> 
> Does this sound suitable to you?

Yes, thanks. With that:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Roger.