[PATCH 0/7] iommu/amd: Cleanup some of the domain_alloc_paging path

[PATCH 0/7] iommu/amd: Cleanup some of the domain_alloc_paging path

[Jason Gunthorpe]

This is a small collection of cleanups
 - Remove amd_iommu_domain_update() (dead code)
 - Remove domain_alloc()
 - Remove now obsolete NULL checks
 - Remove obsolete domain types in the domain_alloc_paging_flags() flow
 - Use PD_MODE consistently instead of mixing with io_pgtable stuff

Alejandro Jimenez (1):
  iommu/amd: Remove unused amd_iommu_domain_update()

Jason Gunthorpe (6):
  iommu/amd: Remove domain_alloc()
  iommu/amd: Remove dev == NULL checks
  iommu/amd: Remove type argument from do_iommu_domain_alloc() and
    related
  iommu/amd: Change amd_iommu_pgtable to use enum protection_domain_mode
  iommu/amd: Move the nid to pdom_setup_pgtable()
  iommu/amd: Fully decode all combinations of alloc_paging_flags

 drivers/iommu/amd/amd_iommu.h |   5 +-
 drivers/iommu/amd/init.c      |  14 ++--
 drivers/iommu/amd/iommu.c     | 145 +++++++++++-----------------------
 drivers/iommu/amd/pasid.c     |   3 +-
 4 files changed, 55 insertions(+), 112 deletions(-)


base-commit: 94c61483a6b67a288f8272d9356eee71e63bfe06
-- 
2.43.0

[PATCH 1/7] iommu/amd: Remove unused amd_iommu_domain_update()

[Jason Gunthorpe]

From: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

All the callers have been removed by the below commit, remove the
implementation and prototypes.

Fixes: 322d889ae7d3 ("iommu/amd: Remove amd_iommu_domain_update() from page table freeing")
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu.h | 1 -
 drivers/iommu/amd/iommu.c     | 9 ---------
 2 files changed, 10 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 1bef5d55b2f9dd..c38e02510cf737 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -89,7 +89,6 @@ int amd_iommu_complete_ppr(struct device *dev, u32 pasid, int status, int tag);
  */
 void amd_iommu_flush_all_caches(struct amd_iommu *iommu);
 void amd_iommu_update_and_flush_device_table(struct protection_domain *domain);
-void amd_iommu_domain_update(struct protection_domain *domain);
 void amd_iommu_domain_flush_pages(struct protection_domain *domain,
 				  u64 address, size_t size);
 void amd_iommu_dev_flush_pasid_pages(struct iommu_dev_data *dev_data,
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 16f40b8000d798..7e7246c49006ad 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1603,15 +1603,6 @@ void amd_iommu_update_and_flush_device_table(struct protection_domain *domain)
 	domain_flush_complete(domain);
 }
 
-void amd_iommu_domain_update(struct protection_domain *domain)
-{
-	/* Update device table */
-	amd_iommu_update_and_flush_device_table(domain);
-
-	/* Flush domain TLB(s) and wait for completion */
-	amd_iommu_domain_flush_all(domain);
-}
-
 int amd_iommu_complete_ppr(struct device *dev, u32 pasid, int status, int tag)
 {
 	struct iommu_dev_data *dev_data;
-- 
2.43.0

Re: [PATCH 1/7] iommu/amd: Remove unused amd_iommu_domain_update()

[Vasant Hegde]

On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> From: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> 
> All the callers have been removed by the below commit, remove the
> implementation and prototypes.
> 
> Fixes: 322d889ae7d3 ("iommu/amd: Remove amd_iommu_domain_update() from page table freeing")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks Alejandro.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

[PATCH 2/7] iommu/amd: Remove domain_alloc()

[Jason Gunthorpe]

IOMMU drivers should not be sensitive to the domain type, a paging domain
should be created based only on the flags passed in, the same for all
callers.

AMD was using the domain_alloc() path to force VFIO into a v1 domain type,
because v1 gives higher performance. However now that
IOMMU_HWPT_ALLOC_PASID is present, and a NULL device is not possible,
domain_alloc_paging_flags() will do the right thing for VFIO.

When invoked from VFIO flags will be 0 and the amd_iommu_pgtable type of
domain will be selected. This is v1 by default unless the kernel command
line has overridden it to v2.

If the admin is forcing v2 assume they know what they are doing so force
it everywhere, including for VFIO.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 7e7246c49006ad..96d87406f89467 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2386,25 +2386,6 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	return &domain->domain;
 }
 
-static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
-{
-	struct iommu_domain *domain;
-	int pgtable = amd_iommu_pgtable;
-
-	/*
-	 * Force IOMMU v1 page table when allocating
-	 * domain for pass-through devices.
-	 */
-	if (type == IOMMU_DOMAIN_UNMANAGED)
-		pgtable = AMD_IOMMU_V1;
-
-	domain = do_iommu_domain_alloc(type, NULL, 0, pgtable);
-	if (IS_ERR(domain))
-		return NULL;
-
-	return domain;
-}
-
 static struct iommu_domain *
 amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 				    const struct iommu_user_data *user_data)
@@ -2881,7 +2862,6 @@ const struct iommu_ops amd_iommu_ops = {
 	.blocked_domain = &blocked_domain,
 	.release_domain = &release_domain,
 	.identity_domain = &identity_domain.domain,
-	.domain_alloc = amd_iommu_domain_alloc,
 	.domain_alloc_paging_flags = amd_iommu_domain_alloc_paging_flags,
 	.domain_alloc_sva = amd_iommu_domain_alloc_sva,
 	.probe_device = amd_iommu_probe_device,
-- 
2.43.0

Re: [PATCH 2/7] iommu/amd: Remove domain_alloc()

[Vasant Hegde]

On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> IOMMU drivers should not be sensitive to the domain type, a paging domain
> should be created based only on the flags passed in, the same for all
> callers.
> 
> AMD was using the domain_alloc() path to force VFIO into a v1 domain type,
> because v1 gives higher performance. However now that
> IOMMU_HWPT_ALLOC_PASID is present, and a NULL device is not possible,
> domain_alloc_paging_flags() will do the right thing for VFIO.
> 
> When invoked from VFIO flags will be 0 and the amd_iommu_pgtable type of
> domain will be selected. This is v1 by default unless the kernel command
> line has overridden it to v2.
> 
> If the admin is forcing v2 assume they know what they are doing so force
> it everywhere, including for VFIO.

Agree. W/ global identity support and domain_alloc_paging_flags() in place, we
don't need domain_alloc() ops.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

[PATCH 3/7] iommu/amd: Remove dev == NULL checks

[Jason Gunthorpe]

This is no longer possible, amd_iommu_domain_alloc_paging_flags() is never
called with dev = NULL from the core code. Similarly
get_amd_iommu_from_dev() can never be NULL either.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 96d87406f89467..12c416abdce7dc 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2344,13 +2344,10 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 						  u32 flags, int pgtable)
 {
 	bool dirty_tracking = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
 	struct protection_domain *domain;
-	struct amd_iommu *iommu = NULL;
 	int ret;
 
-	if (dev)
-		iommu = get_amd_iommu_from_dev(dev);
-
 	/*
 	 * Since DTE[Mode]=0 is prohibited on SNP-enabled system,
 	 * default to use IOMMU_DOMAIN_DMA[_FQ].
@@ -2358,8 +2355,7 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	if (amd_iommu_snp_en && (type == IOMMU_DOMAIN_IDENTITY))
 		return ERR_PTR(-EINVAL);
 
-	domain = protection_domain_alloc(type,
-					 dev ? dev_to_node(dev) : NUMA_NO_NODE);
+	domain = protection_domain_alloc(type, dev_to_node(dev));
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
@@ -2375,13 +2371,11 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	domain->domain.geometry.force_aperture = true;
 	domain->domain.pgsize_bitmap = domain->iop.pgtbl.cfg.pgsize_bitmap;
 
-	if (iommu) {
-		domain->domain.type = type;
-		domain->domain.ops = iommu->iommu.ops->default_domain_ops;
+	domain->domain.type = type;
+	domain->domain.ops = iommu->iommu.ops->default_domain_ops;
 
-		if (dirty_tracking)
-			domain->domain.dirty_ops = &amd_dirty_ops;
-	}
+	if (dirty_tracking)
+		domain->domain.dirty_ops = &amd_dirty_ops;
 
 	return &domain->domain;
 }
@@ -2392,13 +2386,10 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 
 {
 	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
-	struct amd_iommu *iommu = NULL;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
 	const u32 supported_flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
 						IOMMU_HWPT_ALLOC_PASID;
 
-	if (dev)
-		iommu = get_amd_iommu_from_dev(dev);
-
 	if ((flags & ~supported_flags) || user_data)
 		return ERR_PTR(-EOPNOTSUPP);
 
@@ -2412,10 +2403,9 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 
 	/* Allocate domain with v1 page table for dirty tracking */
 	if (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) {
-		if (iommu && amd_iommu_hd_support(iommu)) {
-			return do_iommu_domain_alloc(type, dev,
-						     flags, AMD_IOMMU_V1);
-		}
+		if (amd_iommu_hd_support(iommu))
+			return do_iommu_domain_alloc(type, dev, flags,
+						     AMD_IOMMU_V1);
 
 		return ERR_PTR(-EOPNOTSUPP);
 	}
-- 
2.43.0

Re: [PATCH 3/7] iommu/amd: Remove dev == NULL checks

[Vasant Hegde]

On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> This is no longer possible, amd_iommu_domain_alloc_paging_flags() is never
> called with dev = NULL from the core code. Similarly
> get_amd_iommu_from_dev() can never be NULL either.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

[PATCH 4/7] iommu/amd: Remove type argument from do_iommu_domain_alloc() and related

[Jason Gunthorpe]

do_iommu_domain_alloc() is only called from
amd_iommu_domain_alloc_paging_flags() so type is always
IOMMU_DOMAIN_UNMANAGED. Remove type and all the dead conditionals checking
it.

The caller of protection_domain_alloc() should set the type, fix the miss
in the SVA code.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu.h |  2 +-
 drivers/iommu/amd/iommu.c     | 35 ++++++++++-------------------------
 drivers/iommu/amd/pasid.c     |  3 ++-
 3 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index c38e02510cf737..1d384b2c6e28e7 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -47,7 +47,7 @@ extern unsigned long amd_iommu_pgsize_bitmap;
 
 /* Protection domain ops */
 void amd_iommu_init_identity_domain(void);
-struct protection_domain *protection_domain_alloc(unsigned int type, int nid);
+struct protection_domain *protection_domain_alloc(int nid);
 void protection_domain_free(struct protection_domain *domain);
 struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
 						struct mm_struct *mm);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 12c416abdce7dc..081a5dbe7ba7bb 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2276,7 +2276,7 @@ static void protection_domain_init(struct protection_domain *domain, int nid)
 	domain->iop.pgtbl.cfg.amd.nid = nid;
 }
 
-struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
+struct protection_domain *protection_domain_alloc(int nid)
 {
 	struct protection_domain *domain;
 	int domid;
@@ -2297,15 +2297,10 @@ struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 	return domain;
 }
 
-static int pdom_setup_pgtable(struct protection_domain *domain,
-			      unsigned int type, int pgtable)
+static int pdom_setup_pgtable(struct protection_domain *domain, int pgtable)
 {
 	struct io_pgtable_ops *pgtbl_ops;
 
-	/* No need to allocate io pgtable ops in passthrough mode */
-	if (!(type & __IOMMU_DOMAIN_PAGING))
-		return 0;
-
 	switch (pgtable) {
 	case AMD_IOMMU_V1:
 		domain->pd_mode = PD_MODE_V1;
@@ -2339,27 +2334,19 @@ static bool amd_iommu_hd_support(struct amd_iommu *iommu)
 	return iommu && (iommu->features & FEATURE_HDSUP);
 }
 
-static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
-						  struct device *dev,
-						  u32 flags, int pgtable)
+static struct iommu_domain *do_iommu_domain_alloc(struct device *dev, u32 flags,
+						  int pgtable)
 {
 	bool dirty_tracking = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
 	struct protection_domain *domain;
 	int ret;
 
-	/*
-	 * Since DTE[Mode]=0 is prohibited on SNP-enabled system,
-	 * default to use IOMMU_DOMAIN_DMA[_FQ].
-	 */
-	if (amd_iommu_snp_en && (type == IOMMU_DOMAIN_IDENTITY))
-		return ERR_PTR(-EINVAL);
-
-	domain = protection_domain_alloc(type, dev_to_node(dev));
+	domain = protection_domain_alloc(dev_to_node(dev));
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	ret = pdom_setup_pgtable(domain, type, pgtable);
+	ret = pdom_setup_pgtable(domain, pgtable);
 	if (ret) {
 		pdom_id_free(domain->id);
 		kfree(domain);
@@ -2371,7 +2358,7 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	domain->domain.geometry.force_aperture = true;
 	domain->domain.pgsize_bitmap = domain->iop.pgtbl.cfg.pgsize_bitmap;
 
-	domain->domain.type = type;
+	domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
 	domain->domain.ops = iommu->iommu.ops->default_domain_ops;
 
 	if (dirty_tracking)
@@ -2385,7 +2372,6 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 				    const struct iommu_user_data *user_data)
 
 {
-	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
 	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
 	const u32 supported_flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
 						IOMMU_HWPT_ALLOC_PASID;
@@ -2398,20 +2384,19 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 		if (!amd_iommu_pasid_supported())
 			return ERR_PTR(-EOPNOTSUPP);
 
-		return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V2);
+		return do_iommu_domain_alloc(dev, flags, AMD_IOMMU_V2);
 	}
 
 	/* Allocate domain with v1 page table for dirty tracking */
 	if (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) {
 		if (amd_iommu_hd_support(iommu))
-			return do_iommu_domain_alloc(type, dev, flags,
-						     AMD_IOMMU_V1);
+			return do_iommu_domain_alloc(dev, flags, AMD_IOMMU_V1);
 
 		return ERR_PTR(-EOPNOTSUPP);
 	}
 
 	/* If nothing specific is required use the kernel commandline default */
-	return do_iommu_domain_alloc(type, dev, 0, amd_iommu_pgtable);
+	return do_iommu_domain_alloc(dev, 0, amd_iommu_pgtable);
 }
 
 void amd_iommu_domain_free(struct iommu_domain *dom)
diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
index 8c73a30c2800e7..9101d07b11d3f7 100644
--- a/drivers/iommu/amd/pasid.c
+++ b/drivers/iommu/amd/pasid.c
@@ -185,12 +185,13 @@ struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
 	struct protection_domain *pdom;
 	int ret;
 
-	pdom = protection_domain_alloc(IOMMU_DOMAIN_SVA, dev_to_node(dev));
+	pdom = protection_domain_alloc(dev_to_node(dev));
 	if (!pdom)
 		return ERR_PTR(-ENOMEM);
 
 	pdom->domain.ops = &amd_sva_domain_ops;
 	pdom->mn.ops = &sva_mn;
+	pdom->domain.type = IOMMU_DOMAIN_SVA;
 
 	ret = mmu_notifier_register(&pdom->mn, mm);
 	if (ret) {
-- 
2.43.0

Re: [PATCH 4/7] iommu/amd: Remove type argument from do_iommu_domain_alloc() and related

[Vasant Hegde]

Hi Jason,


On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> do_iommu_domain_alloc() is only called from
> amd_iommu_domain_alloc_paging_flags() so type is always
> IOMMU_DOMAIN_UNMANAGED. Remove type and all the dead conditionals checking
> it.
> 
> The caller of protection_domain_alloc() should set the type, fix the miss
> in the SVA code.


Minor nit, If you are respining then add description saying
IOMMU_DOMAIN_IDENTITY check in do_iommu_domain_alloc() is not required as we
have global idenity domain support and hence removing that check.

Otherwise patch looks fine.


> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

[PATCH 5/7] iommu/amd: Change amd_iommu_pgtable to use enum protection_domain_mode

[Jason Gunthorpe]

Currently it uses enum io_pgtable_fmt which is from the io pagetable code
and most of the enum values are invalid. protection_domain_mode is
internal the driver and has the only two valid values.

Fix some signatures and variables to use the right type as well.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu.h |  2 +-
 drivers/iommu/amd/init.c      | 14 +++++++-------
 drivers/iommu/amd/iommu.c     | 34 +++++++++++++++++-----------------
 3 files changed, 25 insertions(+), 25 deletions(-)

Aside from a cleanup getting enums instead of int in the various spots this
helps iommu_pt where it will remove the AMD io_pgtable code and these related
enum values entirely.

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 1d384b2c6e28e7..6eb0af2e033907 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -41,7 +41,7 @@ void amd_iommu_disable(void);
 int amd_iommu_reenable(int mode);
 int amd_iommu_enable_faulting(unsigned int cpu);
 extern int amd_iommu_guest_ir;
-extern enum io_pgtable_fmt amd_iommu_pgtable;
+extern enum protection_domain_mode amd_iommu_pgtable;
 extern int amd_iommu_gpt_level;
 extern unsigned long amd_iommu_pgsize_bitmap;
 
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 0e0a531042acb3..db4b52aae1fcf1 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -152,7 +152,7 @@ struct ivmd_header {
 bool amd_iommu_dump;
 bool amd_iommu_irq_remap __read_mostly;
 
-enum io_pgtable_fmt amd_iommu_pgtable = AMD_IOMMU_V1;
+enum protection_domain_mode amd_iommu_pgtable = PD_MODE_V1;
 /* Guest page table level */
 int amd_iommu_gpt_level = PAGE_MODE_4_LEVEL;
 
@@ -2145,7 +2145,7 @@ static void print_iommu_info(void)
 		if (amd_iommu_xt_mode == IRQ_REMAP_X2APIC_MODE)
 			pr_info("X2APIC enabled\n");
 	}
-	if (amd_iommu_pgtable == AMD_IOMMU_V2) {
+	if (amd_iommu_pgtable == PD_MODE_V2) {
 		pr_info("V2 page table enabled (Paging mode : %d level)\n",
 			amd_iommu_gpt_level);
 	}
@@ -3059,10 +3059,10 @@ static int __init early_amd_iommu_init(void)
 	    FIELD_GET(FEATURE_GATS, amd_iommu_efr) == GUEST_PGTABLE_5_LEVEL)
 		amd_iommu_gpt_level = PAGE_MODE_5_LEVEL;
 
-	if (amd_iommu_pgtable == AMD_IOMMU_V2) {
+	if (amd_iommu_pgtable == PD_MODE_V2) {
 		if (!amd_iommu_v2_pgtbl_supported()) {
 			pr_warn("Cannot enable v2 page table for DMA-API. Fallback to v1.\n");
-			amd_iommu_pgtable = AMD_IOMMU_V1;
+			amd_iommu_pgtable = PD_MODE_V1;
 		}
 	}
 
@@ -3185,7 +3185,7 @@ static void iommu_snp_enable(void)
 		goto disable_snp;
 	}
 
-	if (amd_iommu_pgtable != AMD_IOMMU_V1) {
+	if (amd_iommu_pgtable != PD_MODE_V1) {
 		pr_warn("SNP: IOMMU is configured with V2 page table mode, SNP cannot be supported.\n");
 		goto disable_snp;
 	}
@@ -3464,9 +3464,9 @@ static int __init parse_amd_iommu_options(char *str)
 		} else if (strncmp(str, "force_isolation", 15) == 0) {
 			amd_iommu_force_isolation = true;
 		} else if (strncmp(str, "pgtbl_v1", 8) == 0) {
-			amd_iommu_pgtable = AMD_IOMMU_V1;
+			amd_iommu_pgtable = PD_MODE_V1;
 		} else if (strncmp(str, "pgtbl_v2", 8) == 0) {
-			amd_iommu_pgtable = AMD_IOMMU_V2;
+			amd_iommu_pgtable = PD_MODE_V2;
 		} else if (strncmp(str, "irtcachedis", 11) == 0) {
 			amd_iommu_irtcachedis = true;
 		} else if (strncmp(str, "nohugepages", 11) == 0) {
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 081a5dbe7ba7bb..e0c12dc44340cd 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2297,32 +2297,30 @@ struct protection_domain *protection_domain_alloc(int nid)
 	return domain;
 }
 
-static int pdom_setup_pgtable(struct protection_domain *domain, int pgtable)
+static int pdom_setup_pgtable(struct protection_domain *domain)
 {
 	struct io_pgtable_ops *pgtbl_ops;
+	enum io_pgtable_fmt fmt;
 
-	switch (pgtable) {
-	case AMD_IOMMU_V1:
-		domain->pd_mode = PD_MODE_V1;
+	switch (domain->pd_mode) {
+	case PD_MODE_V1:
+		fmt = AMD_IOMMU_V1;
 		break;
-	case AMD_IOMMU_V2:
-		domain->pd_mode = PD_MODE_V2;
+	case PD_MODE_V2:
+		fmt = AMD_IOMMU_V2;
 		break;
-	default:
-		return -EINVAL;
 	}
 
-	pgtbl_ops =
-		alloc_io_pgtable_ops(pgtable, &domain->iop.pgtbl.cfg, domain);
+	pgtbl_ops = alloc_io_pgtable_ops(fmt, &domain->iop.pgtbl.cfg, domain);
 	if (!pgtbl_ops)
 		return -ENOMEM;
 
 	return 0;
 }
 
-static inline u64 dma_max_address(int pgtable)
+static inline u64 dma_max_address(enum protection_domain_mode pgtable)
 {
-	if (pgtable == AMD_IOMMU_V1)
+	if (pgtable == PD_MODE_V1)
 		return ~0ULL;
 
 	/* V2 with 4/5 level page table */
@@ -2334,8 +2332,9 @@ static bool amd_iommu_hd_support(struct amd_iommu *iommu)
 	return iommu && (iommu->features & FEATURE_HDSUP);
 }
 
-static struct iommu_domain *do_iommu_domain_alloc(struct device *dev, u32 flags,
-						  int pgtable)
+static struct iommu_domain *
+do_iommu_domain_alloc(struct device *dev, u32 flags,
+		      enum protection_domain_mode pgtable)
 {
 	bool dirty_tracking = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
@@ -2346,7 +2345,8 @@ static struct iommu_domain *do_iommu_domain_alloc(struct device *dev, u32 flags,
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	ret = pdom_setup_pgtable(domain, pgtable);
+	domain->pd_mode = pgtable;
+	ret = pdom_setup_pgtable(domain);
 	if (ret) {
 		pdom_id_free(domain->id);
 		kfree(domain);
@@ -2384,13 +2384,13 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 		if (!amd_iommu_pasid_supported())
 			return ERR_PTR(-EOPNOTSUPP);
 
-		return do_iommu_domain_alloc(dev, flags, AMD_IOMMU_V2);
+		return do_iommu_domain_alloc(dev, flags, PD_MODE_V2);
 	}
 
 	/* Allocate domain with v1 page table for dirty tracking */
 	if (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) {
 		if (amd_iommu_hd_support(iommu))
-			return do_iommu_domain_alloc(dev, flags, AMD_IOMMU_V1);
+			return do_iommu_domain_alloc(dev, flags, PD_MODE_V1);
 
 		return ERR_PTR(-EOPNOTSUPP);
 	}
-- 
2.43.0

Re: [PATCH 5/7] iommu/amd: Change amd_iommu_pgtable to use enum protection_domain_mode

[Vasant Hegde]

On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> Currently it uses enum io_pgtable_fmt which is from the io pagetable code
> and most of the enum values are invalid. protection_domain_mode is
> internal the driver and has the only two valid values.
> 
> Fix some signatures and variables to use the right type as well.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Good catch.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

[PATCH 6/7] iommu/amd: Move the nid to pdom_setup_pgtable()

[Jason Gunthorpe]

The only thing that uses the nid is the io_pgtable code, and it should be
set before calling alloc_io_pgtable_ops() to ensure that the top levels
are allocated on the correct nid.

Since dev is never NULL now we can just do this trivially and remove the
other uses of nid. SVA and identity code paths never use it since they
don't use io_pgtable.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu.h |  2 +-
 drivers/iommu/amd/iommu.c     | 22 +++++++++-------------
 drivers/iommu/amd/pasid.c     |  2 +-
 3 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6eb0af2e033907..b0a30fa717ca26 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -47,7 +47,7 @@ extern unsigned long amd_iommu_pgsize_bitmap;
 
 /* Protection domain ops */
 void amd_iommu_init_identity_domain(void);
-struct protection_domain *protection_domain_alloc(int nid);
+struct protection_domain *protection_domain_alloc(void);
 void protection_domain_free(struct protection_domain *domain);
 struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
 						struct mm_struct *mm);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index e0c12dc44340cd..c84eb3e7bd8bef 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1998,7 +1998,6 @@ static int pdom_attach_iommu(struct amd_iommu *iommu,
 			     struct protection_domain *pdom)
 {
 	struct pdom_iommu_info *pdom_iommu_info, *curr;
-	struct io_pgtable_cfg *cfg = &pdom->iop.pgtbl.cfg;
 	unsigned long flags;
 	int ret = 0;
 
@@ -2027,10 +2026,6 @@ static int pdom_attach_iommu(struct amd_iommu *iommu,
 		goto out_unlock;
 	}
 
-	/* Update NUMA Node ID */
-	if (cfg->amd.nid == NUMA_NO_NODE)
-		cfg->amd.nid = dev_to_node(&iommu->dev->dev);
-
 out_unlock:
 	spin_unlock_irqrestore(&pdom->lock, flags);
 	return ret;
@@ -2267,16 +2262,15 @@ void protection_domain_free(struct protection_domain *domain)
 	kfree(domain);
 }
 
-static void protection_domain_init(struct protection_domain *domain, int nid)
+static void protection_domain_init(struct protection_domain *domain)
 {
 	spin_lock_init(&domain->lock);
 	INIT_LIST_HEAD(&domain->dev_list);
 	INIT_LIST_HEAD(&domain->dev_data_list);
 	xa_init(&domain->iommu_array);
-	domain->iop.pgtbl.cfg.amd.nid = nid;
 }
 
-struct protection_domain *protection_domain_alloc(int nid)
+struct protection_domain *protection_domain_alloc(void)
 {
 	struct protection_domain *domain;
 	int domid;
@@ -2292,12 +2286,13 @@ struct protection_domain *protection_domain_alloc(int nid)
 	}
 	domain->id = domid;
 
-	protection_domain_init(domain, nid);
+	protection_domain_init(domain);
 
 	return domain;
 }
 
-static int pdom_setup_pgtable(struct protection_domain *domain)
+static int pdom_setup_pgtable(struct protection_domain *domain,
+			      struct device *dev)
 {
 	struct io_pgtable_ops *pgtbl_ops;
 	enum io_pgtable_fmt fmt;
@@ -2311,6 +2306,7 @@ static int pdom_setup_pgtable(struct protection_domain *domain)
 		break;
 	}
 
+	domain->iop.pgtbl.cfg.amd.nid = dev_to_node(dev);
 	pgtbl_ops = alloc_io_pgtable_ops(fmt, &domain->iop.pgtbl.cfg, domain);
 	if (!pgtbl_ops)
 		return -ENOMEM;
@@ -2341,12 +2337,12 @@ do_iommu_domain_alloc(struct device *dev, u32 flags,
 	struct protection_domain *domain;
 	int ret;
 
-	domain = protection_domain_alloc(dev_to_node(dev));
+	domain = protection_domain_alloc();
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
 	domain->pd_mode = pgtable;
-	ret = pdom_setup_pgtable(domain);
+	ret = pdom_setup_pgtable(domain, dev);
 	if (ret) {
 		pdom_id_free(domain->id);
 		kfree(domain);
@@ -2445,7 +2441,7 @@ void amd_iommu_init_identity_domain(void)
 
 	identity_domain.id = pdom_id_alloc();
 
-	protection_domain_init(&identity_domain, NUMA_NO_NODE);
+	protection_domain_init(&identity_domain);
 }
 
 /* Same as blocked domain except it supports only ops->attach_dev() */
diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
index 9101d07b11d3f7..11150cfd67182d 100644
--- a/drivers/iommu/amd/pasid.c
+++ b/drivers/iommu/amd/pasid.c
@@ -185,7 +185,7 @@ struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
 	struct protection_domain *pdom;
 	int ret;
 
-	pdom = protection_domain_alloc(dev_to_node(dev));
+	pdom = protection_domain_alloc();
 	if (!pdom)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.43.0

Re: [PATCH 6/7] iommu/amd: Move the nid to pdom_setup_pgtable()

[Vasant Hegde]

On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> The only thing that uses the nid is the io_pgtable code, and it should be
> set before calling alloc_io_pgtable_ops() to ensure that the top levels
> are allocated on the correct nid.
> 
> Since dev is never NULL now we can just do this trivially and remove the
> other uses of nid. SVA and identity code paths never use it since they
> don't use io_pgtable.

Right. Also in attach device path we rely on IOMMU NODE to setup GCR3 table.
This patch looks good.

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

[PATCH 7/7] iommu/amd: Fully decode all combinations of alloc_paging_flags

[Jason Gunthorpe]

Currently AMD does not support IOMMU_HWPT_ALLOC_PASID |
IOMMU_HWPT_ALLOC_DIRTY_TRACKING, it should be rejected. Instead it creates
a V1 domain without dirty tracking support.

Use a switch to fully decode the flags.

Fixes: ce2cd175469f ("iommu/amd: Enhance amd_iommu_domain_alloc_user()")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index c84eb3e7bd8bef..a418a1f48e7fa7 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2375,24 +2375,25 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 	if ((flags & ~supported_flags) || user_data)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	/* Allocate domain with v2 page table if IOMMU supports PASID. */
-	if (flags & IOMMU_HWPT_ALLOC_PASID) {
+	switch (flags &
+		(IOMMU_HWPT_ALLOC_PASID | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)) {
+	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING:
+		/* Allocate domain with v1 page table for dirty tracking */
+		if (!amd_iommu_hd_support(iommu))
+			break;
+		return do_iommu_domain_alloc(dev, flags, PD_MODE_V1);
+	case IOMMU_HWPT_ALLOC_PASID:
+		/* Allocate domain with v2 page table if IOMMU supports PASID. */
 		if (!amd_iommu_pasid_supported())
-			return ERR_PTR(-EOPNOTSUPP);
-
+			break;
 		return do_iommu_domain_alloc(dev, flags, PD_MODE_V2);
+	case 0:
+		/* If nothing specific is required use the kernel commandline default */
+		return do_iommu_domain_alloc(dev, 0, amd_iommu_pgtable);
+	default:
+		break;
 	}
-
-	/* Allocate domain with v1 page table for dirty tracking */
-	if (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) {
-		if (amd_iommu_hd_support(iommu))
-			return do_iommu_domain_alloc(dev, flags, PD_MODE_V1);
-
-		return ERR_PTR(-EOPNOTSUPP);
-	}
-
-	/* If nothing specific is required use the kernel commandline default */
-	return do_iommu_domain_alloc(dev, 0, amd_iommu_pgtable);
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 void amd_iommu_domain_free(struct iommu_domain *dom)
-- 
2.43.0

Re: [PATCH 7/7] iommu/amd: Fully decode all combinations of alloc_paging_flags

[Vasant Hegde]

Hi Jason,


On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> Currently AMD does not support IOMMU_HWPT_ALLOC_PASID |
> IOMMU_HWPT_ALLOC_DIRTY_TRACKING, it should be rejected. Instead it creates
> a V1 domain without dirty tracking support.
> 
> Use a switch to fully decode the flags.
> 
> Fixes: ce2cd175469f ("iommu/amd: Enhance amd_iommu_domain_alloc_user()")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/amd/iommu.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index c84eb3e7bd8bef..a418a1f48e7fa7 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2375,24 +2375,25 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
>  	if ((flags & ~supported_flags) || user_data)
>  		return ERR_PTR(-EOPNOTSUPP);
>  
> -	/* Allocate domain with v2 page table if IOMMU supports PASID. */
> -	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> +	switch (flags &
> +		(IOMMU_HWPT_ALLOC_PASID | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)) {

Better use supported_flags. Otherwise patch looks good to me.
With that

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH 7/7] iommu/amd: Fully decode all combinations of alloc_paging_flags

[Jason Gunthorpe]

On Tue, Jan 07, 2025 at 04:17:59PM +0530, Vasant Hegde wrote:
> > @@ -2375,24 +2375,25 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
> >  	if ((flags & ~supported_flags) || user_data)
                  ^^^^^^

> >  		return ERR_PTR(-EOPNOTSUPP);
> >  
> > -	/* Allocate domain with v2 page table if IOMMU supports PASID. */
> > -	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> > +	switch (flags &
> > +		(IOMMU_HWPT_ALLOC_PASID | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)) {
> 
> Better use supported_flags. Otherwise patch looks good to me.
> With that

For now we can just delete it since we already checked that flags
can't be outside that range.

Maybe when new flags are added it will need to come back someday

-       switch (flags &
-               (IOMMU_HWPT_ALLOC_PASID | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)) {
+       switch (flags & supported_flags) {

Thanks,
Jason

Re: [PATCH 0/7] iommu/amd: Cleanup some of the domain_alloc_paging path

[Vasant Hegde]

Jason, Alejandro,


On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> This is a small collection of cleanups
>  - Remove amd_iommu_domain_update() (dead code)
>  - Remove domain_alloc()
>  - Remove now obsolete NULL checks
>  - Remove obsolete domain types in the domain_alloc_paging_flags() flow
>  - Use PD_MODE consistently instead of mixing with io_pgtable stuff


Thanks for the series. With this we are close to finish the domain allocation
path rework. I have a patch to fix def_domain_type() path. Will rebase and post
it soon.

-Vasant

Re: [PATCH 0/7] iommu/amd: Cleanup some of the domain_alloc_paging path

[Jason Gunthorpe]

On Tue, Jan 07, 2025 at 04:21:42PM +0530, Vasant Hegde wrote:
> On 12/6/2024 5:53 AM, Jason Gunthorpe wrote:
> > This is a small collection of cleanups
> >  - Remove amd_iommu_domain_update() (dead code)
> >  - Remove domain_alloc()
> >  - Remove now obsolete NULL checks
> >  - Remove obsolete domain types in the domain_alloc_paging_flags() flow
> >  - Use PD_MODE consistently instead of mixing with io_pgtable stuff
> 
> Thanks for the series. With this we are close to finish the domain allocation
> path rework. I have a patch to fix def_domain_type() path. Will rebase and post
> it soon.

Great, thanks

Jason