Re: [Xen-devel] [PATCH 1/6] xen: Add support for hiding and unhiding pcie passthrough devices

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.06.17 at 19:14, <venu.busireddy@xxxxxxxxxx> wrote:

First of all, please Cc all maintainers of code you modify.

> xen: Add support for hiding and unhiding pcie passthrough devices

Please don't repeat the subject in the body of the mail.

> Add support for hiding and unhiding (by introducing two new hypercall
> subops) pci devices that trigger AER fatal errors while assigned to
> guests in passthrough mode. Hiding of the device is done by assigning
> it to dom_xen dummy domain.

Would you mind explaining why simply de-assigning the device
(with an existing operation) isn't suitable here? (This explanation
would presumably belong either in the description here or in the
cover letter.)

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -393,9 +393,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>      {
>      case XEN_DOMCTL_createdomain:
>      case XEN_DOMCTL_test_assign_device:
> +    case XEN_DOMCTL_test_hidden_device:
>      case XEN_DOMCTL_gdbsx_guestmemio:
>          d = NULL;
>          break;
> +    case XEN_DOMCTL_hide_device:
> +    case XEN_DOMCTL_unhide_device:
> +        rcu_lock_domain(dom_xen);
> +        d = dom_xen;
> +        break;

I'm opposed to the introduction of new operations which ignore the
input domain ID. See my recent patch eliminating this for
XEN_DOMCTL_test_assign_device [1]. If these really are domain
independent operations, they ought to be sysctls.

> @@ -1333,19 +1334,31 @@ int iommu_remove_device(struct pci_dev *pdev)
>      return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev));
>  }
>  
> +static bool device_assigned_to_domain(struct domain *d, u16 seg, u8 bus, u8 
> devfn)
> +{
> +    bool rc = false;
> +
> +    pcidevs_lock();
> +
> +    if ( pci_get_pdev_by_domain(d, seg, bus, devfn) )
> +        rc = true;
> +
> +    pcidevs_unlock();
> +    return rc;
> +}
> +
>  /*
>   * If the device isn't owned by the hardware domain, it means it already
>   * has been assigned to other domain, or it doesn't exist.
>   */
>  static int device_assigned(u16 seg, u8 bus, u8 devfn)
>  {
> -    struct pci_dev *pdev;
> -
> -    pcidevs_lock();
> -    pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
> -    pcidevs_unlock();
> +    return device_assigned_to_domain(hardware_domain, seg, bus, devfn) ? 0 : 
> -EBUSY;
> +}
>  
> -    return pdev ? 0 : -EBUSY;
> +static int device_hidden(u16 seg, u8 bus, u8 devfn)
> +{
> +    return device_assigned_to_domain(dom_xen, seg, bus, devfn) ? -EBUSY : 0;
>  }

At least this new function you add wants to return bool. I cannot
see how -EBUSY could be an appropriate return value for meaning
"yes".

> @@ -1354,6 +1367,22 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>      struct pci_dev *pdev;
>      int rc = 0;
>  
> +    if ( device_hidden(seg, bus, devfn) )
> +        return -EINVAL;
> +
> +    if ( d == dom_xen )
> +    {
> +        pdev = pci_get_pdev(seg, bus, devfn);
> +        if ( pdev )
> +        {
> +            list_move(&pdev->domain_list, &dom_xen->arch.pdev_list);
> +            pdev->domain = dom_xen;
> +            return rc;
> +        }
> +        else
> +            return -ENODEV;
> +    }
> +
>      if ( !iommu_enabled || !hd->platform_ops )
>          return 0;

Your addition appears to be misplaced (would belong below the
checks seen above). Additionally you fail to acquire the pcidevs
lock. And the code would likely read better if you inverted the
inner if()'s condition and omitted the "else" and the braces.
Finally I'd prefer if you used d instead of dom_xen everywhere
inside the outer if().

> @@ -1679,7 +1743,86 @@ int iommu_do_pci_domctl(
>                     "deassign %04x:%02x:%02x.%u from dom%d failed (%d)\n",
>                     seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>                     d->domain_id, ret);
> +        break;
> +
> +    case XEN_DOMCTL_hide_device:
> +        machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
> +        ret = xsm_hide_device(XSM_HOOK, d, machine_sbdf);
> +        if ( ret )
> +            break;
> +
> +        if ( unlikely(d->is_dying) )
> +        {
> +            ret = -EAGAIN;
> +            break;
> +        }
> +
> +        seg = machine_sbdf >> 16;
> +        bus = PCI_BUS(machine_sbdf);
> +        devfn = PCI_DEVFN2(machine_sbdf);
> +        flag = domctl->u.assign_device.flag;
> +
> +        if ( device_hidden(seg, bus, devfn) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        pcidevs_lock();
> +        ret = assign_device(dom_xen, seg, bus, devfn, flag);
> +        pcidevs_unlock();
> +        if ( ret == -ERESTART )
> +            ret = hypercall_create_continuation(__HYPERVISOR_domctl,
> +                                                "h", u_domctl);
> +        else if ( ret )
> +            printk(XENLOG_G_ERR "XEN_DOMCTL_hide_device: "
> +                   "hide %04x:%02x:%02x.%u failed (%d)\n",
> +                   seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
> +        break;
> +
> +    case XEN_DOMCTL_unhide_device:
> +        machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
> +        ret = xsm_unhide_device(XSM_HOOK, d, machine_sbdf);
> +        if ( ret )
> +            break;
> +
> +        if ( unlikely(d->is_dying) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        seg = machine_sbdf >> 16;
> +        bus = PCI_BUS(machine_sbdf);
> +        devfn = PCI_DEVFN2(machine_sbdf);
> +
> +        if ( !device_hidden(seg, bus, devfn) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        pcidevs_lock();
> +        ret = deassign_device(dom_xen, seg, bus, devfn);
> +        pcidevs_unlock();
> +
> +        if ( ret == -ERESTART )
> +            ret = hypercall_create_continuation(__HYPERVISOR_domctl,
> +                                                "h", u_domctl);
> +        else if ( ret )
> +            printk(XENLOG_G_ERR "XEN_DOMCTL_unhide_device: "
> +                   "assign %04x:%02x:%02x.%u to dom%d failed (%d)\n",
> +                   seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> +                   d->domain_id, ret);
> +        break;

As it looks you're duplicating a whole lot of code here, with just
minor variations to the original. This is not a good idea
maintenance wise, so you'd have to have a good reason for
doing so.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1222,6 +1222,9 @@ struct xen_domctl {
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
>  #define XEN_DOMCTL_gdbsx_domstatus             1003
> +#define XEN_DOMCTL_hide_device                 2001
> +#define XEN_DOMCTL_unhide_device               2002
> +#define XEN_DOMCTL_test_hidden_device          2003

Why these strange numbers?

> @@ -1783,6 +1799,9 @@ static struct xsm_operations flask_ops = {
>      .test_assign_device = flask_test_assign_device,
>      .assign_device = flask_assign_device,
>      .deassign_device = flask_deassign_device,
> +    .hide_device = flask_hide_device,
> +    .unhide_device = flask_unhide_device,
> +    .test_hidden_device = flask_test_hidden_device,
>  #endif

This is contrary to what you say in the description, and without
respective fields being added to struct xsm_operations I can't
see how this would build.

Jan

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-06/msg02871.html

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