| Message ID | 20240412081516.31168-2-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommufd support pasid attach/replace | expand |
On 4/12/24 4:15 PM, Yi Liu wrote: > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 40dd439307e8..1e5e9249c93f 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -631,7 +631,7 @@ struct iommu_ops { > struct iommu_domain_ops { > int (*attach_dev)(struct iommu_domain *domain, struct device *dev); > int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev, > - ioasid_t pasid); > + ioasid_t pasid, struct iommu_domain *old); Is it possible to add another op to replace a domain for pasid? For example, int (*replace_dev_pasid)(domain, dev, pasid, old_domain) Best regards, baolu
On Mon, Apr 15, 2024 at 01:32:03PM +0800, Baolu Lu wrote: > On 4/12/24 4:15 PM, Yi Liu wrote: > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > index 40dd439307e8..1e5e9249c93f 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -631,7 +631,7 @@ struct iommu_ops { > > struct iommu_domain_ops { > > int (*attach_dev)(struct iommu_domain *domain, struct device *dev); > > int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev, > > - ioasid_t pasid); > > + ioasid_t pasid, struct iommu_domain *old); > > Is it possible to add another op to replace a domain for pasid? For > example, > > int (*replace_dev_pasid)(domain, dev, pasid, old_domain) We haven't needed that in the normal case, what would motivate it here? Jason
On 4/15/24 7:54 PM, Jason Gunthorpe wrote: > On Mon, Apr 15, 2024 at 01:32:03PM +0800, Baolu Lu wrote: >> On 4/12/24 4:15 PM, Yi Liu wrote: >>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >>> index 40dd439307e8..1e5e9249c93f 100644 >>> --- a/include/linux/iommu.h >>> +++ b/include/linux/iommu.h >>> @@ -631,7 +631,7 @@ struct iommu_ops { >>> struct iommu_domain_ops { >>> int (*attach_dev)(struct iommu_domain *domain, struct device *dev); >>> int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev, >>> - ioasid_t pasid); >>> + ioasid_t pasid, struct iommu_domain *old); >> Is it possible to add another op to replace a domain for pasid? For >> example, >> >> int (*replace_dev_pasid)(domain, dev, pasid, old_domain) > We haven't needed that in the normal case, what would motivate it > here? My understanding of the difference between set_dev_pasid and replace_dev_pasid is that the former assumes that there is no domain attached to the pasid yet, so it sets the passed domain to it. For the latter, it simply replaces the existing domain with a new one. The set_dev_pasid doesn't need an old domain because it's assumed that the existing domain is NULL. The replace_dev_pasid could have an existing domain as its input. Replace also implies an atomic switch between different domains. This makes it subtly different from a set operation. Best regards, baolu
On Tue, Apr 16, 2024 at 10:07:49AM +0800, Baolu Lu wrote: > On 4/15/24 7:54 PM, Jason Gunthorpe wrote: > > On Mon, Apr 15, 2024 at 01:32:03PM +0800, Baolu Lu wrote: > > > On 4/12/24 4:15 PM, Yi Liu wrote: > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > > index 40dd439307e8..1e5e9249c93f 100644 > > > > --- a/include/linux/iommu.h > > > > +++ b/include/linux/iommu.h > > > > @@ -631,7 +631,7 @@ struct iommu_ops { > > > > struct iommu_domain_ops { > > > > int (*attach_dev)(struct iommu_domain *domain, struct device *dev); > > > > int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev, > > > > - ioasid_t pasid); > > > > + ioasid_t pasid, struct iommu_domain *old); > > > Is it possible to add another op to replace a domain for pasid? For > > > example, > > > > > > int (*replace_dev_pasid)(domain, dev, pasid, old_domain) > > We haven't needed that in the normal case, what would motivate it > > here? > > My understanding of the difference between set_dev_pasid and > replace_dev_pasid is that the former assumes that there is no domain > attached to the pasid yet, so it sets the passed domain to it. For the > latter, it simply replaces the existing domain with a new one. > > The set_dev_pasid doesn't need an old domain because it's assumed that > the existing domain is NULL. The replace_dev_pasid could have an > existing domain as its input. I would just pass in the NULL domain for set than make another op. iommu drivers should be exactly the same implementation for both > Replace also implies an atomic switch between different domains. This > makes it subtly different from a set operation. Well, not necessarily hitless, but at least old/new/blocked - not something corrupt. Jason
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 45c75a8a0ef5..df49aed3df5e 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4626,7 +4626,8 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, } static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t pasid) + struct device *dev, ioasid_t pasid, + struct iommu_domain *old) { struct device_domain_info *info = dev_iommu_priv_get(dev); struct dmar_domain *dmar_domain = to_dmar_domain(domain); diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c index c1bed89b1026..ac8733b61470 100644 --- a/drivers/iommu/intel/svm.c +++ b/drivers/iommu/intel/svm.c @@ -315,7 +315,8 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid, } static int intel_svm_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t pasid) + struct device *dev, ioasid_t pasid, + struct iommu_domain *old) { struct device_domain_info *info = dev_iommu_priv_get(dev); struct intel_iommu *iommu = info->iommu; diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 3183b0ed4cdb..701b02a118db 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3321,7 +3321,8 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain, int ret; for_each_group_device(group, device) { - ret = domain->ops->set_dev_pasid(domain, device->dev, pasid); + ret = domain->ops->set_dev_pasid(domain, device->dev, + pasid, NULL); if (ret) goto err_revert; } diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 40dd439307e8..1e5e9249c93f 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -631,7 +631,7 @@ struct iommu_ops { struct iommu_domain_ops { int (*attach_dev)(struct iommu_domain *domain, struct device *dev); int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev, - ioasid_t pasid); + ioasid_t pasid, struct iommu_domain *old); int (*map_pages)(struct iommu_domain *domain, unsigned long iova, phys_addr_t paddr, size_t pgsize, size_t pgcount,
To support domain replacement for pasid, the underlying iommu driver needs to know the old domain hence be able to clean up the existing attachment. It would be much convenient for iommu layer to pass down the old domain. Otherwise, iommu drivers would need to track domain for pasids by themselves, this would duplicate code among the iommu drivers. Or iommu drivers would rely group->pasid_array to get domain, which may not always the correct one. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/intel/iommu.c | 3 ++- drivers/iommu/intel/svm.c | 3 ++- drivers/iommu/iommu.c | 3 ++- include/linux/iommu.h | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-)