[Xen-devel] [PATCH v2 0/4] iommu groups + cleanup

[Xen-devel] [PATCH v2 0/4] iommu groups + cleanup

[Paul Durrant]

This series is a mixture of tidying and some preparatory work for grouping
PCI devices for the purposes of assignment.

Paul Durrant (4):
  iommu / x86: move call to scan_pci_devices() out of vendor code
  pci: add all-device iterator function...
  iommu: introduce iommu_groups
  iommu / pci: re-implement XEN_DOMCTL_get_device_group...

 xen/drivers/passthrough/Makefile            |   1 +
 xen/drivers/passthrough/amd/pci_amd_iommu.c |   3 +-
 xen/drivers/passthrough/groups.c            | 135 ++++++++++++++++++++++
 xen/drivers/passthrough/pci.c               | 171 +++++++++++-----------------
 xen/drivers/passthrough/vtd/iommu.c         |   4 -
 xen/drivers/passthrough/x86/iommu.c         |  14 ++-
 xen/include/xen/iommu.h                     |   9 ++
 xen/include/xen/pci.h                       |   3 +
 8 files changed, 232 insertions(+), 108 deletions(-)
 create mode 100644 xen/drivers/passthrough/groups.c
---
v2:
 - Drop iommu_get_ops() move and add all-device iterator

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>
-- 
2.11.0


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

[Xen-devel] [PATCH v2 1/4] iommu / x86: move call to scan_pci_devices() out of vendor code

[Paul Durrant]

It's not vendor specific so it doesn't really belong there.

Scanning the PCI topology also really doesn't have much to do with IOMMU
initialization. It doesn't depend on there even being an IOMMU. This patch
moves to the call to the beginning of iommu_hardware_setup() but only
places it there because the topology information would be otherwise unused.

Subsequent patches will actually make use of the PCI topology during
(x86) IOMMU initialization.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>

v2:
 - Expanded commit comment.
 - Moved PCI scan to before IOMMU initialization, rather than after it.
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 3 ++-
 xen/drivers/passthrough/vtd/iommu.c         | 4 ----
 xen/drivers/passthrough/x86/iommu.c         | 6 ++++++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 4afbcd1609..3338a8e0e8 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -180,7 +180,8 @@ static int __init iov_detect(void)
 
     if ( !amd_iommu_perdev_intremap )
         printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
-    return scan_pci_devices();
+
+    return 0;
 }
 
 int amd_iommu_alloc_root(struct domain_iommu *hd)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 8b27d7e775..b0e3bf26b5 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2372,10 +2372,6 @@ static int __init vtd_setup(void)
     P(iommu_hap_pt_share, "Shared EPT tables");
 #undef P
 
-    ret = scan_pci_devices();
-    if ( ret )
-        goto error;
-
     ret = init_vtd_hw();
     if ( ret )
         goto error;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 0fa6dcc3fd..a7438c9c25 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -28,9 +28,15 @@ struct iommu_ops __read_mostly iommu_ops;
 
 int __init iommu_hardware_setup(void)
 {
+    int rc;
+
     if ( !iommu_init_ops )
         return -ENODEV;
 
+    rc = scan_pci_devices();
+    if ( rc )
+        return rc;
+
     if ( !iommu_ops.init )
         iommu_ops = *iommu_init_ops->ops;
     else
-- 
2.11.0


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

[Xen-devel] [PATCH v2 2/4] pci: add all-device iterator function...

[Paul Durrant]

...and use it for setup_hwdom_pci_devices() and dump_pci_devices().

The unlock/process-pending-softirqs/lock sequence that was in
_setup_hwdom_pci_devices() is now done in the generic iterator function,
which does mean it is also done (unnecessarily) in the case of
dump_pci_devices(), since run_all_nonirq_keyhandlers() will call
process_pending_softirqs() before invoking each key handler anyway, but
this is not performance critical code.

The "==== segment XXXX ====" headline that was in _dump_pci_devices() has
been dropped because it is non-trivial to deal with it when using a
generic all-device iterator and, since the segment number is included
in every log line anyway, it didn't add much value anyway.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>

v2:
 - New in v2.
---
 xen/drivers/passthrough/pci.c | 120 +++++++++++++++++++++++-------------------
 xen/include/xen/pci.h         |   1 +
 2 files changed, 68 insertions(+), 53 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index e88689425d..179cb7e17e 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1134,54 +1134,78 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt,
                ctxt->d->domain_id, err);
 }
 
-static int __hwdom_init _setup_hwdom_pci_devices(struct pci_seg *pseg, void *arg)
+static int __hwdom_init setup_hwdom_pci_device(struct pci_dev *pdev, void *arg)
 {
     struct setup_hwdom *ctxt = arg;
-    int bus, devfn;
+    struct domain *d = ctxt->d;
 
-    for ( bus = 0; bus < 256; bus++ )
+    if ( !pdev->domain )
     {
-        for ( devfn = 0; devfn < 256; devfn++ )
+        pdev->domain = d;
+        list_add(&pdev->domain_list, &d->pdev_list);
+        setup_one_hwdom_device(ctxt, pdev);
+    }
+    else if ( pdev->domain == dom_xen )
+    {
+        pdev->domain = d;
+        setup_one_hwdom_device(ctxt, pdev);
+        pdev->domain = dom_xen;
+    }
+    else if ( pdev->domain != d )
+        printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n",
+               pdev->domain->domain_id, pdev->seg, pdev->bus,
+               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+    return 0;
+}
+
+struct psdi_ctxt {
+    int (*cb)(struct pci_dev *, void *);
+    void *arg;
+};
+
+static int pci_segment_devices_iterate(struct pci_seg *pseg, void *arg)
+{
+    struct psdi_ctxt *ctxt = arg;
+    int bus, devfn;
+    int rc = 0;
+
+    /*
+     * We don't iterate by walking pseg->alldevs_list here because that
+     * would make the pcidevs_unlock()/lock() sequence below unsafe.
+     */
+    for ( bus = 0; !rc && bus < 256; bus++ )
+        for ( devfn = 0; !rc && devfn < 256; devfn++ )
         {
             struct pci_dev *pdev = pci_get_pdev(pseg->nr, bus, devfn);
 
             if ( !pdev )
                 continue;
 
-            if ( !pdev->domain )
-            {
-                pdev->domain = ctxt->d;
-                list_add(&pdev->domain_list, &ctxt->d->pdev_list);
-                setup_one_hwdom_device(ctxt, pdev);
-            }
-            else if ( pdev->domain == dom_xen )
-            {
-                pdev->domain = ctxt->d;
-                setup_one_hwdom_device(ctxt, pdev);
-                pdev->domain = dom_xen;
-            }
-            else if ( pdev->domain != ctxt->d )
-                printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n",
-                       pdev->domain->domain_id, pseg->nr, bus,
-                       PCI_SLOT(devfn), PCI_FUNC(devfn));
+            rc = ctxt->cb(pdev, ctxt->arg);
 
-            if ( iommu_verbose )
-            {
-                pcidevs_unlock();
-                process_pending_softirqs();
-                pcidevs_lock();
-            }
-        }
-
-        if ( !iommu_verbose )
-        {
+            /*
+             * Err on the safe side and assume the callback has taken
+             * a significant amount of time.
+             */
             pcidevs_unlock();
             process_pending_softirqs();
             pcidevs_lock();
         }
-    }
 
