Re: [Xen-devel] [PATCH v3] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
> Durrant, Paul
> Sent: 05 March 2020 17:37
> To: Jan Beulich <jbeulich@xxxxxxxx>; Gautam, Varad <vrd@xxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Roger Pau Monné <roger.pau@xxxxxxxxxx>; 
> Julien Grall
> <julien@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v3] x86: irq: Do not BUG_ON multiple unbind 
> calls for shared pirqs
> 
> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> > Beulich
> > Sent: 05 March 2020 09:37
> > To: Gautam, Varad <vrd@xxxxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper 
> > <andrew.cooper3@xxxxxxxxxx>; Julien Grall
> > <julien@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Subject: Re: [Xen-devel] [PATCH v3] x86: irq: Do not BUG_ON multiple unbind 
> > calls for shared pirqs
> >
> > On 29.01.2020 12:47, Jan Beulich wrote:
> > > On 29.01.2020 11:30, Roger Pau Monné wrote:
> > >> Hello,
> > >>
> > >> Thanks for the patch! Next time could you please try to reply to the
> > >> previous questions before sending a new version:
> > >>
> > >> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00257.html
> > >>
> > >> On Wed, Jan 29, 2020 at 10:28:07AM +0100, Varad Gautam 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()
> > >>>
> > >>> For a shared pirq (nr_guests > 1), the first call would zap the current
> > >>> domain from the pirq's guests[] list, but the action handler is never 
> > >>> freed
> > >>> as there are other guests using this pirq. As a result, on the second 
> > >>> call,
> > >>> __pirq_guest_unbind searches for the current domain which has been 
> > >>> removed
> > >>> from the guests[] list, and hits a BUG_ON.
> > >>>
> > >>> Make __pirq_guest_unbind safe to be called multiple times by letting xen
> > >>> continue if a shared pirq has already been unbound from this guest. The
> > >>> PIRQ will be cleaned up from the domain's pirq_tree during the 
> > >>> destruction
> > >>> in complete_domain_destroy anyways.
> > >>
> > >> So AFAICT this is because pt_pirq_softirq_active() returns true in
> > >> pci_clean_dpci_irq() and hence the iteration is stopped and
> > >> hvm_domain_irq(d)->dpci is not set to NULL.
> > >>
> > >> Would it be possible to clean the already processed IRQs from the
> > >> domain pirq_tree?
> > >
> > > This might work, perhaps by way of invoking unmap_domain_pirq()
> > > right after pirq_guest_unbind(), as long as hvm_dirq_assist() (as
> > > called from dpci_softirq()) can be made skip all actual work it
> > > means to do in such a case. Unfortunately the two ->masked fields
> > > acted upon are different between __pirq_guest_unbind() and
> > > hvm_dirq_assist().
> >
> > Ping? Unless I hear back soon, I'm afraid I'm going to drop this
> > patch from my "pending" mail folder, as not being agreed whether
> > to stick to the current version or whether to go this alternative
> > route. A more "natural" approach to fixing the issue would be
> > quite nice, after all.
> 
> I'll try to pick this up tomorrow as I helped diagnose the problem being 
> fixed.
> 

I'm looking at this now and I am finding the code very confusing, but I think 
we could achieve the cleanup by passing back the irq index out of 
__pirq_guest_unbind() such that pirq_guest_unbind() can call 
clean_domain_irq_pirq(). I still haven't got much of a clue as to how all the 
data structures hang together though.

  Paul


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel