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

[Venu Busireddy <venu.busireddy@xxxxxxxxxx>]

On 2017-07-04 09:46:58 -0600, Jan Beulich wrote:
> >>> On 27.06.17 at 19:14, <venu.busireddy@xxxxxxxxxx> wrote:
> 
> First of all, please Cc all maintainers of code you modify.

I was using the names spit out by the scripts/get_maintainer.pl script
for the patch file. I didn't know that the script had a "-f" option, and
without it, the script spits out only two names, which I included. I now
have Cc'ed all the names that the "-f" option produced. Interestingly,
Daniel's name is not in the "-f" output, and hence, I am still confused
what the correct list is!

> > xen: Add support for hiding and unhiding pcie passthrough devices
> 
> Please don't repeat the subject in the body of the mail.

This is a mistake. Will fix it.

> > 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.)

My initial thinking (for the first revision) was that the guest and
the device together are party to the evil things, and hence the guest
should be killed. But I agree that unassigning the device should be
sufficient. Once the device is removed, the guest can't do much that
any other guest can't. Therefore, I will change this patchset to simply
unassign the device from the guest.

Is that acceptable?

> > --- 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.

Do you think there is a need to hide the device after unassigning it
from the guest? If the answer is "no", then this code will go away. If
the answer is "yes", then I will change it as you did in your reference
[1]. Please let me know.

> > @@ -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".

Will change, if this code stays (depends on the answer to question above).

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

Will change, if this code stays...

> checks seen above). Additionally you fail to acquire the pcidevs

I am acquiring the lock in iommu_do_pci_domctl() in the case
"XEN_DOMCTL_hide_device." Is that not sufficient?

> 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().

Will change, if this code stays...

> > @@ -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.

The only good reason I have is that this is much easier to read. If you
prefer to code the way you coded in your reference [1], I can change
it. Do you want me to?

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

I saw the numbers jump from 79 to 1000 thru 1003, and likewise used
different starting numbers. Would you prefer 80 thru 82, or 1004 thru
1006? Of course, depends on whether we support the hide/unhide operations.

> > @@ -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.

My mistake! I did not have XSM enabled in my build, and hence the
build went through, and I didn't realize that I forgot to change the
xsm_operations structure! Will fix it!

Venu

> [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