From: Robin Murphy <robin.murphy@arm.com> To: Baolu Lu <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>, Jason Gunthorpe <jgg@nvidia.com>, Christoph Hellwig <hch@infradead.org>, Kevin Tian <kevin.tian@intel.com>, Ashok Raj <ashok.raj@intel.com>, Will Deacon <will@kernel.org>, Jean-Philippe Brucker <jean-philippe@linaro.com>, Dave Jiang <dave.jiang@intel.com>, Vinod Koul <vkoul@kernel.org> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, Jacob jun Pan <jacob.jun.pan@intel.com> Subject: Re: [PATCH v7 03/10] iommu/sva: Add iommu_sva_domain support Date: Wed, 25 May 2022 11:07:49 +0100 [thread overview] Message-ID: <567dffd4-8f15-ffb2-da69-4f47017c35fd@arm.com> (raw) In-Reply-To: <ff8f23c0-8763-1fac-6526-9095101ca0e5@linux.intel.com> On 2022-05-25 07:20, Baolu Lu wrote: > Hi Robin, > > On 2022/5/24 22:36, Robin Murphy wrote: >> On 2022-05-19 08:20, Lu Baolu wrote: >> [...] >>> diff --git a/drivers/iommu/iommu-sva-lib.c >>> b/drivers/iommu/iommu-sva-lib.c >>> index 106506143896..210c376f6043 100644 >>> --- a/drivers/iommu/iommu-sva-lib.c >>> +++ b/drivers/iommu/iommu-sva-lib.c >>> @@ -69,3 +69,51 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid) >>> return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero); >>> } >>> EXPORT_SYMBOL_GPL(iommu_sva_find); >>> + >>> +/* >>> + * IOMMU SVA driver-oriented interfaces >>> + */ >>> +struct iommu_domain * >>> +iommu_sva_alloc_domain(struct bus_type *bus, struct mm_struct *mm) >> >> Argh, please no new bus-based external interfaces! Domain allocation >> needs to resolve to the right IOMMU instance to solve a number of >> issues, and cleaning up existing users of iommu_domain_alloc() to >> prepare for that is already hard enough. This is arguably even more >> relevant here than for other domain types, since SVA support is more >> likely to depend on specific features that can vary between IOMMU >> instances even with the same driver. Please make the external >> interface take a struct device, then resolve the ops through dev->iommu. >> >> Further nit: the naming inconsistency bugs me a bit - >> iommu_sva_domain_alloc() seems more natural. Also I'd question the >> symmetry vs. usability dichotomy of whether we *really* want two >> different free functions for a struct iommu_domain pointer, where any >> caller which might mix SVA and non-SVA usage then has to remember how >> they allocated any particular domain :/ >> >>> +{ >>> + struct iommu_sva_domain *sva_domain; >>> + struct iommu_domain *domain; >>> + >>> + if (!bus->iommu_ops || !bus->iommu_ops->sva_domain_ops) >>> + return ERR_PTR(-ENODEV); >>> + >>> + sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL); >>> + if (!sva_domain) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + mmgrab(mm); >>> + sva_domain->mm = mm; >>> + >>> + domain = &sva_domain->domain; >>> + domain->type = IOMMU_DOMAIN_SVA; >>> + domain->ops = bus->iommu_ops->sva_domain_ops; >> >> I'd have thought it would be logical to pass IOMMU_DOMAIN_SVA to the >> normal domain_alloc call, so that driver-internal stuff like context >> descriptors can be still be hung off the domain as usual (rather than >> all drivers having to implement some extra internal lookup mechanism >> to handle all the SVA domain ops), but that's something we're free to >> come > > Agreed with above comments. Thanks! I will post an additional patch > for review later. > >> back and change later. FWIW I'd just stick the mm pointer in struct >> iommu_domain, in a union with the fault handler stuff and/or >> iova_cookie - those are mutually exclusive with SVA, right? > > "iova_cookie" is mutually exclusive with SVA, but I am not sure about > the fault handler stuff. To correct myself, the IOVA cookie should be irrelevant to *current* SVA, but if we ever get as far as whole-device-SVA without PASIDs then we might need an MSI cookie (modulo the additional problem of stealing some procvess address space for it). > Did you mean @handler and @handler_token staffs below? > > struct iommu_domain { > unsigned type; > const struct iommu_domain_ops *ops; > unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */ > iommu_fault_handler_t handler; > void *handler_token; > struct iommu_domain_geometry geometry; > struct iommu_dma_cookie *iova_cookie; > }; > > Is it only for DMA domains? From the point view of IOMMU faults, it > seems to be generic. Yes, it's the old common iommu_set_fault_handler() stuff (which arguably is more of a "notifier" than a "handler"), but I assume that that's irrelevant if SVA is using IOPF instead? Thanks, Robin.
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Baolu Lu <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>, Jason Gunthorpe <jgg@nvidia.com>, Christoph Hellwig <hch@infradead.org>, Kevin Tian <kevin.tian@intel.com>, Ashok Raj <ashok.raj@intel.com>, Will Deacon <will@kernel.org>, Jean-Philippe Brucker <jean-philippe@linaro.com>, Dave Jiang <dave.jiang@intel.com>, Vinod Koul <vkoul@kernel.org> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Jacob jun Pan <jacob.jun.pan@intel.com> Subject: Re: [PATCH v7 03/10] iommu/sva: Add iommu_sva_domain support Date: Wed, 25 May 2022 11:07:49 +0100 [thread overview] Message-ID: <567dffd4-8f15-ffb2-da69-4f47017c35fd@arm.com> (raw) In-Reply-To: <ff8f23c0-8763-1fac-6526-9095101ca0e5@linux.intel.com> On 2022-05-25 07:20, Baolu Lu wrote: > Hi Robin, > > On 2022/5/24 22:36, Robin Murphy wrote: >> On 2022-05-19 08:20, Lu Baolu wrote: >> [...] >>> diff --git a/drivers/iommu/iommu-sva-lib.c >>> b/drivers/iommu/iommu-sva-lib.c >>> index 106506143896..210c376f6043 100644 >>> --- a/drivers/iommu/iommu-sva-lib.c >>> +++ b/drivers/iommu/iommu-sva-lib.c >>> @@ -69,3 +69,51 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid) >>> return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero); >>> } >>> EXPORT_SYMBOL_GPL(iommu_sva_find); >>> + >>> +/* >>> + * IOMMU SVA driver-oriented interfaces >>> + */ >>> +struct iommu_domain * >>> +iommu_sva_alloc_domain(struct bus_type *bus, struct mm_struct *mm) >> >> Argh, please no new bus-based external interfaces! Domain allocation >> needs to resolve to the right IOMMU instance to solve a number of >> issues, and cleaning up existing users of iommu_domain_alloc() to >> prepare for that is already hard enough. This is arguably even more >> relevant here than for other domain types, since SVA support is more >> likely to depend on specific features that can vary between IOMMU >> instances even with the same driver. Please make the external >> interface take a struct device, then resolve the ops through dev->iommu. >> >> Further nit: the naming inconsistency bugs me a bit - >> iommu_sva_domain_alloc() seems more natural. Also I'd question the >> symmetry vs. usability dichotomy of whether we *really* want two >> different free functions for a struct iommu_domain pointer, where any >> caller which might mix SVA and non-SVA usage then has to remember how >> they allocated any particular domain :/ >> >>> +{ >>> + struct iommu_sva_domain *sva_domain; >>> + struct iommu_domain *domain; >>> + >>> + if (!bus->iommu_ops || !bus->iommu_ops->sva_domain_ops) >>> + return ERR_PTR(-ENODEV); >>> + >>> + sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL); >>> + if (!sva_domain) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + mmgrab(mm); >>> + sva_domain->mm = mm; >>> + >>> + domain = &sva_domain->domain; >>> + domain->type = IOMMU_DOMAIN_SVA; >>> + domain->ops = bus->iommu_ops->sva_domain_ops; >> >> I'd have thought it would be logical to pass IOMMU_DOMAIN_SVA to the >> normal domain_alloc call, so that driver-internal stuff like context >> descriptors can be still be hung off the domain as usual (rather than >> all drivers having to implement some extra internal lookup mechanism >> to handle all the SVA domain ops), but that's something we're free to >> come > > Agreed with above comments. Thanks! I will post an additional patch > for review later. > >> back and change later. FWIW I'd just stick the mm pointer in struct >> iommu_domain, in a union with the fault handler stuff and/or >> iova_cookie - those are mutually exclusive with SVA, right? > > "iova_cookie" is mutually exclusive with SVA, but I am not sure about > the fault handler stuff. To correct myself, the IOVA cookie should be irrelevant to *current* SVA, but if we ever get as far as whole-device-SVA without PASIDs then we might need an MSI cookie (modulo the additional problem of stealing some procvess address space for it). > Did you mean @handler and @handler_token staffs below? > > struct iommu_domain { > unsigned type; > const struct iommu_domain_ops *ops; > unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */ > iommu_fault_handler_t handler; > void *handler_token; > struct iommu_domain_geometry geometry; > struct iommu_dma_cookie *iova_cookie; > }; > > Is it only for DMA domains? From the point view of IOMMU faults, it > seems to be generic. Yes, it's the old common iommu_set_fault_handler() stuff (which arguably is more of a "notifier" than a "handler"), but I assume that that's irrelevant if SVA is using IOPF instead? Thanks, Robin. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2022-05-25 10:08 UTC|newest] Thread overview: 100+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-19 7:20 [PATCH v7 00/10] iommu: SVA and IOPF refactoring Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 7:20 ` [PATCH v7 01/10] iommu: Add pasids field in struct iommu_device Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 10:37 ` Jean-Philippe Brucker 2022-05-19 10:37 ` Jean-Philippe Brucker 2022-05-19 11:55 ` Baolu Lu 2022-05-19 11:55 ` Baolu Lu 2022-05-24 9:24 ` Tian, Kevin 2022-05-24 9:24 ` Tian, Kevin 2022-05-25 2:03 ` Baolu Lu 2022-05-25 2:03 ` Baolu Lu 2022-05-25 2:13 ` Baolu Lu 2022-05-25 2:13 ` Baolu Lu 2022-05-19 7:20 ` [PATCH v7 02/10] iommu: Remove SVM_FLAG_SUPERVISOR_MODE support Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 16:22 ` Jean-Philippe Brucker 2022-05-19 16:22 ` Jean-Philippe Brucker 2022-05-24 9:27 ` Tian, Kevin 2022-05-24 9:27 ` Tian, Kevin 2022-05-19 7:20 ` [PATCH v7 03/10] iommu/sva: Add iommu_sva_domain support Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 16:33 ` Jean-Philippe Brucker 2022-05-19 16:33 ` Jean-Philippe Brucker 2022-05-20 4:55 ` Baolu Lu 2022-05-20 4:55 ` Baolu Lu 2022-05-23 7:12 ` Baolu Lu 2022-05-23 7:12 ` Baolu Lu 2022-05-24 9:44 ` Tian, Kevin 2022-05-24 9:44 ` Tian, Kevin 2022-05-25 2:18 ` Baolu Lu 2022-05-25 2:18 ` Baolu Lu 2022-05-24 9:39 ` Tian, Kevin 2022-05-24 9:39 ` Tian, Kevin 2022-05-24 13:38 ` Jason Gunthorpe 2022-05-24 13:38 ` Jason Gunthorpe via iommu 2022-05-25 0:44 ` Tian, Kevin 2022-05-25 0:44 ` Tian, Kevin 2022-05-25 2:38 ` Baolu Lu 2022-05-25 2:38 ` Baolu Lu 2022-05-25 4:50 ` Baolu Lu 2022-05-25 4:50 ` Baolu Lu 2022-05-24 13:44 ` Jason Gunthorpe 2022-05-24 13:44 ` Jason Gunthorpe via iommu 2022-05-25 5:19 ` Baolu Lu 2022-05-25 5:19 ` Baolu Lu 2022-05-25 15:25 ` Jason Gunthorpe 2022-05-25 15:25 ` Jason Gunthorpe via iommu 2022-05-26 1:03 ` Baolu Lu 2022-05-26 1:03 ` Baolu Lu 2022-05-25 5:33 ` Baolu Lu 2022-05-25 5:33 ` Baolu Lu 2022-05-24 14:36 ` Robin Murphy 2022-05-24 14:36 ` Robin Murphy 2022-05-25 6:20 ` Baolu Lu 2022-05-25 6:20 ` Baolu Lu 2022-05-25 10:07 ` Robin Murphy [this message] 2022-05-25 10:07 ` Robin Murphy 2022-05-25 11:06 ` Jean-Philippe Brucker 2022-05-25 11:06 ` Jean-Philippe Brucker 2022-05-25 13:11 ` Baolu Lu 2022-05-25 13:11 ` Baolu Lu 2022-05-19 7:20 ` [PATCH v7 04/10] iommu/vt-d: Add SVA domain support Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 7:20 ` [PATCH v7 05/10] arm-smmu-v3/sva: " Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 16:37 ` Jean-Philippe Brucker 2022-05-19 16:37 ` Jean-Philippe Brucker 2022-05-19 7:20 ` [PATCH v7 06/10] iommu/sva: Refactoring iommu_sva_bind/unbind_device() Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 16:39 ` Jean-Philippe Brucker 2022-05-19 16:39 ` Jean-Philippe Brucker 2022-05-20 6:38 ` Baolu Lu 2022-05-20 6:38 ` Baolu Lu 2022-05-20 11:28 ` Jean-Philippe Brucker 2022-05-20 11:28 ` Jean-Philippe Brucker 2022-05-23 3:07 ` Baolu Lu 2022-05-23 3:07 ` Baolu Lu 2022-05-24 10:22 ` Tian, Kevin 2022-05-24 10:22 ` Tian, Kevin 2022-05-24 10:57 ` Jean-Philippe Brucker 2022-05-24 10:57 ` Jean-Philippe Brucker 2022-05-25 2:04 ` Tian, Kevin 2022-05-25 2:04 ` Tian, Kevin 2022-05-25 7:29 ` Jean-Philippe Brucker 2022-05-25 7:29 ` Jean-Philippe Brucker 2022-06-02 6:46 ` Tian, Kevin 2022-06-02 6:46 ` Tian, Kevin 2022-05-19 7:20 ` [PATCH v7 07/10] iommu: Remove SVA related callbacks from iommu ops Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-24 10:23 ` Tian, Kevin 2022-05-24 10:23 ` Tian, Kevin 2022-05-19 7:20 ` [PATCH v7 08/10] iommu: Prepare IOMMU domain for IOPF Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 16:40 ` Jean-Philippe Brucker 2022-05-19 16:40 ` Jean-Philippe Brucker 2022-05-19 7:20 ` [PATCH v7 09/10] iommu: Per-domain I/O page fault handling Lu Baolu 2022-05-19 7:20 ` Lu Baolu 2022-05-19 7:20 ` [PATCH v7 10/10] iommu: Rename iommu-sva-lib.{c,h} Lu Baolu 2022-05-19 7:20 ` Lu Baolu
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=567dffd4-8f15-ffb2-da69-4f47017c35fd@arm.com \ --to=robin.murphy@arm.com \ --cc=ashok.raj@intel.com \ --cc=baolu.lu@linux.intel.com \ --cc=dave.jiang@intel.com \ --cc=hch@infradead.org \ --cc=iommu@lists.linux-foundation.org \ --cc=jacob.jun.pan@intel.com \ --cc=jean-philippe@linaro.com \ --cc=jean-philippe@linaro.org \ --cc=jgg@nvidia.com \ --cc=joro@8bytes.org \ --cc=kevin.tian@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=vkoul@kernel.org \ --cc=will@kernel.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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.