[PATCH 0/5] iommu groups + cleanup

[PATCH 0/5] 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 (5):
  iommu: trivial re-organisation to avoid unnecessary test
  iommu / x86: move call to scan_pci_devices() out of vendor code
  iommu: move iommu_get_ops() into common code
  iommu: introduce iommu_groups
  iommu / pci: re-implement XEN_DOMCTL_get_device_group...

 xen/drivers/passthrough/amd/pci_amd_iommu.c |   3 +-
 xen/drivers/passthrough/arm/iommu.c         |  17 ---
 xen/drivers/passthrough/arm/smmu.c          |   2 +-
 xen/drivers/passthrough/iommu.c             | 160 +++++++++++++++++++++++++++-
 xen/drivers/passthrough/pci.c               |  50 +--------
 xen/drivers/passthrough/vtd/extern.h        |   1 -
 xen/drivers/passthrough/vtd/iommu.c         |   8 +-
 xen/drivers/passthrough/x86/iommu.c         |  25 +++--
 xen/include/asm-arm/iommu.h                 |   3 -
 xen/include/asm-x86/iommu.h                 |  20 ++--
 xen/include/xen/iommu.h                     |  12 +++
 xen/include/xen/pci.h                       |   3 +
 12 files changed, 204 insertions(+), 100 deletions(-)
---
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 <wei.liu2@citrix.com>
-- 
2.11.0


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

[Xen-devel] [PATCH 0/5] 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 (5):
  iommu: trivial re-organisation to avoid unnecessary test
  iommu / x86: move call to scan_pci_devices() out of vendor code
  iommu: move iommu_get_ops() into common code
  iommu: introduce iommu_groups
  iommu / pci: re-implement XEN_DOMCTL_get_device_group...

 xen/drivers/passthrough/amd/pci_amd_iommu.c |   3 +-
 xen/drivers/passthrough/arm/iommu.c         |  17 ---
 xen/drivers/passthrough/arm/smmu.c          |   2 +-
 xen/drivers/passthrough/iommu.c             | 160 +++++++++++++++++++++++++++-
 xen/drivers/passthrough/pci.c               |  50 +--------
 xen/drivers/passthrough/vtd/extern.h        |   1 -
 xen/drivers/passthrough/vtd/iommu.c         |   8 +-
 xen/drivers/passthrough/x86/iommu.c         |  25 +++--
 xen/include/asm-arm/iommu.h                 |   3 -
 xen/include/asm-x86/iommu.h                 |  20 ++--
 xen/include/xen/iommu.h                     |  12 +++
 xen/include/xen/pci.h                       |   3 +
 12 files changed, 204 insertions(+), 100 deletions(-)
---
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 <wei.liu2@citrix.com>
-- 
2.11.0


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

[PATCH 1/5] iommu: trivial re-organisation to avoid unnecessary test

[Paul Durrant]

An 'if ( !iommu_enabled )' followed by an 'if ( iommu_enabled )' with
only a printk() in between seems a little silly. Move the printk() and
use 'else' instead.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/passthrough/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index a6697d58fb..b453b32191 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -512,14 +512,14 @@ int __init iommu_setup(void)
     if ( !iommu_intremap )
         iommu_intpost = 0;
 
+    printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis");
     if ( !iommu_enabled )
     {
         iommu_snoop = 0;
         iommu_hwdom_passthrough = false;
         iommu_hwdom_strict = false;
     }
