[PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 11096 bytes --]

So far callers of the libxc interface passed in a domain ID which was
then ignored in the hypervisor. Instead, make the hypervisor honor it
(accepting DOMID_INVALID to obtain original behavior), allowing to
query whether a device is assigned to a particular domain. Ignore the
passed in domain ID at the libxc layer instead, in order to not break
existing callers. New libxc functions would need to be added if callers
wanted to leverage the new functionality.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Would DOMID_IO be a better fit than DOMID_INVALID here?

--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1539,13 +1539,13 @@ int xc_get_device_group(
 
 int xc_test_assign_device(
     xc_interface *xch,
-    uint32_t domid,
+    uint32_t domid, /* ignored */
     uint32_t machine_sbdf)
 {
     DECLARE_DOMCTL;
 
     domctl.cmd = XEN_DOMCTL_test_assign_device;
-    domctl.domain = domid;
+    domctl.domain = DOMID_INVALID;
     domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
     domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
     domctl.u.assign_device.flags = 0;
@@ -1603,7 +1603,7 @@ int xc_assign_dt_device(
 
 int xc_test_assign_dt_device(
     xc_interface *xch,
-    uint32_t domid,
+    uint32_t domid, /* ignored */
     char *path)
 {
     int rc;
@@ -1615,7 +1615,7 @@ int xc_test_assign_dt_device(
         return -1;
 
     domctl.cmd = XEN_DOMCTL_test_assign_device;
-    domctl.domain = (domid_t)domid;
+    domctl.domain = DOMID_INVALID;
 
     domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
     domctl.u.assign_device.u.dt.size = size;
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -391,11 +391,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     switch ( op->cmd )
     {
-    case XEN_DOMCTL_createdomain:
     case XEN_DOMCTL_test_assign_device:
+        if ( op->domain == DOMID_INVALID )
+        {
+    case XEN_DOMCTL_createdomain:
     case XEN_DOMCTL_gdbsx_guestmemio:
-        d = NULL;
-        break;
+            d = NULL;
+            break;
+        }
+        /* fall through */
     default:
         d = rcu_lock_domain_by_id(op->domain);
         if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -93,7 +93,8 @@ fail:
     return rc;
 }
 
-static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
+static bool_t iommu_dt_device_is_assigned(const struct domain *d,
+                                          const struct dt_device_node *dev)
 {
     bool_t assigned = 0;
 
@@ -101,7 +102,8 @@ static bool_t iommu_dt_device_is_assigne
         return 0;
 
     spin_lock(&dtdevs_lock);
-    assigned = !list_empty(&dev->domain_list);
+    assigned = d ? dt_device_used_by(dev) == d->domain_id
+                 : !list_empty(&dev->domain_list);
     spin_unlock(&dtdevs_lock);
 
     return assigned;
@@ -209,11 +211,11 @@ int iommu_do_dt_domctl(struct xen_domctl
         if ( ret )
             break;
 
-        ret = xsm_test_assign_dtdevice(XSM_HOOK, dt_node_full_name(dev));
+        ret = xsm_test_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
         if ( ret )
             break;
 
-        if ( iommu_dt_device_is_assigned(dev) )
+        if ( iommu_dt_device_is_assigned(d, dev) )
         {
             printk(XENLOG_G_ERR "%s already assigned.\n",
                    dt_node_full_name(dev));
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -522,7 +522,7 @@ struct pci_dev *pci_get_real_pdev(int se
 }
 
 struct pci_dev *pci_get_pdev_by_domain(
-    struct domain *d, int seg, int bus, int devfn)
+    const struct domain *d, int seg, int bus, int devfn)
 {
     struct pci_seg *pseg = get_pseg(seg);
     struct pci_dev *pdev = NULL;
@@ -1337,12 +1337,12 @@ int iommu_remove_device(struct pci_dev *
  * 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)
+static int device_assigned(const struct domain *d, u16 seg, u8 bus, u8 devfn)
 {
-    struct pci_dev *pdev;
+    const struct pci_dev *pdev;
 
     pcidevs_lock();
-    pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
+    pdev = pci_get_pdev_by_domain(d ?: hardware_domain, seg, bus, devfn);
     pcidevs_unlock();
 
     return pdev ? 0 : -EBUSY;
@@ -1590,7 +1590,7 @@ int iommu_do_pci_domctl(
 
         machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
 
-        ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
+        ret = xsm_test_assign_device(XSM_HOOK, d, machine_sbdf);
         if ( ret )
             break;
 
@@ -1598,13 +1598,12 @@ int iommu_do_pci_domctl(
         bus = PCI_BUS(machine_sbdf);
         devfn = PCI_DEVFN2(machine_sbdf);
 
-        if ( device_assigned(seg, bus, devfn) )
-        {
+        ret = device_assigned(d, seg, bus, devfn);
+        if ( ret && !d )
             printk(XENLOG_G_INFO
                    "%04x:%02x:%02x.%u already assigned, or non-existent\n",
                    seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
-            ret = -EINVAL;
-        }
+
         break;
 
     case XEN_DOMCTL_assign_device:
@@ -1634,7 +1633,7 @@ int iommu_do_pci_domctl(
         bus = PCI_BUS(machine_sbdf);
         devfn = PCI_DEVFN2(machine_sbdf);
 
-        ret = device_assigned(seg, bus, devfn) ?:
+        ret = device_assigned(NULL, seg, bus, devfn) ?:
               assign_device(d, seg, bus, devfn, flags);
         if ( ret == -ERESTART )
             ret = hypercall_create_continuation(__HYPERVISOR_domctl,
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -506,7 +506,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendt
 
 /* Assign a device to a guest. Sets up IOMMU structures. */
 /* XEN_DOMCTL_assign_device */
-/* XEN_DOMCTL_test_assign_device */
+/*
+ * XEN_DOMCTL_test_assign_device: Pass DOMID_INVALID to find out whether the
+ * given device is assigned to a DomU at all. Pass a specific domain ID to
+ * find out whether the given device is assigned to that domain.
+ */
 /*
  * XEN_DOMCTL_deassign_device: The behavior of this DOMCTL differs
  * between the different type of device:
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -125,8 +125,8 @@ int pci_ro_device(int seg, int bus, int
 int pci_hide_device(int bus, int devfn);
 struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
 struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn);
-struct pci_dev *pci_get_pdev_by_domain(
-    struct domain *, int seg, int bus, int devfn);
+struct pci_dev *pci_get_pdev_by_domain(const struct domain *,
+                                       int seg, int bus, int devfn);
 void pci_check_disable_device(u16 seg, u8 bus, u8 devfn);
 
 uint8_t pci_conf_read8(
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -337,10 +337,11 @@ static XSM_INLINE int xsm_get_device_gro
     return xsm_default_action(action, current->domain, NULL);
 }
 
-static XSM_INLINE int xsm_test_assign_device(XSM_DEFAULT_ARG uint32_t machine_bdf)
+static XSM_INLINE int xsm_test_assign_device(XSM_DEFAULT_ARG struct domain *d,
+                                             uint32_t machine_bdf)
 {
     XSM_ASSERT_ACTION(XSM_HOOK);
-    return xsm_default_action(action, current->domain, NULL);
+    return xsm_default_action(action, current->domain, d);
 }
 
 static XSM_INLINE int xsm_assign_device(XSM_DEFAULT_ARG struct domain *d, uint32_t machine_bdf)
@@ -358,10 +359,11 @@ static XSM_INLINE int xsm_deassign_devic
 #endif /* HAS_PASSTHROUGH && HAS_PCI */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE)
-static XSM_INLINE int xsm_test_assign_dtdevice(XSM_DEFAULT_ARG const char *dtpath)
+static XSM_INLINE int xsm_test_assign_dtdevice(XSM_DEFAULT_ARG struct domain *d,
+                                               const char *dtpath)
 {
     XSM_ASSERT_ACTION(XSM_HOOK);
-    return xsm_default_action(action, current->domain, NULL);
+    return xsm_default_action(action, current->domain, d);
 }
 
 static XSM_INLINE int xsm_assign_dtdevice(XSM_DEFAULT_ARG struct domain *d,
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -109,13 +109,13 @@ struct xsm_operations {
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
     int (*get_device_group) (uint32_t machine_bdf);
-    int (*test_assign_device) (uint32_t machine_bdf);
+    int (*test_assign_device) (struct domain *d, uint32_t machine_bdf);
     int (*assign_device) (struct domain *d, uint32_t machine_bdf);
     int (*deassign_device) (struct domain *d, uint32_t machine_bdf);
 #endif
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE)
-    int (*test_assign_dtdevice) (const char *dtpath);
+    int (*test_assign_dtdevice) (struct domain *d, const char *dtpath);
     int (*assign_dtdevice) (struct domain *d, const char *dtpath);
     int (*deassign_dtdevice) (struct domain *d, const char *dtpath);
 #endif
@@ -465,9 +465,9 @@ static inline int xsm_get_device_group(x
     return xsm_ops->get_device_group(machine_bdf);
 }
 
-static inline int xsm_test_assign_device(xsm_default_t def, uint32_t machine_bdf)
+static inline int xsm_test_assign_device(xsm_default_t def, struct domain *d, uint32_t machine_bdf)
 {
-    return xsm_ops->test_assign_device(machine_bdf);
+    return xsm_ops->test_assign_device(d, machine_bdf);
 }
 
 static inline int xsm_assign_device(xsm_default_t def, struct domain *d, uint32_t machine_bdf)
@@ -488,10 +488,10 @@ static inline int xsm_assign_dtdevice(xs
     return xsm_ops->assign_dtdevice(d, dtpath);
 }
 
-static inline int xsm_test_assign_dtdevice(xsm_default_t def,
+static inline int xsm_test_assign_dtdevice(xsm_default_t def, struct domain *d,
                                            const char *dtpath)
 {
-    return xsm_ops->test_assign_dtdevice(dtpath);
+    return xsm_ops->test_assign_dtdevice(d, dtpath);
 }
 
 static inline int xsm_deassign_dtdevice(xsm_default_t def, struct domain *d,
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1260,7 +1260,7 @@ static int flask_get_device_group(uint32
     return avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__STAT_DEVICE, NULL);
 }
 
-static int flask_test_assign_device(uint32_t machine_bdf)
+static int flask_test_assign_device(struct domain *d, uint32_t machine_bdf)
 {
     u32 rsid;
     int rc = -EPERM;
@@ -1314,7 +1314,7 @@ static int flask_deassign_device(struct
 #endif /* HAS_PASSTHROUGH && HAS_PCI */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE)
-static int flask_test_assign_dtdevice(const char *dtpath)
+static int flask_test_assign_dtdevice(struct domain *d, const char *dtpath)
 {
     u32 rsid;
     int rc = -EPERM;



[-- Attachment #2: domctl-test-assign-rework.patch --]
[-- Type: text/plain, Size: 11163 bytes --]

passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

So far callers of the libxc interface passed in a domain ID which was
then ignored in the hypervisor. Instead, make the hypervisor honor it
(accepting DOMID_INVALID to obtain original behavior), allowing to
query whether a device is assigned to a particular domain. Ignore the
passed in domain ID at the libxc layer instead, in order to not break
existing callers. New libxc functions would need to be added if callers
wanted to leverage the new functionality.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Would DOMID_IO be a better fit than DOMID_INVALID here?

--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1539,13 +1539,13 @@ int xc_get_device_group(
 
 int xc_test_assign_device(
     xc_interface *xch,
-    uint32_t domid,
+    uint32_t domid, /* ignored */
     uint32_t machine_sbdf)
 {
     DECLARE_DOMCTL;
 
     domctl.cmd = XEN_DOMCTL_test_assign_device;
-    domctl.domain = domid;
+    domctl.domain = DOMID_INVALID;
     domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
     domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
     domctl.u.assign_device.flags = 0;
@@ -1603,7 +1603,7 @@ int xc_assign_dt_device(
 
 int xc_test_assign_dt_device(
     xc_interface *xch,
-    uint32_t domid,
+    uint32_t domid, /* ignored */
     char *path)
 {
     int rc;
@@ -1615,7 +1615,7 @@ int xc_test_assign_dt_device(
         return -1;
 
     domctl.cmd = XEN_DOMCTL_test_assign_device;
-    domctl.domain = (domid_t)domid;
+    domctl.domain = DOMID_INVALID;
 
     domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
     domctl.u.assign_device.u.dt.size = size;
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -391,11 +391,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     switch ( op->cmd )
     {
-    case XEN_DOMCTL_createdomain:
     case XEN_DOMCTL_test_assign_device:
+        if ( op->domain == DOMID_INVALID )
+        {
+    case XEN_DOMCTL_createdomain:
     case XEN_DOMCTL_gdbsx_guestmemio:
-        d = NULL;
-        break;
+            d = NULL;
+            break;
+        }
+        /* fall through */
     default:
         d = rcu_lock_domain_by_id(op->domain);
         if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -93,7 +93,8 @@ fail:
     return rc;
 }
 
-static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
+static bool_t iommu_dt_device_is_assigned(const struct domain *d,
+                                          const struct dt_device_node *dev)
 {
     bool_t assigned = 0;
 
@@ -101,7 +102,8 @@ static bool_t iommu_dt_device_is_assigne
         return 0;
 
     spin_lock(&dtdevs_lock);
-    assigned = !list_empty(&dev->domain_list);
+    assigned = d ? dt_device_used_by(dev) == d->domain_id
+                 : !list_empty(&dev->domain_list);
     spin_unlock(&dtdevs_lock);
 
     return assigned;
@@ -209,11 +211,11 @@ int iommu_do_dt_domctl(struct xen_domctl
         if ( ret )
             break;
 
-        ret = xsm_test_assign_dtdevice(XSM_HOOK, dt_node_full_name(dev));
+        ret = xsm_test_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
         if ( ret )
             break;
 
-        if ( iommu_dt_device_is_assigned(dev) )
+        if ( iommu_dt_device_is_assigned(d, dev) )
         {
             printk(XENLOG_G_ERR "%s already assigned.\n",
                    dt_node_full_name(dev));
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -522,7 +522,7 @@ struct pci_dev *pci_get_real_pdev(int se
 }
 
 struct pci_dev *pci_get_pdev_by_domain(
-    struct domain *d, int seg, int bus, int devfn)
+    const struct domain *d, int seg, int bus, int devfn)
 {
     struct pci_seg *pseg = get_pseg(seg);
     struct pci_dev *pdev = NULL;
@@ -1337,12 +1337,12 @@ int iommu_remove_device(struct pci_dev *
  * 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)
+static int device_assigned(const struct domain *d, u16 seg, u8 bus, u8 devfn)
 {
-    struct pci_dev *pdev;
+    const struct pci_dev *pdev;
 
     pcidevs_lock();
-    pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
+    pdev = pci_get_pdev_by_domain(d ?: hardware_domain, seg, bus, devfn);
     pcidevs_unlock();
 
     return pdev ? 0 : -EBUSY;
@@ -1590,7 +1590,7 @@ int iommu_do_pci_domctl(
 
         machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
 
-        ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
+        ret = xsm_test_assign_device(XSM_HOOK, d, machine_sbdf);
         if ( ret )
             break;
 
@@ -1598,13 +1598,12 @@ int iommu_do_pci_domctl(
         bus = PCI_BUS(machine_sbdf);
         devfn = PCI_DEVFN2(machine_sbdf);
 
-        if ( device_assigned(seg, bus, devfn) )
-        {
+        ret = device_assigned(d, seg, bus, devfn);
+        if ( ret && !d )
             printk(XENLOG_G_INFO
                    "%04x:%02x:%02x.%u already assigned, or non-existent\n",
                    seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
-            ret = -EINVAL;
-        }
+
         break;
 
     case XEN_DOMCTL_assign_device:
@@ -1634,7 +1633,7 @@ int iommu_do_pci_domctl(
         bus = PCI_BUS(machine_sbdf);
         devfn = PCI_DEVFN2(machine_sbdf);
 
-        ret = device_assigned(seg, bus, devfn) ?:
+        ret = device_assigned(NULL, seg, bus, devfn) ?:
               assign_device(d, seg, bus, devfn, flags);
         if ( ret == -ERESTART )
             ret = hypercall_create_continuation(__HYPERVISOR_domctl,
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -506,7 +506,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendt
 
 /* Assign a device to a guest. Sets up IOMMU structures. */
 /* XEN_DOMCTL_assign_device */
-/* XEN_DOMCTL_test_assign_device */
+/*
+ * XEN_DOMCTL_test_assign_device: Pass DOMID_INVALID to find out whether the
+ * given device is assigned to a DomU at all. Pass a specific domain ID to
+ * find out whether the given device is assigned to that domain.
+ */
 /*
  * XEN_DOMCTL_deassign_device: The behavior of this DOMCTL differs
  * between the different type of device:
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -125,8 +125,8 @@ int pci_ro_device(int seg, int bus, int
 int pci_hide_device(int bus, int devfn);
 struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
 struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn);
-struct pci_dev *pci_get_pdev_by_domain(
-    struct domain *, int seg, int bus, int devfn);
+struct pci_dev *pci_get_pdev_by_domain(const struct domain *,
+                                       int seg, int bus, int devfn);
 void pci_check_disable_device(u16 seg, u8 bus, u8 devfn);
 
 uint8_t pci_conf_read8(
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -337,10 +337,11 @@ static XSM_INLINE int xsm_get_device_gro
     return xsm_default_action(action, current->domain, NULL);
 }
 
-static XSM_INLINE int xsm_test_assign_device(XSM_DEFAULT_ARG uint32_t machine_bdf)
+static XSM_INLINE int xsm_test_assign_device(XSM_DEFAULT_ARG struct domain *d,
+                                             uint32_t machine_bdf)
 {
     XSM_ASSERT_ACTION(XSM_HOOK);
-    return xsm_default_action(action, current->domain, NULL);
+    return xsm_default_action(action, current->domain, d);
 }
 
 static XSM_INLINE int xsm_assign_device(XSM_DEFAULT_ARG struct domain *d, uint32_t machine_bdf)
@@ -358,10 +359,11 @@ static XSM_INLINE int xsm_deassign_devic
 #endif /* HAS_PASSTHROUGH && HAS_PCI */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE)
-static XSM_INLINE int xsm_test_assign_dtdevice(XSM_DEFAULT_ARG const char *dtpath)
+static XSM_INLINE int xsm_test_assign_dtdevice(XSM_DEFAULT_ARG struct domain *d,
+                                               const char *dtpath)
 {
     XSM_ASSERT_ACTION(XSM_HOOK);
-    return xsm_default_action(action, current->domain, NULL);
+    return xsm_default_action(action, current->domain, d);
 }
 
 static XSM_INLINE int xsm_assign_dtdevice(XSM_DEFAULT_ARG struct domain *d,
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -109,13 +109,13 @@ struct xsm_operations {
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
     int (*get_device_group) (uint32_t machine_bdf);
-    int (*test_assign_device) (uint32_t machine_bdf);
+    int (*test_assign_device) (struct domain *d, uint32_t machine_bdf);
     int (*assign_device) (struct domain *d, uint32_t machine_bdf);
     int (*deassign_device) (struct domain *d, uint32_t machine_bdf);
 #endif
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE)
-    int (*test_assign_dtdevice) (const char *dtpath);
+    int (*test_assign_dtdevice) (struct domain *d, const char *dtpath);
     int (*assign_dtdevice) (struct domain *d, const char *dtpath);
     int (*deassign_dtdevice) (struct domain *d, const char *dtpath);
 #endif
@@ -465,9 +465,9 @@ static inline int xsm_get_device_group(x
     return xsm_ops->get_device_group(machine_bdf);
 }
 
-static inline int xsm_test_assign_device(xsm_default_t def, uint32_t machine_bdf)
+static inline int xsm_test_assign_device(xsm_default_t def, struct domain *d, uint32_t machine_bdf)
 {
-    return xsm_ops->test_assign_device(machine_bdf);
+    return xsm_ops->test_assign_device(d, machine_bdf);
 }
 
 static inline int xsm_assign_device(xsm_default_t def, struct domain *d, uint32_t machine_bdf)
@@ -488,10 +488,10 @@ static inline int xsm_assign_dtdevice(xs
     return xsm_ops->assign_dtdevice(d, dtpath);
 }
 
-static inline int xsm_test_assign_dtdevice(xsm_default_t def,
+static inline int xsm_test_assign_dtdevice(xsm_default_t def, struct domain *d,
                                            const char *dtpath)
 {
-    return xsm_ops->test_assign_dtdevice(dtpath);
+    return xsm_ops->test_assign_dtdevice(d, dtpath);
 }
 
 static inline int xsm_deassign_dtdevice(xsm_default_t def, struct domain *d,
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1260,7 +1260,7 @@ static int flask_get_device_group(uint32
     return avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__STAT_DEVICE, NULL);
 }
 
-static int flask_test_assign_device(uint32_t machine_bdf)
+static int flask_test_assign_device(struct domain *d, uint32_t machine_bdf)
 {
     u32 rsid;
     int rc = -EPERM;
@@ -1314,7 +1314,7 @@ static int flask_deassign_device(struct
 #endif /* HAS_PASSTHROUGH && HAS_PCI */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE)
-static int flask_test_assign_dtdevice(const char *dtpath)
+static int flask_test_assign_dtdevice(struct domain *d, const char *dtpath)
 {
     u32 rsid;
     int rc = -EPERM;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 21/06/17 11:08, Jan Beulich wrote:
> So far callers of the libxc interface passed in a domain ID which was
> then ignored in the hypervisor. Instead, make the hypervisor honor it
> (accepting DOMID_INVALID to obtain original behavior), allowing to
> query whether a device is assigned to a particular domain. Ignore the
> passed in domain ID at the libxc layer instead, in order to not break
> existing callers. New libxc functions would need to be added if callers
> wanted to leverage the new functionality.

I don't think your modified description matches the name of the call at all.

It looks like the callers expect "test_assign_device" to answer the
question: "Can I assign a device to this domain"?

It looks like it's meant to be used in XSM environments, to allow a
policy to permit or forbid specific guests to have access to specific
devices.  On a default (non-XSM) system, the answer to that question
doesn't depend on the domain it's being assigned to, but only whether
the device is already assigned to another domain; but on XSM systems the
logic can presumably be more complicated.

That sounds like a perfectly sane semantic to me, and this patch removes
that ability.

I think if you want a hypercall that answers the question, "Is device X
assigned to domain Y", I think you should add a new one with a name more
like, "test_device_is_assigned_to" or something.

 -George


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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

>>> On 21.06.17 at 16:38, <george.dunlap@citrix.com> wrote:
> On 21/06/17 11:08, Jan Beulich wrote:
>> So far callers of the libxc interface passed in a domain ID which was
>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>> query whether a device is assigned to a particular domain. Ignore the
>> passed in domain ID at the libxc layer instead, in order to not break
>> existing callers. New libxc functions would need to be added if callers
>> wanted to leverage the new functionality.
> 
> I don't think your modified description matches the name of the call at all.
> 
> It looks like the callers expect "test_assign_device" to answer the
> question: "Can I assign a device to this domain"?

I don't think so - the question being answered by the original
operation is "Is this device assigned to any domain?" with the
implied inverse "Is this device available to be assigned to some
domain (i.e. it is currently unassigned or owned by Dom0)?"

> It looks like it's meant to be used in XSM environments, to allow a
> policy to permit or forbid specific guests to have access to specific
> devices.  On a default (non-XSM) system, the answer to that question
> doesn't depend on the domain it's being assigned to, but only whether
> the device is already assigned to another domain; but on XSM systems the
> logic can presumably be more complicated.
> 
> That sounds like a perfectly sane semantic to me, and this patch removes
> that ability.

And again I don't think so: Prior to the patch, do_domctl() at its
very top makes sure to entirely ignore the passed in domain ID.
This code sits ahead of the XSM check, so XSM has no way of
knowing which domain has been specified by the caller.

Jan


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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 21/06/17 16:59, Jan Beulich wrote:
>>>> On 21.06.17 at 16:38, <george.dunlap@citrix.com> wrote:
>> On 21/06/17 11:08, Jan Beulich wrote:
>>> So far callers of the libxc interface passed in a domain ID which was
>>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>>> query whether a device is assigned to a particular domain. Ignore the
>>> passed in domain ID at the libxc layer instead, in order to not break
>>> existing callers. New libxc functions would need to be added if callers
>>> wanted to leverage the new functionality.
>>
>> I don't think your modified description matches the name of the call at all.
>>
>> It looks like the callers expect "test_assign_device" to answer the
>> question: "Can I assign a device to this domain"?
> 
> I don't think so - the question being answered by the original
> operation is "Is this device assigned to any domain?" with the
> implied inverse "Is this device available to be assigned to some
> domain (i.e. it is currently unassigned or owned by Dom0)?"

If the question were "Is this device assigned to any domain?", then I
would expect:
1. The return value to be a boolean
2. It would always return, "No it's not assigned" in the case where
there is no IOMMU.

However, that's not what happens:
1. It returns "success" if there is an IOMMU and the device is *not*
assigned, and returns an error if the device is assigned
2. It returns an error if there is no IOMMU.

The only place in the code this is called 'for real' in the tree is in
libxl_pci.c:libxl__device_pci_add()

    if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
        rc = xc_test_assign_device(ctx->xch, domid,
pcidev_encode_bdf(pcidev));
        if (rc) {
            LOGD(ERROR, domid,
                 "PCI device %04x:%02x:%02x.%u %s?",
                 pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
                 errno == ENOSYS ? "cannot be assigned - no IOMMU"
                 : "already assigned to a different guest");
            goto out;
        }
    }

Here 'domid' is the domain to which libxl wants to assign the device.
So libxl is now asking Xen, "Am I allowed to assign device $bdf to
domain $domain?"

Your description provides the *algorithm* by which Xen normally provides
an answer: that is, normally the only thing Xen cares about is that it
hasn't already been assigned to a domain.  But it still remains the case
that what libxl is asking is, "Can I assign X to Y?"

>> It looks like it's meant to be used in XSM environments, to allow a
>> policy to permit or forbid specific guests to have access to specific
>> devices.  On a default (non-XSM) system, the answer to that question
>> doesn't depend on the domain it's being assigned to, but only whether
>> the device is already assigned to another domain; but on XSM systems the
>> logic can presumably be more complicated.
>>
>> That sounds like a perfectly sane semantic to me, and this patch removes
>> that ability.
> 
> And again I don't think so: Prior to the patch, do_domctl() at its
> very top makes sure to entirely ignore the passed in domain ID.
> This code sits ahead of the XSM check, so XSM has no way of
> knowing which domain has been specified by the caller.

Right, I see that now.

Still, I assert that the original hypercall semantics is a very useful
one, and what you're doing is changing the hypercall such that the
question can no longer be asked.  It would be better to extend things so
that XSM can actually deny device assignment based on both the bdf and
the domain.

Do you have a particular use case in mind for your alternate hypercall?

 -George


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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

>>> On 21.06.17 at 18:36, <george.dunlap@citrix.com> wrote:
> On 21/06/17 16:59, Jan Beulich wrote:
>>>>> On 21.06.17 at 16:38, <george.dunlap@citrix.com> wrote:
>>> On 21/06/17 11:08, Jan Beulich wrote:
>>>> So far callers of the libxc interface passed in a domain ID which was
>>>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>>>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>>>> query whether a device is assigned to a particular domain. Ignore the
>>>> passed in domain ID at the libxc layer instead, in order to not break
>>>> existing callers. New libxc functions would need to be added if callers
>>>> wanted to leverage the new functionality.
>>>
>>> I don't think your modified description matches the name of the call at all.
>>>
>>> It looks like the callers expect "test_assign_device" to answer the
>>> question: "Can I assign a device to this domain"?
>> 
>> I don't think so - the question being answered by the original
>> operation is "Is this device assigned to any domain?" with the
>> implied inverse "Is this device available to be assigned to some
>> domain (i.e. it is currently unassigned or owned by Dom0)?"
> 
> If the question were "Is this device assigned to any domain?", then I
> would expect:
> 1. The return value to be a boolean
> 2. It would always return, "No it's not assigned" in the case where
> there is no IOMMU.
> 
> However, that's not what happens:
> 1. It returns "success" if there is an IOMMU and the device is *not*
> assigned, and returns an error if the device is assigned
> 2. It returns an error if there is no IOMMU.
> 
> The only place in the code this is called 'for real' in the tree is in
> libxl_pci.c:libxl__device_pci_add()
> 
>     if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
>         rc = xc_test_assign_device(ctx->xch, domid,
> pcidev_encode_bdf(pcidev));
>         if (rc) {
>             LOGD(ERROR, domid,
>                  "PCI device %04x:%02x:%02x.%u %s?",
>                  pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
>                  errno == ENOSYS ? "cannot be assigned - no IOMMU"
>                  : "already assigned to a different guest");
>             goto out;
>         }
>     }
> 
> Here 'domid' is the domain to which libxl wants to assign the device.
> So libxl is now asking Xen, "Am I allowed to assign device $bdf to
> domain $domain?"
> 
> Your description provides the *algorithm* by which Xen normally provides
> an answer: that is, normally the only thing Xen cares about is that it
> hasn't already been assigned to a domain.  But it still remains the case
> that what libxl is asking is, "Can I assign X to Y?"

Taking the log message into account that you quote, I do not
view the code's intention to be what you describe.

>>> It looks like it's meant to be used in XSM environments, to allow a
>>> policy to permit or forbid specific guests to have access to specific
>>> devices.  On a default (non-XSM) system, the answer to that question
>>> doesn't depend on the domain it's being assigned to, but only whether
>>> the device is already assigned to another domain; but on XSM systems the
>>> logic can presumably be more complicated.
>>>
>>> That sounds like a perfectly sane semantic to me, and this patch removes
>>> that ability.
>> 
>> And again I don't think so: Prior to the patch, do_domctl() at its
>> very top makes sure to entirely ignore the passed in domain ID.
>> This code sits ahead of the XSM check, so XSM has no way of
>> knowing which domain has been specified by the caller.
> 
> Right, I see that now.
> 
> Still, I assert that the original hypercall semantics is a very useful
> one, and what you're doing is changing the hypercall such that the
> question can no longer be asked.  It would be better to extend things so
> that XSM can actually deny device assignment based on both the bdf and
> the domain.
> 
> Do you have a particular use case in mind for your alternate hypercall?

No - I'm open to any change to it which makes the currently ignored
argument no longer ignored, without breaking existing (known and
unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
make it work the way you think it was originally meant to work; it is
just that given its current use I've come to a different conclusion as
to what the original intention may have been.

Jan

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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 22/06/17 08:05, Jan Beulich wrote:
>>>> On 21.06.17 at 18:36, <george.dunlap@citrix.com> wrote:
>> On 21/06/17 16:59, Jan Beulich wrote:
>>>>>> On 21.06.17 at 16:38, <george.dunlap@citrix.com> wrote:
>>>> On 21/06/17 11:08, Jan Beulich wrote:
>>>>> So far callers of the libxc interface passed in a domain ID which was
>>>>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>>>>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>>>>> query whether a device is assigned to a particular domain. Ignore the
>>>>> passed in domain ID at the libxc layer instead, in order to not break
>>>>> existing callers. New libxc functions would need to be added if callers
>>>>> wanted to leverage the new functionality.
>>>>
>>>> I don't think your modified description matches the name of the call at all.
>>>>
>>>> It looks like the callers expect "test_assign_device" to answer the
>>>> question: "Can I assign a device to this domain"?
>>>
>>> I don't think so - the question being answered by the original
>>> operation is "Is this device assigned to any domain?" with the
>>> implied inverse "Is this device available to be assigned to some
>>> domain (i.e. it is currently unassigned or owned by Dom0)?"
>>
>> If the question were "Is this device assigned to any domain?", then I
>> would expect:
>> 1. The return value to be a boolean
>> 2. It would always return, "No it's not assigned" in the case where
>> there is no IOMMU.
>>
>> However, that's not what happens:
>> 1. It returns "success" if there is an IOMMU and the device is *not*
>> assigned, and returns an error if the device is assigned
>> 2. It returns an error if there is no IOMMU.
>>
>> The only place in the code this is called 'for real' in the tree is in
>> libxl_pci.c:libxl__device_pci_add()
>>
>>     if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
>>         rc = xc_test_assign_device(ctx->xch, domid,
>> pcidev_encode_bdf(pcidev));
>>         if (rc) {
>>             LOGD(ERROR, domid,
>>                  "PCI device %04x:%02x:%02x.%u %s?",
>>                  pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
>>                  errno == ENOSYS ? "cannot be assigned - no IOMMU"
>>                  : "already assigned to a different guest");
>>             goto out;
>>         }
>>     }
>>
>> Here 'domid' is the domain to which libxl wants to assign the device.
>> So libxl is now asking Xen, "Am I allowed to assign device $bdf to
>> domain $domain?"
>>
>> Your description provides the *algorithm* by which Xen normally provides
>> an answer: that is, normally the only thing Xen cares about is that it
>> hasn't already been assigned to a domain.  But it still remains the case
>> that what libxl is asking is, "Can I assign X to Y?"
> 
> Taking the log message into account that you quote, I do not
> view the code's intention to be what you describe.

Well, I'm not sure what to say, because in my view the log message
supports my view. :-)  Note that there are two errors, both explaining
why the domain cannot be assigned -- one is "no IOMMU", one is "already
assigned to a different guest".

Yes, at the moment it doesn't have a separate message for -EPERM (which
is presumably what XSM would return if there were some other problem).
But it also doesn't correctly report other potential errors: -ENODEV if
you try to assign a DT device on a PCI-based system, or a PCI device on
a DT-based system.  (Apparently we also retirn -EINVAL if you included
inappropriate flags, *or* if the device didn't exist, *or* if the device
was already assigned somehwere else.  As long as we're re-painting
things we should probably change this as well.)

But to make test_assign_device answer the question, "Is this assigned to
a domU?", you'd have to have it return SUCCESS when there is no IOMMU
(since the device is not, in fact, assigned to a domU); and thus libxl
would have to make a separate call to find out if an IOMMU was present.

>>>> It looks like it's meant to be used in XSM environments, to allow a
>>>> policy to permit or forbid specific guests to have access to specific
>>>> devices.  On a default (non-XSM) system, the answer to that question
>>>> doesn't depend on the domain it's being assigned to, but only whether
>>>> the device is already assigned to another domain; but on XSM systems the
>>>> logic can presumably be more complicated.
>>>>
>>>> That sounds like a perfectly sane semantic to me, and this patch removes
>>>> that ability.
>>>
>>> And again I don't think so: Prior to the patch, do_domctl() at its
>>> very top makes sure to entirely ignore the passed in domain ID.
>>> This code sits ahead of the XSM check, so XSM has no way of
>>> knowing which domain has been specified by the caller.
>>
>> Right, I see that now.
>>
>> Still, I assert that the original hypercall semantics is a very useful
>> one, and what you're doing is changing the hypercall such that the
>> question can no longer be asked.  It would be better to extend things so
>> that XSM can actually deny device assignment based on both the bdf and
>> the domain.
>>
>> Do you have a particular use case in mind for your alternate hypercall?
> 
> No - I'm open to any change to it which makes the currently ignored
> argument no longer ignored, without breaking existing (known and
> unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
> make it work the way you think it was originally meant to work; it is
> just that given its current use I've come to a different conclusion as
> to what the original intention may have been.

So the libxc library interface is not meant to be stable.  Before I
looked at how libxl was using it, I was going to say that we should just
remove the domid argument from that function entirely, rather than
labelling it "unused".

I suggest we ask the toolstack maintainers what kind of a function they
think would be most useful, and then we can implement that.

So, Wei and Ian (and Daniel if you're around):

At the moment, xc_test_assign_device() accepts both a domid and a device
identifier.  It will return -ENOSYS if there is no IOMMU, and

The domid, however, is ignored by Xen, and there is no possibility even
of XSM/Flask paying any attention to it because the domid is not passed
to the XSM hook.

Jan thinks the color of this shed is ugly and wants to repaint it.  I
agree that the color of this shed is ugly, but think a different color
would be more suitable.  Since the function is ultimately mainly for the
benefit of toolstacks like libxl, we'd like your input:

Option 1: Make xc_test_assign_device() explicitly take only a device
identifier.  Add another xc function which takes a domain argument,
which will return true if the BDF is assigned to that particular domain,
and false otherwise.
  1a: Leave the domid argument, but add a comment specifying it as ignored
  1b: Remove the domid argument.

Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
policies that can forbid specific devices from being assigned to
specific guests.

NB that neither of us has a particular requirement for the proposed
additional functionality ("Device X is assigned to domid Y" in the case
of Option 1, or "Flask policy can allow or forbid specific devices to
specific domains" in the case of Option 2).

Any preferences?

 -George

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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 22/06/17 10:40, George Dunlap wrote:
> On 22/06/17 08:05, Jan Beulich wrote:
>>>>> On 21.06.17 at 18:36, <george.dunlap@citrix.com> wrote:
>>> On 21/06/17 16:59, Jan Beulich wrote:
>>>>>>> On 21.06.17 at 16:38, <george.dunlap@citrix.com> wrote:
>>>>> On 21/06/17 11:08, Jan Beulich wrote:
>>>>>> So far callers of the libxc interface passed in a domain ID which was
>>>>>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>>>>>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>>>>>> query whether a device is assigned to a particular domain. Ignore the
>>>>>> passed in domain ID at the libxc layer instead, in order to not break
>>>>>> existing callers. New libxc functions would need to be added if callers
>>>>>> wanted to leverage the new functionality.
>>>>>
>>>>> I don't think your modified description matches the name of the call at all.
>>>>>
>>>>> It looks like the callers expect "test_assign_device" to answer the
>>>>> question: "Can I assign a device to this domain"?
>>>>
>>>> I don't think so - the question being answered by the original
>>>> operation is "Is this device assigned to any domain?" with the
>>>> implied inverse "Is this device available to be assigned to some
>>>> domain (i.e. it is currently unassigned or owned by Dom0)?"
>>>
>>> If the question were "Is this device assigned to any domain?", then I
>>> would expect:
>>> 1. The return value to be a boolean
>>> 2. It would always return, "No it's not assigned" in the case where
>>> there is no IOMMU.
>>>
>>> However, that's not what happens:
>>> 1. It returns "success" if there is an IOMMU and the device is *not*
>>> assigned, and returns an error if the device is assigned
>>> 2. It returns an error if there is no IOMMU.
>>>
>>> The only place in the code this is called 'for real' in the tree is in
>>> libxl_pci.c:libxl__device_pci_add()
>>>
>>>     if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
>>>         rc = xc_test_assign_device(ctx->xch, domid,
>>> pcidev_encode_bdf(pcidev));
>>>         if (rc) {
>>>             LOGD(ERROR, domid,
>>>                  "PCI device %04x:%02x:%02x.%u %s?",
>>>                  pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
>>>                  errno == ENOSYS ? "cannot be assigned - no IOMMU"
>>>                  : "already assigned to a different guest");
>>>             goto out;
>>>         }
>>>     }
>>>
>>> Here 'domid' is the domain to which libxl wants to assign the device.
>>> So libxl is now asking Xen, "Am I allowed to assign device $bdf to
>>> domain $domain?"
>>>
>>> Your description provides the *algorithm* by which Xen normally provides
>>> an answer: that is, normally the only thing Xen cares about is that it
>>> hasn't already been assigned to a domain.  But it still remains the case
>>> that what libxl is asking is, "Can I assign X to Y?"
>>
>> Taking the log message into account that you quote, I do not
>> view the code's intention to be what you describe.
> 
> Well, I'm not sure what to say, because in my view the log message
> supports my view. :-)  Note that there are two errors, both explaining
> why the domain cannot be assigned -- one is "no IOMMU", one is "already
> assigned to a different guest".
> 
> Yes, at the moment it doesn't have a separate message for -EPERM (which
> is presumably what XSM would return if there were some other problem).
> But it also doesn't correctly report other potential errors: -ENODEV if
> you try to assign a DT device on a PCI-based system, or a PCI device on
> a DT-based system.  (Apparently we also retirn -EINVAL if you included
> inappropriate flags, *or* if the device didn't exist, *or* if the device
> was already assigned somehwere else.  As long as we're re-painting
> things we should probably change this as well.)
> 
> But to make test_assign_device answer the question, "Is this assigned to
> a domU?", you'd have to have it return SUCCESS when there is no IOMMU
> (since the device is not, in fact, assigned to a domU); and thus libxl
> would have to make a separate call to find out if an IOMMU was present.
> 
>>>>> It looks like it's meant to be used in XSM environments, to allow a
>>>>> policy to permit or forbid specific guests to have access to specific
>>>>> devices.  On a default (non-XSM) system, the answer to that question
>>>>> doesn't depend on the domain it's being assigned to, but only whether
>>>>> the device is already assigned to another domain; but on XSM systems the
>>>>> logic can presumably be more complicated.
>>>>>
>>>>> That sounds like a perfectly sane semantic to me, and this patch removes
>>>>> that ability.
>>>>
>>>> And again I don't think so: Prior to the patch, do_domctl() at its
>>>> very top makes sure to entirely ignore the passed in domain ID.
>>>> This code sits ahead of the XSM check, so XSM has no way of
>>>> knowing which domain has been specified by the caller.
>>>
>>> Right, I see that now.
>>>
>>> Still, I assert that the original hypercall semantics is a very useful
>>> one, and what you're doing is changing the hypercall such that the
>>> question can no longer be asked.  It would be better to extend things so
>>> that XSM can actually deny device assignment based on both the bdf and
>>> the domain.
>>>
>>> Do you have a particular use case in mind for your alternate hypercall?
>>
>> No - I'm open to any change to it which makes the currently ignored
>> argument no longer ignored, without breaking existing (known and
>> unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
>> make it work the way you think it was originally meant to work; it is
>> just that given its current use I've come to a different conclusion as
>> to what the original intention may have been.
> 
> So the libxc library interface is not meant to be stable.  Before I
> looked at how libxl was using it, I was going to say that we should just
> remove the domid argument from that function entirely, rather than
> labelling it "unused".
> 
> I suggest we ask the toolstack maintainers what kind of a function they
> think would be most useful, and then we can implement that.
> 
> So, Wei and Ian (and Daniel if you're around):
> 
> At the moment, xc_test_assign_device() accepts both a domid and a device
> identifier.  It will return -ENOSYS if there is no IOMMU, and

Sorry, didn't finish this:  -ENODEV if it's the wrong bus type (DT or
PCI), -EINVAL if there are any other problems assigning the device
(device doesn't exist, or device already assigned to another domain),
and 0 if everything is OK.

 -George


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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 22/06/17 08:05, Jan Beulich wrote:
> No - I'm open to any change to it which makes the currently ignored
> argument no longer ignored, without breaking existing (known and
> unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
> make it work the way you think it was originally meant to work; it is
> just that given its current use I've come to a different conclusion as
> to what the original intention may have been.

Actually, I think the clincher is this:

test_assign_device, assign_device, and deassign_device all use the same
structure.

That makes it pretty obvious that "test_assign_device" was meant to ask
the question, "If I call this hypercall with assign_device instead, will
it succeed or fail?"

 -George

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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Ian Jackson]

George Dunlap writes ("Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics"):
> Well, I'm not sure what to say, because in my view the log message
> supports my view. :-)  Note that there are two errors, both explaining
> why the domain cannot be assigned -- one is "no IOMMU", one is "already
> assigned to a different guest".
> Yes, at the moment it doesn't have a separate message for -EPERM (which
> is presumably what XSM would return if there were some other problem).
> But it also doesn't correctly report other potential errors: -ENODEV if
> you try to assign a DT device on a PCI-based system, or a PCI device on
> a DT-based system.

The way I would put this is: The log message generation logic is
wrong, in that it sometimes lies.

>  (Apparently we also retirn -EINVAL if you included
> inappropriate flags, *or* if the device didn't exist, *or* if the device
> was already assigned somehwere else.  As long as we're re-painting
> things we should probably change this as well.)

This is quite unhelpful, indeed.

> I suggest we ask the toolstack maintainers what kind of a function they
> think would be most useful, and then we can implement that.
> 
> So, Wei and Ian (and Daniel if you're around):

After having reread the thread I still don't understand why Jan thinks
the ignored argument is a problem per so.  Ignored arguments are often
provided to ease future expansion (whether there is an ABI stability
guarantee or not).

In this case I think that the domid is not passed to the XSM check is
simply a bug.  I don't know if that can be fixed easily.

> Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
> policies that can forbid specific devices from being assigned to
> specific guests.

Is there any possible downside to this ?

> Any preferences?

See above.  George's arguments make much more sense to me than Jan's,
in this thread.

Ian.

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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

>>> On 22.06.17 at 11:56, <george.dunlap@citrix.com> wrote:
> On 22/06/17 08:05, Jan Beulich wrote:
>> No - I'm open to any change to it which makes the currently ignored
>> argument no longer ignored, without breaking existing (known and
>> unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
>> make it work the way you think it was originally meant to work; it is
>> just that given its current use I've come to a different conclusion as
>> to what the original intention may have been.
> 
> Actually, I think the clincher is this:
> 
> test_assign_device, assign_device, and deassign_device all use the same
> structure.
> 
> That makes it pretty obvious that "test_assign_device" was meant to ask
> the question, "If I call this hypercall with assign_device instead, will
> it succeed or fail?"

That's again one possible interpretation, yes, but that would again
collide with the domain left unused in the present implementation.
Anyway, let's see what others would prefer (and in the worst case
we can leave it in the strange state it's in now).

Jan


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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

>>> On 22.06.17 at 11:58, <ian.jackson@eu.citrix.com> wrote:
> George Dunlap writes ("Re: [PATCH] passthrough: give 
> XEN_DOMCTL_test_assign_device more sane semantics"):
>> I suggest we ask the toolstack maintainers what kind of a function they
>> think would be most useful, and then we can implement that.
>> 
>> So, Wei and Ian (and Daniel if you're around):
> 
> After having reread the thread I still don't understand why Jan thinks
> the ignored argument is a problem per so.  Ignored arguments are often
> provided to ease future expansion (whether there is an ABI stability
> guarantee or not).
> 
> In this case I think that the domid is not passed to the XSM check is
> simply a bug.  I don't know if that can be fixed easily.

Well, the patch does that already, just that now the argument is
being ignored in flask. I'd leave that to someone else (Daniel?) to
implement.

>> Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
>> policies that can forbid specific devices from being assigned to
>> specific guests.
> 
> Is there any possible downside to this ?

As soon as flask wouldn't ignore it anymore, there would be the risk
of things currently working with a given XSM policy to stop working.
Much depends on how consistent the test-assign and assign checks
are (going to be) performed.

>> Any preferences?
> 
> See above.  George's arguments make much more sense to me than Jan's,
> in this thread.

Fine with me. Now as well as on past instances of looking at this,
it simply didn't occur to me that the operation could be intended
to work in the way George described. And in the end the patch
will end up smaller with that alternative model. One last question
then is whether retaining the original semantics with some special
domain ID (DOMID_INVALID at present) is of any use.

Jan


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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 22/06/17 11:58, Jan Beulich wrote:
>>> Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
>>> policies that can forbid specific devices from being assigned to
>>> specific guests.
>>
>> Is there any possible downside to this ?
> 
> As soon as flask wouldn't ignore it anymore, there would be the risk
> of things currently working with a given XSM policy to stop working.
> Much depends on how consistent the test-assign and assign checks
> are (going to be) performed.
> 
>>> Any preferences?
>>
>> See above.  George's arguments make much more sense to me than Jan's,
>> in this thread.
> 
> Fine with me. Now as well as on past instances of looking at this,
> it simply didn't occur to me that the operation could be intended
> to work in the way George described. And in the end the patch
> will end up smaller with that alternative model. One last question
> then is whether retaining the original semantics with some special
> domain ID (DOMID_INVALID at present) is of any use.

I think test_assign_device should behave exactly as assign_device,
except that on success the device is not actually assigned.

The commit that introduced test_assign_device btw is 239ad70, "vt-d:
Test device assignability in xend, but defer actual assignment to
qemu-dm."  The obvious purpose was to be able to fail early if there was
going to be a failure.

In fact, there's probably an argument that the "test" functionality
should have been implemented by adding an extra flag, rather than an
extra hypercall.  Another approach would actually be to have the
test_assign_device and assign_device take exactly the same codepath,
except not finally do the assignment.  That would avoid any risk that
the two paths might get out of sync.

 -George

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

Re: [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Daniel De Graaf]

On 06/22/2017 05:40 AM, George Dunlap wrote:
> On 22/06/17 08:05, Jan Beulich wrote:
>>>>> On 21.06.17 at 18:36, <george.dunlap@citrix.com> wrote:
>>> On 21/06/17 16:59, Jan Beulich wrote:
>>>>>>> On 21.06.17 at 16:38, <george.dunlap@citrix.com> wrote:
>>>>> On 21/06/17 11:08, Jan Beulich wrote:
>>>>>> So far callers of the libxc interface passed in a domain ID which was
>>>>>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>>>>>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>>>>>> query whether a device is assigned to a particular domain. Ignore the
>>>>>> passed in domain ID at the libxc layer instead, in order to not break
>>>>>> existing callers. New libxc functions would need to be added if callers
>>>>>> wanted to leverage the new functionality.
>>>>>
>>>>> I don't think your modified description matches the name of the call at all.
>>>>>
>>>>> It looks like the callers expect "test_assign_device" to answer the
>>>>> question: "Can I assign a device to this domain"?
>>>>
>>>> I don't think so - the question being answered by the original
>>>> operation is "Is this device assigned to any domain?" with the
>>>> implied inverse "Is this device available to be assigned to some
>>>> domain (i.e. it is currently unassigned or owned by Dom0)?"
>>>
>>> If the question were "Is this device assigned to any domain?", then I
>>> would expect:
>>> 1. The return value to be a boolean
>>> 2. It would always return, "No it's not assigned" in the case where
>>> there is no IOMMU.
>>>
>>> However, that's not what happens:
>>> 1. It returns "success" if there is an IOMMU and the device is *not*
>>> assigned, and returns an error if the device is assigned
>>> 2. It returns an error if there is no IOMMU.
>>>
>>> The only place in the code this is called 'for real' in the tree is in
>>> libxl_pci.c:libxl__device_pci_add()
>>>
>>>      if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
>>>          rc = xc_test_assign_device(ctx->xch, domid,
>>> pcidev_encode_bdf(pcidev));
>>>          if (rc) {
>>>              LOGD(ERROR, domid,
>>>                   "PCI device %04x:%02x:%02x.%u %s?",
>>>                   pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
>>>                   errno == ENOSYS ? "cannot be assigned - no IOMMU"
>>>                   : "already assigned to a different guest");
>>>              goto out;
>>>          }
>>>      }
>>>
>>> Here 'domid' is the domain to which libxl wants to assign the device.
>>> So libxl is now asking Xen, "Am I allowed to assign device $bdf to
>>> domain $domain?"
>>>
>>> Your description provides the *algorithm* by which Xen normally provides
>>> an answer: that is, normally the only thing Xen cares about is that it
>>> hasn't already been assigned to a domain.  But it still remains the case
>>> that what libxl is asking is, "Can I assign X to Y?"
>>
>> Taking the log message into account that you quote, I do not
>> view the code's intention to be what you describe.
> 
> Well, I'm not sure what to say, because in my view the log message
> supports my view. :-)  Note that there are two errors, both explaining
> why the domain cannot be assigned -- one is "no IOMMU", one is "already
> assigned to a different guest".
> 
> Yes, at the moment it doesn't have a separate message for -EPERM (which
> is presumably what XSM would return if there were some other problem).
> But it also doesn't correctly report other potential errors: -ENODEV if
> you try to assign a DT device on a PCI-based system, or a PCI device on
> a DT-based system.  (Apparently we also retirn -EINVAL if you included
> inappropriate flags, *or* if the device didn't exist, *or* if the device
> was already assigned somehwere else.  As long as we're re-painting
> things we should probably change this as well.)
> 
> But to make test_assign_device answer the question, "Is this assigned to
> a domU?", you'd have to have it return SUCCESS when there is no IOMMU
> (since the device is not, in fact, assigned to a domU); and thus libxl
> would have to make a separate call to find out if an IOMMU was present.
> 
>>>>> It looks like it's meant to be used in XSM environments, to allow a
>>>>> policy to permit or forbid specific guests to have access to specific
>>>>> devices.  On a default (non-XSM) system, the answer to that question
>>>>> doesn't depend on the domain it's being assigned to, but only whether
>>>>> the device is already assigned to another domain; but on XSM systems the
>>>>> logic can presumably be more complicated.
>>>>>
>>>>> That sounds like a perfectly sane semantic to me, and this patch removes
>>>>> that ability.
>>>>
>>>> And again I don't think so: Prior to the patch, do_domctl() at its
>>>> very top makes sure to entirely ignore the passed in domain ID.
>>>> This code sits ahead of the XSM check, so XSM has no way of
>>>> knowing which domain has been specified by the caller.
>>>
>>> Right, I see that now.
>>>
>>> Still, I assert that the original hypercall semantics is a very useful
>>> one, and what you're doing is changing the hypercall such that the
>>> question can no longer be asked.  It would be better to extend things so
>>> that XSM can actually deny device assignment based on both the bdf and
>>> the domain.
>>>
>>> Do you have a particular use case in mind for your alternate hypercall?
>>
>> No - I'm open to any change to it which makes the currently ignored
>> argument no longer ignored, without breaking existing (known and
>> unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
>> make it work the way you think it was originally meant to work; it is
>> just that given its current use I've come to a different conclusion as
>> to what the original intention may have been.
> 
> So the libxc library interface is not meant to be stable.  Before I
> looked at how libxl was using it, I was going to say that we should just
> remove the domid argument from that function entirely, rather than
> labelling it "unused".
> 
> I suggest we ask the toolstack maintainers what kind of a function they
> think would be most useful, and then we can implement that.
> 
> So, Wei and Ian (and Daniel if you're around):
> 
> At the moment, xc_test_assign_device() accepts both a domid and a device
> identifier.  It will return -ENOSYS if there is no IOMMU, and
> 
> The domid, however, is ignored by Xen, and there is no possibility even
> of XSM/Flask paying any attention to it because the domid is not passed
> to the XSM hook.
> 
> Jan thinks the color of this shed is ugly and wants to repaint it.  I
> agree that the color of this shed is ugly, but think a different color
> would be more suitable.  Since the function is ultimately mainly for the
> benefit of toolstacks like libxl, we'd like your input:
> 
> Option 1: Make xc_test_assign_device() explicitly take only a device
> identifier.  Add another xc function which takes a domain argument,
> which will return true if the BDF is assigned to that particular domain,
> and false otherwise.
>    1a: Leave the domid argument, but add a comment specifying it as ignored
>    1b: Remove the domid argument.
> 
> Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
> policies that can forbid specific devices from being assigned to
> specific guests.
> 
> NB that neither of us has a particular requirement for the proposed
> additional functionality ("Device X is assigned to domid Y" in the case
> of Option 1, or "Flask policy can allow or forbid specific devices to
> specific domains" in the case of Option 2).
> 
> Any preferences?
> 
>   -George

One caveat to add to option 2: the XSM permission check done by test_assign
would only cover the PCI device resource (same as assign_device).  There
are also XSM checks that occur when assigning the MMIO and IO port regions
assigned to the device (defined by the BARs), and these ranges are not
passed to the hypervisor during the test function.  A proper test_assign
function as described above would also need to check the XSM permissions
for them, which would require adding test_io{port,mem,q}_permission
functions too.

Alternatively, you could assume that the PCI device and its associated
resources all have the same label (which will be almost always be true in a
properly configured system) and just use this as an early bail out to avoid
user mistakes.

-- 
Daniel De Graaf
National Security Agency

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