-    return 0;
+    return rc;
+}
+
+int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg)
+{
+    struct psdi_ctxt ctxt = { .cb = cb, .arg = arg };
+    int rc;
+
+    pcidevs_lock();
+    rc = pci_segments_iterate(pci_segment_devices_iterate, &ctxt);
+    pcidevs_unlock();
+
+    return rc;
 }
 
 void __hwdom_init setup_hwdom_pci_devices(
@@ -1189,9 +1213,7 @@ void __hwdom_init setup_hwdom_pci_devices(
 {
     struct setup_hwdom ctxt = { .d = d, .handler = handler };
 
-    pcidevs_lock();
-    pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt);
-    pcidevs_unlock();
+    pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt);
 }
 
 #ifdef CONFIG_ACPI
@@ -1294,24 +1316,18 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev)
 }
 #endif
 
-static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
+static int dump_pci_device(struct pci_dev *pdev, void *arg)
 {
-    struct pci_dev *pdev;
     struct msi_desc *msi;
 
-    printk("==== segment %04x ====\n", pseg->nr);
-
-    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
-    {
-        printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ",
-               pseg->nr, pdev->bus,
-               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
-               pdev->domain ? pdev->domain->domain_id : -1,
-               (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
-        list_for_each_entry ( msi, &pdev->msi_list, list )
-               printk("%d ", msi->irq);
-        printk(">\n");
-    }
+    printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ",
+           pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+           PCI_FUNC(pdev->devfn),
+           pdev->domain ? pdev->domain->domain_id : -1,
+           (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
+    list_for_each_entry ( msi, &pdev->msi_list, list )
+        printk("%d ", msi->irq);
+    printk(">\n");
 
     return 0;
 }
@@ -1319,9 +1335,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
 static void dump_pci_devices(unsigned char ch)
 {
     printk("==== PCI devices ====\n");
-    pcidevs_lock();
-    pci_segments_iterate(_dump_pci_devices, NULL);
-    pcidevs_unlock();
+    pci_pdevs_iterate(dump_pci_device, NULL);
 }
 
 static int __init setup_dump_pcidevs(void)
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 04a9f46cc3..79eb25417b 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -154,6 +154,7 @@ int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus);
 struct pci_dev *pci_lock_pdev(int seg, int bus, int devfn);
 struct pci_dev *pci_lock_domain_pdev(
     struct domain *, int seg, int bus, int devfn);
+int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg);
 
 void setup_hwdom_pci_devices(struct domain *,
                             int (*)(u8 devfn, struct pci_dev *));
-- 
2.11.0


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

[Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups

[Paul Durrant]

Some devices may share a single PCIe initiator id, e.g. if they are actually
legacy PCI devices behind a bridge, and hence DMA from such devices will
be subject to the same address translation in the IOMMU. Hence these devices
should be treated as a unit for the purposes of assignment. There are also
other reasons why multiple devices should be treated as a unit, e.g. those
subject to a shared RMRR or those downstream of a bridge that does not
support ACS.

This patch introduces a new struct iommu_group to act as a container for
devices that should be treated as a unit, and builds a list of them as
PCI devices are scanned. The iommu_ops already implement a method,
get_device_group_id(), that is seemingly intended to return the initiator
id for a given SBDF so use this as the mechanism for group assignment in
the first instance. Assignment based on shared RMRR or lack of ACS will be
dealt with in subsequent patches, as will modifications to the device
assignment code.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>

v2:
 - Move code into new drivers/passthrough/groups.c
 - Drop the group index.
 - Handle failure to get group id.
 - Drop the group devs list.
---
 xen/drivers/passthrough/Makefile    |  1 +
 xen/drivers/passthrough/groups.c    | 91 +++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/x86/iommu.c |  8 +++-
 xen/include/xen/iommu.h             |  7 +++
 xen/include/xen/pci.h               |  2 +
 5 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 xen/drivers/passthrough/groups.c

diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
index d50ab188c8..8a77110179 100644
--- a/xen/drivers/passthrough/Makefile
+++ b/xen/drivers/passthrough/Makefile
@@ -4,6 +4,7 @@ subdir-$(CONFIG_X86) += x86
 subdir-$(CONFIG_ARM) += arm
 
 obj-y += iommu.o
+obj-$(CONFIG_HAS_PCI) += groups.o
 obj-$(CONFIG_HAS_PCI) += pci.o
 obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
 
diff --git a/xen/drivers/passthrough/groups.c b/xen/drivers/passthrough/groups.c
new file mode 100644
index 0000000000..1a2f461c87
--- /dev/null
+++ b/xen/drivers/passthrough/groups.c
@@ -0,0 +1,91 @@
+/*
+ * Copyright (c) 2019 Citrix Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <xen/iommu.h>
+#include <xen/radix-tree.h>
+
+struct iommu_group {
+    unsigned int id;
+};
+
+static struct radix_tree_root iommu_groups;
+
+void __init iommu_groups_init(void)
+{
+    radix_tree_init(&iommu_groups);
+}
+
+static struct iommu_group *alloc_iommu_group(unsigned int id)
+{
+    struct iommu_group *grp = xzalloc(struct iommu_group);
+
+    if ( !grp )
+        return NULL;
+
+    grp->id = id;
+
+    if ( radix_tree_insert(&iommu_groups, id, grp) )
+    {
+        xfree(grp);
+        grp = NULL;
+    }
+
+    return grp;
+}
+
+static struct iommu_group *get_iommu_group(unsigned int id)
+{
+    struct iommu_group *grp = radix_tree_lookup(&iommu_groups, id);
+
+    if ( !grp )
+        grp = alloc_iommu_group(id);
+
+    return grp;
+}
+
+int iommu_group_assign(struct pci_dev *pdev, void *arg)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    unsigned int id;
+    struct iommu_group *grp;
+
+    if ( !ops->get_device_group_id )
+        return 0;
+
+    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
+    if ( id < 0 )
+        return -ENODATA;
+
+    grp = get_iommu_group(id);
+    if ( !grp )
+        return -ENOMEM;
+
+    if ( iommu_verbose )
+        printk(XENLOG_INFO "Assign %04x:%02x:%02x.%u -> IOMMU group %x\n",
+               pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+               PCI_FUNC(pdev->devfn), grp->id);
+
+    pdev->grp = grp;
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index a7438c9c25..90fc750456 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -43,7 +43,13 @@ int __init iommu_hardware_setup(void)
         /* x2apic setup may have previously initialised the struct. */
         ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
 
-    return iommu_init_ops->setup();
+    rc = iommu_init_ops->setup();
+    if ( rc )
+        return rc;
+
+    iommu_groups_init();
+
+    return pci_pdevs_iterate(iommu_group_assign, NULL);
 }
 
 int iommu_enable_x2apic(void)
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 48f87480a7..c93f580fdc 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -317,6 +317,13 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
 extern struct spinlock iommu_pt_cleanup_lock;
 extern struct page_list_head iommu_pt_cleanup_list;
 
+#ifdef CONFIG_HAS_PCI
+
+void iommu_groups_init(void);
+int iommu_group_assign(struct pci_dev *pdev, void *arg);
+
+#endif /* CONFIG_HAS_PCI */
+
 #endif /* _IOMMU_H_ */
 
 /*
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 79eb25417b..e1f887af1c 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -79,6 +79,8 @@ struct pci_dev {
     struct list_head alldevs_list;
     struct list_head domain_list;
 
+    struct iommu_group *grp;
+
     struct list_head msi_list;
 
     struct arch_msix *msix;
-- 
2.11.0


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

[Xen-devel] [PATCH v2 4/4] iommu / pci: re-implement XEN_DOMCTL_get_device_group...

[Paul Durrant]

... using the new iommu_group infrastructure.

Because 'sibling' devices are now members of the same iommu_group,
implement the domctl by looking up the iommu_group of the pdev with the
matching SBDF and then finding all the assigned pdevs that are in the
group.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>

v2:
 - Re-implement in the absence of a per-group devs list.
 - Make use of pci_sbdf_t.
---
 xen/drivers/passthrough/groups.c | 44 ++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c    | 51 ++--------------------------------------
 xen/include/xen/iommu.h          |  2 ++
 3 files changed, 48 insertions(+), 49 deletions(-)

diff --git a/xen/drivers/passthrough/groups.c b/xen/drivers/passthrough/groups.c
index 1a2f461c87..6d8064e4f4 100644
--- a/xen/drivers/passthrough/groups.c
+++ b/xen/drivers/passthrough/groups.c
@@ -12,8 +12,12 @@
  * GNU General Public License for more details.
  */
 