-    printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis");
-    if ( iommu_enabled )
+    else
     {
         printk(" - Dom0 mode: %s\n",
                iommu_hwdom_passthrough ? "Passthrough" :
-- 
2.11.0


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

[Xen-devel] [PATCH 1/5] iommu: trivial re-organisation to avoid unnecessary test

[Paul Durrant]

An 'if ( !iommu_enabled )' followed by an 'if ( iommu_enabled )' with
only a printk() in between seems a little silly. Move the printk() and
use 'else' instead.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/passthrough/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index a6697d58fb..b453b32191 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -512,14 +512,14 @@ int __init iommu_setup(void)
     if ( !iommu_intremap )
         iommu_intpost = 0;
 
+    printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis");
     if ( !iommu_enabled )
     {
         iommu_snoop = 0;
         iommu_hwdom_passthrough = false;
         iommu_hwdom_strict = false;
     }
-    printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis");
-    if ( iommu_enabled )
+    else
     {
         printk(" - Dom0 mode: %s\n",
                iommu_hwdom_passthrough ? "Passthrough" :
-- 
2.11.0


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

[PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

[Paul Durrant]

It's not vendor specific so it shouldn't really be there.

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 <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 3 ++-
 xen/drivers/passthrough/vtd/iommu.c         | 4 ----
 xen/drivers/passthrough/x86/iommu.c         | 9 ++++++++-
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index dbc71ca7d5..872bbe21c2 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -182,7 +182,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 7b9e09a084..f9c76f594c 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 034ac903dd..895c7fb564 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -28,6 +28,8 @@ struct iommu_ops __read_mostly iommu_ops;
 
 int __init iommu_hardware_setup(void)
 {
+    int rc;
+
     if ( !iommu_init_ops )
         return -ENODEV;
 
@@ -37,7 +39,12 @@ 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 )
+        rc = scan_pci_devices();
+
+    return rc;
 }
 
 int iommu_enable_x2apic(void)
-- 
2.11.0


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

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

[Paul Durrant]

It's not vendor specific so it shouldn't really be there.

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 <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 3 ++-
 xen/drivers/passthrough/vtd/iommu.c         | 4 ----
 xen/drivers/passthrough/x86/iommu.c         | 9 ++++++++-
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index dbc71ca7d5..872bbe21c2 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -182,7 +182,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 7b9e09a084..f9c76f594c 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 034ac903dd..895c7fb564 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -28,6 +28,8 @@ struct iommu_ops __read_mostly iommu_ops;
 
 int __init iommu_hardware_setup(void)
 {
+    int rc;
+
     if ( !iommu_init_ops )
         return -ENODEV;
 
@@ -37,7 +39,12 @@ 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 )
+        rc = scan_pci_devices();
+
+    return rc;
 }
 
 int iommu_enable_x2apic(void)
-- 
2.11.0


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

[PATCH 3/5] iommu: move iommu_get_ops() into common code

[Paul Durrant]

Currently x86 and ARM differ in their implementation for no good reason.
This patch moves the ARM variant of iommu_get/set_ops() helpers into
common code and modifies them so they deal with the __initconstrel
ops structures used by the x86 IOMMU vendor implementations (adding
__initconstrel to the SMMU code to bring it in line). Consequently, a lack
of init() method is now taken to mean uninitialized iommu_ops. Also, the
printk warning in iommu_set_ops() now becomes an ASSERT.

NOTE: This patch also gets rid of the extern intel_iommu_ops as it is
      no longer necessary.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/drivers/passthrough/arm/iommu.c  | 17 -----------------
 xen/drivers/passthrough/arm/smmu.c   |  2 +-
 xen/drivers/passthrough/iommu.c      | 15 +++++++++++++++
 xen/drivers/passthrough/vtd/extern.h |  1 -
 xen/drivers/passthrough/vtd/iommu.c  |  4 ++--
 xen/drivers/passthrough/x86/iommu.c  | 16 +++++++---------
 xen/include/asm-arm/iommu.h          |  3 ---
 xen/include/asm-x86/iommu.h          | 20 ++++++++------------
 xen/include/xen/iommu.h              |  3 +++
 9 files changed, 36 insertions(+), 45 deletions(-)

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 325997b19f..c226ed18e3 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -20,23 +20,6 @@
 #include <xen/device_tree.h>
 #include <asm/device.h>
 
-static const struct iommu_ops *iommu_ops;
-
-const struct iommu_ops *iommu_get_ops(void)
-{
-    return iommu_ops;
-}
-
-void __init iommu_set_ops(const struct iommu_ops *ops)
-{
-    BUG_ON(ops == NULL);
-
-    if ( iommu_ops && iommu_ops != ops )
-        printk("WARNING: Cannot set IOMMU ops, already set to a different value\n");
-
-    iommu_ops = ops;
-}
-
 int __init iommu_hardware_setup(void)
 {
     struct dt_device_node *np;
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index f151b9f5b5..f01061a218 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1989,7 +1989,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	}
 }
 
-static const struct iommu_ops arm_smmu_ops = {
+static const struct iommu_ops __initconstrel arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_init		= arm_smmu_domain_init,
 	.domain_destroy		= arm_smmu_domain_destroy,
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index b453b32191..d3a6199b77 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -21,6 +21,21 @@
 #include <xen/keyhandler.h>
 #include <xsm/xsm.h>
 
+static struct iommu_ops __read_mostly iommu_ops;
+
+const struct iommu_ops *iommu_get_ops(void)
+{
+    return &iommu_ops;
+}
+
+void __init iommu_set_ops(const struct iommu_ops *ops)
+{
+    BUG_ON(!ops);
+
+    ASSERT(!iommu_ops.init || iommu_ops.init == ops->init);
+    iommu_ops = *ops;
+}
+
 static void iommu_dump_p2m_table(unsigned char key);
 
 unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000;
diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index 331d6e64f7..0ae5ddf6d0 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -28,7 +28,6 @@
 struct pci_ats_dev;
 extern bool_t rwbf_quirk;
 extern const struct iommu_init_ops intel_iommu_init_ops;
-extern const struct iommu_ops intel_iommu_ops;
 
 void print_iommu_regs(struct acpi_drhd_unit *drhd);
 void print_vtd_entries(struct iommu *iommu, int bus, int devfn, u64 gmfn);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index f9c76f594c..db77655260 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2700,7 +2700,7 @@ static void vtd_dump_p2m_table(struct domain *d)
     vtd_dump_p2m_table_level(hd->arch.pgd_maddr, agaw_to_level(hd->arch.agaw), 0, 0);
 }
 
-const struct iommu_ops __initconstrel intel_iommu_ops = {
+static const struct iommu_ops __initconstrel _iommu_ops = {
     .init = intel_iommu_domain_init,
     .hwdom_init = intel_iommu_hwdom_init,
     .add_device = intel_iommu_add_device,
@@ -2733,7 +2733,7 @@ const struct iommu_ops __initconstrel intel_iommu_ops = {
 };
 
 const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
-    .ops = &intel_iommu_ops,
+    .ops = &_iommu_ops,
     .setup = vtd_setup,
     .supports_x2apic = intel_iommu_supports_eim,
 };
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 895c7fb564..d9eaf1e62b 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -24,7 +24,6 @@
 #include <asm/setup.h>
 
 const struct iommu_init_ops *__initdata iommu_init_ops;
-struct iommu_ops __read_mostly iommu_ops;
 
 int __init iommu_hardware_setup(void)
 {
@@ -33,11 +32,7 @@ int __init iommu_hardware_setup(void)
     if ( !iommu_init_ops )
         return -ENODEV;
 
-    if ( !iommu_ops.init )
-        iommu_ops = *iommu_init_ops->ops;
-    else
-        /* x2apic setup may have previously initialised the struct. */
-        ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
+    iommu_set_ops(iommu_init_ops->ops);
 
     rc = iommu_init_ops->setup();
 
@@ -49,20 +44,23 @@ int __init iommu_hardware_setup(void)
 
 int iommu_enable_x2apic(void)
 {
+    const struct iommu_ops *ops;
+
     if ( system_state < SYS_STATE_active )
     {
         if ( !iommu_supports_x2apic() )
             return -EOPNOTSUPP;
 
-        iommu_ops = *iommu_init_ops->ops;
+        iommu_set_ops(iommu_init_ops->ops);
     }
     else if ( !x2apic_enabled )
         return -EOPNOTSUPP;
 
-    if ( !iommu_ops.enable_x2apic )
+    ops = iommu_get_ops();
+    if ( !ops->enable_x2apic )
         return -EOPNOTSUPP;
 
-    return iommu_ops.enable_x2apic();
+    return ops->enable_x2apic();
 }
 
 void iommu_update_ire_from_apic(
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index 904c9aec11..fb4ca23b69 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -23,9 +23,6 @@ struct arch_iommu
 /* Always share P2M Table between the CPU and the IOMMU */
 #define iommu_use_hap_pt(d) (has_iommu_pt(d))
 
-const struct iommu_ops *iommu_get_ops(void);
-void iommu_set_ops(const struct iommu_ops *ops);
-
 #endif /* __ARCH_ARM_IOMMU_H__ */
 
 /*
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index bbdb05f5f0..2d8716d673 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -57,14 +57,6 @@ struct arch_iommu
     struct guest_iommu *g_iommu;
 };
 
-extern struct iommu_ops iommu_ops;
-
-static inline const struct iommu_ops *iommu_get_ops(void)
-{
-    BUG_ON(!iommu_ops.init);
-    return &iommu_ops;
-}
-
 struct iommu_init_ops {
     const struct iommu_ops *ops;
     int (*setup)(void);
@@ -83,8 +75,10 @@ int iommu_setup_hpet_msi(struct msi_desc *);
 
 static inline int iommu_adjust_irq_affinities(void)
 {
-    return iommu_ops.adjust_irq_affinities
-           ? iommu_ops.adjust_irq_affinities()
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    return ops->adjust_irq_affinities
+           ? ops->adjust_irq_affinities()
            : 0;
 }
 
@@ -103,8 +97,10 @@ int iommu_enable_x2apic(void);
 
 static inline void iommu_disable_x2apic(void)
 {
-    if ( x2apic_enabled && iommu_ops.disable_x2apic )
-        iommu_ops.disable_x2apic();
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( x2apic_enabled && ops->disable_x2apic )
+        ops->disable_x2apic();
 }
 
 extern bool untrusted_msi;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5d3c1619c4..b2d429a6ef 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -64,6 +64,9 @@ extern int8_t iommu_hwdom_reserved;
 
 extern unsigned int iommu_dev_iotlb_timeout;
 
+const struct iommu_ops *iommu_get_ops(void);
+void iommu_set_ops(const struct iommu_ops *ops);
+
 int iommu_setup(void);
 int iommu_hardware_setup(void);
 
-- 
2.11.0


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

[Xen-devel] [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Paul Durrant]

Currently x86 and ARM differ in their implementation for no good reason.
This patch moves the ARM variant of iommu_get/set_ops() helpers into
common code and modifies them so they deal with the __initconstrel
ops structures used by the x86 IOMMU vendor implementations (adding
__initconstrel to the SMMU code to bring it in line). Consequently, a lack
of init() method is now taken to mean uninitialized iommu_ops. Also, the
printk warning in iommu_set_ops() now becomes an ASSERT.

NOTE: This patch also gets rid of the extern intel_iommu_ops as it is
      no longer necessary.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/drivers/passthrough/arm/iommu.c  | 17 -----------------
 xen/drivers/passthrough/arm/smmu.c   |  2 +-
 xen/drivers/passthrough/iommu.c      | 15 +++++++++++++++
 xen/drivers/passthrough/vtd/extern.h |  1 -
 xen/drivers/passthrough/vtd/iommu.c  |  4 ++--
 xen/drivers/passthrough/x86/iommu.c  | 16 +++++++---------
 xen/include/asm-arm/iommu.h          |  3 ---
 xen/include/asm-x86/iommu.h          | 20 ++++++++------------
 xen/include/xen/iommu.h              |  3 +++
 9 files changed, 36 insertions(+), 45 deletions(-)

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 325997b19f..c226ed18e3 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -20,23 +20,6 @@
 #include <xen/device_tree.h>
 #include <asm/device.h>
 
-static const struct iommu_ops *iommu_ops;
-
-const struct iommu_ops *iommu_get_ops(void)
-{
-    return iommu_ops;
-}
-
-void __init iommu_set_ops(const struct iommu_ops *ops)
-{
-    BUG_ON(ops == NULL);
-
-    if ( iommu_ops && iommu_ops != ops )
-        printk("WARNING: Cannot set IOMMU ops, already set to a different value\n");
-
-    iommu_ops = ops;
-}
-
 int __init iommu_hardware_setup(void)
 {
     struct dt_device_node *np;
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index f151b9f5b5..f01061a218 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1989,7 +1989,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	}
 }
 
-static const struct iommu_ops arm_smmu_ops = {
+static const struct iommu_ops __initconstrel arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_init		= arm_smmu_domain_init,
 	.domain_destroy		= arm_smmu_domain_destroy,
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index b453b32191..d3a6199b77 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -21,6 +21,21 @@
 #include <xen/keyhandler.h>
 #include <xsm/xsm.h>
 
+static struct iommu_ops __read_mostly iommu_ops;
+
+const struct iommu_ops *iommu_get_ops(void)
+{
+    return &iommu_ops;
+}
+
+void __init iommu_set_ops(const struct iommu_ops *ops)
+{
+    BUG_ON(!ops);
+
+    ASSERT(!iommu_ops.init || iommu_ops.init == ops->init);
+    iommu_ops = *ops;
+}
+
 static void iommu_dump_p2m_table(unsigned char key);
 
 unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000;
diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index 331d6e64f7..0ae5ddf6d0 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -28,7 +28,6 @@
 struct pci_ats_dev;
 extern bool_t rwbf_quirk;
 extern const struct iommu_init_ops intel_iommu_init_ops;
-extern const struct iommu_ops intel_iommu_ops;
 
 void print_iommu_regs(struct acpi_drhd_unit *drhd);
 void print_vtd_entries(struct iommu *iommu, int bus, int devfn, u64 gmfn);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index f9c76f594c..db77655260 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2700,7 +2700,7 @@ static void vtd_dump_p2m_table(struct domain *d)
     vtd_dump_p2m_table_level(hd->arch.pgd_maddr, agaw_to_level(hd->arch.agaw), 0, 0);
 }
 
-const struct iommu_ops __initconstrel intel_iommu_ops = {
+static const struct iommu_ops __initconstrel _iommu_ops = {
     .init = intel_iommu_domain_init,
     .hwdom_init = intel_iommu_hwdom_init,
     .add_device = intel_iommu_add_device,
@@ -2733,7 +2733,7 @@ const struct iommu_ops __initconstrel intel_iommu_ops = {
 };
 
 const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
-    .ops = &intel_iommu_ops,
+    .ops = &_iommu_ops,
     .setup = vtd_setup,
     .supports_x2apic = intel_iommu_supports_eim,
 };
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 895c7fb564..d9eaf1e62b 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -24,7 +24,6 @@
 #include <asm/setup.h>
 
 const struct iommu_init_ops *__initdata iommu_init_ops;
-struct iommu_ops __read_mostly iommu_ops;
 
 int __init iommu_hardware_setup(void)
 {
@@ -33,11 +32,7 @@ int __init iommu_hardware_setup(void)
     if ( !iommu_init_ops )
         return -ENODEV;
 
-    if ( !iommu_ops.init )
-        iommu_ops = *iommu_init_ops->ops;
-    else
-        /* x2apic setup may have previously initialised the struct. */
-        ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
+    iommu_set_ops(iommu_init_ops->ops);
 
     rc = iommu_init_ops->setup();
 
@@ -49,20 +44,23 @@ int __init iommu_hardware_setup(void)
 
 int iommu_enable_x2apic(void)
 {
+    const struct iommu_ops *ops;
+
     if ( system_state < SYS_STATE_active )
     {
         if ( !iommu_supports_x2apic() )
             return -EOPNOTSUPP;
 
-        iommu_ops = *iommu_init_ops->ops;
+        iommu_set_ops(iommu_init_ops->ops);
     }
     else if ( !x2apic_enabled )
         return -EOPNOTSUPP;
 
-    if ( !iommu_ops.enable_x2apic )
+    ops = iommu_get_ops();
+    if ( !ops->enable_x2apic )
         return -EOPNOTSUPP;
 
-    return iommu_ops.enable_x2apic();
+    return ops->enable_x2apic();
 }
 
 void iommu_update_ire_from_apic(
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index 904c9aec11..fb4ca23b69 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -23,9 +23,6 @@ struct arch_iommu
 /* Always share P2M Table between the CPU and the IOMMU */
 #define iommu_use_hap_pt(d) (has_iommu_pt(d))
 
-const struct iommu_ops *iommu_get_ops(void);
-void iommu_set_ops(const struct iommu_ops *ops);
-
 #endif /* __ARCH_ARM_IOMMU_H__ */
 
 /*
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index bbdb05f5f0..2d8716d673 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -57,14 +57,6 @@ struct arch_iommu
     struct guest_iommu *g_iommu;
 };
 
-extern struct iommu_ops iommu_ops;
-
-static inline const struct iommu_ops *iommu_get_ops(void)
-{
-    BUG_ON(!iommu_ops.init);
-    return &iommu_ops;
-}
-
 struct iommu_init_ops {
     const struct iommu_ops *ops;
     int (*setup)(void);
@@ -83,8 +75,10 @@ int iommu_setup_hpet_msi(struct msi_desc *);
 
 static inline int iommu_adjust_irq_affinities(void)
 {
-    return iommu_ops.adjust_irq_affinities
-           ? iommu_ops.adjust_irq_affinities()
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    return ops->adjust_irq_affinities
+           ? ops->adjust_irq_affinities()
            : 0;
 }
 
@@ -103,8 +97,10 @@ int iommu_enable_x2apic(void);
 
 static inline void iommu_disable_x2apic(void)
 {
-    if ( x2apic_enabled && iommu_ops.disable_x2apic )
-        iommu_ops.disable_x2apic();
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( x2apic_enabled && ops->disable_x2apic )
+        ops->disable_x2apic();
 }
 
 extern bool untrusted_msi;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5d3c1619c4..b2d429a6ef 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -64,6 +64,9 @@ extern int8_t iommu_hwdom_reserved;
 
 extern unsigned int iommu_dev_iotlb_timeout;
 
+const struct iommu_ops *iommu_get_ops(void);
+void iommu_set_ops(const struct iommu_ops *ops);
+
 int iommu_setup(void);
 int iommu_hardware_setup(void);
 
-- 
2.11.0


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

[PATCH 4/5] 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 <wei.liu2@citrix.com>
---
 xen/drivers/passthrough/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c   |  3 ++
 xen/include/xen/iommu.h         |  7 ++++
 xen/include/xen/pci.h           |  3 ++
 4 files changed, 89 insertions(+)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index d3a6199b77..11319fbaae 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
     }
 }
 
+#ifdef CONFIG_HAS_PCI
+
+struct iommu_group {
+    unsigned int id;
+    unsigned int index;
+    struct list_head devs_list;
+};
+
+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;
+    static unsigned int index;
+
+    grp = xzalloc(struct iommu_group);
+    if ( !grp )
+        return NULL;
+
+    grp->id = id;
+    grp->index = index++;
+    INIT_LIST_HEAD(&grp->devs_list);
+
+    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)
+{
+    const struct iommu_ops *ops;
+    unsigned int id;
+    struct iommu_group *grp;
+
+    ops = iommu_get_ops();
+    if ( !ops || !ops->get_device_group_id )
+        return 0;
+
+    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
+    grp = get_iommu_group(id);
+
+    if ( ! grp )
+        return -ENOMEM;
+
+    if ( iommu_verbose )
+        printk(XENLOG_INFO "Assign %04x:%02x:%02x.%u -> IOMMU group %u\n",
+               pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+               PCI_FUNC(pdev->devfn), grp->index);
+
+    list_add(&pdev->grpdevs_list, &grp->devs_list);
+    pdev->grp = grp;
+
+    return 0;
+}
+
+#endif /* CONFIG_HAS_PCI */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 8108ed5f9a..6210409741 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -427,6 +427,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
 
     check_pdev(pdev);
     apply_quirks(pdev);
+    iommu_group_assign(pdev);
 
     return pdev;
 }
@@ -1098,6 +1099,8 @@ int __init scan_pci_devices(void)
 {
     int ret;
 
+    iommu_groups_init();
+
     pcidevs_lock();
     ret = pci_segments_iterate(_scan_pci_devices, NULL);
     pcidevs_unlock();
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index b2d429a6ef..6d6aa12314 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -315,6 +315,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);
+
+#endif /* CONFIG_HAS_PCI */
+
 #endif /* _IOMMU_H_ */
 
 /*
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 8b21e8dc84..5fe7525db6 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -75,6 +75,9 @@ struct pci_dev {
     struct list_head alldevs_list;
     struct list_head domain_list;
 
+    struct list_head grpdevs_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 4/5] 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 <wei.liu2@citrix.com>
---
 xen/drivers/passthrough/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c   |  3 ++
 xen/include/xen/iommu.h         |  7 ++++
 xen/include/xen/pci.h           |  3 ++
 4 files changed, 89 insertions(+)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index d3a6199b77..11319fbaae 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
     }
 }
 
+#ifdef CONFIG_HAS_PCI
+
+struct iommu_group {
+    unsigned int id;
+    unsigned int index;
+    struct list_head devs_list;
+};
+
+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;
+    static unsigned int index;
+
+    grp = xzalloc(struct iommu_group);
+    if ( !grp )
+        return NULL;
+
+    grp->id = id;
+    grp->index = index++;
+    INIT_LIST_HEAD(&grp->devs_list);
+
+    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)
+{
+    const struct iommu_ops *ops;
+    unsigned int id;
+    struct iommu_group *grp;
+
+    ops = iommu_get_ops();
+    if ( !ops || !ops->get_device_group_id )
+        return 0;
+
+    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
+    grp = get_iommu_group(id);
+
+    if ( ! grp )
+        return -ENOMEM;
+
+    if ( iommu_verbose )
+        printk(XENLOG_INFO "Assign %04x:%02x:%02x.%u -> IOMMU group %u\n",
+               pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+               PCI_FUNC(pdev->devfn), grp->index);
+
+    list_add(&pdev->grpdevs_list, &grp->devs_list);
+    pdev->grp = grp;
+
+    return 0;
+}
+
+#endif /* CONFIG_HAS_PCI */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 8108ed5f9a..6210409741 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -427,6 +427,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
 
     check_pdev(pdev);
     apply_quirks(pdev);
+    iommu_group_assign(pdev);
 
     return pdev;
 }
@@ -1098,6 +1099,8 @@ int __init scan_pci_devices(void)
 {
     int ret;
 
+    iommu_groups_init();
+
     pcidevs_lock();
     ret = pci_segments_iterate(_scan_pci_devices, NULL);
     pcidevs_unlock();
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index b2d429a6ef..6d6aa12314 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -315,6 +315,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);
+
+#endif /* CONFIG_HAS_PCI */
+
 #endif /* _IOMMU_H_ */
 
 /*
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 8b21e8dc84..5fe7525db6 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -75,6 +75,9 @@ struct pci_dev {
     struct list_head alldevs_list;
     struct list_head domain_list;
 
+    struct list_head grpdevs_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

[PATCH 5/5] 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 relevant iommu_group and walking
the membership list.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/passthrough/iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c   | 47 -----------------------------
 xen/include/xen/iommu.h         |  2 ++
 3 files changed, 67 insertions(+), 47 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 11319fbaae..49140c652e 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -729,6 +729,71 @@ int iommu_group_assign(struct pci_dev *pdev)
     return 0;
 }
 
+static struct iommu_group *iommu_group_lookup(uint16_t seg, uint8_t bus,
+                                              uint8_t devfn)
+{
+    unsigned int id = 0;
+    struct iommu_group *grp;
+
+    while ( radix_tree_gang_lookup(&iommu_groups, (void **)&grp, id, 1) )
+    {
+        struct pci_dev *pdev;
+
+        list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
+            if ( pdev->seg == seg && pdev->bus == bus &&
+                 pdev->devfn == devfn )
+                return grp;
+
+        id = grp->id + 1;
+    }
+
+    return NULL;
+}
+
+int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,
+                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
+{
+    struct iommu_group *grp;
+    struct pci_dev *pdev;
+    int i = 0;
+
+    pcidevs_lock();
+
+    grp = iommu_group_lookup(seg, bus, devfn);
+    if ( !grp )
+    {
+        pcidevs_unlock();
+        return 0;
+    }
+
+    list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
+    {
+        uint32_t sbdf;
+
+        if ( i >= max_sdevs )
+            break;
+
+        if ( pdev->domain != d )
+            continue;
+
+        sbdf = PCI_SBDF3(pdev->seg, pdev->bus, pdev->devfn);
+
+        if ( xsm_get_device_group(XSM_HOOK, sbdf) )
+            continue;
+
+        if ( unlikely(copy_to_guest_offset(buf, i, &sbdf, 1)) )
+        {
+            pcidevs_unlock();
+            return -1;
+        }
+        i++;
+    }
+
+    pcidevs_unlock();
+
+    return i;
+}
+
 #endif /* CONFIG_HAS_PCI */
 
 /*
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 6210409741..68b2883ed6 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1554,53 +1554,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();
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 6d6aa12314..c4e1604bb8 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -319,6 +319,8 @@ extern struct page_list_head iommu_pt_cleanup_list;
 
 void iommu_groups_init(void);
 int iommu_group_assign(struct pci_dev *pdev);
+int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,
+                           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

[Xen-devel] [PATCH 5/5] 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 relevant iommu_group and walking
the membership list.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/passthrough/iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c   | 47 -----------------------------
 xen/include/xen/iommu.h         |  2 ++
 3 files changed, 67 insertions(+), 47 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 11319fbaae..49140c652e 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -729,6 +729,71 @@ int iommu_group_assign(struct pci_dev *pdev)
     return 0;
 }
 
+static struct iommu_group *iommu_group_lookup(uint16_t seg, uint8_t bus,
+                                              uint8_t devfn)
+{
+    unsigned int id = 0;
+    struct iommu_group *grp;
+
+    while ( radix_tree_gang_lookup(&iommu_groups, (void **)&grp, id, 1) )
+    {
+        struct pci_dev *pdev;
+
+        list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
+            if ( pdev->seg == seg && pdev->bus == bus &&
+                 pdev->devfn == devfn )
+                return grp;
+
+        id = grp->id + 1;
+    }
+
+    return NULL;
+}
+
+int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,
+                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
+{
+    struct iommu_group *grp;
+    struct pci_dev *pdev;
+    int i = 0;
+
+    pcidevs_lock();
+
+    grp = iommu_group_lookup(seg, bus, devfn);
+    if ( !grp )
+    {
+        pcidevs_unlock();
+        return 0;
+    }
+
+    list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
+    {
+        uint32_t sbdf;
+
+        if ( i >= max_sdevs )
+            break;
+
+        if ( pdev->domain != d )
+            continue;
+
+        sbdf = PCI_SBDF3(pdev->seg, pdev->bus, pdev->devfn);
+
+        if ( xsm_get_device_group(XSM_HOOK, sbdf) )
+            continue;
+
+        if ( unlikely(copy_to_guest_offset(buf, i, &sbdf, 1)) )
+        {
+            pcidevs_unlock();
+            return -1;
+        }
+        i++;
+    }
+
+    pcidevs_unlock();
+
+    return i;
+}
+
 #endif /* CONFIG_HAS_PCI */
 
 /*
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 6210409741..68b2883ed6 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1554,53 +1554,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();
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 6d6aa12314..c4e1604bb8 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -319,6 +319,8 @@ extern struct page_list_head iommu_pt_cleanup_list;
 
 void iommu_groups_init(void);
 int iommu_group_assign(struct pci_dev *pdev);
+int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,
+                           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: [PATCH 1/5] iommu: trivial re-organisation to avoid unnecessary test

[Jan Beulich]

>>> On 08.05.19 at 15:23, <paul.durrant@citrix.com> wrote:
> An 'if ( !iommu_enabled )' followed by an 'if ( iommu_enabled )' with
> only a printk() in between seems a little silly. Move the printk() and
> use 'else' instead.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [Xen-devel] [PATCH 1/5] iommu: trivial re-organisation to avoid unnecessary test

[Jan Beulich]

>>> On 08.05.19 at 15:23, <paul.durrant@citrix.com> wrote:
> An 'if ( !iommu_enabled )' followed by an 'if ( iommu_enabled )' with
> only a printk() in between seems a little silly. Move the printk() and
> use 'else' instead.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

[Jan Beulich]

>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> It's not vendor specific so it shouldn't really be there.

Perhaps, but this needs better justification:

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

Even after some looking around, it's not obvious to me that the latter
call doesn't depend on PCI devices being known, more specifically
segment 0's bus2bridge[] having been filled. Nor can I tell whether
there would be some noticeable misbehavior (prior to any guests
starting) if there was a dependency and it got broken by the re-
ordering.

Jan



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

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

[Jan Beulich]

>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> It's not vendor specific so it shouldn't really be there.

Perhaps, but this needs better justification:

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

Even after some looking around, it's not obvious to me that the latter
call doesn't depend on PCI devices being known, more specifically
segment 0's bus2bridge[] having been filled. Nor can I tell whether
there would be some noticeable misbehavior (prior to any guests
starting) if there was a dependency and it got broken by the re-
ordering.

Jan



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

Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Jan Beulich]

>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> Currently x86 and ARM differ in their implementation for no good reason.
> This patch moves the ARM variant of iommu_get/set_ops() helpers into
> common code and modifies them so they deal with the __initconstrel
> ops structures used by the x86 IOMMU vendor implementations (adding
> __initconstrel to the SMMU code to bring it in line). Consequently, a lack
> of init() method is now taken to mean uninitialized iommu_ops. Also, the
> printk warning in iommu_set_ops() now becomes an ASSERT.

When having submitted the indirect call overhead reduction series
including IOMMU changes for the first time, I was told that the Arm
folks would like to retain the ability to eventually support
heterogeneous IOMMUs (and hence I shouldn't provide patching
infrastructure there). A single global iommu_[gs]et_ops() is sort of
getting in the way of this as well, I think, and hence I'm not sure it
is a desirable step to make this so far Arm-specific arrangement
the general model. At least it would further complicate Arm side
changes towards that (mid / long term?) goal.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -21,6 +21,21 @@
>  #include <xen/keyhandler.h>
>  #include <xsm/xsm.h>
>  
> +static struct iommu_ops __read_mostly iommu_ops;
> +
> +const struct iommu_ops *iommu_get_ops(void)
> +{
> +    return &iommu_ops;
> +}
> +
> +void __init iommu_set_ops(const struct iommu_ops *ops)
> +{
> +    BUG_ON(!ops);
> +
> +    ASSERT(!iommu_ops.init || iommu_ops.init == ops->init);
> +    iommu_ops = *ops;
> +}

I realize that you merely move (and slightly re-arrange) what has
been there, but now that I look at it again I think ops->init should
also be verified to be non-NULL, or else installing such a set of
hooks would effectively revert back to the "no hooks yet" state.

> @@ -33,11 +32,7 @@ int __init iommu_hardware_setup(void)
>      if ( !iommu_init_ops )
>          return -ENODEV;
>  
> -    if ( !iommu_ops.init )
> -        iommu_ops = *iommu_init_ops->ops;
> -    else
> -        /* x2apic setup may have previously initialised the struct. */
> -        ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
> +    iommu_set_ops(iommu_init_ops->ops);

I was specifically asked to add the comment that you get rid of.
While mentioning x2APIC in common code may no be appropriate,
I'm sure this could be worded in a more general way and attached
to the moved check.

Jan



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

Re: [Xen-devel] [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Jan Beulich]

>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> Currently x86 and ARM differ in their implementation for no good reason.
> This patch moves the ARM variant of iommu_get/set_ops() helpers into
> common code and modifies them so they deal with the __initconstrel
> ops structures used by the x86 IOMMU vendor implementations (adding
> __initconstrel to the SMMU code to bring it in line). Consequently, a lack
> of init() method is now taken to mean uninitialized iommu_ops. Also, the
> printk warning in iommu_set_ops() now becomes an ASSERT.

When having submitted the indirect call overhead reduction series
including IOMMU changes for the first time, I was told that the Arm
folks would like to retain the ability to eventually support
heterogeneous IOMMUs (and hence I shouldn't provide patching
infrastructure there). A single global iommu_[gs]et_ops() is sort of
getting in the way of this as well, I think, and hence I'm not sure it
is a desirable step to make this so far Arm-specific arrangement
the general model. At least it would further complicate Arm side
changes towards that (mid / long term?) goal.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -21,6 +21,21 @@
>  #include <xen/keyhandler.h>
>  #include <xsm/xsm.h>
>  
> +static struct iommu_ops __read_mostly iommu_ops;
> +
> +const struct iommu_ops *iommu_get_ops(void)
> +{
> +    return &iommu_ops;
> +}
> +
> +void __init iommu_set_ops(const struct iommu_ops *ops)
> +{
> +    BUG_ON(!ops);
> +
> +    ASSERT(!iommu_ops.init || iommu_ops.init == ops->init);
> +    iommu_ops = *ops;
> +}

I realize that you merely move (and slightly re-arrange) what has
been there, but now that I look at it again I think ops->init should
also be verified to be non-NULL, or else installing such a set of
hooks would effectively revert back to the "no hooks yet" state.

> @@ -33,11 +32,7 @@ int __init iommu_hardware_setup(void)
>      if ( !iommu_init_ops )
>          return -ENODEV;
>  
> -    if ( !iommu_ops.init )
> -        iommu_ops = *iommu_init_ops->ops;
> -    else
> -        /* x2apic setup may have previously initialised the struct. */
> -        ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
> +    iommu_set_ops(iommu_init_ops->ops);

I was specifically asked to add the comment that you get rid of.
While mentioning x2APIC in common code may no be appropriate,
I'm sure this could be worded in a more general way and attached
to the moved check.

Jan



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

Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 May 2019 08:36
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; xen-devel <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code
> 
> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> > It's not vendor specific so it shouldn't really be there.
> 
> Perhaps, but this needs better justification:
> 
> > --- 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();
> 
> Even after some looking around, it's not obvious to me that the latter
> call doesn't depend on PCI devices being known, more specifically
> segment 0's bus2bridge[] having been filled. Nor can I tell whether
> there would be some noticeable misbehavior (prior to any guests
> starting) if there was a dependency and it got broken by the re-
> ordering.

I don't see any dependency but the code is somewhat tangled. Perhaps it would be better to build the PCI topology *before* IOMMU init and then iterate over the the devices after init to do the group assignment. I certainly can't see anything in the scan as it stands that would need the IOMMU to have been initialized.

  Paul

> 
> Jan
> 


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 May 2019 08:36
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; xen-devel <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code
> 
> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> > It's not vendor specific so it shouldn't really be there.
> 
> Perhaps, but this needs better justification:
> 
> > --- 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();
> 
> Even after some looking around, it's not obvious to me that the latter
> call doesn't depend on PCI devices being known, more specifically
> segment 0's bus2bridge[] having been filled. Nor can I tell whether
> there would be some noticeable misbehavior (prior to any guests
> starting) if there was a dependency and it got broken by the re-
> ordering.

I don't see any dependency but the code is somewhat tangled. Perhaps it would be better to build the PCI topology *before* IOMMU init and then iterate over the the devices after init to do the group assignment. I certainly can't see anything in the scan as it stands that would need the IOMMU to have been initialized.

  Paul

> 
> Jan
> 


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

Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 May 2019 09:11
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien
> Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code
> 
> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> > Currently x86 and ARM differ in their implementation for no good reason.
> > This patch moves the ARM variant of iommu_get/set_ops() helpers into
> > common code and modifies them so they deal with the __initconstrel
> > ops structures used by the x86 IOMMU vendor implementations (adding
> > __initconstrel to the SMMU code to bring it in line). Consequently, a lack
> > of init() method is now taken to mean uninitialized iommu_ops. Also, the
> > printk warning in iommu_set_ops() now becomes an ASSERT.
> 
> When having submitted the indirect call overhead reduction series
> including IOMMU changes for the first time, I was told that the Arm
> folks would like to retain the ability to eventually support
> heterogeneous IOMMUs (and hence I shouldn't provide patching
> infrastructure there). A single global iommu_[gs]et_ops() is sort of
> getting in the way of this as well, I think, and hence I'm not sure it
> is a desirable step to make this so far Arm-specific arrangement
> the general model. At least it would further complicate Arm side
> changes towards that (mid / long term?) goal.
>

Ok. Do you have any more information on what such an architecture would look like? I guess it is also conceivable that an x86 architecture might have slightly different IOMMU implementations (or at least quirks) for different PCI segments. So perhaps a global ops structure is not a good idea in the long run.

  Paul
 
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -21,6 +21,21 @@
> >  #include <xen/keyhandler.h>
> >  #include <xsm/xsm.h>
> >
> > +static struct iommu_ops __read_mostly iommu_ops;
> > +
> > +const struct iommu_ops *iommu_get_ops(void)
> > +{
> > +    return &iommu_ops;
> > +}
> > +
> > +void __init iommu_set_ops(const struct iommu_ops *ops)
> > +{
> > +    BUG_ON(!ops);
> > +
> > +    ASSERT(!iommu_ops.init || iommu_ops.init == ops->init);
> > +    iommu_ops = *ops;
> > +}
> 
> I realize that you merely move (and slightly re-arrange) what has
> been there, but now that I look at it again I think ops->init should
> also be verified to be non-NULL, or else installing such a set of
> hooks would effectively revert back to the "no hooks yet" state.
> 
> > @@ -33,11 +32,7 @@ int __init iommu_hardware_setup(void)
> >      if ( !iommu_init_ops )
> >          return -ENODEV;
> >
> > -    if ( !iommu_ops.init )
> > -        iommu_ops = *iommu_init_ops->ops;
> > -    else
> > -        /* x2apic setup may have previously initialised the struct. */
> > -        ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
> > +    iommu_set_ops(iommu_init_ops->ops);
> 
> I was specifically asked to add the comment that you get rid of.
> While mentioning x2APIC in common code may no be appropriate,
> I'm sure this could be worded in a more general way and attached
> to the moved check.
> 
> Jan
> 


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

Re: [Xen-devel] [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 May 2019 09:11
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien
> Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code
> 
> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> > Currently x86 and ARM differ in their implementation for no good reason.
> > This patch moves the ARM variant of iommu_get/set_ops() helpers into
> > common code and modifies them so they deal with the __initconstrel
> > ops structures used by the x86 IOMMU vendor implementations (adding
> > __initconstrel to the SMMU code to bring it in line). Consequently, a lack
> > of init() method is now taken to mean uninitialized iommu_ops. Also, the
> > printk warning in iommu_set_ops() now becomes an ASSERT.
> 
> When having submitted the indirect call overhead reduction series
> including IOMMU changes for the first time, I was told that the Arm
> folks would like to retain the ability to eventually support
> heterogeneous IOMMUs (and hence I shouldn't provide patching
> infrastructure there). A single global iommu_[gs]et_ops() is sort of
> getting in the way of this as well, I think, and hence I'm not sure it
> is a desirable step to make this so far Arm-specific arrangement
> the general model. At least it would further complicate Arm side
> changes towards that (mid / long term?) goal.
>

Ok. Do you have any more information on what such an architecture would look like? I guess it is also conceivable that an x86 architecture might have slightly different IOMMU implementations (or at least quirks) for different PCI segments. So perhaps a global ops structure is not a good idea in the long run.

  Paul
 
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -21,6 +21,21 @@
> >  #include <xen/keyhandler.h>
> >  #include <xsm/xsm.h>
> >
> > +static struct iommu_ops __read_mostly iommu_ops;
> > +
> > +const struct iommu_ops *iommu_get_ops(void)
> > +{
> > +    return &iommu_ops;
> > +}
> > +
> > +void __init iommu_set_ops(const struct iommu_ops *ops)
> > +{
> > +    BUG_ON(!ops);
> > +
> > +    ASSERT(!iommu_ops.init || iommu_ops.init == ops->init);
> > +    iommu_ops = *ops;
> > +}
> 
> I realize that you merely move (and slightly re-arrange) what has
> been there, but now that I look at it again I think ops->init should
> also be verified to be non-NULL, or else installing such a set of
> hooks would effectively revert back to the "no hooks yet" state.
> 
> > @@ -33,11 +32,7 @@ int __init iommu_hardware_setup(void)
> >      if ( !iommu_init_ops )
> >          return -ENODEV;
> >
> > -    if ( !iommu_ops.init )
> > -        iommu_ops = *iommu_init_ops->ops;
> > -    else
> > -        /* x2apic setup may have previously initialised the struct. */
> > -        ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
> > +    iommu_set_ops(iommu_init_ops->ops);
> 
> I was specifically asked to add the comment that you get rid of.
> While mentioning x2APIC in common code may no be appropriate,
> I'm sure this could be worded in a more general way and attached
> to the moved check.
> 
> Jan
> 


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

Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Julien Grall]

Hi,

On 5/14/19 5:19 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 May 2019 09:11
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien
>> Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>
>> Subject: Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code
>>
>>>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
>>> Currently x86 and ARM differ in their implementation for no good reason.
>>> This patch moves the ARM variant of iommu_get/set_ops() helpers into
>>> common code and modifies them so they deal with the __initconstrel
>>> ops structures used by the x86 IOMMU vendor implementations (adding
>>> __initconstrel to the SMMU code to bring it in line). Consequently, a lack
>>> of init() method is now taken to mean uninitialized iommu_ops. Also, the
>>> printk warning in iommu_set_ops() now becomes an ASSERT.
>>
>> When having submitted the indirect call overhead reduction series
>> including IOMMU changes for the first time, I was told that the Arm
>> folks would like to retain the ability to eventually support
>> heterogeneous IOMMUs (and hence I shouldn't provide patching
>> infrastructure there). A single global iommu_[gs]et_ops() is sort of
>> getting in the way of this as well, I think, and hence I'm not sure it
>> is a desirable step to make this so far Arm-specific arrangement
>> the general model. At least it would further complicate Arm side
>> changes towards that (mid / long term?) goal.

That's correct, it is a mid / long term plan.

>>
> 
> Ok. Do you have any more information on what such an architecture would look like? I guess it is also conceivable that an x86 architecture might have slightly different IOMMU implementations (or at least quirks) for different PCI segments. So perhaps a global ops structure is not a good idea in the long run.
I can see two uses cases:
     1) Finding the IOMMU associated to a device
     2) Applying an operation (i.e domain creation/destruction, 
map/unmap) on all the IOMMU

Actually, we already have similar concept within the SMMU driver because 
a platform may contain multiple SMMUs.

Any generic interface would actually be quite beneficial as we could 
simplify a lot the driver and avoid duplicating the logic in all the new 
Arm drivers.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Julien Grall]

Hi,

On 5/14/19 5:19 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 May 2019 09:11
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien
>> Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>
>> Subject: Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code
>>
>>>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
>>> Currently x86 and ARM differ in their implementation for no good reason.
>>> This patch moves the ARM variant of iommu_get/set_ops() helpers into
>>> common code and modifies them so they deal with the __initconstrel
>>> ops structures used by the x86 IOMMU vendor implementations (adding
>>> __initconstrel to the SMMU code to bring it in line). Consequently, a lack
>>> of init() method is now taken to mean uninitialized iommu_ops. Also, the
>>> printk warning in iommu_set_ops() now becomes an ASSERT.
>>
>> When having submitted the indirect call overhead reduction series
>> including IOMMU changes for the first time, I was told that the Arm
>> folks would like to retain the ability to eventually support
>> heterogeneous IOMMUs (and hence I shouldn't provide patching
>> infrastructure there). A single global iommu_[gs]et_ops() is sort of
>> getting in the way of this as well, I think, and hence I'm not sure it
>> is a desirable step to make this so far Arm-specific arrangement
>> the general model. At least it would further complicate Arm side
>> changes towards that (mid / long term?) goal.

That's correct, it is a mid / long term plan.

>>
> 
> Ok. Do you have any more information on what such an architecture would look like? I guess it is also conceivable that an x86 architecture might have slightly different IOMMU implementations (or at least quirks) for different PCI segments. So perhaps a global ops structure is not a good idea in the long run.
I can see two uses cases:
     1) Finding the IOMMU associated to a device
     2) Applying an operation (i.e domain creation/destruction, 
map/unmap) on all the IOMMU

