Re: [Xen-devel] [PATCH for-4.13 v2] passthrough: simplify locking and logging

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.11.2019 19:28, Igor Druzhinin wrote:
> This patch instead acquires the lock once for assignment (or test assign)
> operations directly in iommu_do_pci_domctl() and thus can remove the
> duplicate domain ownership check in assign_device(). Whilst in the
> neighbourhood, the patch also removes some debug logging from
> assign_device() and deassign_device() and replaces it with proper error
> logging, which allows error logging in iommu_do_pci_domctl() to be
> removed. Also, since device_assigned() can tell the difference between a
> guest assigned device and a non-existent one, log the actual error
> condition rather then being ambiguous for the sake a few extra lines of
> code.

In this last sentence it looks like you mean the caller of
device_assigned(), not the function itself.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -932,30 +932,27 @@ static int deassign_device(struct domain *d, uint16_t 
> seg, uint8_t bus,
>              break;
>          ret = hd->platform_ops->reassign_device(d, target, devfn,
>                                                  pci_to_dev(pdev));
> -        if ( !ret )
> -            continue;
> -
> -        printk(XENLOG_G_ERR "%pd: deassign %04x:%02x:%02x.%u failed (%d)\n",
> -               d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
> -        return ret;
> +        if ( ret )
> +            goto out;
>      }
>  
>      devfn = pdev->devfn;
>      ret = hd->platform_ops->reassign_device(d, target, devfn,
>                                              pci_to_dev(pdev));
>      if ( ret )
> -    {
> -        dprintk(XENLOG_G_ERR,
> -                "%pd: deassign device (%04x:%02x:%02x.%u) failed\n",
> -                d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> -        return ret;
> -    }
> +        goto out;
>  
>      if ( pdev->domain == hardware_domain  )
>          pdev->quarantine = false;
>  
>      pdev->fault.count = 0;
>  
> +out:
> +    if ( ret )
> +        printk(XENLOG_G_ERR
> +               "%pd: deassign device (%04x:%02x:%02x.%u) failed (%d)\n", d,
> +               seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
> +
>      return ret;
>  }

There would have been quite a bit less code churn if you simply
replaced the dprintk(). If you really want to stick to the goto
approach you're introducing (which I dislike, but I know others
prefer it in cases like this one), then please indent the label and
shorten the message to the one that was used in the original printk().

> @@ -1549,11 +1542,10 @@ static int device_assigned(u16 seg, u8 bus, u8 devfn)
>                pdev->domain != dom_io )
>          rc = -EBUSY;
>  
> -    pcidevs_unlock();
> -
>      return rc;
>  }
>  
> +/* caller should hold the pcidevs_lock */
>  static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 
> flag)

Just like the comment ahead of deassign_device(), this one should
start with a capital letter.

> @@ -1608,19 +1588,17 @@ static int assign_device(struct domain *d, u16 seg, 
> u8 bus, u8 devfn, u32 flag)
>          if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>              break;
>          rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), 
> flag);
> -        if ( rc )
> -            printk(XENLOG_G_WARNING "d%d: assign %04x:%02x:%02x.%u failed 
> (%d)\n",
> -                   d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> -                   rc);
>      }
>  
>   done:
> +    if ( rc )
> +        printk(XENLOG_G_ERR
> +               "%pd: assign device (%04x:%02x:%02x.%u) failed (%d)\n", d,
> +               seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), rc);

I'd prefer if we could stick to this being XENLOG_G_WARNING: Other
than device de-assignment failure, assignment failure is unlikely
to be an issue to the host as a whole. Also please again shorten
the message to what it was before.

> @@ -1776,29 +1754,40 @@ int iommu_do_pci_domctl(
>          bus = PCI_BUS(machine_sbdf);
>          devfn = PCI_DEVFN2(machine_sbdf);
>  
> +        pcidevs_lock();
>          ret = device_assigned(seg, bus, devfn);
>          if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
>          {
> -            if ( ret )
> +            switch ( ret )
>              {
> +            case 0:
> +                break;
> +
> +            case -ENODEV:
>                  printk(XENLOG_G_INFO
> -                       "%04x:%02x:%02x.%u already assigned, or 
> non-existent\n",
> +                       "%04x:%02x:%02x.%u non-existent\n",
>                         seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>                  ret = -EINVAL;
> +                break;
> +
> +            case -EBUSY:
> +                printk(XENLOG_G_INFO
> +                       "%04x:%02x:%02x.%u already assigned\n",
> +                       seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> +                ret = -EINVAL;
> +                break;
> +
> +            default:
> +                ret = -EINVAL;
> +                break;
>              }

Three separate but identical assignments to ret look a little odd at
least. Is there a reason why we need to (continue to) convert the
original error code? I can't seem to be able to find any after some
looking around, but I surely may have missed something.

Jan

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