+#include <xen/guest_access.h>
 #include <xen/iommu.h>
+#include <xen/pci.h>
 #include <xen/radix-tree.h>
+#include <xen/sched.h>
+#include <xsm/xsm.h>
 
 struct iommu_group {
     unsigned int id;
@@ -81,6 +85,46 @@ int iommu_group_assign(struct pci_dev *pdev, void *arg)
     return 0;
 }
 
+int iommu_get_device_group(struct domain *d, pci_sbdf_t sbdf,
+                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
+{
+    struct iommu_group *grp = NULL;
+    struct pci_dev *pdev;
+    unsigned int i = 0;
+
+    pcidevs_lock();
+
+    for_each_pdev ( d, pdev )
+    {
+        if ( pdev->sbdf.sbdf == sbdf.sbdf )
+        {
+            grp = pdev->grp;
+            break;
+        }
+    }
+
+    if ( !grp )
+        goto out;
+
+    for_each_pdev ( d, pdev )
+    {
+        if ( xsm_get_device_group(XSM_HOOK, pdev->sbdf.sbdf) ||
+             pdev->grp != grp )
+            continue;
+
+        if ( unlikely(copy_to_guest_offset(buf, i++, &pdev->sbdf.sbdf, 1)) )
+        {
+            pcidevs_unlock();
+            return -EFAULT;
+        }
+    }
+
+ out:
+    pcidevs_unlock();
+
+    return i;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 179cb7e17e..3a5e90c176 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1563,53 +1563,6 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
     return ret;
 }
 
-static int iommu_get_device_group(
-    struct domain *d, u16 seg, u8 bus, u8 devfn,
-    XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
-{
-    const struct domain_iommu *hd = dom_iommu(d);
-    struct pci_dev *pdev;
-    int group_id, sdev_id;
-    u32 bdf;
-    int i = 0;
-    const struct iommu_ops *ops = hd->platform_ops;
-
-    if ( !iommu_enabled || !ops || !ops->get_device_group_id )
-        return 0;
-
-    group_id = ops->get_device_group_id(seg, bus, devfn);
-
-    pcidevs_lock();
-    for_each_pdev( d, pdev )
-    {
-        if ( (pdev->seg != seg) ||
-             ((pdev->bus == bus) && (pdev->devfn == devfn)) )
-            continue;
-
-        if ( xsm_get_device_group(XSM_HOOK, (seg << 16) | (pdev->bus << 8) | pdev->devfn) )
-            continue;
-
-        sdev_id = ops->get_device_group_id(seg, pdev->bus, pdev->devfn);
-        if ( (sdev_id == group_id) && (i < max_sdevs) )
-        {
-            bdf = 0;
-            bdf |= (pdev->bus & 0xff) << 16;
-            bdf |= (pdev->devfn & 0xff) << 8;
-
-            if ( unlikely(copy_to_guest_offset(buf, i, &bdf, 1)) )
-            {
-                pcidevs_unlock();
-                return -1;
-            }
-            i++;
-        }
-    }
-
-    pcidevs_unlock();
-
-    return i;
-}
-
 void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev)
 {
     pcidevs_lock();
@@ -1666,11 +1619,11 @@ int iommu_do_pci_domctl(
         max_sdevs = domctl->u.get_device_group.max_sdevs;
         sdevs = domctl->u.get_device_group.sdev_array;
 
-        ret = iommu_get_device_group(d, seg, bus, devfn, sdevs, max_sdevs);
+        ret = iommu_get_device_group(d, PCI_SBDF3(seg, bus, devfn), sdevs,
+                                     max_sdevs);
         if ( ret < 0 )
         {
             dprintk(XENLOG_ERR, "iommu_get_device_group() failed!\n");
-            ret = -EFAULT;
             domctl->u.get_device_group.num_sdevs = 0;
         }
         else
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index c93f580fdc..ac764b41f9 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -321,6 +321,8 @@ extern struct page_list_head iommu_pt_cleanup_list;
 
 void iommu_groups_init(void);
 int iommu_group_assign(struct pci_dev *pdev, void *arg);
+int iommu_get_device_group(struct domain *d, pci_sbdf_t sbdf,
+                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs);
 
 #endif /* CONFIG_HAS_PCI */
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v2 1/4] iommu / x86: move call to scan_pci_devices() out of vendor code

[Roger Pau Monné]

On Mon, Jul 15, 2019 at 01:37:07PM +0100, Paul Durrant wrote:
> It's not vendor specific so it doesn't really belong there.
> 
> Scanning the PCI topology also really doesn't have much to do with IOMMU
> initialization. It doesn't depend on there even being an IOMMU. This patch
> moves to the call to the beginning of iommu_hardware_setup() but only
> places it there because the topology information would be otherwise unused.
> 
> Subsequent patches will actually make use of the PCI topology during
> (x86) IOMMU initialization.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I would even consider moving the call to scan_pci_devices into
pci_segments_init instead of doing it in the IOMMU code, as you
suggest above.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 2/4] pci: add all-device iterator function...

[Roger Pau Monné]

On Mon, Jul 15, 2019 at 01:37:08PM +0100, Paul Durrant wrote:
> ...and use it for setup_hwdom_pci_devices() and dump_pci_devices().
> 
> The unlock/process-pending-softirqs/lock sequence that was in
> _setup_hwdom_pci_devices() is now done in the generic iterator function,
> which does mean it is also done (unnecessarily) in the case of
> dump_pci_devices(), since run_all_nonirq_keyhandlers() will call
> process_pending_softirqs() before invoking each key handler anyway, but
> this is not performance critical code.
> 
> The "==== segment XXXX ====" headline that was in _dump_pci_devices() has
> been dropped because it is non-trivial to deal with it when using a
> generic all-device iterator and, since the segment number is included
> in every log line anyway, it didn't add much value anyway.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just some trivial comments.

Thanks.

> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wl@xen.org>
> 
> v2:
>  - New in v2.
> ---
>  xen/drivers/passthrough/pci.c | 120 +++++++++++++++++++++++-------------------
>  xen/include/xen/pci.h         |   1 +
>  2 files changed, 68 insertions(+), 53 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index e88689425d..179cb7e17e 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1134,54 +1134,78 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt,
>                 ctxt->d->domain_id, err);
>  }
>  
> -static int __hwdom_init _setup_hwdom_pci_devices(struct pci_seg *pseg, void *arg)
> +static int __hwdom_init setup_hwdom_pci_device(struct pci_dev *pdev, void *arg)
>  {
>      struct setup_hwdom *ctxt = arg;
> -    int bus, devfn;
> +    struct domain *d = ctxt->d;
>  
> -    for ( bus = 0; bus < 256; bus++ )
> +    if ( !pdev->domain )
>      {
> -        for ( devfn = 0; devfn < 256; devfn++ )
> +        pdev->domain = d;
> +        list_add(&pdev->domain_list, &d->pdev_list);
> +        setup_one_hwdom_device(ctxt, pdev);
> +    }
> +    else if ( pdev->domain == dom_xen )
> +    {
> +        pdev->domain = d;
> +        setup_one_hwdom_device(ctxt, pdev);
> +        pdev->domain = dom_xen;
> +    }
> +    else if ( pdev->domain != d )
> +        printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n",
> +               pdev->domain->domain_id, pdev->seg, pdev->bus,

You can use %pd here to print the domain.

> +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +
> +    return 0;
> +}
> +
> +struct psdi_ctxt {
> +    int (*cb)(struct pci_dev *, void *);
> +    void *arg;
> +};
> +
> +static int pci_segment_devices_iterate(struct pci_seg *pseg, void *arg)
> +{
> +    struct psdi_ctxt *ctxt = arg;
> +    int bus, devfn;

unsigned for both the above.

> +    int rc = 0;
> +
> +    /*
> +     * We don't iterate by walking pseg->alldevs_list here because that
> +     * would make the pcidevs_unlock()/lock() sequence below unsafe.
> +     */
> +    for ( bus = 0; !rc && bus < 256; bus++ )
> +        for ( devfn = 0; !rc && devfn < 256; devfn++ )
>          {
>              struct pci_dev *pdev = pci_get_pdev(pseg->nr, bus, devfn);
>  
>              if ( !pdev )
>                  continue;
>  
> -            if ( !pdev->domain )
> -            {
> -                pdev->domain = ctxt->d;
> -                list_add(&pdev->domain_list, &ctxt->d->pdev_list);
> -                setup_one_hwdom_device(ctxt, pdev);
> -            }
> -            else if ( pdev->domain == dom_xen )
> -            {
> -                pdev->domain = ctxt->d;
> -                setup_one_hwdom_device(ctxt, pdev);
> -                pdev->domain = dom_xen;
> -            }
> -            else if ( pdev->domain != ctxt->d )
> -                printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n",
> -                       pdev->domain->domain_id, pseg->nr, bus,
> -                       PCI_SLOT(devfn), PCI_FUNC(devfn));
> +            rc = ctxt->cb(pdev, ctxt->arg);
>  
> -            if ( iommu_verbose )
> -            {
> -                pcidevs_unlock();
> -                process_pending_softirqs();
> -                pcidevs_lock();
> -            }
> -        }
> -
> -        if ( !iommu_verbose )
> -        {
> +            /*
> +             * Err on the safe side and assume the callback has taken
> +             * a significant amount of time.
> +             */
>              pcidevs_unlock();
>              process_pending_softirqs();
>              pcidevs_lock();
>          }
> -    }
>  
> -    return 0;
> +    return rc;
> +}
> +
> +int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg)
> +{
> +    struct psdi_ctxt ctxt = { .cb = cb, .arg = arg };
> +    int rc;
> +
> +    pcidevs_lock();
> +    rc = pci_segments_iterate(pci_segment_devices_iterate, &ctxt);
> +    pcidevs_unlock();
> +
> +    return rc;
>  }
>  
>  void __hwdom_init setup_hwdom_pci_devices(
> @@ -1189,9 +1213,7 @@ void __hwdom_init setup_hwdom_pci_devices(
>  {
>      struct setup_hwdom ctxt = { .d = d, .handler = handler };
>  
> -    pcidevs_lock();
> -    pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt);
> -    pcidevs_unlock();
> +    pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt);

Since this now returns an error code it wold be good to handle it,
even if it's just:

rc = pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt);
if ( rc )
    ASSERT_UNREACHABLE();