Actually, we already have similar concept within the SMMU driver because 
a platform may contain multiple SMMUs.

Any generic interface would actually be quite beneficial as we could 
simplify a lot the driver and avoid duplicating the logic in all the new 
Arm drivers.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

[Jan Beulich]

>>> On 14.05.19 at 18:13, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 May 2019 08:36
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit 
> <suravee.suthikulpanit@amd.com>; Andrew
>> Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei 
> Liu
>> <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; xen-devel 
> <xen-devel@lists.xenproject.org>
>> Subject: Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of 
> vendor code
>> 
>> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
>> > It's not vendor specific so it shouldn't really be there.
>> 
>> Perhaps, but this needs better justification:
>> 
>> > --- 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();
>> 
>> Even after some looking around, it's not obvious to me that the latter
>> call doesn't depend on PCI devices being known, more specifically
>> segment 0's bus2bridge[] having been filled. Nor can I tell whether
>> there would be some noticeable misbehavior (prior to any guests
>> starting) if there was a dependency and it got broken by the re-
>> ordering.
> 
> I don't see any dependency but the code is somewhat tangled. Perhaps it 
> would be better to build the PCI topology *before* IOMMU init and then 
> iterate over the the devices after init to do the group assignment. I 
> certainly can't see anything in the scan as it stands that would need the 
> IOMMU to have been initialized.

