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

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

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.

Thanks, Roger.