setup_hwdom_pci_device will always return 0, hence a failure here is
not possible AFAICT. Same with the other usage of pci_pdevs_iterate
below.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups

[Roger Pau Monné]

On Mon, Jul 15, 2019 at 01:37:09PM +0100, Paul Durrant wrote:
> Some devices may share a single PCIe initiator id, e.g. if they are actually
> legacy PCI devices behind a bridge, and hence DMA from such devices will
> be subject to the same address translation in the IOMMU. Hence these devices
> should be treated as a unit for the purposes of assignment. There are also
> other reasons why multiple devices should be treated as a unit, e.g. those
> subject to a shared RMRR or those downstream of a bridge that does not
> support ACS.
> 
> This patch introduces a new struct iommu_group to act as a container for
> devices that should be treated as a unit, and builds a list of them as
> PCI devices are scanned. The iommu_ops already implement a method,
> get_device_group_id(), that is seemingly intended to return the initiator
> id for a given SBDF so use this as the mechanism for group assignment in
> the first instance. Assignment based on shared RMRR or lack of ACS will be
> dealt with in subsequent patches, as will modifications to the device
> assignment code.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

LGTM, just two comments below.

> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wl@xen.org>
> 
> v2:
>  - Move code into new drivers/passthrough/groups.c
>  - Drop the group index.
>  - Handle failure to get group id.
>  - Drop the group devs list.
> ---
>  xen/drivers/passthrough/Makefile    |  1 +
>  xen/drivers/passthrough/groups.c    | 91 +++++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/x86/iommu.c |  8 +++-
>  xen/include/xen/iommu.h             |  7 +++
>  xen/include/xen/pci.h               |  2 +
>  5 files changed, 108 insertions(+), 1 deletion(-)
>  create mode 100644 xen/drivers/passthrough/groups.c
> 
> diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
> index d50ab188c8..8a77110179 100644
> --- a/xen/drivers/passthrough/Makefile
> +++ b/xen/drivers/passthrough/Makefile
> @@ -4,6 +4,7 @@ subdir-$(CONFIG_X86) += x86
>  subdir-$(CONFIG_ARM) += arm
>  
>  obj-y += iommu.o
> +obj-$(CONFIG_HAS_PCI) += groups.o
>  obj-$(CONFIG_HAS_PCI) += pci.o
>  obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
>  
> diff --git a/xen/drivers/passthrough/groups.c b/xen/drivers/passthrough/groups.c
> new file mode 100644
> index 0000000000..1a2f461c87
> --- /dev/null
> +++ b/xen/drivers/passthrough/groups.c
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (c) 2019 Citrix Systems Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/iommu.h>
> +#include <xen/radix-tree.h>
> +
> +struct iommu_group {
> +    unsigned int id;
> +};
> +
> +static struct radix_tree_root iommu_groups;
> +
> +void __init iommu_groups_init(void)
> +{
> +    radix_tree_init(&iommu_groups);
> +}
> +
> +static struct iommu_group *alloc_iommu_group(unsigned int id)
> +{
> +    struct iommu_group *grp = xzalloc(struct iommu_group);
> +
> +    if ( !grp )
> +        return NULL;
> +
> +    grp->id = id;
> +
> +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> +    {
> +        xfree(grp);
> +        grp = NULL;
> +    }
> +
> +    return grp;
> +}
> +
> +static struct iommu_group *get_iommu_group(unsigned int id)
> +{
> +    struct iommu_group *grp = radix_tree_lookup(&iommu_groups, id);
> +
> +    if ( !grp )
> +        grp = alloc_iommu_group(id);
> +
> +    return grp;
> +}
> +
> +int iommu_group_assign(struct pci_dev *pdev, void *arg)

I'm not sure I see the point of the arg parameter, AFAICT it's
completely unused.

> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    unsigned int id;
> +    struct iommu_group *grp;
> +
> +    if ( !ops->get_device_group_id )
> +        return 0;
> +
> +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);

