This splits the preparation works of the iommu and the Intel iommu driver out from the iommufd pasid attach/replace series. [1]
To support domain replacement, the definition of the set_dev_pasid op needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks should be extended as well to suit the new definition.
This series first prepares the Intel iommu set_dev_pasid op for the new definition, adds the missing set_dev_pasid support for nested domain, makes ARM SMMUv3 set_dev_pasid op to suit the new definition, and in the end enhances the definition of set_dev_pasid op. The AMD set_dev_pasid callback is extended to fail if the caller tries to do domain replacement to meet the new definition of set_dev_pasid op. AMD iommu driver would support it later per Vasant [2].
[1] https://lore.kernel.org/linux-iommu/20240412081516.31168-1-yi.l.liu@intel.co... [2] https://lore.kernel.org/linux-iommu/fa9c4fc3-9365-465e-8926-b4d2d6361b9c@amd...
v2: - Make ARM SMMUv3 set_dev_pasid op support domain replacement (Jason) - Drop patch 03 of v1 (Kevin) - Multiple tweaks in VT-d driver (Kevin)
v1: https://lore.kernel.org/linux-iommu/20240628085538.47049-1-yi.l.liu@intel.co...
Regards, Yi Liu
Jason Gunthorpe (1): iommu/arm-smmu-v3: Make smmuv3 set_dev_pasid() op support replace
Lu Baolu (1): iommu/vt-d: Add set_dev_pasid callback for nested domain
Yi Liu (4): iommu: Pass old domain to set_dev_pasid op iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry() iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement iommu: Make set_dev_pasid op support domain replacement
drivers/iommu/amd/amd_iommu.h | 3 +- drivers/iommu/amd/pasid.c | 6 +- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 5 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +- drivers/iommu/intel/iommu.c | 122 ++++++++++++------ drivers/iommu/intel/iommu.h | 3 + drivers/iommu/intel/nested.c | 1 + drivers/iommu/intel/pasid.c | 13 +- drivers/iommu/intel/pasid.h | 8 +- drivers/iommu/intel/svm.c | 6 +- drivers/iommu/iommu.c | 3 +- include/linux/iommu.h | 5 +- 13 files changed, 129 insertions(+), 56 deletions(-)
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/amd/amd_iommu.h | 3 ++- drivers/iommu/amd/pasid.c | 3 ++- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 ++- drivers/iommu/intel/iommu.c | 6 ++++-- drivers/iommu/intel/svm.c | 3 ++- drivers/iommu/iommu.c | 3 ++- include/linux/iommu.h | 2 +- 7 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h index 2d5945c982bd..271406a45e33 100644 --- a/drivers/iommu/amd/amd_iommu.h +++ b/drivers/iommu/amd/amd_iommu.h @@ -51,7 +51,8 @@ struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev, struct mm_struct *mm); void amd_iommu_domain_free(struct iommu_domain *dom); int iommu_sva_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t pasid); + struct device *dev, ioasid_t pasid, + struct iommu_domain *old); void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, struct iommu_domain *domain);
diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c index a68215f2b3e1..77bf5f5f947a 100644 --- a/drivers/iommu/amd/pasid.c +++ b/drivers/iommu/amd/pasid.c @@ -100,7 +100,8 @@ static const struct mmu_notifier_ops sva_mn = { };
int iommu_sva_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t pasid) + struct device *dev, ioasid_t pasid, + struct iommu_domain *old) { struct pdom_dev_data *pdom_dev_data; struct protection_domain *sva_pdom = to_pdomain(domain); diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index a7c36654dee5..645da7b69bed 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -332,7 +332,8 @@ void arm_smmu_sva_notifier_synchronize(void) }
static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t id) + struct device *dev, ioasid_t id, + struct iommu_domain *old) { struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_master *master = dev_iommu_priv_get(dev); diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 34006b6e89eb..1a2a5cf4ef60 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4283,7 +4283,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); @@ -4569,7 +4570,8 @@ static int identity_domain_attach_dev(struct iommu_domain *domain, struct device }
static int identity_domain_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/intel/svm.c b/drivers/iommu/intel/svm.c index 0e3a9b38bef2..5ae1df7598b7 100644 --- a/drivers/iommu/intel/svm.c +++ b/drivers/iommu/intel/svm.c @@ -194,7 +194,8 @@ static const struct mmu_notifier_ops intel_mmuops = { };
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 dmar_domain *dmar_domain = to_dmar_domain(domain); diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 83c8e617a2c5..f3f81c04b8fb 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3331,7 +3331,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 bd722f473635..32dce80aa7fd 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -642,7 +642,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,
On Thu, Sep 12, 2024 at 06:04:22AM -0700, Yi Liu wrote:
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/amd/amd_iommu.h | 3 ++- drivers/iommu/amd/pasid.c | 3 ++- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 ++- drivers/iommu/intel/iommu.c | 6 ++++-- drivers/iommu/intel/svm.c | 3 ++- drivers/iommu/iommu.c | 3 ++- include/linux/iommu.h | 2 +- 7 files changed, 15 insertions(+), 8 deletions(-)
Reviewed-by: Jason Gunthorpe jgg@nvidia.com
Jason
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:04 PM
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
Reviewed-by: Kevin Tian kevin.tian@intel.com
Draining PRQ is mostly conjuncted with pasid teardown, and with more callers coming, move it into it in the intel_pasid_tear_down_entry(). But there is scenario that only teardown pasid entry but no PRQ drain, so passing a flag to mark it.
Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/intel/iommu.c | 8 ++++---- drivers/iommu/intel/pasid.c | 13 +++++++++++-- drivers/iommu/intel/pasid.h | 8 +++++--- drivers/iommu/intel/svm.c | 3 ++- 4 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 1a2a5cf4ef60..80b587de226d 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -3376,7 +3376,7 @@ void device_block_translation(struct device *dev) if (!dev_is_real_dma_subdevice(dev)) { if (sm_supported(iommu)) intel_pasid_tear_down_entry(iommu, dev, - IOMMU_NO_PASID, false); + IOMMU_NO_PASID, 0); else domain_context_clear(info); } @@ -4258,7 +4258,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, unsigned long flags;
if (domain->type == IOMMU_DOMAIN_IDENTITY) { - intel_pasid_tear_down_entry(iommu, dev, pasid, false); + intel_pasid_tear_down_entry(iommu, dev, pasid, 0); return; }
@@ -4278,8 +4278,8 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, domain_detach_iommu(dmar_domain, iommu); intel_iommu_debugfs_remove_dev_pasid(dev_pasid); kfree(dev_pasid); - intel_pasid_tear_down_entry(iommu, dev, pasid, false); - intel_drain_pasid_prq(dev, pasid); + intel_pasid_tear_down_entry(iommu, dev, pasid, + INTEL_PASID_TEARDOWN_DRAIN_PRQ); }
static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index b51fc268dc84..ceb9c5274a39 100644 --- a/drivers/iommu/intel/pasid.c +++ b/drivers/iommu/intel/pasid.c @@ -236,8 +236,13 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu, qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 - VTD_PAGE_SHIFT); }
+/* + * Not all PASID entry destroy requires PRQ drain as it can be handled in + * the remove_dev_pasid path. Caller should be clear about it and set the + * @flags properly. + */ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev, - u32 pasid, bool fault_ignore) + u32 pasid, u32 flags) { struct pasid_entry *pte; u16 did, pgtt; @@ -251,7 +256,8 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
did = pasid_get_domain_id(pte); pgtt = pasid_pte_get_pgtt(pte); - intel_pasid_clear_entry(dev, pasid, fault_ignore); + intel_pasid_clear_entry(dev, pasid, + flags & INTEL_PASID_TEARDOWN_IGNORE_FAULT); spin_unlock(&iommu->lock);
if (!ecap_coherent(iommu->ecap)) @@ -267,6 +273,9 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev, /* Device IOTLB doesn't need to be flushed in caching mode. */ if (!cap_caching_mode(iommu->cap)) devtlb_invalidation_with_pasid(iommu, dev, pasid); + + if (flags & INTEL_PASID_TEARDOWN_DRAIN_PRQ) + intel_drain_pasid_prq(dev, pasid); }
/* diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h index dde6d3ba5ae0..6eb849ec5fb8 100644 --- a/drivers/iommu/intel/pasid.h +++ b/drivers/iommu/intel/pasid.h @@ -303,9 +303,11 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu, struct device *dev, u32 pasid); int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev, u32 pasid, struct dmar_domain *domain); -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, - struct device *dev, u32 pasid, - bool fault_ignore); + +#define INTEL_PASID_TEARDOWN_IGNORE_FAULT (1U << 0) +#define INTEL_PASID_TEARDOWN_DRAIN_PRQ (1U << 1) +void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev, + u32 pasid, u32 flags); void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu, struct device *dev, u32 pasid); int intel_pasid_setup_sm_context(struct device *dev); diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c index 5ae1df7598b7..3c1e105b9da6 100644 --- a/drivers/iommu/intel/svm.c +++ b/drivers/iommu/intel/svm.c @@ -176,7 +176,8 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm) list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) { info = dev_iommu_priv_get(dev_pasid->dev); intel_pasid_tear_down_entry(info->iommu, dev_pasid->dev, - dev_pasid->pasid, true); + dev_pasid->pasid, + INTEL_PASID_TEARDOWN_IGNORE_FAULT); } spin_unlock_irqrestore(&domain->lock, flags);
On 2024/9/12 21:04, Yi Liu wrote:
Draining PRQ is mostly conjuncted with pasid teardown, and with more callers coming, move it into it in the intel_pasid_tear_down_entry(). But there is scenario that only teardown pasid entry but no PRQ drain, so passing a flag to mark it.
Is it a reasonable case where PRI needs to be drained but the pasid entry won't be torn down? For example, could this happen when a PRI is disabled?
Signed-off-by: Yi Liuyi.l.liu@intel.com
drivers/iommu/intel/iommu.c | 8 ++++---- drivers/iommu/intel/pasid.c | 13 +++++++++++-- drivers/iommu/intel/pasid.h | 8 +++++--- drivers/iommu/intel/svm.c | 3 ++- 4 files changed, 22 insertions(+), 10 deletions(-)
Thanks, baolu
On 2024/9/12 21:22, Baolu Lu wrote:
On 2024/9/12 21:04, Yi Liu wrote:
Draining PRQ is mostly conjuncted with pasid teardown, and with more callers coming, move it into it in the intel_pasid_tear_down_entry(). But there is scenario that only teardown pasid entry but no PRQ drain, so passing a flag to mark it.
Is it a reasonable case where PRI needs to be drained but the pasid entry won't be torn down? For example, could this happen when a PRI is disabled?
in concept, yes. But it seems no more than a debugging method in my opinion. I cannot map it to a usage so far.
Signed-off-by: Yi Liuyi.l.liu@intel.com
drivers/iommu/intel/iommu.c | 8 ++++---- drivers/iommu/intel/pasid.c | 13 +++++++++++-- drivers/iommu/intel/pasid.h | 8 +++++--- drivers/iommu/intel/svm.c | 3 ++- 4 files changed, 22 insertions(+), 10 deletions(-)
Thanks, baolu
On 9/12/24 9:04 PM, Yi Liu wrote:
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index b51fc268dc84..ceb9c5274a39 100644 --- a/drivers/iommu/intel/pasid.c +++ b/drivers/iommu/intel/pasid.c @@ -236,8 +236,13 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu, qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 - VTD_PAGE_SHIFT); } +/*
- Not all PASID entry destroy requires PRQ drain as it can be handled in
- the remove_dev_pasid path. Caller should be clear about it and set the
- @flags properly.
- */ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
u32 pasid, bool fault_ignore)
u32 pasid, u32 flags)
As a generic opt-in bit flags, why not using 'unsigned int'?
Thanks, baolu
On 2024/9/13 10:11, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index b51fc268dc84..ceb9c5274a39 100644 --- a/drivers/iommu/intel/pasid.c +++ b/drivers/iommu/intel/pasid.c @@ -236,8 +236,13 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu, qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64
- VTD_PAGE_SHIFT);
} +/*
- Not all PASID entry destroy requires PRQ drain as it can be handled in
- the remove_dev_pasid path. Caller should be clear about it and set the
- @flags properly.
- */
void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev, - u32 pasid, bool fault_ignore) + u32 pasid, u32 flags)
As a generic opt-in bit flags, why not using 'unsigned int'?
aha, I can use 'unsigned int'.
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:04 PM
+/*
- Not all PASID entry destroy requires PRQ drain as it can be handled in
- the remove_dev_pasid path. Caller should be clear about it and set the
- @flags properly.
- */
/* * Caller can request to drain PRQ in this helper if it hasn't done so, * e.g. in a path which doesn't follow remove_dev_pasid(). */
Reviewed-by: Kevin Tian kevin.tian@intel.com
set_dev_pasid op is going to support domain replacement and keep the old hardware config if it fails. Make the Intel iommu driver be prepared for it.
Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++------------- 1 file changed, 65 insertions(+), 33 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 80b587de226d..6f5a8e549f3f 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4248,8 +4248,8 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain, return 0; }
-static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, - struct iommu_domain *domain) +static void domain_remove_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) { struct device_domain_info *info = dev_iommu_priv_get(dev); struct dev_pasid_info *curr, *dev_pasid = NULL; @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, struct dmar_domain *dmar_domain; unsigned long flags;
- if (domain->type == IOMMU_DOMAIN_IDENTITY) { - intel_pasid_tear_down_entry(iommu, dev, pasid, 0); - return; - } - dmar_domain = to_dmar_domain(domain); spin_lock_irqsave(&dmar_domain->lock, flags); list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) { @@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, domain_detach_iommu(dmar_domain, iommu); intel_iommu_debugfs_remove_dev_pasid(dev_pasid); kfree(dev_pasid); +} + +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct intel_iommu *iommu = info->iommu; + intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (domain->type == IOMMU_DOMAIN_IDENTITY) + return; + domain_remove_dev_pasid(domain, dev, pasid); }
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t pasid, - struct iommu_domain *old) +static struct dev_pasid_info * +domain_prepare_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) { struct device_domain_info *info = dev_iommu_priv_get(dev); struct dmar_domain *dmar_domain = to_dmar_domain(domain); @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, unsigned long flags; int ret;
- if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) - return -EOPNOTSUPP; - - if (domain->dirty_ops) - return -EINVAL; - - if (context_copied(iommu, info->bus, info->devfn)) - return -EBUSY; - ret = prepare_domain_attach_device(domain, dev); if (ret) - return ret; + return ERR_PTR(ret);
dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); if (!dev_pasid) - return -ENOMEM; + return ERR_PTR(-ENOMEM);
ret = domain_attach_iommu(dmar_domain, iommu); if (ret) @@ -4318,6 +4315,47 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, if (ret) goto out_detach_iommu;
+ dev_pasid->dev = dev; + dev_pasid->pasid = pasid; + spin_lock_irqsave(&dmar_domain->lock, flags); + list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); + spin_unlock_irqrestore(&dmar_domain->lock, flags); + + return dev_pasid; +out_detach_iommu: + domain_detach_iommu(dmar_domain, iommu); +out_free: + kfree(dev_pasid); + return ERR_PTR(ret); +} + +static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, + 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); + struct intel_iommu *iommu = info->iommu; + struct dev_pasid_info *dev_pasid; + int ret; + + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) + return -EOPNOTSUPP; + + if (domain->dirty_ops) + return -EINVAL; + + if (context_copied(iommu, info->bus, info->devfn)) + return -EBUSY; + + dev_pasid = domain_prepare_dev_pasid(domain, dev, pasid); + if (IS_ERR(dev_pasid)) + return PTR_ERR(dev_pasid); + + /* Clear the old configuration if it already exists */ + intel_pasid_tear_down_entry(iommu, dev, pasid, + INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (dmar_domain->use_first_level) ret = domain_setup_first_level(iommu, dmar_domain, dev, pasid); @@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, ret = intel_pasid_setup_second_level(iommu, dmar_domain, dev, pasid); if (ret) - goto out_unassign_tag; + goto out_undo_dev_pasid;
- dev_pasid->dev = dev; - dev_pasid->pasid = pasid; - spin_lock_irqsave(&dmar_domain->lock, flags); - list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); - spin_unlock_irqrestore(&dmar_domain->lock, flags); + if (old) + domain_remove_dev_pasid(old, dev, pasid);
if (domain->type & __IOMMU_DOMAIN_PAGING) intel_iommu_debugfs_create_dev_pasid(dev_pasid);
return 0; -out_unassign_tag: - cache_tag_unassign_domain(dmar_domain, dev, pasid); -out_detach_iommu: - domain_detach_iommu(dmar_domain, iommu); -out_free: - kfree(dev_pasid); + +out_undo_dev_pasid: + domain_remove_dev_pasid(domain, dev, pasid); return ret; }
On 9/12/24 9:04 PM, Yi Liu wrote:
set_dev_pasid op is going to support domain replacement and keep the old hardware config if it fails. Make the Intel iommu driver be prepared for it.
Signed-off-by: Yi Liu yi.l.liu@intel.com
drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++------------- 1 file changed, 65 insertions(+), 33 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 80b587de226d..6f5a8e549f3f 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4248,8 +4248,8 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain, return 0; } -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
struct iommu_domain *domain)+static void domain_remove_dev_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t pasid)@@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, struct dmar_domain *dmar_domain; unsigned long flags;
- if (domain->type == IOMMU_DOMAIN_IDENTITY) {
intel_pasid_tear_down_entry(iommu, dev, pasid, 0);return;- }
- dmar_domain = to_dmar_domain(domain); spin_lock_irqsave(&dmar_domain->lock, flags); list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
@@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, domain_detach_iommu(dmar_domain, iommu); intel_iommu_debugfs_remove_dev_pasid(dev_pasid); kfree(dev_pasid); +}
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
struct iommu_domain *domain)+{
- struct device_domain_info *info = dev_iommu_priv_get(dev);
- struct intel_iommu *iommu = info->iommu;
- intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
- if (domain->type == IOMMU_DOMAIN_IDENTITY)
return;
The static identity domain is not capable of handling page requests. Therefore there is no need to drain PRQ for an identity domain removal.
So it probably should be something like this:
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, 0); return; }
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
- domain_remove_dev_pasid(domain, dev, pasid); }
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t pasid,struct iommu_domain *old)+static struct dev_pasid_info * +domain_prepare_dev_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t pasid)
Why do you want to return a struct pointer instead of an integer? The returned pointer is not used after it is returned.
Also, how about renaming this helper to domain_add_dev_pasid() to pair it with domain_remove_dev_pasid()?
{ struct device_domain_info *info = dev_iommu_priv_get(dev); struct dmar_domain *dmar_domain = to_dmar_domain(domain); @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, unsigned long flags; int ret;
- if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
return -EOPNOTSUPP;- if (domain->dirty_ops)
return -EINVAL;- if (context_copied(iommu, info->bus, info->devfn))
return -EBUSY;- ret = prepare_domain_attach_device(domain, dev); if (ret)
return ret;
return ERR_PTR(ret);dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); if (!dev_pasid)
return -ENOMEM;
return ERR_PTR(-ENOMEM);ret = domain_attach_iommu(dmar_domain, iommu); if (ret)
Thanks, baolu
On 9/13/24 9:35 AM, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
set_dev_pasid op is going to support domain replacement and keep the old hardware config if it fails. Make the Intel iommu driver be prepared for it.
Signed-off-by: Yi Liu yi.l.liu@intel.com
drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++------------- 1 file changed, 65 insertions(+), 33 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 80b587de226d..6f5a8e549f3f 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4248,8 +4248,8 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain, return 0; } -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, - struct iommu_domain *domain) +static void domain_remove_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) { struct device_domain_info *info = dev_iommu_priv_get(dev); struct dev_pasid_info *curr, *dev_pasid = NULL; @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, struct dmar_domain *dmar_domain; unsigned long flags; - if (domain->type == IOMMU_DOMAIN_IDENTITY) { - intel_pasid_tear_down_entry(iommu, dev, pasid, 0); - return; - }
dmar_domain = to_dmar_domain(domain); spin_lock_irqsave(&dmar_domain->lock, flags); list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) { @@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, domain_detach_iommu(dmar_domain, iommu); intel_iommu_debugfs_remove_dev_pasid(dev_pasid); kfree(dev_pasid); +}
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct intel_iommu *iommu = info->iommu;
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (domain->type == IOMMU_DOMAIN_IDENTITY) + return;
The static identity domain is not capable of handling page requests. Therefore there is no need to drain PRQ for an identity domain removal.
So it probably should be something like this:
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, 0); return; }
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
Just revisited this. It seems that we just need to drain PRQ if the attached domain is iopf-capable. Therefore, how about making it like this?
unsigned int flags = 0;
if (domain->iopf_handler) flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
/* Identity domain has no meta data for pasid. */ if (domain->type == IOMMU_DOMAIN_IDENTITY) return;
Thanks, baolu
On 2024/9/13 10:17, Baolu Lu wrote:
On 9/13/24 9:35 AM, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
set_dev_pasid op is going to support domain replacement and keep the old hardware config if it fails. Make the Intel iommu driver be prepared for it.
Signed-off-by: Yi Liu yi.l.liu@intel.com
drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++------------- 1 file changed, 65 insertions(+), 33 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 80b587de226d..6f5a8e549f3f 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4248,8 +4248,8 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain, return 0; } -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, - struct iommu_domain *domain) +static void domain_remove_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) { struct device_domain_info *info = dev_iommu_priv_get(dev); struct dev_pasid_info *curr, *dev_pasid = NULL; @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, struct dmar_domain *dmar_domain; unsigned long flags; - if (domain->type == IOMMU_DOMAIN_IDENTITY) { - intel_pasid_tear_down_entry(iommu, dev, pasid, 0); - return; - }
dmar_domain = to_dmar_domain(domain); spin_lock_irqsave(&dmar_domain->lock, flags); list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) { @@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, domain_detach_iommu(dmar_domain, iommu); intel_iommu_debugfs_remove_dev_pasid(dev_pasid); kfree(dev_pasid); +}
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct intel_iommu *iommu = info->iommu;
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (domain->type == IOMMU_DOMAIN_IDENTITY) + return;
The static identity domain is not capable of handling page requests. Therefore there is no need to drain PRQ for an identity domain removal.
So it probably should be something like this:
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, 0); return; }
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
Just revisited this. It seems that we just need to drain PRQ if the attached domain is iopf-capable. Therefore, how about making it like this?
unsigned int flags = 0;
if (domain->iopf_handler) flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
/* Identity domain has no meta data for pasid. */ if (domain->type == IOMMU_DOMAIN_IDENTITY) return;
got it.
From: Baolu Lu baolu.lu@linux.intel.com Sent: Friday, September 13, 2024 10:17 AM
On 9/13/24 9:35 AM, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct intel_iommu *iommu = info->iommu;
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (domain->type == IOMMU_DOMAIN_IDENTITY) + return;
The static identity domain is not capable of handling page requests. Therefore there is no need to drain PRQ for an identity domain removal.
So it probably should be something like this:
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, 0); return; }
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
Just revisited this. It seems that we just need to drain PRQ if the attached domain is iopf-capable. Therefore, how about making it like this?
unsigned int flags = 0;
if (domain->iopf_handler) flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
/* Identity domain has no meta data for pasid. */ if (domain->type == IOMMU_DOMAIN_IDENTITY) return;
this is the right thing to do, but also suggesting a bug in existing code. intel_pasid_tear_down_entry() is not just for PRQ drain. It's also about iotlb/devtlb invalidation. From device p.o.v it has no idea about the translation mode in the IOMMU side and always caches the valid mappings in devtlb when ATS is enabled.
Existing code skips all those housekeeping for identify domain by early return before intel_pasid_tear_down_entry(). We need a separate fix for it before this series?
On 2024/9/30 15:19, Tian, Kevin wrote:
From: Baolu Lu baolu.lu@linux.intel.com Sent: Friday, September 13, 2024 10:17 AM
On 9/13/24 9:35 AM, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct intel_iommu *iommu = info->iommu;
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (domain->type == IOMMU_DOMAIN_IDENTITY) + return;
The static identity domain is not capable of handling page requests. Therefore there is no need to drain PRQ for an identity domain removal.
So it probably should be something like this:
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, 0); return; }
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
Just revisited this. It seems that we just need to drain PRQ if the attached domain is iopf-capable. Therefore, how about making it like this?
unsigned int flags = 0;
if (domain->iopf_handler) flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
/* Identity domain has no meta data for pasid. */ if (domain->type == IOMMU_DOMAIN_IDENTITY) return;
this is the right thing to do, but also suggesting a bug in existing code. intel_pasid_tear_down_entry() is not just for PRQ drain. It's also about iotlb/devtlb invalidation. From device p.o.v it has no idea about the translation mode in the IOMMU side and always caches the valid mappings in devtlb when ATS is enabled.
Yes. You are right.
intel_pasid_tear_down_entry() takes care of iotlb/devtlb invalidation. So it's fine as long as intel_pasid_tear_down_entry() is called for the IDENTITY domain path, right?
Existing code skips all those housekeeping for identify domain by early return before intel_pasid_tear_down_entry(). We need a separate fix for it before this series?
Existing code doesn't skip intel_pasid_tear_down_entry().
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, false); return; }
Or anything I overlooked?
Thanks, baolu
From: Baolu Lu baolu.lu@linux.intel.com Sent: Wednesday, October 9, 2024 9:09 AM
On 2024/9/30 15:19, Tian, Kevin wrote:
From: Baolu Lu baolu.lu@linux.intel.com Sent: Friday, September 13, 2024 10:17 AM
On 9/13/24 9:35 AM, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
+static void intel_iommu_remove_dev_pasid(struct device *dev,
ioasid_t
pasid, + struct iommu_domain *domain) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct intel_iommu *iommu = info->iommu;
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (domain->type == IOMMU_DOMAIN_IDENTITY) + return;
The static identity domain is not capable of handling page requests. Therefore there is no need to drain PRQ for an identity domain removal.
So it probably should be something like this:
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, 0); return; }
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
Just revisited this. It seems that we just need to drain PRQ if the attached domain is iopf-capable. Therefore, how about making it like this?
unsigned int flags = 0;
if (domain->iopf_handler) flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
/* Identity domain has no meta data for pasid. */ if (domain->type == IOMMU_DOMAIN_IDENTITY) return;
this is the right thing to do, but also suggesting a bug in existing code. intel_pasid_tear_down_entry() is not just for PRQ drain. It's also about iotlb/devtlb invalidation. From device p.o.v it has no idea about the translation mode in the IOMMU side and always caches the valid mappings in devtlb when ATS is enabled.
Yes. You are right.
intel_pasid_tear_down_entry() takes care of iotlb/devtlb invalidation. So it's fine as long as intel_pasid_tear_down_entry() is called for the IDENTITY domain path, right?
Existing code skips all those housekeeping for identify domain by early return before intel_pasid_tear_down_entry(). We need a separate fix for it before this series?
Existing code doesn't skip intel_pasid_tear_down_entry().
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, false); return; }Or anything I overlooked?
No. Looks I was confused by this patch and misunderstood the existing code.
On 2024/9/13 09:35, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
set_dev_pasid op is going to support domain replacement and keep the old hardware config if it fails. Make the Intel iommu driver be prepared for it.
Signed-off-by: Yi Liu yi.l.liu@intel.com
drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++------------- 1 file changed, 65 insertions(+), 33 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 80b587de226d..6f5a8e549f3f 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4248,8 +4248,8 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain, return 0; } -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, - struct iommu_domain *domain) +static void domain_remove_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) { struct device_domain_info *info = dev_iommu_priv_get(dev); struct dev_pasid_info *curr, *dev_pasid = NULL; @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, struct dmar_domain *dmar_domain; unsigned long flags; - if (domain->type == IOMMU_DOMAIN_IDENTITY) { - intel_pasid_tear_down_entry(iommu, dev, pasid, 0); - return; - }
dmar_domain = to_dmar_domain(domain); spin_lock_irqsave(&dmar_domain->lock, flags); list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) { @@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, domain_detach_iommu(dmar_domain, iommu); intel_iommu_debugfs_remove_dev_pasid(dev_pasid); kfree(dev_pasid); +}
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct intel_iommu *iommu = info->iommu;
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (domain->type == IOMMU_DOMAIN_IDENTITY) + return;
The static identity domain is not capable of handling page requests. Therefore there is no need to drain PRQ for an identity domain removal.
oh, yes. so maybe for SI domain, there is no PRQ at all.
So it probably should be something like this:
if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, 0); return; }
intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
+ domain_remove_dev_pasid(domain, dev, pasid); } -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t pasid, - struct iommu_domain *old) +static struct dev_pasid_info * +domain_prepare_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid)
Why do you want to return a struct pointer instead of an integer? The returned pointer is not used after it is returned.
it's for the intel_iommu_debugfs_create_dev_pasid(). it needs the dev_pasid.
Also, how about renaming this helper to domain_add_dev_pasid() to pair it with domain_remove_dev_pasid()?
sure.
{ struct device_domain_info *info = dev_iommu_priv_get(dev); struct dmar_domain *dmar_domain = to_dmar_domain(domain); @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, unsigned long flags; int ret; - if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) - return -EOPNOTSUPP;
- if (domain->dirty_ops) - return -EINVAL;
- if (context_copied(iommu, info->bus, info->devfn)) - return -EBUSY;
ret = prepare_domain_attach_device(domain, dev); if (ret) - return ret; + return ERR_PTR(ret); dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); if (!dev_pasid) - return -ENOMEM; + return ERR_PTR(-ENOMEM); ret = domain_attach_iommu(dmar_domain, iommu); if (ret)
Thanks, baolu
On 9/12/24 9:04 PM, Yi Liu wrote:
@@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, ret = intel_pasid_setup_second_level(iommu, dmar_domain, dev, pasid); if (ret)
goto out_unassign_tag;
goto out_undo_dev_pasid;
- dev_pasid->dev = dev;
- dev_pasid->pasid = pasid;
- spin_lock_irqsave(&dmar_domain->lock, flags);
- list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
- spin_unlock_irqrestore(&dmar_domain->lock, flags);
- if (old)
domain_remove_dev_pasid(old, dev, pasid);if (domain->type & __IOMMU_DOMAIN_PAGING) intel_iommu_debugfs_create_dev_pasid(dev_pasid); return 0; -out_unassign_tag:
- cache_tag_unassign_domain(dmar_domain, dev, pasid);
-out_detach_iommu:
- domain_detach_iommu(dmar_domain, iommu);
-out_free:
- kfree(dev_pasid);
+out_undo_dev_pasid:
- domain_remove_dev_pasid(domain, dev, pasid); return ret; }
Do you need to re-install the old domain to the pasid entry in the failure path?
Thanks, baolu
On 2024/9/13 09:42, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
@@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, ret = intel_pasid_setup_second_level(iommu, dmar_domain, dev, pasid); if (ret) - goto out_unassign_tag; + goto out_undo_dev_pasid; - dev_pasid->dev = dev; - dev_pasid->pasid = pasid; - spin_lock_irqsave(&dmar_domain->lock, flags); - list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); - spin_unlock_irqrestore(&dmar_domain->lock, flags); + if (old) + domain_remove_dev_pasid(old, dev, pasid); if (domain->type & __IOMMU_DOMAIN_PAGING) intel_iommu_debugfs_create_dev_pasid(dev_pasid); return 0; -out_unassign_tag: - cache_tag_unassign_domain(dmar_domain, dev, pasid); -out_detach_iommu: - domain_detach_iommu(dmar_domain, iommu); -out_free: - kfree(dev_pasid);
+out_undo_dev_pasid: + domain_remove_dev_pasid(domain, dev, pasid); return ret; }
Do you need to re-install the old domain to the pasid entry in the failure path?
yes, but no. The old domain is still installed in the pasid entry when the failure happened. :)
On 9/13/24 8:21 PM, Yi Liu wrote:
On 2024/9/13 09:42, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
@@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, ret = intel_pasid_setup_second_level(iommu, dmar_domain, dev, pasid); if (ret) - goto out_unassign_tag; + goto out_undo_dev_pasid; - dev_pasid->dev = dev; - dev_pasid->pasid = pasid; - spin_lock_irqsave(&dmar_domain->lock, flags); - list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); - spin_unlock_irqrestore(&dmar_domain->lock, flags); + if (old) + domain_remove_dev_pasid(old, dev, pasid); if (domain->type & __IOMMU_DOMAIN_PAGING) intel_iommu_debugfs_create_dev_pasid(dev_pasid); return 0; -out_unassign_tag: - cache_tag_unassign_domain(dmar_domain, dev, pasid); -out_detach_iommu: - domain_detach_iommu(dmar_domain, iommu); -out_free: - kfree(dev_pasid);
+out_undo_dev_pasid: + domain_remove_dev_pasid(domain, dev, pasid); return ret; }
Do you need to re-install the old domain to the pasid entry in the failure path?
yes, but no. The old domain is still installed in the pasid entry when the failure happened. :)
I am afraid not. The old domain has already been cleaned up by intel_pasid_tear_down_entry(). Or not?
Thanks, baolu
On Sep 14, 2024, at 09:08, Baolu Lu baolu.lu@linux.intel.com wrote:
On 9/13/24 8:21 PM, Yi Liu wrote:
On 2024/9/13 09:42, Baolu Lu wrote: On 9/12/24 9:04 PM, Yi Liu wrote:
@@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, ret = intel_pasid_setup_second_level(iommu, dmar_domain, dev, pasid); if (ret)
goto out_unassign_tag;
goto out_undo_dev_pasid;
- dev_pasid->dev = dev;
- dev_pasid->pasid = pasid;
- spin_lock_irqsave(&dmar_domain->lock, flags);
- list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
- spin_unlock_irqrestore(&dmar_domain->lock, flags);
- if (old)
domain_remove_dev_pasid(old, dev, pasid); if (domain->type & __IOMMU_DOMAIN_PAGING) intel_iommu_debugfs_create_dev_pasid(dev_pasid); return 0;-out_unassign_tag:
- cache_tag_unassign_domain(dmar_domain, dev, pasid);
-out_detach_iommu:
- domain_detach_iommu(dmar_domain, iommu);
-out_free:
- kfree(dev_pasid);
+out_undo_dev_pasid:
- domain_remove_dev_pasid(domain, dev, pasid); return ret; }
Do you need to re-install the old domain to the pasid entry in the failure path?
yes, but no. The old domain is still installed in the pasid entry when the failure happened. :)
I am afraid not. The old domain has already been cleaned up by intel_pasid_tear_down_entry(). Or not?
oops, yes. The tear down was lifted to this function now instead of in the setup helpers. I may move them back to the setup helpers just like v1 does.
Good catch!
Regards, Yi Liu
From: Lu Baolu baolu.lu@linux.intel.com
Extend intel_iommu_set_dev_pasid() to set a nested type domain to a PASID of a device.
Signed-off-by: Lu Baolu baolu.lu@linux.intel.com Co-developed-by: Yi Liu yi.l.liu@intel.com Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/intel/iommu.c | 22 +++++++++++++++++----- drivers/iommu/intel/iommu.h | 3 +++ drivers/iommu/intel/nested.c | 1 + 3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 6f5a8e549f3f..749ee7741ec4 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -284,6 +284,11 @@ static int __init intel_iommu_setup(char *str) } __setup("intel_iommu=", intel_iommu_setup);
+static int domain_type_is_nested(struct dmar_domain *domain) +{ + return domain->domain.type == IOMMU_DOMAIN_NESTED; +} + static int domain_pfn_supported(struct dmar_domain *domain, unsigned long pfn) { int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT; @@ -4299,7 +4304,12 @@ domain_prepare_dev_pasid(struct iommu_domain *domain, unsigned long flags; int ret;
- ret = prepare_domain_attach_device(domain, dev); + /* Nested type domain should prepare its parent domain */ + if (domain_type_is_nested(dmar_domain)) + ret = prepare_domain_attach_device( + &dmar_domain->s2_domain->domain, dev); + else + ret = prepare_domain_attach_device(domain, dev); if (ret) return ERR_PTR(ret);
@@ -4329,9 +4339,9 @@ domain_prepare_dev_pasid(struct iommu_domain *domain, return ERR_PTR(ret); }
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t pasid, - struct iommu_domain *old) +int intel_iommu_set_dev_pasid(struct iommu_domain *domain, + 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); @@ -4356,7 +4366,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, intel_pasid_tear_down_entry(iommu, dev, pasid, INTEL_PASID_TEARDOWN_DRAIN_PRQ);
- if (dmar_domain->use_first_level) + if (domain_type_is_nested(dmar_domain)) + ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain); + else if (dmar_domain->use_first_level) ret = domain_setup_first_level(iommu, dmar_domain, dev, pasid); else diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h index b3b295e60626..36cf59bfe4ea 100644 --- a/drivers/iommu/intel/iommu.h +++ b/drivers/iommu/intel/iommu.h @@ -1109,6 +1109,9 @@ void device_block_translation(struct device *dev); int prepare_domain_attach_device(struct iommu_domain *domain, struct device *dev); void domain_update_iommu_cap(struct dmar_domain *domain); +int intel_iommu_set_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid, + struct iommu_domain *old);
int dmar_ir_support(void);
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c index 36a91b1b52be..c5a94c00b396 100644 --- a/drivers/iommu/intel/nested.c +++ b/drivers/iommu/intel/nested.c @@ -131,6 +131,7 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
static const struct iommu_domain_ops intel_nested_domain_ops = { .attach_dev = intel_nested_attach_dev, + .set_dev_pasid = intel_iommu_set_dev_pasid, .free = intel_nested_domain_free, .cache_invalidate_user = intel_nested_cache_invalidate_user, };
On 9/12/24 9:04 PM, Yi Liu wrote:
@@ -4299,7 +4304,12 @@ domain_prepare_dev_pasid(struct iommu_domain *domain, unsigned long flags; int ret;
- ret = prepare_domain_attach_device(domain, dev);
- /* Nested type domain should prepare its parent domain */
- if (domain_type_is_nested(dmar_domain))
ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev);- else
ret = prepare_domain_attach_device(domain, dev);
'prepare' is a bit confusing in this context. It actually means checking whether a domain is compatible with the hardware capabilities of RID or PASID. Or not?
I know this confusion comes from the naming of prepare_domain_attach_device(). Hence, do you mind renaming this helper in a small patch?
Thanks, baolu
On 2024/9/13 09:52, Baolu Lu wrote:
On 9/12/24 9:04 PM, Yi Liu wrote:
@@ -4299,7 +4304,12 @@ domain_prepare_dev_pasid(struct iommu_domain *domain, unsigned long flags; int ret; - ret = prepare_domain_attach_device(domain, dev); + /* Nested type domain should prepare its parent domain */ + if (domain_type_is_nested(dmar_domain)) + ret = prepare_domain_attach_device( + &dmar_domain->s2_domain->domain, dev); + else + ret = prepare_domain_attach_device(domain, dev); if (ret) return ERR_PTR(ret);
'prepare' is a bit confusing in this context. It actually means checking whether a domain is compatible with the hardware capabilities of RID or PASID. Or not?
I know this confusion comes from the naming of prepare_domain_attach_device(). Hence, do you mind renaming this helper in a small patch?
good suggestion.
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:04 PM
From: Lu Baolu baolu.lu@linux.intel.com
Extend intel_iommu_set_dev_pasid() to set a nested type domain to a PASID of a device.
Signed-off-by: Lu Baolu baolu.lu@linux.intel.com Co-developed-by: Yi Liu yi.l.liu@intel.com Signed-off-by: Yi Liu yi.l.liu@intel.com
Reviewed-by: Kevin Tian kevin.tian@intel.com
From: Jason Gunthorpe jgg@nvidia.com
set_dev_pasid() op is going to be enhanced to support domain replacement of a pasid. This prepares for this op definition.
Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index 645da7b69bed..1d3e71569775 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -349,7 +349,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, * get reassigned */ arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->cd.asid); - ret = arm_smmu_set_pasid(master, smmu_domain, id, &target); + ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old);
mmput(domain->mm); return ret; diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index ed2b106e02dd..f7a73b854151 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2824,7 +2824,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) }
static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t id) + struct device *dev, ioasid_t id, + struct iommu_domain *old) { struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_master *master = dev_iommu_priv_get(dev); @@ -2850,7 +2851,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain, */ arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); return arm_smmu_set_pasid(master, to_smmu_domain(domain), id, - &target_cd); + &target_cd, old); }
static void arm_smmu_update_ste(struct arm_smmu_master *master, @@ -2880,7 +2881,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master,
int arm_smmu_set_pasid(struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, ioasid_t pasid, - struct arm_smmu_cd *cd) + struct arm_smmu_cd *cd, struct iommu_domain *old) { struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev); struct arm_smmu_attach_state state = { @@ -2890,6 +2891,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, * already attached, no need to set old_domain. */ .ssid = pasid, + .old_domain = old, }; struct arm_smmu_cd *cdptr; int ret; diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 14bca41a981b..a942f5d40051 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -806,7 +806,7 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
int arm_smmu_set_pasid(struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, ioasid_t pasid, - struct arm_smmu_cd *cd); + struct arm_smmu_cd *cd, struct iommu_domain *old);
void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid); void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:04 PM
From: Jason Gunthorpe jgg@nvidia.com
set_dev_pasid() op is going to be enhanced to support domain replacement of a pasid. This prepares for this op definition.
Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com
Reviewed-by: Kevin Tian kevin.tian@intel.com
On Thu, Sep 12, 2024 at 06:04:26AM -0700, Yi Liu wrote:
From: Jason Gunthorpe jgg@nvidia.com
set_dev_pasid() op is going to be enhanced to support domain replacement of a pasid. This prepares for this op definition.
Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index 645da7b69bed..1d3e71569775 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -349,7 +349,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, * get reassigned */ arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->cd.asid);
- ret = arm_smmu_set_pasid(master, smmu_domain, id, &target);
- ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old);
mmput(domain->mm); return ret; diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index ed2b106e02dd..f7a73b854151 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2824,7 +2824,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t id)
struct device *dev, ioasid_t id,struct iommu_domain *old){ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_master *master = dev_iommu_priv_get(dev); @@ -2850,7 +2851,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain, */ arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); return arm_smmu_set_pasid(master, to_smmu_domain(domain), id,
&target_cd);
&target_cd, old);} static void arm_smmu_update_ste(struct arm_smmu_master *master, @@ -2880,7 +2881,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master, int arm_smmu_set_pasid(struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
struct arm_smmu_cd *cd)
struct arm_smmu_cd *cd, struct iommu_domain *old){ struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev); struct arm_smmu_attach_state state = { @@ -2890,6 +2891,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, * already attached, no need to set old_domain. */ .ssid = pasid,
.old_domain = old,
nit: The existing comment says "no need to set old_domain" and now you're doing exactly that! Please can you update the commentary (probably just remove the comment entirely)?
Cheers,
Will
On 2024/10/15 16:43, Will Deacon wrote:
On Thu, Sep 12, 2024 at 06:04:26AM -0700, Yi Liu wrote:
From: Jason Gunthorpe jgg@nvidia.com
set_dev_pasid() op is going to be enhanced to support domain replacement of a pasid. This prepares for this op definition.
Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index 645da7b69bed..1d3e71569775 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -349,7 +349,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, * get reassigned */ arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->cd.asid);
- ret = arm_smmu_set_pasid(master, smmu_domain, id, &target);
- ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old);
mmput(domain->mm); return ret; diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index ed2b106e02dd..f7a73b854151 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2824,7 +2824,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t id)
struct device *dev, ioasid_t id,struct iommu_domain *old)@@ -2850,7 +2851,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain, */ arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); return arm_smmu_set_pasid(master, to_smmu_domain(domain), id,
&target_cd);
&target_cd, old);static void arm_smmu_update_ste(struct arm_smmu_master *master, @@ -2880,7 +2881,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master, int arm_smmu_set_pasid(struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
struct arm_smmu_cd *cd)
struct arm_smmu_cd *cd, struct iommu_domain *old)@@ -2890,6 +2891,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, * already attached, no need to set old_domain. */ .ssid = pasid,
.old_domain = old,nit: The existing comment says "no need to set old_domain" and now you're doing exactly that! Please can you update the commentary (probably just remove the comment entirely)?
oops, indeed, should remove this comment entirely. thanks for spotting it.
On Tue, Oct 15, 2024 at 09:43:24AM +0100, Will Deacon wrote:
@@ -2890,6 +2891,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, * already attached, no need to set old_domain. */ .ssid = pasid,
.old_domain = old,nit: The existing comment says "no need to set old_domain" and now you're doing exactly that! Please can you update the commentary (probably just remove the comment entirely)?
+1
Jason
The iommu core is going to support domain replacement for pasid, it needs to make the set_dev_pasid op support replacing domain and keep the old domain config in the failure case.
AMD iommu driver does not support domain replacement for pasid yet, so it would fail the set_dev_pasid op to keep the old config if the input @old is non-NULL.
Suggested-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com --- drivers/iommu/amd/pasid.c | 3 +++ include/linux/iommu.h | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c index 77bf5f5f947a..30e27bda3fac 100644 --- a/drivers/iommu/amd/pasid.c +++ b/drivers/iommu/amd/pasid.c @@ -109,6 +109,9 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain, unsigned long flags; int ret = -EINVAL;
+ if (old) + return -EOPNOTSUPP; + /* PASID zero is used for requests from the I/O device without PASID */ if (!is_pasid_valid(dev_data, pasid)) return ret; diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 32dce80aa7fd..27f923450a7c 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -616,7 +616,8 @@ struct iommu_ops { * * EBUSY - device is attached to a domain and cannot be changed * * ENODEV - device specific errors, not able to be attached * * <others> - treated as ENODEV by the caller. Use is discouraged - * @set_dev_pasid: set an iommu domain to a pasid of device + * @set_dev_pasid: set or replace an iommu domain to a pasid of device. The pasid of + * the device should be left in the old config in error case. * @map_pages: map a physically contiguous set of pages of the same size to * an iommu domain. * @unmap_pages: unmap a number of pages of the same size from an iommu domain
On Thu, Sep 12, 2024 at 06:04:27AM -0700, Yi Liu wrote:
The iommu core is going to support domain replacement for pasid, it needs to make the set_dev_pasid op support replacing domain and keep the old domain config in the failure case.
AMD iommu driver does not support domain replacement for pasid yet, so it would fail the set_dev_pasid op to keep the old config if the input @old is non-NULL.
Suggested-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com
drivers/iommu/amd/pasid.c | 3 +++ include/linux/iommu.h | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Jason Gunthorpe jgg@nvidia.com
Jason
From: Liu, Yi L yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:04 PM
The iommu core is going to support domain replacement for pasid, it needs to make the set_dev_pasid op support replacing domain and keep the old domain config in the failure case.
AMD iommu driver does not support domain replacement for pasid yet, so it would fail the set_dev_pasid op to keep the old config if the input @old is non-NULL.
Suggested-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com
Reviewed-by: Kevin Tian kevin.tian@intel.com
linux-kselftest-mirror@lists.linaro.org
-
Baolu Lu -
Jason Gunthorpe -
Jason Gunthorpe -
Liu, Yi L -
Tian, Kevin -
Will Deacon -
Yi Liu