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.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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
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;
        }

> @@ -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'?

> +     * 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