I think I would prefer id to be of signed type here, then when you
pass it to get_iommu_group it's already made unsigned.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 15 July 2019 16:35
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> Julien Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups
> 
> On Mon, Jul 15, 2019 at 01:37:09PM +0100, Paul Durrant wrote:
> > Some devices may share a single PCIe initiator id, e.g. if they are actually
> > legacy PCI devices behind a bridge, and hence DMA from such devices will
> > be subject to the same address translation in the IOMMU. Hence these devices
> > should be treated as a unit for the purposes of assignment. There are also
> > other reasons why multiple devices should be treated as a unit, e.g. those
> > subject to a shared RMRR or those downstream of a bridge that does not
> > support ACS.
> >
> > This patch introduces a new struct iommu_group to act as a container for
> > devices that should be treated as a unit, and builds a list of them as
> > PCI devices are scanned. The iommu_ops already implement a method,
> > get_device_group_id(), that is seemingly intended to return the initiator
> > id for a given SBDF so use this as the mechanism for group assignment in
> > the first instance. Assignment based on shared RMRR or lack of ACS will be
> > dealt with in subsequent patches, as will modifications to the device
> > assignment code.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> LGTM, just two comments below.
> 
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wl@xen.org>
> >
> > v2:
> >  - Move code into new drivers/passthrough/groups.c
> >  - Drop the group index.
> >  - Handle failure to get group id.
> >  - Drop the group devs list.
> > ---
> >  xen/drivers/passthrough/Makefile    |  1 +
> >  xen/drivers/passthrough/groups.c    | 91 +++++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/x86/iommu.c |  8 +++-
> >  xen/include/xen/iommu.h             |  7 +++
> >  xen/include/xen/pci.h               |  2 +
> >  5 files changed, 108 insertions(+), 1 deletion(-)
> >  create mode 100644 xen/drivers/passthrough/groups.c
> >
> > diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
> > index d50ab188c8..8a77110179 100644
> > --- a/xen/drivers/passthrough/Makefile
> > +++ b/xen/drivers/passthrough/Makefile
> > @@ -4,6 +4,7 @@ subdir-$(CONFIG_X86) += x86
> >  subdir-$(CONFIG_ARM) += arm
> >
> >  obj-y += iommu.o
> > +obj-$(CONFIG_HAS_PCI) += groups.o
> >  obj-$(CONFIG_HAS_PCI) += pci.o
> >  obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
> >
> > diff --git a/xen/drivers/passthrough/groups.c b/xen/drivers/passthrough/groups.c
> > new file mode 100644
> > index 0000000000..1a2f461c87
> > --- /dev/null
> > +++ b/xen/drivers/passthrough/groups.c
> > @@ -0,0 +1,91 @@
> > +/*
> > + * Copyright (c) 2019 Citrix Systems Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <xen/iommu.h>
> > +#include <xen/radix-tree.h>
> > +
> > +struct iommu_group {
> > +    unsigned int id;
> > +};
> > +
> > +static struct radix_tree_root iommu_groups;
> > +
> > +void __init iommu_groups_init(void)
> > +{
> > +    radix_tree_init(&iommu_groups);
> > +}
> > +
> > +static struct iommu_group *alloc_iommu_group(unsigned int id)
> > +{
> > +    struct iommu_group *grp = xzalloc(struct iommu_group);
> > +
> > +    if ( !grp )
> > +        return NULL;
> > +
> > +    grp->id = id;
> > +
> > +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> > +    {
> > +        xfree(grp);
> > +        grp = NULL;
> > +    }
> > +
> > +    return grp;
> > +}
> > +
> > +static struct iommu_group *get_iommu_group(unsigned int id)
> > +{
> > +    struct iommu_group *grp = radix_tree_lookup(&iommu_groups, id);
> > +
> > +    if ( !grp )
> > +        grp = alloc_iommu_group(id);
> > +
> > +    return grp;
> > +}
> > +
> > +int iommu_group_assign(struct pci_dev *pdev, void *arg)
> 
> I'm not sure I see the point of the arg parameter, AFAICT it's
> completely unused.

It needs to be there because it needs to conform to the all device iterator function callback prototype. It is indeed unused though.

> 
> > +{
> > +    const struct iommu_ops *ops = iommu_get_ops();
> > +    unsigned int id;
> > +    struct iommu_group *grp;
> > +
> > +    if ( !ops->get_device_group_id )
> > +        return 0;
> > +
> > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> 
> I think I would prefer id to be of signed type here, then when you
> pass it to get_iommu_group it's already made unsigned.

That's a good catch. It's tested for < 0 below so it does need to be signed at this point.

  Paul

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 4/4] iommu / pci: re-implement XEN_DOMCTL_get_device_group...

[Roger Pau Monné]

On Mon, Jul 15, 2019 at 01:37:10PM +0100, Paul Durrant wrote:
> ... using the new iommu_group infrastructure.
> 
> Because 'sibling' devices are now members of the same iommu_group,
> implement the domctl by looking up the iommu_group of the pdev with the
> matching SBDF and then finding all the assigned pdevs that are in the
> group.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> 
> v2:
>  - Re-implement in the absence of a per-group devs list.
>  - Make use of pci_sbdf_t.
> ---
>  xen/drivers/passthrough/groups.c | 44 ++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c    | 51 ++--------------------------------------
>  xen/include/xen/iommu.h          |  2 ++
>  3 files changed, 48 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/groups.c b/xen/drivers/passthrough/groups.c
> index 1a2f461c87..6d8064e4f4 100644
> --- a/xen/drivers/passthrough/groups.c
> +++ b/xen/drivers/passthrough/groups.c
> @@ -12,8 +12,12 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <xen/guest_access.h>
>  #include <xen/iommu.h>
> +#include <xen/pci.h>
>  #include <xen/radix-tree.h>
> +#include <xen/sched.h>
> +#include <xsm/xsm.h>
>  
>  struct iommu_group {
>      unsigned int id;
> @@ -81,6 +85,46 @@ int iommu_group_assign(struct pci_dev *pdev, void *arg)
>      return 0;
>  }
>  
> +int iommu_get_device_group(struct domain *d, pci_sbdf_t sbdf,
> +                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)

max_sdevs should be unsigned AFAICT, but it seems to be completely
unused. I think you want to do...

> +{
> +    struct iommu_group *grp = NULL;
> +    struct pci_dev *pdev;
> +    unsigned int i = 0;
> +
> +    pcidevs_lock();
> +
> +    for_each_pdev ( d, pdev )
> +    {
> +        if ( pdev->sbdf.sbdf == sbdf.sbdf )
> +        {
> +            grp = pdev->grp;
> +            break;
> +        }
> +    }
> +
> +    if ( !grp )
> +        goto out;
> +
> +    for_each_pdev ( d, pdev )
> +    {

if ( i == max_sdevs )
{
    pcidevs_unlock();
    return -ENOSPC;
}

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 4/4] iommu / pci: re-implement XEN_DOMCTL_get_device_group...

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 15 July 2019 16:46
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 4/4] iommu / pci: re-implement XEN_DOMCTL_get_device_group...
> 
> On Mon, Jul 15, 2019 at 01:37:10PM +0100, Paul Durrant wrote:
> > ... using the new iommu_group infrastructure.
> >
> > Because 'sibling' devices are now members of the same iommu_group,
> > implement the domctl by looking up the iommu_group of the pdev with the
> > matching SBDF and then finding all the assigned pdevs that are in the
> > group.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> >
> > v2:
> >  - Re-implement in the absence of a per-group devs list.
> >  - Make use of pci_sbdf_t.
> > ---
> >  xen/drivers/passthrough/groups.c | 44 ++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/pci.c    | 51 ++--------------------------------------
> >  xen/include/xen/iommu.h          |  2 ++
> >  3 files changed, 48 insertions(+), 49 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/groups.c b/xen/drivers/passthrough/groups.c
> > index 1a2f461c87..6d8064e4f4 100644
> > --- a/xen/drivers/passthrough/groups.c
> > +++ b/xen/drivers/passthrough/groups.c
> > @@ -12,8 +12,12 @@
> >   * GNU General Public License for more details.
> >   */
> >
> > +#include <xen/guest_access.h>
> >  #include <xen/iommu.h>
> > +#include <xen/pci.h>
> >  #include <xen/radix-tree.h>
> > +#include <xen/sched.h>
> > +#include <xsm/xsm.h>
> >
> >  struct iommu_group {
> >      unsigned int id;
> > @@ -81,6 +85,46 @@ int iommu_group_assign(struct pci_dev *pdev, void *arg)
> >      return 0;
> >  }
> >
> > +int iommu_get_device_group(struct domain *d, pci_sbdf_t sbdf,
> > +                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
> 
> max_sdevs should be unsigned AFAICT, but it seems to be completely
> unused. I think you want to do...
> 
> > +{
> > +    struct iommu_group *grp = NULL;
> > +    struct pci_dev *pdev;
> > +    unsigned int i = 0;
> > +
> > +    pcidevs_lock();
> > +
> > +    for_each_pdev ( d, pdev )
> > +    {
> > +        if ( pdev->sbdf.sbdf == sbdf.sbdf )
> > +        {
> > +            grp = pdev->grp;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if ( !grp )
> > +        goto out;
> > +
> > +    for_each_pdev ( d, pdev )
> > +    {
> 
> if ( i == max_sdevs )
> {
>     pcidevs_unlock();
>     return -ENOSPC;
> }

Oh, I'm sure I used to have that... I don't know how it got dropped. It certainly needs to be there.

  Paul

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups

[Roger Pau Monné]

On Mon, Jul 15, 2019 at 05:41:27PM +0200, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: 15 July 2019 16:35
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>;
> > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew
> > Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> > Julien Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>
> > Subject: Re: [Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups
> > 
> > On Mon, Jul 15, 2019 at 01:37:09PM +0100, Paul Durrant wrote:
> > > Some devices may share a single PCIe initiator id, e.g. if they are actually
> > > legacy PCI devices behind a bridge, and hence DMA from such devices will
> > > be subject to the same address translation in the IOMMU. Hence these devices
> > > should be treated as a unit for the purposes of assignment. There are also
> > > other reasons why multiple devices should be treated as a unit, e.g. those
> > > subject to a shared RMRR or those downstream of a bridge that does not
> > > support ACS.
> > >
> > > This patch introduces a new struct iommu_group to act as a container for
> > > devices that should be treated as a unit, and builds a list of them as
> > > PCI devices are scanned. The iommu_ops already implement a method,
> > > get_device_group_id(), that is seemingly intended to return the initiator
> > > id for a given SBDF so use this as the mechanism for group assignment in
> > > the first instance. Assignment based on shared RMRR or lack of ACS will be
> > > dealt with in subsequent patches, as will modifications to the device
> > > assignment code.
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > 
> > LGTM, just two comments below.
> > 
> > > ---
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Julien Grall <julien.grall@arm.com>
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > > Cc: Tim Deegan <tim@xen.org>
> > > Cc: Wei Liu <wl@xen.org>
> > >
> > > v2:
> > >  - Move code into new drivers/passthrough/groups.c
> > >  - Drop the group index.
> > >  - Handle failure to get group id.
> > >  - Drop the group devs list.
> > > ---
> > >  xen/drivers/passthrough/Makefile    |  1 +
> > >  xen/drivers/passthrough/groups.c    | 91 +++++++++++++++++++++++++++++++++++++
> > >  xen/drivers/passthrough/x86/iommu.c |  8 +++-
> > >  xen/include/xen/iommu.h             |  7 +++
> > >  xen/include/xen/pci.h               |  2 +
> > >  5 files changed, 108 insertions(+), 1 deletion(-)
> > >  create mode 100644 xen/drivers/passthrough/groups.c
> > >
> > > diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
> > > index d50ab188c8..8a77110179 100644
> > > --- a/xen/drivers/passthrough/Makefile
> > > +++ b/xen/drivers/passthrough/Makefile
> > > @@ -4,6 +4,7 @@ subdir-$(CONFIG_X86) += x86
> > >  subdir-$(CONFIG_ARM) += arm
> > >
> > >  obj-y += iommu.o
> > > +obj-$(CONFIG_HAS_PCI) += groups.o
> > >  obj-$(CONFIG_HAS_PCI) += pci.o
> > >  obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
> > >
> > > diff --git a/xen/drivers/passthrough/groups.c b/xen/drivers/passthrough/groups.c
> > > new file mode 100644
> > > index 0000000000..1a2f461c87
> > > --- /dev/null
> > > +++ b/xen/drivers/passthrough/groups.c
> > > @@ -0,0 +1,91 @@
> > > +/*
> > > + * Copyright (c) 2019 Citrix Systems Inc.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <xen/iommu.h>
> > > +#include <xen/radix-tree.h>
> > > +
> > > +struct iommu_group {
> > > +    unsigned int id;
> > > +};
> > > +
> > > +static struct radix_tree_root iommu_groups;
> > > +
> > > +void __init iommu_groups_init(void)
> > > +{
> > > +    radix_tree_init(&iommu_groups);
> > > +}
> > > +
> > > +static struct iommu_group *alloc_iommu_group(unsigned int id)
> > > +{
> > > +    struct iommu_group *grp = xzalloc(struct iommu_group);
> > > +
> > > +    if ( !grp )
> > > +        return NULL;
> > > +
> > > +    grp->id = id;
> > > +
> > > +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> > > +    {
> > > +        xfree(grp);
> > > +        grp = NULL;
> > > +    }
> > > +
> > > +    return grp;
> > > +}
> > > +
> > > +static struct iommu_group *get_iommu_group(unsigned int id)
> > > +{
> > > +    struct iommu_group *grp = radix_tree_lookup(&iommu_groups, id);
> > > +
> > > +    if ( !grp )
> > > +        grp = alloc_iommu_group(id);
> > > +
> > > +    return grp;
> > > +}
> > > +
> > > +int iommu_group_assign(struct pci_dev *pdev, void *arg)
> > 
> > I'm not sure I see the point of the arg parameter, AFAICT it's
> > completely unused.
> 
> It needs to be there because it needs to conform to the all device iterator function callback prototype. It is indeed unused though.

Oh right, sorry for the noise.

> > 
> > > +{
> > > +    const struct iommu_ops *ops = iommu_get_ops();
> > > +    unsigned int id;
> > > +    struct iommu_group *grp;
> > > +
> > > +    if ( !ops->get_device_group_id )
> > > +        return 0;
> > > +
> > > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> > 
> > I think I would prefer id to be of signed type here, then when you
> > pass it to get_iommu_group it's already made unsigned.
> 
> That's a good catch. It's tested for < 0 below so it does need to be signed at this point.

With that you can add my:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 16 July 2019 09:57
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> Julien Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups
> 
> On Mon, Jul 15, 2019 at 05:41:27PM +0200, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > Sent: 15 July 2019 16:35
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wl@xen.org>;
> > > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew
> > > Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org)
> <tim@xen.org>;
> > > Julien Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>
> > > Subject: Re: [Xen-devel] [PATCH v2 3/4] iommu: introduce iommu_groups
> > >
> > > On Mon, Jul 15, 2019 at 01:37:09PM +0100, Paul Durrant wrote:
> > > > Some devices may share a single PCIe initiator id, e.g. if they are actually
> > > > legacy PCI devices behind a bridge, and hence DMA from such devices will
> > > > be subject to the same address translation in the IOMMU. Hence these devices
> > > > should be treated as a unit for the purposes of assignment. There are also
> > > > other reasons why multiple devices should be treated as a unit, e.g. those
> > > > subject to a shared RMRR or those downstream of a bridge that does not
> > > > support ACS.
> > > >
> > > > This patch introduces a new struct iommu_group to act as a container for
> > > > devices that should be treated as a unit, and builds a list of them as
> > > > PCI devices are scanned. The iommu_ops already implement a method,
> > > > get_device_group_id(), that is seemingly intended to return the initiator
> > > > id for a given SBDF so use this as the mechanism for group assignment in
> > > > the first instance. Assignment based on shared RMRR or lack of ACS will be
> > > > dealt with in subsequent patches, as will modifications to the device
> > > > assignment code.
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > >
> > > LGTM, just two comments below.
> > >
> > > > ---
> > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > Cc: Julien Grall <julien.grall@arm.com>
> > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > > > Cc: Tim Deegan <tim@xen.org>
> > > > Cc: Wei Liu <wl@xen.org>
> > > >
> > > > v2:
> > > >  - Move code into new drivers/passthrough/groups.c
> > > >  - Drop the group index.
> > > >  - Handle failure to get group id.
> > > >  - Drop the group devs list.
> > > > ---
> > > >  xen/drivers/passthrough/Makefile    |  1 +
> > > >  xen/drivers/passthrough/groups.c    | 91 +++++++++++++++++++++++++++++++++++++
> > > >  xen/drivers/passthrough/x86/iommu.c |  8 +++-
> > > >  xen/include/xen/iommu.h             |  7 +++
> > > >  xen/include/xen/pci.h               |  2 +
> > > >  5 files changed, 108 insertions(+), 1 deletion(-)
> > > >  create mode 100644 xen/drivers/passthrough/groups.c
> > > >
> > > > diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
> > > > index d50ab188c8..8a77110179 100644
> > > > --- a/xen/drivers/passthrough/Makefile
> > > > +++ b/xen/drivers/passthrough/Makefile
> > > > @@ -4,6 +4,7 @@ subdir-$(CONFIG_X86) += x86
> > > >  subdir-$(CONFIG_ARM) += arm
> > > >
> > > >  obj-y += iommu.o
> > > > +obj-$(CONFIG_HAS_PCI) += groups.o
> > > >  obj-$(CONFIG_HAS_PCI) += pci.o
> > > >  obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
> > > >
> > > > diff --git a/xen/drivers/passthrough/groups.c b/xen/drivers/passthrough/groups.c
> > > > new file mode 100644
> > > > index 0000000000..1a2f461c87
> > > > --- /dev/null
> > > > +++ b/xen/drivers/passthrough/groups.c
> > > > @@ -0,0 +1,91 @@
> > > > +/*
> > > > + * Copyright (c) 2019 Citrix Systems Inc.
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the GNU General Public License as published by
> > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > + * (at your option) any later version.
> > > > + *
> > > > + * This program is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > + * GNU General Public License for more details.
> > > > + */
> > > > +
> > > > +#include <xen/iommu.h>
> > > > +#include <xen/radix-tree.h>
> > > > +
> > > > +struct iommu_group {
> > > > +    unsigned int id;
> > > > +};
> > > > +
> > > > +static struct radix_tree_root iommu_groups;
> > > > +
> > > > +void __init iommu_groups_init(void)
> > > > +{
> > > > +    radix_tree_init(&iommu_groups);
> > > > +}
> > > > +
> > > > +static struct iommu_group *alloc_iommu_group(unsigned int id)
> > > > +{
> > > > +    struct iommu_group *grp = xzalloc(struct iommu_group);
> > > > +
> > > > +    if ( !grp )
> > > > +        return NULL;
> > > > +
> > > > +    grp->id = id;
> > > > +
> > > > +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> > > > +    {
> > > > +        xfree(grp);
> > > > +        grp = NULL;
> > > > +    }
> > > > +
> > > > +    return grp;
> > > > +}
> > > > +
> > > > +static struct iommu_group *get_iommu_group(unsigned int id)
> > > > +{
> > > > +    struct iommu_group *grp = radix_tree_lookup(&iommu_groups, id);
> > > > +
> > > > +    if ( !grp )
> > > > +        grp = alloc_iommu_group(id);
> > > > +
> > > > +    return grp;
> > > > +}
> > > > +
> > > > +int iommu_group_assign(struct pci_dev *pdev, void *arg)
> > >
> > > I'm not sure I see the point of the arg parameter, AFAICT it's
> > > completely unused.
> >
> > It needs to be there because it needs to conform to the all device iterator function callback
> prototype. It is indeed unused though.
> 
> Oh right, sorry for the noise.
> 
> > >
> > > > +{
> > > > +    const struct iommu_ops *ops = iommu_get_ops();
> > > > +    unsigned int id;
> > > > +    struct iommu_group *grp;
> > > > +
> > > > +    if ( !ops->get_device_group_id )
> > > > +        return 0;
> > > > +
> > > > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> > >
> > > I think I would prefer id to be of signed type here, then when you
> > > pass it to get_iommu_group it's already made unsigned.
> >
> > That's a good catch. It's tested for < 0 below so it does need to be signed at this point.
> 
> With that you can add my:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks :-)

  Paul

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 1/4] iommu / x86: move call to scan_pci_devices() out of vendor code

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 15 July 2019 15:39
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Brian Woods
> <brian.woods@amd.com>; Kevin Tian <kevin.tian@intel.com>; Jan Beulich <jbeulich@suse.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v2 1/4] iommu / x86: move call to scan_pci_devices() out of vendor code
> 
> On Mon, Jul 15, 2019 at 01:37:07PM +0100, Paul Durrant wrote:
> > It's not vendor specific so it doesn't really belong there.
> >
> > Scanning the PCI topology also really doesn't have much to do with IOMMU
> > initialization. It doesn't depend on there even being an IOMMU. This patch
> > moves to the call to the beginning of iommu_hardware_setup() but only
> > places it there because the topology information would be otherwise unused.
> >
> > Subsequent patches will actually make use of the PCI topology during
> > (x86) IOMMU initialization.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 

Thanks.

> I would even consider moving the call to scan_pci_devices into
> pci_segments_init instead of doing it in the IOMMU code, as you
> suggest above.

Possibly, although without an IOMMU in the system I don't think there is currently a lot of point in populating the pdev list.

  Paul

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 2/4] pci: add all-device iterator function...

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 15 July 2019 16:21
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> Julien Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 2/4] pci: add all-device iterator function...
> 
> On Mon, Jul 15, 2019 at 01:37:08PM +0100, Paul Durrant wrote:
> > ...and use it for setup_hwdom_pci_devices() and dump_pci_devices().
> >
> > The unlock/process-pending-softirqs/lock sequence that was in
> > _setup_hwdom_pci_devices() is now done in the generic iterator function,
> > which does mean it is also done (unnecessarily) in the case of
> > dump_pci_devices(), since run_all_nonirq_keyhandlers() will call
> > process_pending_softirqs() before invoking each key handler anyway, but
> > this is not performance critical code.
> >
> > The "==== segment XXXX ====" headline that was in _dump_pci_devices() has
> > been dropped because it is non-trivial to deal with it when using a
> > generic all-device iterator and, since the segment number is included
> > in every log line anyway, it didn't add much value anyway.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 

Thanks.

> Just some trivial comments.
> 
> Thanks.
> 
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wl@xen.org>
> >
> > v2:
> >  - New in v2.
> > ---
> >  xen/drivers/passthrough/pci.c | 120 +++++++++++++++++++++++-------------------
> >  xen/include/xen/pci.h         |   1 +
> >  2 files changed, 68 insertions(+), 53 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> > index e88689425d..179cb7e17e 100644
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -1134,54 +1134,78 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom
> *ctxt,
> >                 ctxt->d->domain_id, err);
> >  }
> >
> > -static int __hwdom_init _setup_hwdom_pci_devices(struct pci_seg *pseg, void *arg)
> > +static int __hwdom_init setup_hwdom_pci_device(struct pci_dev *pdev, void *arg)
> >  {
> >      struct setup_hwdom *ctxt = arg;
> > -    int bus, devfn;
> > +    struct domain *d = ctxt->d;
> >
> > -    for ( bus = 0; bus < 256; bus++ )
> > +    if ( !pdev->domain )
> >      {
> > -        for ( devfn = 0; devfn < 256; devfn++ )
> > +        pdev->domain = d;
> > +        list_add(&pdev->domain_list, &d->pdev_list);
> > +        setup_one_hwdom_device(ctxt, pdev);
> > +    }
> > +    else if ( pdev->domain == dom_xen )
> > +    {
> > +        pdev->domain = d;
> > +        setup_one_hwdom_device(ctxt, pdev);
> > +        pdev->domain = dom_xen;
> > +    }
> > +    else if ( pdev->domain != d )
> > +        printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n",
> > +               pdev->domain->domain_id, pdev->seg, pdev->bus,
> 
> You can use %pd here to print the domain.
> 

Oh, I didn't know about that one.

> > +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > +
> > +    return 0;
> > +}
> > +
> > +struct psdi_ctxt {
> > +    int (*cb)(struct pci_dev *, void *);
> > +    void *arg;
> > +};
> > +
> > +static int pci_segment_devices_iterate(struct pci_seg *pseg, void *arg)
> > +{
> > +    struct psdi_ctxt *ctxt = arg;
> > +    int bus, devfn;
> 
> unsigned for both the above.
> 

Ok.

> > +    int rc = 0;
> > +
> > +    /*
> > +     * We don't iterate by walking pseg->alldevs_list here because that
> > +     * would make the pcidevs_unlock()/lock() sequence below unsafe.
> > +     */
> > +    for ( bus = 0; !rc && bus < 256; bus++ )
> > +        for ( devfn = 0; !rc && devfn < 256; devfn++ )
> >          {
> >              struct pci_dev *pdev = pci_get_pdev(pseg->nr, bus, devfn);
> >
> >              if ( !pdev )
> >                  continue;
> >
> > -            if ( !pdev->domain )
> > -            {
> > -                pdev->domain = ctxt->d;
> > -                list_add(&pdev->domain_list, &ctxt->d->pdev_list);
> > -                setup_one_hwdom_device(ctxt, pdev);
> > -            }
> > -            else if ( pdev->domain == dom_xen )
> > -            {
> > -                pdev->domain = ctxt->d;
> > -                setup_one_hwdom_device(ctxt, pdev);
> > -                pdev->domain = dom_xen;
> > -            }
> > -            else if ( pdev->domain != ctxt->d )
> > -                printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n",
> > -                       pdev->domain->domain_id, pseg->nr, bus,
> > -                       PCI_SLOT(devfn), PCI_FUNC(devfn));
> > +            rc = ctxt->cb(pdev, ctxt->arg);
> >
> > -            if ( iommu_verbose )
> > -            {
> > -                pcidevs_unlock();
> > -                process_pending_softirqs();
> > -                pcidevs_lock();
> > -            }
> > -        }
> > -
> > -        if ( !iommu_verbose )
> > -        {
> > +            /*
> > +             * Err on the safe side and assume the callback has taken
> > +             * a significant amount of time.
> > +             */
> >              pcidevs_unlock();
> >              process_pending_softirqs();
> >              pcidevs_lock();
> >          }
> > -    }
> >
> > -    return 0;
> > +    return rc;
> > +}
> > +
> > +int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg)
> > +{
> > +    struct psdi_ctxt ctxt = { .cb = cb, .arg = arg };
> > +    int rc;
> > +
> > +    pcidevs_lock();
> > +    rc = pci_segments_iterate(pci_segment_devices_iterate, &ctxt);
> > +    pcidevs_unlock();
> > +
> > +    return rc;
> >  }
> >
> >  void __hwdom_init setup_hwdom_pci_devices(
> > @@ -1189,9 +1213,7 @@ void __hwdom_init setup_hwdom_pci_devices(
> >  {
> >      struct setup_hwdom ctxt = { .d = d, .handler = handler };
> >
> > -    pcidevs_lock();
> > -    pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt);
> > -    pcidevs_unlock();
> > +    pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt);
> 
> Since this now returns an error code it wold be good to handle it,
> even if it's just:
> 
> rc = pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt);
> if ( rc )
>     ASSERT_UNREACHABLE();
> 
> setup_hwdom_pci_device will always return 0, hence a failure here is
> not possible AFAICT. Same with the other usage of pci_pdevs_iterate
> below.

