Re: [PATCH v7 for-xen-4.5 2/2] dpci: Replace tasklet with an softirq (v7)

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.09.14 at 03:33, <konrad.wilk@oracle.com> wrote:
> +/*
> + * Should only be called from hvm_do_IRQ_dpci. We use the
> + * 'masked' as an gate to thwart multiple interrupts.
> + *
> + * The 'masked' is cleared by 'softirq_dpci' when it has
> + * completed executing 'hvm_dirq_assist'.
> + *
> + */
> +static void schedule_softirq_for(struct hvm_pirq_dpci *pirq_dpci)

Perhaps better name this raise_... than schedule_...?

> +/*
> + * If we are racing with softirq_dpci (masked is still set) we return
> + * -EAGAIN. Otherwise we return 0.
> + *
> + *  If it is -EAGAIN, it is the callers responsibility to make sure
> + *  that the softirq (with the event_lock dropped) has ran. We need
> + *  to flush out the outstanding 'dpci_softirq' (no more of them
> + *  will be added for this pirq as the IRQ action handler has been
> + *  reset in pt_irq_destroy_bind).
> + */
> +int pt_pirq_softirq_active(struct hvm_pirq_dpci *pirq_dpci)
> +{
> +    if ( pirq_dpci->masked )
> +        return -EAGAIN;

I think this would better be -ERESTART with the post-4.4 conversion
from -EAGAIN to -ERESTART.

> @@ -544,14 +607,12 @@ static void hvm_dirq_assist(unsigned long arg)
>       * do nothing except clear the ->masked field anyhow.
>       */
>      if ( !d )
> -    {
> -        pirq_dpci->masked = 0;
>          return;
> -    }

This renders stale the comment right above.

> -void pci_release_devices(struct domain *d)
> +int pci_release_devices(struct domain *d)
>  {
>      struct pci_dev *pdev;
>      u8 bus, devfn;
> +    int ret;
>  
>      spin_lock(&pcidevs_lock);
> -    pci_clean_dpci_irqs(d);
> +    ret = pci_clean_dpci_irqs(d);
> +    if ( ret == -EAGAIN )

I don't think you should special case -EAGAIN here.

> --- a/xen/include/xen/softirq.h
> +++ b/xen/include/xen/softirq.h
> @@ -8,6 +8,9 @@ enum {
>      NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ,
>      RCU_SOFTIRQ,
>      TASKLET_SOFTIRQ,
> +#ifdef HAS_PASSTHROUGH
> +    HVM_DPCI_SOFTIRQ,
> +#endif

The conditional is wrong actually (sorry, noticed this just now) - as
io.c is x86-specific, this should be an arch-specific softirq.

Also I notice you dropped the domain ref-counting you previously
used, yet I don't clearly see how doing so is safe.

Jan