Ah, yes, that's likely a better model. As to the dependency
aspect: pci_add_device() calls iommu_add_device(). But oddly
enough _scan_pci_devices() calls alloc_pdev(), not
pci_add_device(). So indeed there doesn't look to be any
dependency at present.

Jan



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

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

[Jan Beulich]

>>> On 14.05.19 at 18:13, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 May 2019 08:36
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit 
> <suravee.suthikulpanit@amd.com>; Andrew
>> Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei 
> Liu
>> <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; xen-devel 
> <xen-devel@lists.xenproject.org>
>> Subject: Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of 
> vendor code
>> 
>> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
>> > It's not vendor specific so it shouldn't really be there.
>> 
>> Perhaps, but this needs better justification:
>> 
>> > --- 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();
>> 
>> Even after some looking around, it's not obvious to me that the latter
>> call doesn't depend on PCI devices being known, more specifically
>> segment 0's bus2bridge[] having been filled. Nor can I tell whether
>> there would be some noticeable misbehavior (prior to any guests
>> starting) if there was a dependency and it got broken by the re-
>> ordering.
> 
> I don't see any dependency but the code is somewhat tangled. Perhaps it 
> would be better to build the PCI topology *before* IOMMU init and then 
> iterate over the the devices after init to do the group assignment. I 
> certainly can't see anything in the scan as it stands that would need the 
> IOMMU to have been initialized.