An ASSERT in setup_hwdom_pci_devices() sounds like a good idea. I think I'll leave one out of dump_pci_devices() though, since that's a debug-key handler.

  Paul

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 1/4] iommu / x86: move call to scan_pci_devices() out of vendor code

[Woods, Brian]

On July 15, 2019 7:37:17 AM Paul Durrant <paul.durrant@citrix.com> wrote:

> It's not vendor specific so it doesn't really belong there.
>
>
> Scanning the PCI topology also really doesn't have much to do with IOMMU
> initialization. It doesn't depend on there even being an IOMMU. This patch
> moves to the call to the beginning of iommu_hardware_setup() but only
> places it there because the topology information would be otherwise unused.
>
>
> Subsequent patches will actually make use of the PCI topology during
> (x86) IOMMU initialization.
>
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Brian Woods <brian.woods@amd.com>

> ---
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
>
>
> v2:
> - Expanded commit comment.
> - Moved PCI scan to before IOMMU initialization, rather than after it.
> ---
> xen/drivers/passthrough/amd/pci_amd_iommu.c | 3 ++-
> xen/drivers/passthrough/vtd/iommu.c         | 4 ----
> xen/drivers/passthrough/x86/iommu.c         | 6 ++++++
> 3 files changed, 8 insertions(+), 5 deletions(-)
>
>
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 4afbcd1609..3338a8e0e8 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -180,7 +180,8 @@ static int __init iov_detect(void)
>
>     if ( !amd_iommu_perdev_intremap )
>         printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
> -    return scan_pci_devices();
> +
> +    return 0;
> }
>
> int amd_iommu_alloc_root(struct domain_iommu *hd)
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 8b27d7e775..b0e3bf26b5 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2372,10 +2372,6 @@ static int __init vtd_setup(void)
>     P(iommu_hap_pt_share, "Shared EPT tables");
> #undef P
>
> -    ret = scan_pci_devices();
> -    if ( ret )
> -        goto error;
> -
>     ret = init_vtd_hw();
>     if ( ret )
>         goto error;
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index 0fa6dcc3fd..a7438c9c25 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -28,9 +28,15 @@ struct iommu_ops __read_mostly iommu_ops;
>
> int __init iommu_hardware_setup(void)
> {
> +    int rc;
> +
>     if ( !iommu_init_ops )
>         return -ENODEV;
>
> +    rc = scan_pci_devices();
> +    if ( rc )
> +        return rc;
> +
>     if ( !iommu_ops.init )
>         iommu_ops = *iommu_init_ops->ops;
>     else
> --
> 2.11.0


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