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

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

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

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

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