Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Mon, Jan 24, 2022 at 12:33:02PM -0400, Jason Gunthorpe wrote: > On Mon, Jan 24, 2022 at 10:16:07AM +, Jean-Philippe Brucker wrote: > > On Mon, Jan 24, 2022 at 09:58:18AM +, Tian, Kevin wrote: > > > > From: Lu Baolu > > > > Sent: Monday, January 24, 2022 3:11 PM > > > > +/** > > > > + * struct domain_ops - per-domain ops > > > > + * @attach_dev: attach an iommu domain to a device > > > > + * @detach_dev: detach an iommu domain from a device > > > > > > What is the criteria about whether an op should be iommu_ops or domain_ops > > > when it requires both domain and device pointers like above two (and > > > future > > > PASID-based attach)? > > > > > > Other examples include: > > > @apply_resv_region > > > @is_attach_deferred > > > > Could attach_dev() be an IOMMU op? So a driver could set the domain ops > > in attach_dev() rather than domain_alloc(). That would allow to install > > map()/unmap() ops that are tailored for the device's IOMMU, which we don't > > know at domain_alloc() time. > > I think we should be moving toward 'domain_alloc' returning the > correct domain and the way the driver implements the domain shouldn't > change after that. > > > I'm thinking about a guest that has both physical and virtual > > endpoints, which would ideally use different kinds of domain ops to > > support both efficiently (caching mode vs page tables) > > In this case shouldn't domain_alloc() reached from the struct device > already do the correct thing? Sure, if we can finalise the domains before attach that could also clean up the drivers a bit. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Tue, Jan 25, 2022 at 02:23:52PM +, Robin Murphy wrote: > On 2022-01-25 06:27, Lu Baolu wrote: > Where it's just about which operations are valid for which domains, it's > even simpler for the core interface wrappers to validate the domain type, > rather than forcing drivers to implement multiple ops structures purely for > the sake of having different callbacks populated. We already have this in > places, e.g. where iommu_map() checks for __IOMMU_DOMAIN_PAGING. In my experience it is usually much clearer to directly test the op for NULL to know if a feature is supported than to invent flags to do the same test. eg ops->map/etc == NULL means no paging. I think we should not be afraid to have multiple ops in drivers for things that are actually different in the driver. This is usually a net win vs tying to handle all the cases with different 'if' flows. eg identity domains and others really would ideally eventually have a NULL ops for map/unmap too. The 'type' should conceptually be part of the ops, not the mutable struct - but we don't have to get there all at once. > Paging domains are also effectively the baseline level of IOMMU API > functionality. All drivers support them, and for the majority of drivers > it's all they will ever support. Those drivers really don't benefit from any > of the churn and boilerplate in this patch as-is, and it's so easy to > compromise with a couple of lines of core code to handle the common case by > default when the driver *isn't* one of the handful which ever actually cares > to install their own per-domain ops. Consider how much cleaner this patch > would look if the typical driver diff could be something completely minimal > like this: It is clever, but I'm not sure if hoisting a single assignment out of the driver is worth the small long term complexity of having different driver flows? Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On 2022-01-25 06:27, Lu Baolu wrote:
On 1/25/22 8:57 AM, Robin Murphy wrote:
On 2022-01-24 07:11, Lu Baolu wrote:
Add a domain specific callback set, domain_ops, for vendor iommu driver
to provide domain specific operations. Move domain-specific callbacks
from iommu_ops to the domain_ops and hook them when a domain is
allocated.
I think it would make a lot of sense for iommu_domain_ops to be a
substructure *within* iommu_ops. That way we can save most of the
driver boilerplate here by not needing extra variables everywhere, and
letting iommu_domain_alloc() still do a default initialisation like
"domain->ops = bus->iommu_ops->domain_ops;"
In the new model, iommu_domain_ops and iommu_ops are not 1:1 mapped.
For example, a PASID-capable IOMMU could support DMA domain (which
supports map/unmap), SVA domain (which does not), and others in the
future. Different type of domain has its own domain_ops.
Sure, it's clear that that's the direction in which this is headed, and
as I say I'm quite excited about that. However there are a couple of
points I think are really worth considering:
Where it's just about which operations are valid for which domains, it's
even simpler for the core interface wrappers to validate the domain
type, rather than forcing drivers to implement multiple ops structures
purely for the sake of having different callbacks populated. We already
have this in places, e.g. where iommu_map() checks for
__IOMMU_DOMAIN_PAGING.
Paging domains are also effectively the baseline level of IOMMU API
functionality. All drivers support them, and for the majority of drivers
it's all they will ever support. Those drivers really don't benefit from
any of the churn and boilerplate in this patch as-is, and it's so easy
to compromise with a couple of lines of core code to handle the common
case by default when the driver *isn't* one of the handful which ever
actually cares to install their own per-domain ops. Consider how much
cleaner this patch would look if the typical driver diff could be
something completely minimal like this:
->8-
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index be22fcf988ce..6aff493e37ee 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -514,12 +514,14 @@ static int mtk_iommu_hw_init(const struct
mtk_iommu_data *data)
static const struct iommu_ops mtk_iommu_ops = {
.domain_alloc = mtk_iommu_domain_alloc,
- .domain_free= mtk_iommu_domain_free,
- .attach_dev = mtk_iommu_attach_device,
- .detach_dev = mtk_iommu_detach_device,
- .map= mtk_iommu_map,
- .unmap = mtk_iommu_unmap,
- .iova_to_phys = mtk_iommu_iova_to_phys,
+ .domain_ops = &(const struct iommu_domain_ops){
+ .domain_free= mtk_iommu_domain_free,
+ .attach_dev = mtk_iommu_attach_device,
+ .detach_dev = mtk_iommu_detach_device,
+ .map= mtk_iommu_map,
+ .unmap = mtk_iommu_unmap,
+ .iova_to_phys = mtk_iommu_iova_to_phys,
+ }
.probe_device = mtk_iommu_probe_device,
.probe_finalize = mtk_iommu_probe_finalize,
.release_device = mtk_iommu_release_device,
-8<-
And of course I have to shy away from calling it default_domain_ops,
since although it's logically default ops for domains, it is not
specifically ops for default domains, sigh... :)
Cheers,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Tue, Jan 25, 2022 at 12:59:14PM +0800, Lu Baolu wrote: > On 1/24/22 5:58 PM, Tian, Kevin wrote: > > > From: Lu Baolu > > > Sent: Monday, January 24, 2022 3:11 PM > > > +/** > > > + * struct domain_ops - per-domain ops > > > + * @attach_dev: attach an iommu domain to a device > > > + * @detach_dev: detach an iommu domain from a device > > > > What is the criteria about whether an op should be iommu_ops or domain_ops > > when it requires both domain and device pointers like above two (and future > > PASID-based attach)? > > Generally ops belong to iommu_ops if they are only device oriented, and > domain_ops if they are only domain oriented. But it's really hard to > judge when both device and domain are involved. Good question. :-) > > Perhaps we should leave the attach/detach interfaces in iommu_ops since > they are related to device capabilities. For example, some devices > support attach_dev_pasid, while others not. Isn't the attach process for something like SVA or the KVM version actually rather different code? Ideally the function pointers should help minimize case statements/etc inside the driver.. Or stated another way, I expect drivers will have different structs container_of'ing back to the iommu_domain (ie intel_iommu_domain_sva, say). Any code that needs to work differently depending on the struct should have an op in the domain so it can sanely container_of without a mess. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On 1/25/22 8:57 AM, Robin Murphy wrote:
On 2022-01-24 07:11, Lu Baolu wrote:
Add a domain specific callback set, domain_ops, for vendor iommu driver
to provide domain specific operations. Move domain-specific callbacks
from iommu_ops to the domain_ops and hook them when a domain is
allocated.
I think it would make a lot of sense for iommu_domain_ops to be a
substructure *within* iommu_ops. That way we can save most of the driver
boilerplate here by not needing extra variables everywhere, and letting
iommu_domain_alloc() still do a default initialisation like "domain->ops
= bus->iommu_ops->domain_ops;"
In the new model, iommu_domain_ops and iommu_ops are not 1:1 mapped.
For example, a PASID-capable IOMMU could support DMA domain (which
supports map/unmap), SVA domain (which does not), and others in the
future. Different type of domain has its own domain_ops.
I do like the future possibility of trying to flatten some of the
io-pgtable indirection by having the SMMU drivers subsequently swizzle
in alternate domain ops once they've picked a format, though. That was a
bit too clunky to achieve at the whole iommu_ops level when I
experimented with it years ago, but now it might well be worth another
try...
One other comment below.
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 93 -
drivers/iommu/amd/iommu.c | 21 +++--
drivers/iommu/apple-dart.c | 24 --
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 +++--
drivers/iommu/arm/arm-smmu/arm-smmu.c | 23 +++--
drivers/iommu/arm/arm-smmu/qcom_iommu.c | 17 ++--
drivers/iommu/exynos-iommu.c | 17 ++--
drivers/iommu/fsl_pamu_domain.c | 13 ++-
drivers/iommu/intel/iommu.c | 25 --
drivers/iommu/iommu.c | 15 ++--
drivers/iommu/ipmmu-vmsa.c | 21 +++--
drivers/iommu/msm_iommu.c | 17 ++--
drivers/iommu/mtk_iommu.c | 24 --
drivers/iommu/mtk_iommu_v1.c | 19 +++--
drivers/iommu/omap-iommu.c | 15 ++--
drivers/iommu/rockchip-iommu.c | 17 ++--
drivers/iommu/s390-iommu.c | 15 ++--
drivers/iommu/sprd-iommu.c | 19 +++--
drivers/iommu/sun50i-iommu.c | 18 ++--
drivers/iommu/tegra-gart.c | 15 ++--
drivers/iommu/tegra-smmu.c | 16 ++--
drivers/iommu/virtio-iommu.c | 18 ++--
22 files changed, 305 insertions(+), 179 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 111b3e9c79bb..33c5c0e5c465 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -88,7 +88,7 @@ struct iommu_domain_geometry {
struct iommu_domain {
unsigned type;
- const struct iommu_ops *ops;
+ const struct domain_ops *ops;
unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */
iommu_fault_handler_t handler;
void *handler_token;
@@ -192,26 +192,11 @@ struct iommu_iotlb_gather {
* struct iommu_ops - iommu ops and capabilities
* @capable: check capability
* @domain_alloc: allocate iommu domain
- * @domain_free: free iommu domain
- * @attach_dev: attach device to an iommu domain
- * @detach_dev: detach device from an iommu domain
- * @map: map a physically contiguous memory region to an iommu domain
- * @map_pages: map a physically contiguous set of pages of the same
size to
- * an iommu domain.
- * @unmap: unmap a physically contiguous memory region from an iommu
domain
- * @unmap_pages: unmap a number of pages of the same size from an
iommu domain
- * @flush_iotlb_all: Synchronously flush all hardware TLBs for this
domain
- * @iotlb_sync_map: Sync mappings created recently using @map to the
hardware
- * @iotlb_sync: Flush all queued ranges from the hardware TLBs and
empty flush
- * queue
- * @iova_to_phys: translate iova to physical address
* @probe_device: Add device to iommu driver handling
* @release_device: Remove device from iommu driver handling
* @probe_finalize: Do final setup work after the device is added to
an IOMMU
* group and attached to the groups domain
* @device_group: find iommu group for a particular device
- * @enable_nesting: Enable nesting
- * @set_pgtable_quirks: Set io page table quirks (IO_PGTABLE_QUIRK_*)
* @get_resv_regions: Request list of reserved regions for a device
* @put_resv_regions: Free list of reserved regions for a device
* @apply_resv_region: Temporary helper call-back for iova reserved
ranges
@@ -237,33 +222,11 @@ struct iommu_ops {
/* Domain allocation and freeing by the iommu driver */
struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
- void (*domain_free)(struct iommu_domain *);
- int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
- void
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On 1/25/22 1:55 AM, Jason Gunthorpe wrote: On Mon, Jan 24, 2022 at 03:11:02PM +0800, Lu Baolu wrote: - int (*enable_nesting)(struct iommu_domain *domain); Lu, there is an implementation in the Intel driver here, is it usable at all? It's useless and I have cleaned it up in this series. Jason Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On 1/24/22 5:58 PM, Tian, Kevin wrote: From: Lu Baolu Sent: Monday, January 24, 2022 3:11 PM +/** + * struct domain_ops - per-domain ops + * @attach_dev: attach an iommu domain to a device + * @detach_dev: detach an iommu domain from a device What is the criteria about whether an op should be iommu_ops or domain_ops when it requires both domain and device pointers like above two (and future PASID-based attach)? Generally ops belong to iommu_ops if they are only device oriented, and domain_ops if they are only domain oriented. But it's really hard to judge when both device and domain are involved. Good question. :-) Perhaps we should leave the attach/detach interfaces in iommu_ops since they are related to device capabilities. For example, some devices support attach_dev_pasid, while others not. Other examples include: @apply_resv_region This will be deprecated. @is_attach_deferred Should be at least device centric (domain doesn't play any role here). Further step is to save the is_attach_deferred at a flag in dev_iommu and remove the ops (as Robin suggested). Thanks Kevin Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On 1/25/22 1:24 AM, Jason Gunthorpe wrote: On Mon, Jan 24, 2022 at 01:37:21AM -0800, Christoph Hellwig wrote: On Mon, Jan 24, 2022 at 03:11:02PM +0800, Lu Baolu wrote: Add a domain specific callback set, domain_ops, for vendor iommu driver to provide domain specific operations. Move domain-specific callbacks from iommu_ops to the domain_ops and hook them when a domain is allocated. Ah, that's what I meant earlier. Perfect! Nit: I don't think vendor is the right term here. +1 please don't use the confusing 'vendor' in the kernel, if you feel you want to write 'vendor' writing 'device driver' is usually a good choice.. Sure! This whole series is nice, the few small notes aside, thanks for making it! Thank you! Jason Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On 1/24/22 5:37 PM, Christoph Hellwig wrote:
On Mon, Jan 24, 2022 at 03:11:02PM +0800, Lu Baolu wrote:
Add a domain specific callback set, domain_ops, for vendor iommu driver
to provide domain specific operations. Move domain-specific callbacks
from iommu_ops to the domain_ops and hook them when a domain is allocated.
Ah, that's what I meant earlier. Perfect!
Nit: I don't think vendor is the right term here.
Maybe something like:
iommut: split struct iommu_ops
Move the domain specific operations out of struct iommu_ops into a new
structure that only has domain specific operations. This solves
the problem of needing to know if the method vector for a given
operation needs to be retreived from the device or th domain.
Sure. Will use above description.
+struct domain_ops {
This needs to keep an iommu in the name, i.e. iommu_domain_ops.
Sure.
+ phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t
iova);
Overly long line.
./scripts/checkpatch.pl --strict *.patch
didn't give me a WARN or CHECK. I will make it short anyway.
+static inline void iommu_domain_set_ops(struct iommu_domain *domain,
+ const struct domain_ops *ops)
+{
+ domain->ops = ops;
+}
Do we really need this helper?
Unnecessary. I can set the pointer directly in the drivers.
+static const struct domain_ops amd_domain_ops;
Can we avoid these forward declarations or would that cause too much
churn?
I don't like this either. But it's common to put the ops at the bottom
of the file in almost all iommu drivers.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On 2022-01-24 07:11, Lu Baolu wrote:
Add a domain specific callback set, domain_ops, for vendor iommu driver
to provide domain specific operations. Move domain-specific callbacks
from iommu_ops to the domain_ops and hook them when a domain is allocated.
I think it would make a lot of sense for iommu_domain_ops to be a
substructure *within* iommu_ops. That way we can save most of the driver
boilerplate here by not needing extra variables everywhere, and letting
iommu_domain_alloc() still do a default initialisation like "domain->ops
= bus->iommu_ops->domain_ops;"
I do like the future possibility of trying to flatten some of the
io-pgtable indirection by having the SMMU drivers subsequently swizzle
in alternate domain ops once they've picked a format, though. That was a
bit too clunky to achieve at the whole iommu_ops level when I
experimented with it years ago, but now it might well be worth another
try...
One other comment below.
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 93 -
drivers/iommu/amd/iommu.c | 21 +++--
drivers/iommu/apple-dart.c | 24 --
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 +++--
drivers/iommu/arm/arm-smmu/arm-smmu.c | 23 +++--
drivers/iommu/arm/arm-smmu/qcom_iommu.c | 17 ++--
drivers/iommu/exynos-iommu.c| 17 ++--
drivers/iommu/fsl_pamu_domain.c | 13 ++-
drivers/iommu/intel/iommu.c | 25 --
drivers/iommu/iommu.c | 15 ++--
drivers/iommu/ipmmu-vmsa.c | 21 +++--
drivers/iommu/msm_iommu.c | 17 ++--
drivers/iommu/mtk_iommu.c | 24 --
drivers/iommu/mtk_iommu_v1.c| 19 +++--
drivers/iommu/omap-iommu.c | 15 ++--
drivers/iommu/rockchip-iommu.c | 17 ++--
drivers/iommu/s390-iommu.c | 15 ++--
drivers/iommu/sprd-iommu.c | 19 +++--
drivers/iommu/sun50i-iommu.c| 18 ++--
drivers/iommu/tegra-gart.c | 15 ++--
drivers/iommu/tegra-smmu.c | 16 ++--
drivers/iommu/virtio-iommu.c| 18 ++--
22 files changed, 305 insertions(+), 179 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 111b3e9c79bb..33c5c0e5c465 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -88,7 +88,7 @@ struct iommu_domain_geometry {
struct iommu_domain {
unsigned type;
- const struct iommu_ops *ops;
+ const struct domain_ops *ops;
unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
iommu_fault_handler_t handler;
void *handler_token;
@@ -192,26 +192,11 @@ struct iommu_iotlb_gather {
* struct iommu_ops - iommu ops and capabilities
* @capable: check capability
* @domain_alloc: allocate iommu domain
- * @domain_free: free iommu domain
- * @attach_dev: attach device to an iommu domain
- * @detach_dev: detach device from an iommu domain
- * @map: map a physically contiguous memory region to an iommu domain
- * @map_pages: map a physically contiguous set of pages of the same size to
- * an iommu domain.
- * @unmap: unmap a physically contiguous memory region from an iommu domain
- * @unmap_pages: unmap a number of pages of the same size from an iommu domain
- * @flush_iotlb_all: Synchronously flush all hardware TLBs for this domain
- * @iotlb_sync_map: Sync mappings created recently using @map to the hardware
- * @iotlb_sync: Flush all queued ranges from the hardware TLBs and empty flush
- *queue
- * @iova_to_phys: translate iova to physical address
* @probe_device: Add device to iommu driver handling
* @release_device: Remove device from iommu driver handling
* @probe_finalize: Do final setup work after the device is added to an IOMMU
* group and attached to the groups domain
* @device_group: find iommu group for a particular device
- * @enable_nesting: Enable nesting
- * @set_pgtable_quirks: Set io page table quirks (IO_PGTABLE_QUIRK_*)
* @get_resv_regions: Request list of reserved regions for a device
* @put_resv_regions: Free list of reserved regions for a device
* @apply_resv_region: Temporary helper call-back for iova reserved ranges
@@ -237,33 +222,11 @@ struct iommu_ops {
/* Domain allocation and freeing by the iommu driver */
struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
- void (*domain_free)(struct iommu_domain *);
- int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
- void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
- int (*map)(struct iommu_domain *domain, unsigned long iova,
- phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
- int (*map_pages)(struct iommu_domain *domain, unsigned long iova,
-
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Mon, Jan 24, 2022 at 03:11:02PM +0800, Lu Baolu wrote:
> - int (*enable_nesting)(struct iommu_domain *domain);
Perhaps for another series, but enable_nesting looks like dead code
too, AFAICT.
Or at the very least I can't figure how out VFIO_TYPE1_NESTING_IOMMU
is supposed to work, or find any implementation of it in userspace.
8 years after it was merged in commit f5c9ecebaf2a ("vfio/iommu_type1:
add new VFIO_TYPE1_NESTING_IOMMU IOMMU type")
The commit comment says 'which are nested with the existing
translation installed by VFIO.' but it doesn't explain how the newly
created domain knows what its parent nest is..
Lu, there is an implementation in the Intel driver here, is it usable
at all?
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Mon, Jan 24, 2022 at 01:37:21AM -0800, Christoph Hellwig wrote: > On Mon, Jan 24, 2022 at 03:11:02PM +0800, Lu Baolu wrote: > > Add a domain specific callback set, domain_ops, for vendor iommu driver > > to provide domain specific operations. Move domain-specific callbacks > > from iommu_ops to the domain_ops and hook them when a domain is allocated. > > Ah, that's what I meant earlier. Perfect! > > Nit: I don't think vendor is the right term here. +1 please don't use the confusing 'vendor' in the kernel, if you feel you want to write 'vendor' writing 'device driver' is usually a good choice.. This whole series is nice, the few small notes aside, thanks for making it! Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Mon, Jan 24, 2022 at 09:58:18AM +, Tian, Kevin wrote: > > From: Lu Baolu > > Sent: Monday, January 24, 2022 3:11 PM > > +/** > > + * struct domain_ops - per-domain ops > > + * @attach_dev: attach an iommu domain to a device > > + * @detach_dev: detach an iommu domain from a device > > What is the criteria about whether an op should be iommu_ops or domain_ops > when it requires both domain and device pointers like above two (and future > PASID-based attach)? > > Other examples include: > @apply_resv_region For apply_resv_region the 'dev' argument is really selecting a device that is already attached to the domain, so it should be in the domain ops. > @is_attach_deferred Only two drivers implement this and neither use the domain argument. Remove the domain argument and keep it in the iommu_ops Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Mon, Jan 24, 2022 at 10:16:07AM +, Jean-Philippe Brucker wrote: > On Mon, Jan 24, 2022 at 09:58:18AM +, Tian, Kevin wrote: > > > From: Lu Baolu > > > Sent: Monday, January 24, 2022 3:11 PM > > > +/** > > > + * struct domain_ops - per-domain ops > > > + * @attach_dev: attach an iommu domain to a device > > > + * @detach_dev: detach an iommu domain from a device > > > > What is the criteria about whether an op should be iommu_ops or domain_ops > > when it requires both domain and device pointers like above two (and future > > PASID-based attach)? > > > > Other examples include: > > @apply_resv_region > > @is_attach_deferred > > Could attach_dev() be an IOMMU op? So a driver could set the domain ops > in attach_dev() rather than domain_alloc(). That would allow to install > map()/unmap() ops that are tailored for the device's IOMMU, which we don't > know at domain_alloc() time. I think we should be moving toward 'domain_alloc' returning the correct domain and the way the driver implements the domain shouldn't change after that. > I'm thinking about a guest that has both physical and virtual > endpoints, which would ideally use different kinds of domain ops to > support both efficiently (caching mode vs page tables) In this case shouldn't domain_alloc() reached from the struct device already do the correct thing? Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Mon, Jan 24, 2022 at 09:58:18AM +, Tian, Kevin wrote: > > From: Lu Baolu > > Sent: Monday, January 24, 2022 3:11 PM > > +/** > > + * struct domain_ops - per-domain ops > > + * @attach_dev: attach an iommu domain to a device > > + * @detach_dev: detach an iommu domain from a device > > What is the criteria about whether an op should be iommu_ops or domain_ops > when it requires both domain and device pointers like above two (and future > PASID-based attach)? > > Other examples include: > @apply_resv_region > @is_attach_deferred Could attach_dev() be an IOMMU op? So a driver could set the domain ops in attach_dev() rather than domain_alloc(). That would allow to install map()/unmap() ops that are tailored for the device's IOMMU, which we don't know at domain_alloc() time. I'm thinking about a guest that has both physical and virtual endpoints, which would ideally use different kinds of domain ops to support both efficiently (caching mode vs page tables) Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
> From: Lu Baolu > Sent: Monday, January 24, 2022 3:11 PM > +/** > + * struct domain_ops - per-domain ops > + * @attach_dev: attach an iommu domain to a device > + * @detach_dev: detach an iommu domain from a device What is the criteria about whether an op should be iommu_ops or domain_ops when it requires both domain and device pointers like above two (and future PASID-based attach)? Other examples include: @apply_resv_region @is_attach_deferred Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops
On Mon, Jan 24, 2022 at 03:11:02PM +0800, Lu Baolu wrote:
> Add a domain specific callback set, domain_ops, for vendor iommu driver
> to provide domain specific operations. Move domain-specific callbacks
> from iommu_ops to the domain_ops and hook them when a domain is allocated.
Ah, that's what I meant earlier. Perfect!
Nit: I don't think vendor is the right term here.
Maybe something like:
iommut: split struct iommu_ops
Move the domain specific operations out of struct iommu_ops into a new
structure that only has domain specific operations. This solves
the problem of needing to know if the method vector for a given
operation needs to be retreived from the device or th domain.
> +struct domain_ops {
This needs to keep an iommu in the name, i.e. iommu_domain_ops.
> + phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t
> iova);
Overly long line.
> +static inline void iommu_domain_set_ops(struct iommu_domain *domain,
> + const struct domain_ops *ops)
> +{
> + domain->ops = ops;
> +}
Do we really need this helper?
> +static const struct domain_ops amd_domain_ops;
Can we avoid these forward declarations or would that cause too much
churn?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
