| Message ID | 20190508132403.1454-4-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu groups + cleanup | expand |
>>> 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
> -----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 >
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,
>>> 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
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);
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(-)