Ah, yes, that's likely a better model. As to the dependency
aspect: pci_add_device() calls iommu_add_device(). But oddly
enough _scan_pci_devices() calls alloc_pdev(), not
pci_add_device(). So indeed there doesn't look to be any
dependency at present.

Jan



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

Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Jan Beulich]

>>> On 14.05.19 at 18:19, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 May 2019 09:11
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit 
> <suravee.suthikulpanit@amd.com>; Julien
>> Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger 
> Pau Monne
>> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin Tian 
> <kevin.tian@intel.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>
>> Subject: Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code
>> 
>> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
>> > Currently x86 and ARM differ in their implementation for no good reason.
>> > This patch moves the ARM variant of iommu_get/set_ops() helpers into
>> > common code and modifies them so they deal with the __initconstrel
>> > ops structures used by the x86 IOMMU vendor implementations (adding
>> > __initconstrel to the SMMU code to bring it in line). Consequently, a lack
>> > of init() method is now taken to mean uninitialized iommu_ops. Also, the
>> > printk warning in iommu_set_ops() now becomes an ASSERT.
>> 
>> When having submitted the indirect call overhead reduction series
>> including IOMMU changes for the first time, I was told that the Arm
>> folks would like to retain the ability to eventually support
>> heterogeneous IOMMUs (and hence I shouldn't provide patching
>> infrastructure there). A single global iommu_[gs]et_ops() is sort of
>> getting in the way of this as well, I think, and hence I'm not sure it
>> is a desirable step to make this so far Arm-specific arrangement
>> the general model. At least it would further complicate Arm side
>> changes towards that (mid / long term?) goal.
>>
> 
> Ok. Do you have any more information on what such an architecture would look 
> like? I guess it is also conceivable that an x86 architecture might have 
> slightly different IOMMU implementations (or at least quirks) for different 
> PCI segments. So perhaps a global ops structure is not a good idea in the 
> long run.

Different quirks could likely be handled with a global ops instance.
The indirect call overhead elimination alone will imo make it
undesirable to switch to a non-global-ops model on x86, unless
there's a strong reason (like truly different IOMMUs in a single
system).

Jan



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

Re: [Xen-devel] [PATCH 3/5] iommu: move iommu_get_ops() into common code

[Jan Beulich]

>>> On 14.05.19 at 18:19, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 May 2019 09:11
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit 
> <suravee.suthikulpanit@amd.com>; Julien
>> Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger 
> Pau Monne
>> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin Tian 
> <kevin.tian@intel.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>
>> Subject: Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code
>> 
>> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
>> > Currently x86 and ARM differ in their implementation for no good reason.
>> > This patch moves the ARM variant of iommu_get/set_ops() helpers into
>> > common code and modifies them so they deal with the __initconstrel
>> > ops structures used by the x86 IOMMU vendor implementations (adding
>> > __initconstrel to the SMMU code to bring it in line). Consequently, a lack
>> > of init() method is now taken to mean uninitialized iommu_ops. Also, the
>> > printk warning in iommu_set_ops() now becomes an ASSERT.
>> 
>> When having submitted the indirect call overhead reduction series
>> including IOMMU changes for the first time, I was told that the Arm
>> folks would like to retain the ability to eventually support
>> heterogeneous IOMMUs (and hence I shouldn't provide patching
>> infrastructure there). A single global iommu_[gs]et_ops() is sort of
>> getting in the way of this as well, I think, and hence I'm not sure it
>> is a desirable step to make this so far Arm-specific arrangement
>> the general model. At least it would further complicate Arm side
>> changes towards that (mid / long term?) goal.
>>
> 
> Ok. Do you have any more information on what such an architecture would look 
> like? I guess it is also conceivable that an x86 architecture might have 
> slightly different IOMMU implementations (or at least quirks) for different 
> PCI segments. So perhaps a global ops structure is not a good idea in the 
> long run.

Different quirks could likely be handled with a global ops instance.
The indirect call overhead elimination alone will imo make it
undesirable to switch to a non-global-ops model on x86, unless
there's a strong reason (like truly different IOMMUs in a single
system).

Jan



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

Re: [PATCH 4/5] iommu: introduce iommu_groups

[Roger Pau Monné]

On Wed, May 08, 2019 at 02:24:02PM +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>
> ---
> 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 <wei.liu2@citrix.com>
> ---
>  xen/drivers/passthrough/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c   |  3 ++
>  xen/include/xen/iommu.h         |  7 ++++
>  xen/include/xen/pci.h           |  3 ++
>  4 files changed, 89 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index d3a6199b77..11319fbaae 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
>      }
>  }
>  
> +#ifdef CONFIG_HAS_PCI
> +
> +struct iommu_group {
> +    unsigned int id;
> +    unsigned int index;

I'm not sure I see the point of the index field, isn't it enough to
just use the ID field?

The ID is already used as a unique key in the code below for the radix
tree operations.

> +    struct list_head devs_list;
> +};
> +
> +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;
> +    static unsigned int index;
> +
> +    grp = xzalloc(struct iommu_group);

Can be moved with the declaration above.

> +    if ( !grp )
> +        return NULL;
> +
> +    grp->id = id;
> +    grp->index = index++;

AFAICT none of this is subject to races because it's always protected
by the pcidevs lock?

> +    INIT_LIST_HEAD(&grp->devs_list);
> +
> +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> +    {
> +        xfree(grp);
> +        grp = NULL;

Do you need to decrease index here, or is it fine to burn numbers on
failure?

> +    }
> +
> +    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)
> +{
> +    const struct iommu_ops *ops;
> +    unsigned int id;
> +    struct iommu_group *grp;
> +
> +    ops = iommu_get_ops();

This initialization can be done at declaration time.

> +    if ( !ops || !ops->get_device_group_id )
> +        return 0;
> +
> +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> +    grp = get_iommu_group(id);
> +
> +    if ( ! grp )
             ^ extra space
> +        return -ENOMEM;
> +
> +    if ( iommu_verbose )
> +        printk(XENLOG_INFO "Assign %04x:%02x:%02x.%u -> IOMMU group %u\n",
> +               pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +               PCI_FUNC(pdev->devfn), grp->index);

Wouldn't it be more helpful to print the group ID rather than the Xen
internal index?

Thanks, Roger.

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

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

[Roger Pau Monné]

On Wed, May 08, 2019 at 02:24:02PM +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>
> ---
> 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 <wei.liu2@citrix.com>
> ---
>  xen/drivers/passthrough/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c   |  3 ++
>  xen/include/xen/iommu.h         |  7 ++++
>  xen/include/xen/pci.h           |  3 ++
>  4 files changed, 89 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index d3a6199b77..11319fbaae 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
>      }
>  }
>  
> +#ifdef CONFIG_HAS_PCI
> +
> +struct iommu_group {
> +    unsigned int id;
> +    unsigned int index;

I'm not sure I see the point of the index field, isn't it enough to
just use the ID field?

The ID is already used as a unique key in the code below for the radix
tree operations.

> +    struct list_head devs_list;
> +};
> +
> +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;
> +    static unsigned int index;
> +
> +    grp = xzalloc(struct iommu_group);

Can be moved with the declaration above.

> +    if ( !grp )
> +        return NULL;
> +
> +    grp->id = id;
> +    grp->index = index++;

AFAICT none of this is subject to races because it's always protected
by the pcidevs lock?

> +    INIT_LIST_HEAD(&grp->devs_list);
> +
> +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> +    {
> +        xfree(grp);
> +        grp = NULL;

Do you need to decrease index here, or is it fine to burn numbers on
failure?

> +    }
> +
> +    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)
> +{
> +    const struct iommu_ops *ops;
> +    unsigned int id;
> +    struct iommu_group *grp;
> +
> +    ops = iommu_get_ops();

This initialization can be done at declaration time.

> +    if ( !ops || !ops->get_device_group_id )
> +        return 0;
> +
> +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> +    grp = get_iommu_group(id);
> +
> +    if ( ! grp )
             ^ extra space
> +        return -ENOMEM;
> +
> +    if ( iommu_verbose )
> +        printk(XENLOG_INFO "Assign %04x:%02x:%02x.%u -> IOMMU group %u\n",
> +               pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +               PCI_FUNC(pdev->devfn), grp->index);

Wouldn't it be more helpful to print the group ID rather than the Xen
internal index?

Thanks, Roger.

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

Re: [PATCH 5/5] iommu / pci: re-implement XEN_DOMCTL_get_device_group...

[Roger Pau Monné]

On Wed, May 08, 2019 at 02:24:03PM +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 relevant iommu_group and walking
> the membership list.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/drivers/passthrough/iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c   | 47 -----------------------------
>  xen/include/xen/iommu.h         |  2 ++
>  3 files changed, 67 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 11319fbaae..49140c652e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -729,6 +729,71 @@ int iommu_group_assign(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +static struct iommu_group *iommu_group_lookup(uint16_t seg, uint8_t bus,
> +                                              uint8_t devfn)

Could you use pci_sbdf_t to pass the SBDF?

> +{
> +    unsigned int id = 0;
> +    struct iommu_group *grp;
> +
> +    while ( radix_tree_gang_lookup(&iommu_groups, (void **)&grp, id, 1) )
> +    {
> +        struct pci_dev *pdev;
> +
> +        list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> +            if ( pdev->seg == seg && pdev->bus == bus &&
> +                 pdev->devfn == devfn )
> +                return grp;
> +
> +        id = grp->id + 1;
> +    }
> +
> +    return NULL;
> +}
> +
> +int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,

Using pci_sbdf_t would be better here to pass the SBDF IMO, or
uint<size>_t, or just plain unsigned int.

Also, I wonder about the usefulness of the domain parameter, shouldn't
you do the ownership check somewhere else (if required) and have this
function just check the IOMMU group of a given PCI  device?

(Note you probably want to constify the domain parameter if it needs to
stay).

> +                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
> +{
> +    struct iommu_group *grp;
> +    struct pci_dev *pdev;
> +    int i = 0;

It seems like this should be unsigned int?

> +
> +    pcidevs_lock();
> +
> +    grp = iommu_group_lookup(seg, bus, devfn);
> +    if ( !grp )
> +    {
> +        pcidevs_unlock();
> +        return 0;
> +    }
> +
> +    list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> +    {
> +        uint32_t sbdf;
> +
> +        if ( i >= max_sdevs )
> +            break;
> +
> +        if ( pdev->domain != d )
> +            continue;
> +
> +        sbdf = PCI_SBDF3(pdev->seg, pdev->bus, pdev->devfn);
> +
> +        if ( xsm_get_device_group(XSM_HOOK, sbdf) )
> +            continue;
> +
> +        if ( unlikely(copy_to_guest_offset(buf, i, &sbdf, 1)) )
> +        {
> +            pcidevs_unlock();
> +            return -1;

-EFAULT?

> +        }
> +        i++;
> +    }
> +
> +    pcidevs_unlock();
> +
> +    return i;
> +}
> +
>  #endif /* CONFIG_HAS_PCI */
>  
>  /*
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 6210409741..68b2883ed6 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1554,53 +1554,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)

Oh, I see this is code movement and changes to an existing function,
hence my comments above might be stale, or will require to fixup the
callers which might be cumbersome.

Thanks, Roger.

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

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

[Roger Pau Monné]

On Wed, May 08, 2019 at 02:24:03PM +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 relevant iommu_group and walking
> the membership list.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/drivers/passthrough/iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c   | 47 -----------------------------
>  xen/include/xen/iommu.h         |  2 ++
>  3 files changed, 67 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 11319fbaae..49140c652e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -729,6 +729,71 @@ int iommu_group_assign(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +static struct iommu_group *iommu_group_lookup(uint16_t seg, uint8_t bus,
> +                                              uint8_t devfn)

Could you use pci_sbdf_t to pass the SBDF?

> +{
> +    unsigned int id = 0;
> +    struct iommu_group *grp;
> +
> +    while ( radix_tree_gang_lookup(&iommu_groups, (void **)&grp, id, 1) )
> +    {
> +        struct pci_dev *pdev;
> +
> +        list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> +            if ( pdev->seg == seg && pdev->bus == bus &&
> +                 pdev->devfn == devfn )
> +                return grp;
> +
> +        id = grp->id + 1;
> +    }
> +
> +    return NULL;
> +}
> +
> +int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,

Using pci_sbdf_t would be better here to pass the SBDF IMO, or
uint<size>_t, or just plain unsigned int.

Also, I wonder about the usefulness of the domain parameter, shouldn't
you do the ownership check somewhere else (if required) and have this
function just check the IOMMU group of a given PCI  device?

(Note you probably want to constify the domain parameter if it needs to
stay).

> +                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
> +{
> +    struct iommu_group *grp;
> +    struct pci_dev *pdev;
> +    int i = 0;

It seems like this should be unsigned int?

> +
> +    pcidevs_lock();
> +
> +    grp = iommu_group_lookup(seg, bus, devfn);
> +    if ( !grp )
> +    {
> +        pcidevs_unlock();
> +        return 0;
> +    }
> +
> +    list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> +    {
> +        uint32_t sbdf;
> +
> +        if ( i >= max_sdevs )
> +            break;
> +
> +        if ( pdev->domain != d )
> +            continue;
> +
> +        sbdf = PCI_SBDF3(pdev->seg, pdev->bus, pdev->devfn);
> +
> +        if ( xsm_get_device_group(XSM_HOOK, sbdf) )
> +            continue;
> +
> +        if ( unlikely(copy_to_guest_offset(buf, i, &sbdf, 1)) )
> +        {
> +            pcidevs_unlock();
> +            return -1;

-EFAULT?

> +        }
> +        i++;
> +    }
> +
> +    pcidevs_unlock();
> +
> +    return i;
> +}
> +
>  #endif /* CONFIG_HAS_PCI */
>  
>  /*
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 6210409741..68b2883ed6 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1554,53 +1554,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)

Oh, I see this is code movement and changes to an existing function,
hence my comments above might be stale, or will require to fixup the
callers which might be cumbersome.

Thanks, Roger.

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

Re: [PATCH 4/5] iommu: introduce iommu_groups

[Jan Beulich]

>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
>      }
>  }
>  
> +#ifdef CONFIG_HAS_PCI
> +
> +struct iommu_group {
> +    unsigned int id;
> +    unsigned int index;
> +    struct list_head devs_list;
> +};

Could these additions as a whole go into a new groups.c?

> +int iommu_group_assign(struct pci_dev *pdev)
> +{
> +    const struct iommu_ops *ops;
> +    unsigned int id;
> +    struct iommu_group *grp;
> +
> +    ops = iommu_get_ops();
> +    if ( !ops || !ops->get_device_group_id )

The way iommu_get_ops() works the left side of the || is pointless.

> +        return 0;
> +
> +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> +    grp = get_iommu_group(id);

I don't think solitary devices should be allocated a group. Also
you don't handle failure of ops->get_device_group_id().

> +    if ( ! grp )

Nit: Stray blank.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -75,6 +75,9 @@ struct pci_dev {
>      struct list_head alldevs_list;
>      struct list_head domain_list;
>  
> +    struct list_head grpdevs_list;

Does this separate list provide much value? The devices in a group
are going to move between two domain_list-s all in one go, so
once you know the domain of one you'll be able to find the rest by
iterating that domain's list. Is the fear that such an iteration may
be tens of thousands of entries long, and hence become an issue
when traversed? I have no idea how many PCI devices the biggest
systems today would have, but if traversal was an issue, then it
would already be with the code we've got now.

Jan



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

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

[Jan Beulich]

>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
>      }
>  }
>  
> +#ifdef CONFIG_HAS_PCI
> +
> +struct iommu_group {
> +    unsigned int id;
> +    unsigned int index;
> +    struct list_head devs_list;
> +};

Could these additions as a whole go into a new groups.c?

> +int iommu_group_assign(struct pci_dev *pdev)
> +{
> +    const struct iommu_ops *ops;
> +    unsigned int id;
> +    struct iommu_group *grp;
> +
> +    ops = iommu_get_ops();
> +    if ( !ops || !ops->get_device_group_id )

The way iommu_get_ops() works the left side of the || is pointless.

> +        return 0;
> +
> +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> +    grp = get_iommu_group(id);

I don't think solitary devices should be allocated a group. Also
you don't handle failure of ops->get_device_group_id().

> +    if ( ! grp )

Nit: Stray blank.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -75,6 +75,9 @@ struct pci_dev {
>      struct list_head alldevs_list;
>      struct list_head domain_list;
>  
> +    struct list_head grpdevs_list;

Does this separate list provide much value? The devices in a group
are going to move between two domain_list-s all in one go, so
once you know the domain of one you'll be able to find the rest by
iterating that domain's list. Is the fear that such an iteration may
be tens of thousands of entries long, and hence become an issue
when traversed? I have no idea how many PCI devices the biggest
systems today would have, but if traversal was an issue, then it
would already be with the code we've got now.

Jan



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

Re: [PATCH 4/5] iommu: introduce iommu_groups

[Jan Beulich]

>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -427,6 +427,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
> u8 bus, u8 devfn)
>  
>      check_pdev(pdev);
>      apply_quirks(pdev);
> +    iommu_group_assign(pdev);
>  
>      return pdev;
>  }
> @@ -1098,6 +1099,8 @@ int __init scan_pci_devices(void)
>  {
>      int ret;
>  
> +    iommu_groups_init();
> +
>      pcidevs_lock();
>      ret = pci_segments_iterate(_scan_pci_devices, NULL);
>      pcidevs_unlock();

In free_pdev() you also want to remove a device from its group.

Jan



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

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

[Jan Beulich]

>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -427,6 +427,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
> u8 bus, u8 devfn)
>  
>      check_pdev(pdev);
>      apply_quirks(pdev);
> +    iommu_group_assign(pdev);
>  
>      return pdev;
>  }
> @@ -1098,6 +1099,8 @@ int __init scan_pci_devices(void)
>  {
>      int ret;
>  
> +    iommu_groups_init();
> +
>      pcidevs_lock();
>      ret = pci_segments_iterate(_scan_pci_devices, NULL);
>      pcidevs_unlock();

In free_pdev() you also want to remove a device from its group.

Jan



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

Re: [PATCH 4/5] iommu: introduce iommu_groups

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 15 May 2019 09:45
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>; 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 4/5] iommu: introduce iommu_groups
> 
> On Wed, May 08, 2019 at 02:24:02PM +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>
> > ---
> > 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 <wei.liu2@citrix.com>
> > ---
> >  xen/drivers/passthrough/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/pci.c   |  3 ++
> >  xen/include/xen/iommu.h         |  7 ++++
> >  xen/include/xen/pci.h           |  3 ++
> >  4 files changed, 89 insertions(+)
> >
> > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index d3a6199b77..11319fbaae 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
> >      }
> >  }
> >
> > +#ifdef CONFIG_HAS_PCI
> > +
> > +struct iommu_group {
> > +    unsigned int id;
> > +    unsigned int index;
> 
> I'm not sure I see the point of the index field, isn't it enough to
> just use the ID field?

The index is just supposed to just be an index to refer to the group. Linux has similar, and this is what ends up in sysfs, but I guess there's not much point having this in Xen as yet... It can always be added later if it proves desirable.

> 
> The ID is already used as a unique key in the code below for the radix
> tree operations.
> 

Yes, that's the meaningful number.

> > +    struct list_head devs_list;
> > +};
> > +
> > +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;
> > +    static unsigned int index;
> > +
> > +    grp = xzalloc(struct iommu_group);
> 
> Can be moved with the declaration above.
> 

Sure.

> > +    if ( !grp )
> > +        return NULL;
> > +
> > +    grp->id = id;
> > +    grp->index = index++;
> 
> AFAICT none of this is subject to races because it's always protected
> by the pcidevs lock?
> 

Yes, it's under lock.

> > +    INIT_LIST_HEAD(&grp->devs_list);
> > +
> > +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> > +    {
> > +        xfree(grp);
> > +        grp = NULL;
> 
> Do you need to decrease index here, or is it fine to burn numbers on
> failure?
> 

I'll get rid of the index.

> > +    }
> > +
> > +    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)
> > +{
> > +    const struct iommu_ops *ops;
> > +    unsigned int id;
> > +    struct iommu_group *grp;
> > +
> > +    ops = iommu_get_ops();
> 
> This initialization can be done at declaration time.
> 

Sure.

> > +    if ( !ops || !ops->get_device_group_id )
> > +        return 0;
> > +
> > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> > +    grp = get_iommu_group(id);
> > +
> > +    if ( ! grp )
>              ^ extra space
> > +        return -ENOMEM;
> > +
> > +    if ( iommu_verbose )
> > +        printk(XENLOG_INFO "Assign %04x:%02x:%02x.%u -> IOMMU group %u\n",
> > +               pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> > +               PCI_FUNC(pdev->devfn), grp->index);
> 
> Wouldn't it be more helpful to print the group ID rather than the Xen
> internal index?

Yes, once the index is gone I'll do that instead.

  Paul

> 
> Thanks, Roger.

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

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

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 15 May 2019 09:45
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>; 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 4/5] iommu: introduce iommu_groups
> 
> On Wed, May 08, 2019 at 02:24:02PM +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>
> > ---
> > 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 <wei.liu2@citrix.com>
> > ---
> >  xen/drivers/passthrough/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/pci.c   |  3 ++
> >  xen/include/xen/iommu.h         |  7 ++++
> >  xen/include/xen/pci.h           |  3 ++
> >  4 files changed, 89 insertions(+)
> >
> > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index d3a6199b77..11319fbaae 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
> >      }
> >  }
> >
> > +#ifdef CONFIG_HAS_PCI
> > +
> > +struct iommu_group {
> > +    unsigned int id;
> > +    unsigned int index;
> 
> I'm not sure I see the point of the index field, isn't it enough to
> just use the ID field?

The index is just supposed to just be an index to refer to the group. Linux has similar, and this is what ends up in sysfs, but I guess there's not much point having this in Xen as yet... It can always be added later if it proves desirable.

> 
> The ID is already used as a unique key in the code below for the radix
> tree operations.
> 

Yes, that's the meaningful number.

> > +    struct list_head devs_list;
> > +};
> > +
> > +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;
> > +    static unsigned int index;
> > +
> > +    grp = xzalloc(struct iommu_group);
> 
> Can be moved with the declaration above.
> 

Sure.

> > +    if ( !grp )
> > +        return NULL;
> > +
> > +    grp->id = id;
> > +    grp->index = index++;
> 
> AFAICT none of this is subject to races because it's always protected
> by the pcidevs lock?
> 

Yes, it's under lock.

> > +    INIT_LIST_HEAD(&grp->devs_list);
> > +
> > +    if ( radix_tree_insert(&iommu_groups, id, grp) )
> > +    {
> > +        xfree(grp);
> > +        grp = NULL;
> 
> Do you need to decrease index here, or is it fine to burn numbers on
> failure?
> 

I'll get rid of the index.

> > +    }
> > +
> > +    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)
> > +{
> > +    const struct iommu_ops *ops;
> > +    unsigned int id;
> > +    struct iommu_group *grp;
> > +
> > +    ops = iommu_get_ops();
> 
> This initialization can be done at declaration time.
> 

Sure.

> > +    if ( !ops || !ops->get_device_group_id )
> > +        return 0;
> > +
> > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> > +    grp = get_iommu_group(id);
> > +
> > +    if ( ! grp )
>              ^ extra space
> > +        return -ENOMEM;
> > +
> > +    if ( iommu_verbose )
> > +        printk(XENLOG_INFO "Assign %04x:%02x:%02x.%u -> IOMMU group %u\n",
> > +               pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> > +               PCI_FUNC(pdev->devfn), grp->index);
> 
> Wouldn't it be more helpful to print the group ID rather than the Xen
> internal index?

Yes, once the index is gone I'll do that instead.

  Paul

> 
> Thanks, Roger.

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

Re: [PATCH 4/5] iommu: introduce iommu_groups

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 15 May 2019 15:18
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH 4/5] iommu: introduce iommu_groups
> 
> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
> >      }
> >  }
> >
> > +#ifdef CONFIG_HAS_PCI
> > +
> > +struct iommu_group {
> > +    unsigned int id;
> > +    unsigned int index;
> > +    struct list_head devs_list;
> > +};
> 
> Could these additions as a whole go into a new groups.c?

Sure.

> 
> > +int iommu_group_assign(struct pci_dev *pdev)
> > +{
> > +    const struct iommu_ops *ops;
> > +    unsigned int id;
> > +    struct iommu_group *grp;
> > +
> > +    ops = iommu_get_ops();
> > +    if ( !ops || !ops->get_device_group_id )
> 
> The way iommu_get_ops() works the left side of the || is pointless.

Yes, this was a hangover from a previous variant of patch #3, which I'm going to drop anyway.

> 
> > +        return 0;
> > +
> > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> > +    grp = get_iommu_group(id);
> 
> I don't think solitary devices should be allocated a group. Also
> you don't handle failure of ops->get_device_group_id().

True, it can fail in the VT-d case. Not clear what to do in that case though; I guess assume - for now - that the device gets its own group.
I think all devices should get a group. The group will ultimately be the unit of assignment to a VM and, in the best case, we *expect* each device to have its own group... it's only when there are quirks, legacy bridges etc. that multiple devices should end up in the same group. This is consistent with Linux's IOMMU groups.

> 
> > +    if ( ! grp )
> 
> Nit: Stray blank.

Oh yes.

> 
> > --- a/xen/include/xen/pci.h
> > +++ b/xen/include/xen/pci.h
> > @@ -75,6 +75,9 @@ struct pci_dev {
> >      struct list_head alldevs_list;
> >      struct list_head domain_list;
> >
> > +    struct list_head grpdevs_list;
> 
> Does this separate list provide much value? The devices in a group
> are going to move between two domain_list-s all in one go, so
> once you know the domain of one you'll be able to find the rest by
> iterating that domain's list. Is the fear that such an iteration may
> be tens of thousands of entries long, and hence become an issue
> when traversed? I have no idea how many PCI devices the biggest
> systems today would have, but if traversal was an issue, then it
> would already be with the code we've got now.

I'd prefer to keep it... It makes the re-implementation of the domctl in the next patch more straightforward.

  Paul

> 
> Jan
> 


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 15 May 2019 15:18
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH 4/5] iommu: introduce iommu_groups
> 
> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -655,6 +655,82 @@ static void iommu_dump_p2m_table(unsigned char key)
> >      }
> >  }
> >
> > +#ifdef CONFIG_HAS_PCI
> > +
> > +struct iommu_group {
> > +    unsigned int id;
> > +    unsigned int index;
> > +    struct list_head devs_list;
> > +};
> 
> Could these additions as a whole go into a new groups.c?

Sure.

> 
> > +int iommu_group_assign(struct pci_dev *pdev)
> > +{
> > +    const struct iommu_ops *ops;
> > +    unsigned int id;
> > +    struct iommu_group *grp;
> > +
> > +    ops = iommu_get_ops();
> > +    if ( !ops || !ops->get_device_group_id )
> 
> The way iommu_get_ops() works the left side of the || is pointless.

Yes, this was a hangover from a previous variant of patch #3, which I'm going to drop anyway.

> 
> > +        return 0;
> > +
> > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
> > +    grp = get_iommu_group(id);
> 
> I don't think solitary devices should be allocated a group. Also
> you don't handle failure of ops->get_device_group_id().

True, it can fail in the VT-d case. Not clear what to do in that case though; I guess assume - for now - that the device gets its own group.
I think all devices should get a group. The group will ultimately be the unit of assignment to a VM and, in the best case, we *expect* each device to have its own group... it's only when there are quirks, legacy bridges etc. that multiple devices should end up in the same group. This is consistent with Linux's IOMMU groups.

> 
> > +    if ( ! grp )
> 
> Nit: Stray blank.

Oh yes.

> 
> > --- a/xen/include/xen/pci.h
> > +++ b/xen/include/xen/pci.h
> > @@ -75,6 +75,9 @@ struct pci_dev {
> >      struct list_head alldevs_list;
> >      struct list_head domain_list;
> >
> > +    struct list_head grpdevs_list;
> 
> Does this separate list provide much value? The devices in a group
> are going to move between two domain_list-s all in one go, so
> once you know the domain of one you'll be able to find the rest by
> iterating that domain's list. Is the fear that such an iteration may
> be tens of thousands of entries long, and hence become an issue
> when traversed? I have no idea how many PCI devices the biggest
> systems today would have, but if traversal was an issue, then it
> would already be with the code we've got now.

I'd prefer to keep it... It makes the re-implementation of the domctl in the next patch more straightforward.

  Paul

> 
> Jan
> 


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

Re: [PATCH 4/5] iommu: introduce iommu_groups

[Jan Beulich]

>>> On 31.05.19 at 15:55, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 15 May 2019 15:18
>> 
>> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
>> > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
>> > +    grp = get_iommu_group(id);
>> 
>> I don't think solitary devices should be allocated a group. Also
>> you don't handle failure of ops->get_device_group_id().
> 
> True, it can fail in the VT-d case. Not clear what to do in that case though; 
> I guess assume - for now - that the device gets its own group.
> I think all devices should get a group. The group will ultimately be the 
> unit of assignment to a VM and, in the best case, we *expect* each device to 
> have its own group... it's only when there are quirks, legacy bridges etc. 
> that multiple devices should end up in the same group. This is consistent 
> with Linux's IOMMU groups.

Well, I'm not worried much about consistency with Linux here, as
you're not cloning their implementation anyway (afaict). To me at
this point wrapping individual devices in groups looks like just extra
overhead with no real gain. But, granted, the gain may appear
later.

>> > --- a/xen/include/xen/pci.h
>> > +++ b/xen/include/xen/pci.h
>> > @@ -75,6 +75,9 @@ struct pci_dev {
>> >      struct list_head alldevs_list;
>> >      struct list_head domain_list;
>> >
>> > +    struct list_head grpdevs_list;
>> 
>> Does this separate list provide much value? The devices in a group
>> are going to move between two domain_list-s all in one go, so
>> once you know the domain of one you'll be able to find the rest by
>> iterating that domain's list. Is the fear that such an iteration may
>> be tens of thousands of entries long, and hence become an issue
>> when traversed? I have no idea how many PCI devices the biggest
>> systems today would have, but if traversal was an issue, then it
>> would already be with the code we've got now.
> 
> I'd prefer to keep it... It makes the re-implementation of the domctl in the 
> next patch more straightforward.

I can accept this as the positive side. But there's extra storage
needed (not much, but anyway), and the more (independent)
lists we have that devices can be on, the more likely it'll be that
one of them gets screwed up at some point (e.g. by forgetting
to remove a device from one of them prior to de-allocation).

Jan



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

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

[Jan Beulich]

>>> On 31.05.19 at 15:55, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 15 May 2019 15:18
>> 
>> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote:
>> > +    id = ops->get_device_group_id(pdev->seg, pdev->bus, pdev->devfn);
>> > +    grp = get_iommu_group(id);
>> 
>> I don't think solitary devices should be allocated a group. Also
>> you don't handle failure of ops->get_device_group_id().
> 
> True, it can fail in the VT-d case. Not clear what to do in that case though; 
> I guess assume - for now - that the device gets its own group.
> I think all devices should get a group. The group will ultimately be the 
> unit of assignment to a VM and, in the best case, we *expect* each device to 
> have its own group... it's only when there are quirks, legacy bridges etc. 
> that multiple devices should end up in the same group. This is consistent 
> with Linux's IOMMU groups.

Well, I'm not worried much about consistency with Linux here, as
you're not cloning their implementation anyway (afaict). To me at
this point wrapping individual devices in groups looks like just extra
overhead with no real gain. But, granted, the gain may appear
later.

>> > --- a/xen/include/xen/pci.h
>> > +++ b/xen/include/xen/pci.h
>> > @@ -75,6 +75,9 @@ struct pci_dev {
>> >      struct list_head alldevs_list;
>> >      struct list_head domain_list;
>> >
>> > +    struct list_head grpdevs_list;
>> 
>> Does this separate list provide much value? The devices in a group
>> are going to move between two domain_list-s all in one go, so
>> once you know the domain of one you'll be able to find the rest by
>> iterating that domain's list. Is the fear that such an iteration may
>> be tens of thousands of entries long, and hence become an issue
>> when traversed? I have no idea how many PCI devices the biggest
>> systems today would have, but if traversal was an issue, then it
>> would already be with the code we've got now.
> 
> I'd prefer to keep it... It makes the re-implementation of the domctl in the 
> next patch more straightforward.

I can accept this as the positive side. But there's extra storage
needed (not much, but anyway), and the more (independent)
lists we have that devices can be on, the more likely it'll be that
one of them gets screwed up at some point (e.g. by forgetting
to remove a device from one of them prior to de-allocation).

Jan



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

Re: [PATCH 4/5] iommu: introduce iommu_groups

[Paul Durrant]

> -----Original Message-----
[snip]
> >> > --- a/xen/include/xen/pci.h
> >> > +++ b/xen/include/xen/pci.h
> >> > @@ -75,6 +75,9 @@ struct pci_dev {
> >> >      struct list_head alldevs_list;
> >> >      struct list_head domain_list;
> >> >
> >> > +    struct list_head grpdevs_list;
> >>
> >> Does this separate list provide much value? The devices in a group
> >> are going to move between two domain_list-s all in one go, so
> >> once you know the domain of one you'll be able to find the rest by
> >> iterating that domain's list. Is the fear that such an iteration may
> >> be tens of thousands of entries long, and hence become an issue
> >> when traversed? I have no idea how many PCI devices the biggest
> >> systems today would have, but if traversal was an issue, then it
> >> would already be with the code we've got now.
> >
> > I'd prefer to keep it... It makes the re-implementation of the domctl in the
> > next patch more straightforward.
> 
> I can accept this as the positive side. But there's extra storage
> needed (not much, but anyway), and the more (independent)
> lists we have that devices can be on, the more likely it'll be that
> one of them gets screwed up at some point (e.g. by forgetting
> to remove a device from one of them prior to de-allocation).

Ok, I'll drop the list and just match on the grp pointer.

  Paul

> 
> Jan
> 


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

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

[Paul Durrant]

> -----Original Message-----
[snip]
> >> > --- a/xen/include/xen/pci.h
> >> > +++ b/xen/include/xen/pci.h
> >> > @@ -75,6 +75,9 @@ struct pci_dev {
> >> >      struct list_head alldevs_list;
> >> >      struct list_head domain_list;
> >> >
> >> > +    struct list_head grpdevs_list;
> >>
> >> Does this separate list provide much value? The devices in a group
> >> are going to move between two domain_list-s all in one go, so
> >> once you know the domain of one you'll be able to find the rest by
> >> iterating that domain's list. Is the fear that such an iteration may
> >> be tens of thousands of entries long, and hence become an issue
> >> when traversed? I have no idea how many PCI devices the biggest
> >> systems today would have, but if traversal was an issue, then it
> >> would already be with the code we've got now.
> >
> > I'd prefer to keep it... It makes the re-implementation of the domctl in the
> > next patch more straightforward.
> 
> I can accept this as the positive side. But there's extra storage
> needed (not much, but anyway), and the more (independent)
> lists we have that devices can be on, the more likely it'll be that
> one of them gets screwed up at some point (e.g. by forgetting
> to remove a device from one of them prior to de-allocation).

Ok, I'll drop the list and just match on the grp pointer.

  Paul

> 
> Jan
> 


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

Re: [PATCH 5/5] iommu / pci: re-implement XEN_DOMCTL_get_device_group...

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 15 May 2019 10:07
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 5/5] iommu / pci: re-implement XEN_DOMCTL_get_device_group...
> 
> On Wed, May 08, 2019 at 02:24:03PM +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 relevant iommu_group and walking
> > the membership list.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > ---
> >  xen/drivers/passthrough/iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/pci.c   | 47 -----------------------------
> >  xen/include/xen/iommu.h         |  2 ++
> >  3 files changed, 67 insertions(+), 47 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index 11319fbaae..49140c652e 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -729,6 +729,71 @@ int iommu_group_assign(struct pci_dev *pdev)
> >      return 0;
> >  }
> >
> > +static struct iommu_group *iommu_group_lookup(uint16_t seg, uint8_t bus,
> > +                                              uint8_t devfn)
> 
> Could you use pci_sbdf_t to pass the SBDF?
> 

Probably, I'd not noticed its existence so I'll use it when I can.

> > +{
> > +    unsigned int id = 0;
> > +    struct iommu_group *grp;
> > +
> > +    while ( radix_tree_gang_lookup(&iommu_groups, (void **)&grp, id, 1) )
> > +    {
> > +        struct pci_dev *pdev;
> > +
> > +        list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> > +            if ( pdev->seg == seg && pdev->bus == bus &&
> > +                 pdev->devfn == devfn )
> > +                return grp;
> > +
> > +        id = grp->id + 1;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,
> 
> Using pci_sbdf_t would be better here to pass the SBDF IMO, or
> uint<size>_t, or just plain unsigned int.
> 
> Also, I wonder about the usefulness of the domain parameter, shouldn't
> you do the ownership check somewhere else (if required) and have this
> function just check the IOMMU group of a given PCI  device?
> 
> (Note you probably want to constify the domain parameter if it needs to
> stay).

Yes and no. This is the implementation of an existing domctl so it's semantics are baked in. I think I can use pci_sbdf_t but the domain parameter needs to stay.

> 
> > +                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
> > +{
> > +    struct iommu_group *grp;
> > +    struct pci_dev *pdev;
> > +    int i = 0;
> 
> It seems like this should be unsigned int?
> 

Yes, I guess it could be.

> > +
> > +    pcidevs_lock();
> > +
> > +    grp = iommu_group_lookup(seg, bus, devfn);
> > +    if ( !grp )
> > +    {
> > +        pcidevs_unlock();
> > +        return 0;
> > +    }
> > +
> > +    list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> > +    {
> > +        uint32_t sbdf;
> > +
> > +        if ( i >= max_sdevs )
> > +            break;
> > +
> > +        if ( pdev->domain != d )
> > +            continue;
> > +
> > +        sbdf = PCI_SBDF3(pdev->seg, pdev->bus, pdev->devfn);
> > +
> > +        if ( xsm_get_device_group(XSM_HOOK, sbdf) )
> > +            continue;
> > +
> > +        if ( unlikely(copy_to_guest_offset(buf, i, &sbdf, 1)) )
> > +        {
> > +            pcidevs_unlock();
> > +            return -1;
> 
> -EFAULT?
> 

Yes... then I can get rid of the override of the ret value in the calling code.

  Paul

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

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

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 15 May 2019 10:07
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 5/5] iommu / pci: re-implement XEN_DOMCTL_get_device_group...
> 
> On Wed, May 08, 2019 at 02:24:03PM +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 relevant iommu_group and walking
> > the membership list.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > ---
> >  xen/drivers/passthrough/iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/pci.c   | 47 -----------------------------
> >  xen/include/xen/iommu.h         |  2 ++
> >  3 files changed, 67 insertions(+), 47 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index 11319fbaae..49140c652e 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -729,6 +729,71 @@ int iommu_group_assign(struct pci_dev *pdev)
> >      return 0;
> >  }
> >
> > +static struct iommu_group *iommu_group_lookup(uint16_t seg, uint8_t bus,
> > +                                              uint8_t devfn)
> 
> Could you use pci_sbdf_t to pass the SBDF?
> 

Probably, I'd not noticed its existence so I'll use it when I can.

> > +{
> > +    unsigned int id = 0;
> > +    struct iommu_group *grp;
> > +
> > +    while ( radix_tree_gang_lookup(&iommu_groups, (void **)&grp, id, 1) )
> > +    {
> > +        struct pci_dev *pdev;
> > +
> > +        list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> > +            if ( pdev->seg == seg && pdev->bus == bus &&
> > +                 pdev->devfn == devfn )
> > +                return grp;
> > +
> > +        id = grp->id + 1;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,
> 
> Using pci_sbdf_t would be better here to pass the SBDF IMO, or
> uint<size>_t, or just plain unsigned int.
> 
> Also, I wonder about the usefulness of the domain parameter, shouldn't
> you do the ownership check somewhere else (if required) and have this
> function just check the IOMMU group of a given PCI  device?
> 
> (Note you probably want to constify the domain parameter if it needs to
> stay).

Yes and no. This is the implementation of an existing domctl so it's semantics are baked in. I think I can use pci_sbdf_t but the domain parameter needs to stay.

> 
> > +                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
> > +{
> > +    struct iommu_group *grp;
> > +    struct pci_dev *pdev;
> > +    int i = 0;
> 
> It seems like this should be unsigned int?
> 

Yes, I guess it could be.

> > +
> > +    pcidevs_lock();
> > +
> > +    grp = iommu_group_lookup(seg, bus, devfn);
> > +    if ( !grp )
> > +    {
> > +        pcidevs_unlock();
> > +        return 0;
> > +    }
> > +
> > +    list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> > +    {
> > +        uint32_t sbdf;
> > +
> > +        if ( i >= max_sdevs )
> > +            break;
> > +
> > +        if ( pdev->domain != d )
> > +            continue;
> > +
> > +        sbdf = PCI_SBDF3(pdev->seg, pdev->bus, pdev->devfn);
> > +
> > +        if ( xsm_get_device_group(XSM_HOOK, sbdf) )
> > +            continue;
> > +
> > +        if ( unlikely(copy_to_guest_offset(buf, i, &sbdf, 1)) )
> > +        {
> > +            pcidevs_unlock();
> > +            return -1;
> 
> -EFAULT?
> 

Yes... then I can get rid of the override of the ret value in the calling code.

  Paul

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