From: Paul Durrant <Paul.Durrant@citrix.com> To: 'Jan Beulich' <JBeulich@suse.com> Cc: Kevin Tian <kevin.tian@intel.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wei.liu2@citrix.com>, Andrew Cooper <Andrew.Cooper3@citrix.com>, Julien Grall <julien.grall@arm.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, xen-devel <xen-devel@lists.xenproject.org>, Brian Woods <brian.woods@amd.com>, Roger Pau Monne <roger.pau@citrix.com> Subject: Re: [PATCH 3/5] iommu: move iommu_get_ops() into common code Date: Tue, 14 May 2019 16:19:55 +0000 [thread overview] Message-ID: <246a87e654194e5082852b63853415d6@AMSPEX02CL03.citrite.net> (raw) In-Reply-To: <5CD99729020000780022E4B1@prv1-mh.provo.novell.com> > -----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
WARNING: multiple messages have this Message-ID (diff)
From: Paul Durrant <Paul.Durrant@citrix.com> To: 'Jan Beulich' <JBeulich@suse.com> Cc: Kevin Tian <kevin.tian@intel.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wei.liu2@citrix.com>, Andrew Cooper <Andrew.Cooper3@citrix.com>, Julien Grall <julien.grall@arm.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, xen-devel <xen-devel@lists.xenproject.org>, Brian Woods <brian.woods@amd.com>, Roger Pau Monne <roger.pau@citrix.com> Subject: Re: [Xen-devel] [PATCH 3/5] iommu: move iommu_get_ops() into common code Date: Tue, 14 May 2019 16:19:55 +0000 [thread overview] Message-ID: <246a87e654194e5082852b63853415d6@AMSPEX02CL03.citrite.net> (raw) Message-ID: <20190514161955.UvIVVjynTHVl0qVNtlNvhMEvI7kT4HgCk-87z6RcpO0@z> (raw) In-Reply-To: <5CD99729020000780022E4B1@prv1-mh.provo.novell.com> > -----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
next prev parent reply other threads:[~2019-05-14 16:20 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-08 13:23 [PATCH 0/5] iommu groups + cleanup Paul Durrant 2019-05-08 13:23 ` [Xen-devel] " Paul Durrant 2019-05-08 13:23 ` [PATCH 1/5] iommu: trivial re-organisation to avoid unnecessary test Paul Durrant 2019-05-08 13:23 ` [Xen-devel] " Paul Durrant 2019-05-13 15:22 ` Jan Beulich 2019-05-13 15:22 ` [Xen-devel] " Jan Beulich 2019-05-08 13:24 ` [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code Paul Durrant 2019-05-08 13:24 ` [Xen-devel] " Paul Durrant 2019-05-13 15:35 ` Jan Beulich 2019-05-13 15:35 ` [Xen-devel] " Jan Beulich 2019-05-14 16:13 ` Paul Durrant 2019-05-14 16:13 ` [Xen-devel] " Paul Durrant 2019-05-15 6:29 ` Jan Beulich 2019-05-15 6:29 ` [Xen-devel] " Jan Beulich 2019-05-08 13:24 ` [PATCH 3/5] iommu: move iommu_get_ops() into common code Paul Durrant 2019-05-08 13:24 ` [Xen-devel] " Paul Durrant 2019-05-13 16:11 ` Jan Beulich 2019-05-13 16:11 ` [Xen-devel] " Jan Beulich 2019-05-14 16:19 ` Paul Durrant [this message] 2019-05-14 16:19 ` Paul Durrant 2019-05-14 21:36 ` Julien Grall 2019-05-14 21:36 ` [Xen-devel] " Julien Grall 2019-05-15 6:32 ` Jan Beulich 2019-05-15 6:32 ` [Xen-devel] " Jan Beulich 2019-05-08 13:24 ` [PATCH 4/5] iommu: introduce iommu_groups Paul Durrant 2019-05-08 13:24 ` [Xen-devel] " Paul Durrant 2019-05-15 8:44 ` Roger Pau Monné 2019-05-15 8:44 ` [Xen-devel] " Roger Pau Monné 2019-05-31 13:48 ` Paul Durrant 2019-05-31 13:48 ` [Xen-devel] " Paul Durrant 2019-05-15 14:17 ` Jan Beulich 2019-05-15 14:17 ` [Xen-devel] " Jan Beulich 2019-05-31 13:55 ` Paul Durrant 2019-05-31 13:55 ` [Xen-devel] " Paul Durrant 2019-05-31 14:13 ` Jan Beulich 2019-05-31 14:13 ` [Xen-devel] " Jan Beulich 2019-05-31 14:21 ` Paul Durrant 2019-05-31 14:21 ` [Xen-devel] " Paul Durrant 2019-05-15 14:24 ` Jan Beulich 2019-05-15 14:24 ` [Xen-devel] " Jan Beulich 2019-05-08 13:24 ` [PATCH 5/5] iommu / pci: re-implement XEN_DOMCTL_get_device_group Paul Durrant 2019-05-08 13:24 ` [Xen-devel] " Paul Durrant 2019-05-15 9:06 ` Roger Pau Monné 2019-05-15 9:06 ` [Xen-devel] " Roger Pau Monné 2019-06-03 9:58 ` Paul Durrant 2019-06-03 9:58 ` [Xen-devel] " Paul Durrant
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=246a87e654194e5082852b63853415d6@AMSPEX02CL03.citrite.net \ --to=paul.durrant@citrix.com \ --cc=Andrew.Cooper3@citrix.com \ --cc=JBeulich@suse.com \ --cc=brian.woods@amd.com \ --cc=julien.grall@arm.com \ --cc=kevin.tian@intel.com \ --cc=roger.pau@citrix.com \ --cc=sstabellini@kernel.org \ --cc=suravee.suthikulpanit@amd.com \ --cc=wei.liu2@citrix.com \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).