Re: [Xen-devel] [PATCH v7 for-xen-4.5 1/2] dpci: Move from an hvm_irq_dpci (and struct domain) to an hvm_dirq_dpci model.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Sep 29, 2014 at 02:22:06PM +0100, Jan Beulich wrote:
> >>> On 27.09.14 at 03:33, <konrad.wilk@xxxxxxxxxx> wrote:
> > @@ -130,6 +127,18 @@ int pt_irq_create_bind(
> >          return -ENOMEM;
> >      }
> >      pirq_dpci = pirq_dpci(info);
> > +    /*
> > +     * The 'pt_irq_create_bind' can be called right after 
> > 'pt_irq_destroy_bind'
> > +     * was called. The 'pirq_cleanup_check' which would free the structure
> > +     * is only called if the event channel for the PIRQ is active. However
> > +     * OS-es that use event channels usually bind the PIRQ to an event 
> > channel
> > +     * and also unbind it before 'pt_irq_destroy_bind' is called which 
> > means
> > +     * we end up re-using the 'dpci' structure. This can be easily 
> > reproduced
> > +     * with unloading and loading the driver for the device.
> > +     *
> > +     * As such on every 'pt_irq_create_bind' call we MUST reset the values.
> > +     */
> > +    pirq_dpci->dom = d;
> 
> I continue to be unconvinced of the correctness of this placement:
> As said before, you only need this in place by the time
> pirq_guest_bind() gets called. And with the patch applied there's

Correct.
> now at least one error path where this doesn't get zapped to NULL:
> 
>         if ( !digl || !girq )
>         {
>             spin_unlock(&d->event_lock);
>             xfree(girq);
>             xfree(digl);
>             return -ENOMEM;
>         }

Right, and the issue I am facing is that with zapping of it to
NULL we run in the problem of refcounting the domain. Or rather
not being able to refcount the domain properly because
it got zapped. I shall respond to your email (this one
http://mid.gmane.org/542924B1020000780003A403@xxxxxxxxxxxxxxxxxxxx).

> 
> > @@ -513,9 +530,27 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
> >      spin_unlock(&d->event_lock);
> >  }
> >  
> > -static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci 
> > *pirq_dpci,
> > -                            void *arg)
> > +static void hvm_dirq_assist(unsigned long arg)
> >  {
> > +    struct hvm_pirq_dpci *pirq_dpci = (struct hvm_pirq_dpci *)arg;
> > +    struct domain *d = pirq_dpci->dom;
> > +
> > +    /*
> > +     * We can be racing with 'pt_irq_destroy_bind' - with us being 
> > scheduled
> > +     * right before 'pirq_guest_unbind' gets called - but us not yet 
> > executed.
> > +     *
> > +     * And '->dom' gets cleared later in the destroy path. We exit and 
> > clear
> > +     * 'mapping' - which is OK as later in this code we would
> 
> Does this comment mean 'masked' instead of 'mapping'?

'masked'.
> 
> > +     * do nothing except clear the ->masked field anyhow.
> > +     */
> > +    if ( !d )
> > +    {
> > +        pirq_dpci->masked = 0;
> > +        return;
> > +    }
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel