[PATCH v2 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring

[PATCH v2 00/16] iommu/amd: SVA Support (Part 1) - cleanup/refactoring

[Vasant Hegde]

This is part 1 of the 3-part series to introduce Share Virtual Address
(SVA) support, which focuses on cleaning up and refactoring the existing
code in preparation for subsequent series.

It contains the following enhancements:

* Patch 1 - 10:
  Clean up and refactoring and miscellaneous improvements.

* Patch 11 - 16:
  Modify logic to independently enable PCI capabilities (ATS/PASID/PRI)
  for devices. This allows more flexibility in preparation for SVA and
  IOPF supports.

This patch series is based on top of iommu/next branch.
Base commit : a5003e75a171

This is also available at github :
 https://github.com/AMDESE/linux/tree/iommu_sva_part1_v2_v6.5_rc1

Changes from v1 -> v2:
  - Dropped GCR3 related changes from Part1. We are reworking
    GCR3 management based on Jason's comment. We will post them
    as separate part.
  - Addressed review comment from Jason
  - Added iommu_dev_data.ppr to track PPR status

v1 : https://lore.kernel.org/linux-iommu/20230712141516.154144-1-vasant.hegde@amd.com/


Thank you,
Vasant / Suravee


Suravee Suthikulpanit (8):
  iommu/amd: Remove unused amd_io_pgtable.pt_root variable
  iommu/amd: Consolidate timeout pre-define to amd_iommu_type.h
  iommu/amd: Consolidate logic to allocate protection domain
  iommu/amd: Introduce helper functions for managing GCR3 table
  iommu/amd: Use struct protection_domain in helper functions
  iommu/amd: Miscellaneous clean up when free domain
  iommu/amd: Modify logic for checking GT and PPR features
  iommu/amd: Add support for different types of PPR handler

Vasant Hegde (8):
  iommu/amd: Refactor protection domain allocation code
  iommu/amd/iommu_v2: Use protection_domain in struct device_state
  iommu/amd: Do not set amd_iommu_pgtable in pass-through mode
  iommu/amd: Rename ats related variables
  iommu/amd: Introduce iommu_dev_data.flags to track device capabilities
  iommu/amd: Enable device ATS/PASID/PRI capabilities independently
  iommu/amd: Initialize iommu_device->max_pasids
  iommu/amd: Simplify amd_iommu_device_info()

 drivers/iommu/amd/amd_iommu.h       |  29 +-
 drivers/iommu/amd/amd_iommu_types.h |  34 +-
 drivers/iommu/amd/init.c            |  37 +--
 drivers/iommu/amd/io_pgtable_v2.c   |  14 +-
 drivers/iommu/amd/iommu.c           | 484 ++++++++++++++--------------
 drivers/iommu/amd/iommu_v2.c        |  51 +--
 6 files changed, 336 insertions(+), 313 deletions(-)

-- 
2.31.1

[PATCH v2 01/16] iommu/amd: Remove unused amd_io_pgtable.pt_root variable

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

It has been no longer used since the commit 6eedb59c18a3 ("iommu/amd:
Remove amd_iommu_domain_get_pgtable").

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h       | 1 -
 drivers/iommu/amd/amd_iommu_types.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index f23ad6043f04..6bf55de9fbeb 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -108,7 +108,6 @@ static inline void *iommu_phys_to_virt(unsigned long paddr)
 static inline
 void amd_iommu_domain_set_pt_root(struct protection_domain *domain, u64 root)
 {
-	atomic64_set(&domain->iop.pt_root, root);
 	domain->iop.root = (u64 *)(root & PAGE_MASK);
 	domain->iop.mode = root & 7; /* lowest 3 bits encode pgtable mode */
 }
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 781ab9c2ea70..1930e918188f 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -544,7 +544,6 @@ struct amd_io_pgtable {
 	struct io_pgtable	iop;
 	int			mode;
 	u64			*root;
-	atomic64_t		pt_root;	/* pgtable root and pgtable mode */
 	u64			*pgd;		/* v2 pgtable pgd pointer */
 };
 
-- 
2.31.1

Re: [PATCH v2 01/16] iommu/amd: Remove unused amd_io_pgtable.pt_root variable

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:35:54AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> It has been no longer used since the commit 6eedb59c18a3 ("iommu/amd:
> Remove amd_iommu_domain_get_pgtable").
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h       | 1 -
>  drivers/iommu/amd/amd_iommu_types.h | 1 -
>  2 files changed, 2 deletions(-)

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

Jason

[PATCH v2 02/16] iommu/amd: Consolidate timeout pre-define to amd_iommu_type.h

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

To allow inclusion in other files in subsequent patches.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  4 ++++
 drivers/iommu/amd/init.c            | 10 ++++------
 drivers/iommu/amd/iommu.c           |  2 --
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 1930e918188f..13c582a2454a 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -451,6 +451,10 @@
 #define PD_IOMMUV2_MASK		BIT(3) /* domain has gcr3 table */
 #define PD_GIOV_MASK		BIT(4) /* domain enable GIOV support */
 
+/* Timeout stuff */
+#define LOOP_TIMEOUT		100000
+#define MMIO_STATUS_TIMEOUT	2000000
+
 extern bool amd_iommu_dump;
 #define DUMP_printk(format, arg...)				\
 	do {							\
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 45efb7e5d725..852e40b13d20 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -83,8 +83,6 @@
 #define ACPI_DEVFLAG_LINT1              0x80
 #define ACPI_DEVFLAG_ATSDIS             0x10000000
 
-#define LOOP_TIMEOUT	2000000
-
 #define IVRS_GET_SBDF_ID(seg, bus, dev, fn)	(((seg & 0xffff) << 16) | ((bus & 0xff) << 8) \
 						 | ((dev & 0x1f) << 3) | (fn & 0x7))
 
@@ -985,14 +983,14 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
 	iommu_feature_enable(iommu, CONTROL_GAINT_EN);
 	iommu_feature_enable(iommu, CONTROL_GALOG_EN);
 
-	for (i = 0; i < LOOP_TIMEOUT; ++i) {
+	for (i = 0; i < MMIO_STATUS_TIMEOUT; ++i) {
 		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
 		if (status & (MMIO_STATUS_GALOG_RUN_MASK))
 			break;
 		udelay(10);
 	}
 
-	if (WARN_ON(i >= LOOP_TIMEOUT))
+	if (WARN_ON(i >= MMIO_STATUS_TIMEOUT))
 		return -EINVAL;
 
 	return 0;
@@ -2900,14 +2898,14 @@ static void enable_iommus_vapic(void)
 		 * Need to set and poll check the GALOGRun bit to zero before
 		 * we can set/ modify GA Log registers safely.
 		 */
-		for (i = 0; i < LOOP_TIMEOUT; ++i) {
+		for (i = 0; i < MMIO_STATUS_TIMEOUT; ++i) {
 			status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
 			if (!(status & MMIO_STATUS_GALOG_RUN_MASK))
 				break;
 			udelay(10);
 		}
 
-		if (WARN_ON(i >= LOOP_TIMEOUT))
+		if (WARN_ON(i >= MMIO_STATUS_TIMEOUT))
 			return;
 	}
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 56b6cf8bf03f..dd586f69a9d8 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -44,8 +44,6 @@
 
 #define CMD_SET_TYPE(cmd, t) ((cmd)->data[1] |= ((t) << 28))
 
-#define LOOP_TIMEOUT	100000
-
 /* IO virtual address start page frame number */
 #define IOVA_START_PFN		(1)
 #define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)
-- 
2.31.1

Re: [PATCH v2 02/16] iommu/amd: Consolidate timeout pre-define to amd_iommu_type.h

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:35:55AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> To allow inclusion in other files in subsequent patches.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  4 ++++
>  drivers/iommu/amd/init.c            | 10 ++++------
>  drivers/iommu/amd/iommu.c           |  2 --
>  3 files changed, 8 insertions(+), 8 deletions(-)

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

Jason

[PATCH v2 03/16] iommu/amd: Consolidate logic to allocate protection domain

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Move the logic into the common caller function to simplify the code.

No functional changes intended.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index dd586f69a9d8..c2cb541b0553 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2046,12 +2046,6 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 
 	BUG_ON(mode < PAGE_MODE_NONE || mode > PAGE_MODE_6_LEVEL);
 
-	spin_lock_init(&domain->lock);
-	domain->id = domain_id_alloc();
-	if (!domain->id)
-		return -ENOMEM;
-	INIT_LIST_HEAD(&domain->dev_list);
-
 	if (mode != PAGE_MODE_NONE) {
 		pt_root = (void *)get_zeroed_page(GFP_KERNEL);
 		if (!pt_root) {
@@ -2067,12 +2061,6 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 
 static int protection_domain_init_v2(struct protection_domain *domain)
 {
-	spin_lock_init(&domain->lock);
-	domain->id = domain_id_alloc();
-	if (!domain->id)
-		return -ENOMEM;
-	INIT_LIST_HEAD(&domain->dev_list);
-
 	domain->flags |= PD_GIOV_MASK;
 
 	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
@@ -2112,6 +2100,13 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
 	if (!domain)
 		return NULL;
 
+	domain->id = domain_id_alloc();
+	if (!domain->id)
+		goto out_err;
+
+	spin_lock_init(&domain->lock);
+	INIT_LIST_HEAD(&domain->dev_list);
+
 	switch (pgtable) {
 	case AMD_IOMMU_V1:
 		ret = protection_domain_init_v1(domain, mode);
-- 
2.31.1

Re: [PATCH v2 03/16] iommu/amd: Consolidate logic to allocate protection domain

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:35:56AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Move the logic into the common caller function to simplify the code.
> 
> No functional changes intended.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)

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

Jason

[PATCH v2 04/16] iommu/amd: Refactor protection domain allocation code

[Vasant Hegde]

To replace if-else with switch-case statement due to increasing number of
domain types.

No functional changes intended.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 46 +++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index c2cb541b0553..09749ad4445c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2078,24 +2078,8 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
 	struct io_pgtable_ops *pgtbl_ops;
 	struct protection_domain *domain;
 	int pgtable;
-	int mode = DEFAULT_PGTABLE_LEVEL;
 	int ret;
 
-	/*
-	 * Force IOMMU v1 page table when iommu=pt and
-	 * when allocating domain for pass-through devices.
-	 */
-	if (type == IOMMU_DOMAIN_IDENTITY) {
-		pgtable = AMD_IOMMU_V1;
-		mode = PAGE_MODE_NONE;
-	} else if (type == IOMMU_DOMAIN_UNMANAGED) {
-		pgtable = AMD_IOMMU_V1;
-	} else if (type == IOMMU_DOMAIN_DMA || type == IOMMU_DOMAIN_DMA_FQ) {
-		pgtable = amd_iommu_pgtable;
-	} else {
-		return NULL;
-	}
-
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
@@ -2106,27 +2090,43 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
 
 	spin_lock_init(&domain->lock);
 	INIT_LIST_HEAD(&domain->dev_list);
+	domain->nid = NUMA_NO_NODE;
+
+	switch (type) {
+	/* No need to allocate io pgtable ops in passthrough mode */
+	case IOMMU_DOMAIN_IDENTITY:
+		return domain;
+	case IOMMU_DOMAIN_DMA:
+		fallthrough;
+	case IOMMU_DOMAIN_DMA_FQ:
+		pgtable = amd_iommu_pgtable;
+		break;
+	/*
+	 * Force IOMMU v1 page table when allocating
+	 * domain for pass-through devices.
+	 */
+	case IOMMU_DOMAIN_UNMANAGED:
+		pgtable = AMD_IOMMU_V1;
+		break;
+	default:
+		goto out_err;
+	}
 
 	switch (pgtable) {
 	case AMD_IOMMU_V1:
-		ret = protection_domain_init_v1(domain, mode);
+		ret = protection_domain_init_v1(domain, DEFAULT_PGTABLE_LEVEL);
 		break;
 	case AMD_IOMMU_V2:
 		ret = protection_domain_init_v2(domain);
 		break;
 	default:
 		ret = -EINVAL;
+		break;
 	}
 
 	if (ret)
 		goto out_err;
 
-	/* No need to allocate io pgtable ops in passthrough mode */
-	if (type == IOMMU_DOMAIN_IDENTITY)
-		return domain;
-
-	domain->nid = NUMA_NO_NODE;
-
 	pgtbl_ops = alloc_io_pgtable_ops(pgtable, &domain->iop.pgtbl_cfg, domain);
 	if (!pgtbl_ops) {
 		domain_id_free(domain->id);
-- 
2.31.1

Re: [PATCH v2 04/16] iommu/amd: Refactor protection domain allocation code

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:35:57AM +0000, Vasant Hegde wrote:
> To replace if-else with switch-case statement due to increasing number of
> domain types.
> 
> No functional changes intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 46 +++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index c2cb541b0553..09749ad4445c 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2078,24 +2078,8 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
>  	struct io_pgtable_ops *pgtbl_ops;
>  	struct protection_domain *domain;
>  	int pgtable;
> -	int mode = DEFAULT_PGTABLE_LEVEL;
>  	int ret;
>  
> -	/*
> -	 * Force IOMMU v1 page table when iommu=pt and
> -	 * when allocating domain for pass-through devices.
> -	 */
> -	if (type == IOMMU_DOMAIN_IDENTITY) {
> -		pgtable = AMD_IOMMU_V1;
> -		mode = PAGE_MODE_NONE;
> -	} else if (type == IOMMU_DOMAIN_UNMANAGED) {
> -		pgtable = AMD_IOMMU_V1;
> -	} else if (type == IOMMU_DOMAIN_DMA || type == IOMMU_DOMAIN_DMA_FQ) {
> -		pgtable = amd_iommu_pgtable;
> -	} else {
> -		return NULL;
> -	}
> -
>  	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>  	if (!domain)
>  		return NULL;
> @@ -2106,27 +2090,43 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
>  
>  	spin_lock_init(&domain->lock);
>  	INIT_LIST_HEAD(&domain->dev_list);
> +	domain->nid = NUMA_NO_NODE;
> +
> +	switch (type) {
> +	/* No need to allocate io pgtable ops in passthrough mode */
> +	case IOMMU_DOMAIN_IDENTITY:
> +		return domain;
> +	case IOMMU_DOMAIN_DMA:
> +		fallthrough;
> +	case IOMMU_DOMAIN_DMA_FQ:

you don't need the fallthrough there, just put the two cases together,
the compiler recognizes the pattern

> +		pgtable = amd_iommu_pgtable;
> +		break;
> +	/*
> +	 * Force IOMMU v1 page table when allocating
> +	 * domain for pass-through devices.
> +	 */
> +	case IOMMU_DOMAIN_UNMANAGED:
> +		pgtable = AMD_IOMMU_V1;
> +		break;

I've been wondering why this was done, it really should not be
done. Unmanaged and DMA should be the same. The driver should pick the
one with the highest performance.

Otherwise it looks fine

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

Jason

Re: [PATCH v2 04/16] iommu/amd: Refactor protection domain allocation code

[Vasant Hegde]

Hi Jason,

On 7/28/2023 7:23 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:35:57AM +0000, Vasant Hegde wrote:
>> To replace if-else with switch-case statement due to increasing number of
>> domain types.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 46 +++++++++++++++++++--------------------
>>  1 file changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index c2cb541b0553..09749ad4445c 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2078,24 +2078,8 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
>>  	struct io_pgtable_ops *pgtbl_ops;
>>  	struct protection_domain *domain;
>>  	int pgtable;
>> -	int mode = DEFAULT_PGTABLE_LEVEL;
>>  	int ret;
>>  
>> -	/*
>> -	 * Force IOMMU v1 page table when iommu=pt and
>> -	 * when allocating domain for pass-through devices.
>> -	 */
>> -	if (type == IOMMU_DOMAIN_IDENTITY) {
>> -		pgtable = AMD_IOMMU_V1;
>> -		mode = PAGE_MODE_NONE;
>> -	} else if (type == IOMMU_DOMAIN_UNMANAGED) {
>> -		pgtable = AMD_IOMMU_V1;
>> -	} else if (type == IOMMU_DOMAIN_DMA || type == IOMMU_DOMAIN_DMA_FQ) {
>> -		pgtable = amd_iommu_pgtable;
>> -	} else {
>> -		return NULL;
>> -	}
>> -
>>  	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>>  	if (!domain)
>>  		return NULL;
>> @@ -2106,27 +2090,43 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
>>  
>>  	spin_lock_init(&domain->lock);
>>  	INIT_LIST_HEAD(&domain->dev_list);
>> +	domain->nid = NUMA_NO_NODE;
>> +
>> +	switch (type) {
>> +	/* No need to allocate io pgtable ops in passthrough mode */
>> +	case IOMMU_DOMAIN_IDENTITY:
>> +		return domain;
>> +	case IOMMU_DOMAIN_DMA:
>> +		fallthrough;
>> +	case IOMMU_DOMAIN_DMA_FQ:
> 
> you don't need the fallthrough there, just put the two cases together,
> the compiler recognizes the pattern

Sure. Will fix it.

> 
>> +		pgtable = amd_iommu_pgtable;
>> +		break;
>> +	/*
>> +	 * Force IOMMU v1 page table when allocating
>> +	 * domain for pass-through devices.
>> +	 */
>> +	case IOMMU_DOMAIN_UNMANAGED:
>> +		pgtable = AMD_IOMMU_V1;
>> +		break;
> 
> I've been wondering why this was done, it really should not be
> done. Unmanaged and DMA should be the same. The driver should pick the
> one with the highest performance.

This is to make sure in guest we can use V2 page table.


> 
> Otherwise it looks fine
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks

-Vasant

Re: [PATCH v2 04/16] iommu/amd: Refactor protection domain allocation code

[Jason Gunthorpe]

On Mon, Jul 31, 2023 at 12:00:02PM +0530, Vasant Hegde wrote:

> >> +		pgtable = amd_iommu_pgtable;
> >> +		break;
> >> +	/*
> >> +	 * Force IOMMU v1 page table when allocating
> >> +	 * domain for pass-through devices.
> >> +	 */
> >> +	case IOMMU_DOMAIN_UNMANAGED:
> >> +		pgtable = AMD_IOMMU_V1;
> >> +		break;
> > 
> > I've been wondering why this was done, it really should not be
> > done. Unmanaged and DMA should be the same. The driver should pick the
> > one with the highest performance.
> 
> This is to make sure in guest we can use V2 page table.

Upstream does not support that anyhow, so why is this in the mainline
kernel?

Jason

[PATCH v2 05/16] iommu/amd/iommu_v2: Use protection_domain in struct device_state

[Vasant Hegde]

Replace struct iommu_domain with struct protection_domain to simplify code
since the protection_domain contains AMD IOMMU specific information.

No functional changes intended.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu_v2.c | 38 ++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index c5825e0a6770..6a423928d5b5 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -55,7 +55,7 @@ struct device_state {
 	atomic_t count;
 	struct pci_dev *pdev;
 	struct pasid_state **states;
-	struct iommu_domain *domain;
+	struct protection_domain *pdom;
 	int pasid_levels;
 	int max_pasids;
 	amd_iommu_invalid_ppr_cb inv_ppr_cb;
@@ -112,6 +112,7 @@ static struct device_state *get_device_state(u32 sbdf)
 static void free_device_state(struct device_state *dev_state)
 {
 	struct iommu_group *group;
+	struct iommu_domain *domain = &dev_state->pdom->domain;
 
 	/* Get rid of any remaining pasid states */
 	free_pasid_states(dev_state);
@@ -130,12 +131,12 @@ static void free_device_state(struct device_state *dev_state)
 	if (WARN_ON(!group))
 		return;
 
-	iommu_detach_group(dev_state->domain, group);
+	iommu_detach_group(domain, group);
 
 	iommu_group_put(group);
 
 	/* Everything is down now, free the IOMMUv2 domain */
-	iommu_domain_free(dev_state->domain);
+	iommu_domain_free(domain);
 
 	/* Finally get rid of the device-state */
 	kfree(dev_state);
@@ -269,9 +270,7 @@ static void put_pasid_state_wait(struct pasid_state *pasid_state)
 
 static void unbind_pasid(struct pasid_state *pasid_state)
 {
-	struct iommu_domain *domain;
-
-	domain = pasid_state->device_state->domain;
+	struct device_state *dev_state = pasid_state->device_state;
 
 	/*
 	 * Mark pasid_state as invalid, no more faults will we added to the
@@ -283,7 +282,7 @@ static void unbind_pasid(struct pasid_state *pasid_state)
 	smp_wmb();
 
 	/* After this the device/pasid can't access the mm anymore */
-	amd_iommu_domain_clear_gcr3(domain, pasid_state->pasid);
+	amd_iommu_domain_clear_gcr3(&dev_state->pdom->domain, pasid_state->pasid);
 
 	/* Make sure no more pending faults are in the queue */
 	flush_workqueue(iommu_wq);
@@ -369,10 +368,10 @@ static void mn_invalidate_range(struct mmu_notifier *mn,
 	dev_state   = pasid_state->device_state;
 
 	if ((start ^ (end - 1)) < PAGE_SIZE)
-		amd_iommu_flush_page(dev_state->domain, pasid_state->pasid,
+		amd_iommu_flush_page(&dev_state->pdom->domain, pasid_state->pasid,
 				     start);
 	else
-		amd_iommu_flush_tlb(dev_state->domain, pasid_state->pasid);
+		amd_iommu_flush_tlb(&dev_state->pdom->domain, pasid_state->pasid);
 }
 
 static void mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
@@ -651,7 +650,7 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, u32 pasid,
 	if (ret)
 		goto out_unregister;
 
-	ret = amd_iommu_domain_set_gcr3(dev_state->domain, pasid,
+	ret = amd_iommu_domain_set_gcr3(&dev_state->pdom->domain, pasid,
 					__pa(pasid_state->mm->pgd));
 	if (ret)
 		goto out_clear_state;
@@ -735,6 +734,7 @@ EXPORT_SYMBOL(amd_iommu_unbind_pasid);
 
 int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 {
+	struct iommu_domain *domain;
 	struct device_state *dev_state;
 	struct iommu_group *group;
 	unsigned long flags;
@@ -779,15 +779,19 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 	if (dev_state->states == NULL)
 		goto out_free_dev_state;
 
-	dev_state->domain = iommu_domain_alloc(&pci_bus_type);
-	if (dev_state->domain == NULL)
+	domain = iommu_domain_alloc(&pci_bus_type);
+	if (domain == NULL)
 		goto out_free_states;
 
+	/* Retrieve new protection_domain that has just been allocated */
+	dev_state->pdom = container_of(domain,
+				       struct protection_domain, domain);
+
 	/* See iommu_is_default_domain() */
-	dev_state->domain->type = IOMMU_DOMAIN_IDENTITY;
-	amd_iommu_domain_direct_map(dev_state->domain);
+	domain->type = IOMMU_DOMAIN_IDENTITY;
+	amd_iommu_domain_direct_map(&dev_state->pdom->domain);
 
-	ret = amd_iommu_domain_enable_v2(dev_state->domain, pasids);
+	ret = amd_iommu_domain_enable_v2(domain, pasids);
 	if (ret)
 		goto out_free_domain;
 
@@ -797,7 +801,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 		goto out_free_domain;
 	}
 
-	ret = iommu_attach_group(dev_state->domain, group);
+	ret = iommu_attach_group(domain, group);
 	if (ret != 0)
 		goto out_drop_group;
 
@@ -821,7 +825,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 	iommu_group_put(group);
 
 out_free_domain:
-	iommu_domain_free(dev_state->domain);
+	iommu_domain_free(domain);
 
 out_free_states:
 	free_page((unsigned long)dev_state->states);
-- 
2.31.1

Re: [PATCH v2 05/16] iommu/amd/iommu_v2: Use protection_domain in struct device_state

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:35:58AM +0000, Vasant Hegde wrote:
> Replace struct iommu_domain with struct protection_domain to simplify code
> since the protection_domain contains AMD IOMMU specific information.
> 
> No functional changes intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu_v2.c | 38 ++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)

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

Jason

[PATCH v2 06/16] iommu/amd: Introduce helper functions for managing GCR3 table

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Refactor domain_enable_v2() into helper functions for managing GCR3 table
(i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
subsequent patches. Also re-arrange code and remove forward declaration.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 63 ++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 09749ad4445c..3c14c49d6d72 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -77,7 +77,6 @@ struct iommu_cmd {
 struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
-static int domain_enable_v2(struct protection_domain *domain, int pasids);
 
 /****************************************************************************
  *
@@ -1575,6 +1574,40 @@ static void free_gcr3_table(struct protection_domain *domain)
 	free_page((unsigned long)domain->gcr3_tbl);
 }
 
+static int get_gcr3_levels(int pasids)
+{
+	int levels = 0;
+
+	if (pasids == -1)
+		return amd_iommu_max_glx_val;
+
+	/* Number of GCR3 table levels required */
+	for ( ; (pasids != 0) && ((pasids - 1) & ~0x1ff); pasids >>= 9)
+		levels += 1;
+
+	return levels;
+}
+
+/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
+static int setup_gcr3_table(struct protection_domain *domain, int pasids)
+{
+	int levels = get_gcr3_levels(pasids);
+
+	if (levels > amd_iommu_max_glx_val)
+		return -EINVAL;
+
+	domain->gcr3_tbl = (void *)get_zeroed_page(GFP_ATOMIC);
+	if (domain->gcr3_tbl == NULL)
+		return -ENOMEM;
+
+	domain->glx      = levels;
+	domain->flags   |= PD_IOMMUV2_MASK;
+
+	amd_iommu_domain_update(domain);
+
+	return 0;
+}
+
 static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 			  struct protection_domain *domain, bool ats, bool ppr)
 {
@@ -2065,7 +2098,7 @@ static int protection_domain_init_v2(struct protection_domain *domain)
 
 	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
 
-	if (domain_enable_v2(domain, 1)) {
+	if (setup_gcr3_table(domain, 1)) {
 		domain_id_free(domain->id);
 		return -ENOMEM;
 	}
@@ -2515,30 +2548,6 @@ void amd_iommu_domain_direct_map(struct iommu_domain *dom)
 }
 EXPORT_SYMBOL(amd_iommu_domain_direct_map);
 
-/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
-static int domain_enable_v2(struct protection_domain *domain, int pasids)
-{
-	int levels;
-
-	/* Number of GCR3 table levels required */
-	for (levels = 0; (pasids - 1) & ~0x1ff; pasids >>= 9)
-		levels += 1;
-
-	if (levels > amd_iommu_max_glx_val)
-		return -EINVAL;
-
-	domain->gcr3_tbl = (void *)get_zeroed_page(GFP_ATOMIC);
-	if (domain->gcr3_tbl == NULL)
-		return -ENOMEM;
-
-	domain->glx      = levels;
-	domain->flags   |= PD_IOMMUV2_MASK;
-
-	amd_iommu_domain_update(domain);
-
-	return 0;
-}
-
 int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
 {
 	struct protection_domain *pdom = to_pdomain(dom);
@@ -2557,7 +2566,7 @@ int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
 		goto out;
 
 	if (!pdom->gcr3_tbl)
-		ret = domain_enable_v2(pdom, pasids);
+		ret = setup_gcr3_table(pdom, pasids);
 
 out:
 	spin_unlock_irqrestore(&pdom->lock, flags);
-- 
2.31.1

Re: [PATCH v2 06/16] iommu/amd: Introduce helper functions for managing GCR3 table

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:35:59AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Refactor domain_enable_v2() into helper functions for managing GCR3 table
> (i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
> subsequent patches. Also re-arrange code and remove forward declaration.

It makes alot of sense to change the name like this
 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 63 ++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 09749ad4445c..3c14c49d6d72 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -77,7 +77,6 @@ struct iommu_cmd {
>  struct kmem_cache *amd_iommu_irq_cache;
>  
>  static void detach_device(struct device *dev);
> -static int domain_enable_v2(struct protection_domain *domain, int pasids);
>  
>  /****************************************************************************
>   *
> @@ -1575,6 +1574,40 @@ static void free_gcr3_table(struct protection_domain *domain)
>  	free_page((unsigned long)domain->gcr3_tbl);
>  }
>  
> +static int get_gcr3_levels(int pasids)
> +{
> +	int levels = 0;
> +
> +	if (pasids == -1)
> +		return amd_iommu_max_glx_val;
> +
> +	/* Number of GCR3 table levels required */
> +	for ( ; (pasids != 0) && ((pasids - 1) & ~0x1ff); pasids >>= 9)
> +		levels += 1;

This can surely be a closed expression, something like:

DIV_ROUND_UP(get_count_order(pasids), 9)

?

> +
> +	return levels;
> +}
> +
> +/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
> +static int setup_gcr3_table(struct protection_domain *domain, int pasids)
> +{
> +	int levels = get_gcr3_levels(pasids);
> +
> +	if (levels > amd_iommu_max_glx_val)
> +		return -EINVAL;
> +
> +	domain->gcr3_tbl = (void *)get_zeroed_page(GFP_ATOMIC);

I gather it is recommended to just call kzalloc(PAGE_SIZE) now. And
really this shouldn't be PAGE_SIZE but some constant reflecting the
size of the HW's gcr3 levels.

Jason

Re: [PATCH v2 06/16] iommu/amd: Introduce helper functions for managing GCR3 table

[Vasant Hegde]

Hi Jason,

On 7/28/2023 7:39 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:35:59AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Refactor domain_enable_v2() into helper functions for managing GCR3 table
>> (i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
>> subsequent patches. Also re-arrange code and remove forward declaration.
> 
> It makes alot of sense to change the name like this
>  
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 63 ++++++++++++++++++++++-----------------
>>  1 file changed, 36 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 09749ad4445c..3c14c49d6d72 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -77,7 +77,6 @@ struct iommu_cmd {
>>  struct kmem_cache *amd_iommu_irq_cache;
>>  
>>  static void detach_device(struct device *dev);
>> -static int domain_enable_v2(struct protection_domain *domain, int pasids);
>>  
>>  /****************************************************************************
>>   *
>> @@ -1575,6 +1574,40 @@ static void free_gcr3_table(struct protection_domain *domain)
>>  	free_page((unsigned long)domain->gcr3_tbl);
>>  }
>>  
>> +static int get_gcr3_levels(int pasids)
>> +{
>> +	int levels = 0;
>> +
>> +	if (pasids == -1)
>> +		return amd_iommu_max_glx_val;
>> +
>> +	/* Number of GCR3 table levels required */
>> +	for ( ; (pasids != 0) && ((pasids - 1) & ~0x1ff); pasids >>= 9)
>> +		levels += 1;
> 
> This can surely be a closed expression, something like:
> 
> DIV_ROUND_UP(get_count_order(pasids), 9)

Yeah. We can use above macro. I will change in next version.

> 
> ?
> 
>> +
>> +	return levels;
>> +}
>> +
>> +/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
>> +static int setup_gcr3_table(struct protection_domain *domain, int pasids)
>> +{
>> +	int levels = get_gcr3_levels(pasids);
>> +
>> +	if (levels > amd_iommu_max_glx_val)
>> +		return -EINVAL;
>> +
>> +	domain->gcr3_tbl = (void *)get_zeroed_page(GFP_ATOMIC);
> 
> I gather it is recommended to just call kzalloc(PAGE_SIZE) now. And
> really this shouldn't be PAGE_SIZE but some constant reflecting the
> size of the HW's gcr3 levels.

I will use the helper function we have .

-Vasant

[PATCH v2 07/16] iommu/amd: Use struct protection_domain in helper functions

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

To simplify the unnecessary use of container_of() on the struct
iommu_domain to get the container structure.

No functional changes intended.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h     | 13 +++++++++----
 drivers/iommu/amd/io_pgtable_v2.c |  6 +++---
 drivers/iommu/amd/iommu.c         | 18 ++++--------------
 drivers/iommu/amd/iommu_v2.c      | 11 +++++------
 4 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6bf55de9fbeb..a5a350ee36fe 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -58,15 +58,15 @@ int amd_iommu_register_ppr_notifier(struct notifier_block *nb);
 int amd_iommu_unregister_ppr_notifier(struct notifier_block *nb);
 void amd_iommu_domain_direct_map(struct iommu_domain *dom);
 int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids);
-int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid, u64 address);
+int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address);
 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_complete(struct protection_domain *domain);
 void amd_iommu_domain_flush_tlb_pde(struct protection_domain *domain);
-int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid);
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
+int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid);
+int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
 			      unsigned long cr3);
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid);
+int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid);
 
 #ifdef CONFIG_IRQ_REMAP
 int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
@@ -134,6 +134,11 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
 	return page ? page_address(page) : NULL;
 }
 
+static inline struct protection_domain *to_pdomain(struct iommu_domain *dom)
+{
+	return container_of(dom, struct protection_domain, domain);
+}
+
 bool translation_pre_enabled(struct amd_iommu *iommu);
 bool amd_iommu_is_attach_deferred(struct device *dev);
 int __init add_special_device(u8 type, u8 id, u32 *devid, bool cmd_line);
diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index e9ef2e0a62f6..a4fa830eab54 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -274,9 +274,9 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
 out:
 	if (updated) {
 		if (count > 1)
-			amd_iommu_flush_tlb(&pdom->domain, 0);
+			amd_iommu_flush_tlb(pdom, 0);
 		else
-			amd_iommu_flush_page(&pdom->domain, 0, o_iova);
+			amd_iommu_flush_page(pdom, 0, o_iova);
 	}
 
 	if (mapped)
@@ -384,7 +384,7 @@ static struct io_pgtable *v2_alloc_pgtable(struct io_pgtable_cfg *cfg, void *coo
 	if (!pgtable->pgd)
 		return NULL;
 
-	ret = amd_iommu_domain_set_gcr3(&pdom->domain, 0, iommu_virt_to_phys(pgtable->pgd));
+	ret = amd_iommu_domain_set_gcr3(pdom, 0, iommu_virt_to_phys(pgtable->pgd));
 	if (ret)
 		goto err_free_pgd;
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 3c14c49d6d72..f5a05779573f 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -174,11 +174,6 @@ static struct amd_iommu *rlookup_amd_iommu(struct device *dev)
 	return __rlookup_amd_iommu(seg, PCI_SBDF_TO_DEVID(devid));
 }
 
-static struct protection_domain *to_pdomain(struct iommu_domain *dom)
-{
-	return container_of(dom, struct protection_domain, domain);
-}
-
 static struct iommu_dev_data *alloc_dev_data(struct amd_iommu *iommu, u16 devid)
 {
 	struct iommu_dev_data *dev_data;
@@ -2642,10 +2637,8 @@ static int __amd_iommu_flush_page(struct protection_domain *domain, u32 pasid,
 	return __flush_pasid(domain, pasid, address, false);
 }
 
-int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid,
-			 u64 address)
+int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address)
 {
-	struct protection_domain *domain = to_pdomain(dom);
 	unsigned long flags;
 	int ret;
 
@@ -2663,9 +2656,8 @@ static int __amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid)
 			     true);
 }
 
-int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid)
+int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid)
 {
-	struct protection_domain *domain = to_pdomain(dom);
 	unsigned long flags;
 	int ret;
 
@@ -2742,10 +2734,9 @@ static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
 	return __amd_iommu_flush_tlb(domain, pasid);
 }
 
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
+int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
 			      unsigned long cr3)
 {
-	struct protection_domain *domain = to_pdomain(dom);
 	unsigned long flags;
 	int ret;
 
@@ -2757,9 +2748,8 @@ int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
 }
 EXPORT_SYMBOL(amd_iommu_domain_set_gcr3);
 
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid)
+int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid)
 {
-	struct protection_domain *domain = to_pdomain(dom);
 	unsigned long flags;
 	int ret;
 
diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 6a423928d5b5..9bf114382c00 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -282,7 +282,7 @@ static void unbind_pasid(struct pasid_state *pasid_state)
 	smp_wmb();
 
 	/* After this the device/pasid can't access the mm anymore */
-	amd_iommu_domain_clear_gcr3(&dev_state->pdom->domain, pasid_state->pasid);
+	amd_iommu_domain_clear_gcr3(dev_state->pdom, pasid_state->pasid);
 
 	/* Make sure no more pending faults are in the queue */
 	flush_workqueue(iommu_wq);
@@ -368,10 +368,10 @@ static void mn_invalidate_range(struct mmu_notifier *mn,
 	dev_state   = pasid_state->device_state;
 
 	if ((start ^ (end - 1)) < PAGE_SIZE)
-		amd_iommu_flush_page(&dev_state->pdom->domain, pasid_state->pasid,
+		amd_iommu_flush_page(dev_state->pdom, pasid_state->pasid,
 				     start);
 	else
-		amd_iommu_flush_tlb(&dev_state->pdom->domain, pasid_state->pasid);
+		amd_iommu_flush_tlb(dev_state->pdom, pasid_state->pasid);
 }
 
 static void mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
@@ -650,7 +650,7 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, u32 pasid,
 	if (ret)
 		goto out_unregister;
 
-	ret = amd_iommu_domain_set_gcr3(&dev_state->pdom->domain, pasid,
+	ret = amd_iommu_domain_set_gcr3(dev_state->pdom, pasid,
 					__pa(pasid_state->mm->pgd));
 	if (ret)
 		goto out_clear_state;
@@ -784,8 +784,7 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 		goto out_free_states;
 
 	/* Retrieve new protection_domain that has just been allocated */
-	dev_state->pdom = container_of(domain,
-				       struct protection_domain, domain);
+	dev_state->pdom = to_pdomain(domain);
 
 	/* See iommu_is_default_domain() */
 	domain->type = IOMMU_DOMAIN_IDENTITY;
-- 
2.31.1

Re: [PATCH v2 07/16] iommu/amd: Use struct protection_domain in helper functions

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:00AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> To simplify the unnecessary use of container_of() on the struct
> iommu_domain to get the container structure.
> 
> No functional changes intended.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h     | 13 +++++++++----
>  drivers/iommu/amd/io_pgtable_v2.c |  6 +++---
>  drivers/iommu/amd/iommu.c         | 18 ++++--------------
>  drivers/iommu/amd/iommu_v2.c      | 11 +++++------
>  4 files changed, 21 insertions(+), 27 deletions(-)

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

Jason

[PATCH v2 08/16] iommu/amd: Do not set amd_iommu_pgtable in pass-through mode

[Vasant Hegde]

Since AMD IOMMU page table is not used in passthrough mode, switching to
v1 page table is not required.

Therefore, remove redundant amd_iommu_pgtable update and misleading
warning message.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/init.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 852e40b13d20..2b01dfde6cab 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2134,9 +2134,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 		    !iommu_feature(iommu, FEATURE_GT)) {
 			pr_warn("Cannot enable v2 page table for DMA-API. Fallback to v1.\n");
 			amd_iommu_pgtable = AMD_IOMMU_V1;
-		} else if (iommu_default_passthrough()) {
-			pr_warn("V2 page table doesn't support passthrough mode. Fallback to v1.\n");
-			amd_iommu_pgtable = AMD_IOMMU_V1;
 		}
 	}
 
-- 
2.31.1

Re: [PATCH v2 08/16] iommu/amd: Do not set amd_iommu_pgtable in pass-through mode

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:01AM +0000, Vasant Hegde wrote:
> Since AMD IOMMU page table is not used in passthrough mode, switching to
> v1 page table is not required.
> 
> Therefore, remove redundant amd_iommu_pgtable update and misleading
> warning message.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/init.c | 3 ---
>  1 file changed, 3 deletions(-)

I never understood what this code was for, happy to see it go

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

Jason

Re: [PATCH v2 08/16] iommu/amd: Do not set amd_iommu_pgtable in pass-through mode

[Vasant Hegde]

Hi Jason,

On 7/28/2023 7:41 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:36:01AM +0000, Vasant Hegde wrote:
>> Since AMD IOMMU page table is not used in passthrough mode, switching to
>> v1 page table is not required.
>>
>> Therefore, remove redundant amd_iommu_pgtable update and misleading
>> warning message.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/init.c | 3 ---
>>  1 file changed, 3 deletions(-)
> 
> I never understood what this code was for, happy to see it go

It was a mistake from my side when I added v2 page table support!

-Vasant

[PATCH v2 09/16] iommu/amd: Miscellaneous clean up when free domain

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

* Use the protection_domain_free() helper function to free domain.
  The function has been modified to also free memory used for the v1 and v2
  page tables. Also clear gcr3 table in v2 page table free path.

* Refactor code into cleanup_domain() for reusability. Change BUG_ON to
  WARN_ON in cleanup path.

* Protection domain dev_cnt should be read when the domain is locked.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/io_pgtable_v2.c |  8 +++---
 drivers/iommu/amd/iommu.c         | 43 +++++++++++++++----------------
 2 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index a4fa830eab54..c17cda83bca5 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -363,10 +363,10 @@ static void v2_free_pgtable(struct io_pgtable *iop)
 	if (!(pdom->flags & PD_IOMMUV2_MASK))
 		return;
 
-	/*
-	 * Make changes visible to IOMMUs. No need to clear gcr3 entry
-	 * as gcr3 table is already freed.
-	 */
+	/* Clear gcr3 entry */
+	amd_iommu_domain_clear_gcr3(pdom, 0);
+
+	/* Make changes visible to IOMMUs */
 	amd_iommu_domain_update(pdom);
 
 	/* Free page table */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index f5a05779573f..a0b0deb6fbcb 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2037,12 +2037,13 @@ void amd_iommu_domain_update(struct protection_domain *domain)
  *
  *****************************************************************************/
 
+/* Must be called with domain->lock held */
 static void cleanup_domain(struct protection_domain *domain)
 {
 	struct iommu_dev_data *entry;
-	unsigned long flags;
 
-	spin_lock_irqsave(&domain->lock, flags);
+	if (!domain->dev_cnt)
+		return;
 
 	while (!list_empty(&domain->dev_list)) {
 		entry = list_first_entry(&domain->dev_list,
@@ -2050,8 +2051,7 @@ static void cleanup_domain(struct protection_domain *domain)
 		BUG_ON(!entry->domain);
 		do_detach(entry);
 	}
-
-	spin_unlock_irqrestore(&domain->lock, flags);
+	WARN_ON(domain->dev_cnt != 0);
 }
 
 static void protection_domain_free(struct protection_domain *domain)
@@ -2062,6 +2062,12 @@ static void protection_domain_free(struct protection_domain *domain)
 	if (domain->iop.pgtbl_cfg.tlb)
 		free_io_pgtable_ops(&domain->iop.iop.ops);
 
+	if (domain->flags & PD_IOMMUV2_MASK)
+		free_gcr3_table(domain);
+
+	if (domain->iop.root)
+		free_page((unsigned long)domain->iop.root);
+
 	if (domain->id)
 		domain_id_free(domain->id);
 
@@ -2076,10 +2082,8 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 
 	if (mode != PAGE_MODE_NONE) {
 		pt_root = (void *)get_zeroed_page(GFP_KERNEL);
-		if (!pt_root) {
-			domain_id_free(domain->id);
+		if (!pt_root)
 			return -ENOMEM;
-		}
 	}
 
 	amd_iommu_domain_set_pgtable(domain, pt_root, mode);
@@ -2093,10 +2097,8 @@ static int protection_domain_init_v2(struct protection_domain *domain)
 
 	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
 
-	if (setup_gcr3_table(domain, 1)) {
-		domain_id_free(domain->id);
+	if (setup_gcr3_table(domain, 1))
 		return -ENOMEM;
-	}
 
 	return 0;
 }
@@ -2156,14 +2158,12 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
 		goto out_err;
 
 	pgtbl_ops = alloc_io_pgtable_ops(pgtable, &domain->iop.pgtbl_cfg, domain);
-	if (!pgtbl_ops) {
-		domain_id_free(domain->id);
+	if (!pgtbl_ops)
 		goto out_err;
-	}
 
 	return domain;
 out_err:
-	kfree(domain);
+	protection_domain_free(domain);
 	return NULL;
 }
 
@@ -2201,19 +2201,18 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 static void amd_iommu_domain_free(struct iommu_domain *dom)
 {
 	struct protection_domain *domain;
+	unsigned long flags;
 
-	domain = to_pdomain(dom);
+	if (!dom)
+		return;
 
-	if (domain->dev_cnt > 0)
-		cleanup_domain(domain);
+	domain = to_pdomain(dom);
 
-	BUG_ON(domain->dev_cnt != 0);
+	spin_lock_irqsave(&domain->lock, flags);
 
-	if (!dom)
-		return;
+	cleanup_domain(domain);
 
-	if (domain->flags & PD_IOMMUV2_MASK)
-		free_gcr3_table(domain);
+	spin_unlock_irqrestore(&domain->lock, flags);
 
 	protection_domain_free(domain);
 }
-- 
2.31.1

Re: [PATCH v2 09/16] iommu/amd: Miscellaneous clean up when free domain

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:02AM +0000, Vasant Hegde wrote:

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index f5a05779573f..a0b0deb6fbcb 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2037,12 +2037,13 @@ void amd_iommu_domain_update(struct protection_domain *domain)
>   *
>   *****************************************************************************/
>  
> +/* Must be called with domain->lock held */

Don't write locking assertions in comments. Put a
lockdep_assert_held() at the top of the function.

Jason

Re: [PATCH v2 09/16] iommu/amd: Miscellaneous clean up when free domain

[Vasant Hegde]

Hi Jason,

On 7/28/2023 7:43 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:36:02AM +0000, Vasant Hegde wrote:
> 
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index f5a05779573f..a0b0deb6fbcb 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2037,12 +2037,13 @@ void amd_iommu_domain_update(struct protection_domain *domain)
>>   *
>>   *****************************************************************************/
>>  
>> +/* Must be called with domain->lock held */
> 
> Don't write locking assertions in comments. Put a
> lockdep_assert_held() at the top of the function.

Ack. Will fix it in next version.

-Vasant

[PATCH v2 10/16] iommu/amd: Modify logic for checking GT and PPR features

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

In order to support v2 page table, IOMMU driver need to check if the
hardware can support Guest Translation (GT) and Peripheral Page Requet
(PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
features. There variables area redundant since we could simply just check
the global EFR mask.

Therefore, replace it with a helper function with appropriate name.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h       | 11 +++++++++++
 drivers/iommu/amd/amd_iommu_types.h |  9 ++++-----
 drivers/iommu/amd/init.c            | 14 +-------------
 drivers/iommu/amd/iommu.c           |  2 +-
 4 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index a5a350ee36fe..0605f02fa711 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -95,6 +95,17 @@ static inline bool iommu_feature(struct amd_iommu *iommu, u64 mask)
 	return !!(iommu->features & mask);
 }
 
+static inline bool check_feature_on_all_iommus(u64 mask)
+{
+	return !!(amd_iommu_efr & mask);
+}
+
+static inline bool amd_iommu_gt_ppr_supported(void)
+{
+	return (check_feature_on_all_iommus(FEATURE_GT) &&
+		check_feature_on_all_iommus(FEATURE_PPR));
+}
+
 static inline u64 iommu_virt_to_phys(void *vaddr)
 {
 	return (u64)__sme_set(virt_to_phys(vaddr));
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 13c582a2454a..bbb5ac4f0d39 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -484,6 +484,10 @@ extern bool amdr_ivrs_remap_support;
 /* kmem_cache to get tables with 128 byte alignement */
 extern struct kmem_cache *amd_iommu_irq_cache;
 
+/* Global EFR and EFR2 registers */
+extern u64 amd_iommu_efr;
+extern u64 amd_iommu_efr2;
+
 #define PCI_SBDF_TO_SEGID(sbdf)		(((sbdf) >> 16) & 0xffff)
 #define PCI_SBDF_TO_DEVID(sbdf)		((sbdf) & 0xffff)
 #define PCI_SEG_DEVID_TO_SBDF(seg, devid)	((((u32)(seg) & 0xffff) << 16) | \
@@ -679,9 +683,6 @@ struct amd_iommu {
 	/* Extended features 2 */
 	u64 features2;
 
-	/* IOMMUv2 */
-	bool is_iommu_v2;
-
 	/* PCI device id of the IOMMU device */
 	u16 devid;
 
@@ -890,8 +891,6 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
 /* Smallest max PASID supported by any IOMMU in the system */
 extern u32 amd_iommu_max_pasid;
 
-extern bool amd_iommu_v2_present;
-
 extern bool amd_iommu_force_isolation;
 
 /* Max levels of glxval supported */
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 2b01dfde6cab..1f56478ae74e 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -187,7 +187,6 @@ bool amd_iommu_iotlb_sup __read_mostly = true;
 
 u32 amd_iommu_max_pasid __read_mostly = ~0;
 
-bool amd_iommu_v2_present __read_mostly;
 static bool amd_iommu_pc_present __read_mostly;
 bool amdr_ivrs_remap_support __read_mostly;
 
@@ -302,11 +301,6 @@ static void get_global_efr(void)
 	pr_info("Using global IVHD EFR:%#llx, EFR2:%#llx\n", amd_iommu_efr, amd_iommu_efr2);
 }
 
-static bool check_feature_on_all_iommus(u64 mask)
-{
-	return !!(amd_iommu_efr & mask);
-}
-
 static inline int check_feature_gpt_level(void)
 {
 	return ((amd_iommu_efr >> FEATURE_GATS_SHIFT) & FEATURE_GATS_MASK);
@@ -2112,12 +2106,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 			amd_iommu_max_glx_val = min(amd_iommu_max_glx_val, glxval);
 	}
 
-	if (iommu_feature(iommu, FEATURE_GT) &&
-	    iommu_feature(iommu, FEATURE_PPR)) {
-		iommu->is_iommu_v2   = true;
-		amd_iommu_v2_present = true;
-	}
-
 	if (iommu_feature(iommu, FEATURE_PPR) && alloc_ppr_log(iommu))
 		return -ENOMEM;
 
@@ -3693,7 +3681,7 @@ bool amd_iommu_v2_supported(void)
 	 * (i.e. EFR[SNPSup]=1), IOMMUv2 page table cannot be used without
 	 * setting up IOMMUv1 page table.
 	 */
-	return amd_iommu_v2_present && !amd_iommu_snp_en;
+	return amd_iommu_gt_ppr_supported() && !amd_iommu_snp_en;
 }
 EXPORT_SYMBOL(amd_iommu_v2_supported);
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index a0b0deb6fbcb..1f707944b23f 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -392,7 +392,7 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
 	 */
 	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
 	    dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
-		dev_data->iommu_v2 = iommu->is_iommu_v2;
+		dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();
 	}
 
 	dev_iommu_priv_set(dev, dev_data);
-- 
2.31.1

Re: [PATCH v2 10/16] iommu/amd: Modify logic for checking GT and PPR features

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:03AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> In order to support v2 page table, IOMMU driver need to check if the
> hardware can support Guest Translation (GT) and Peripheral Page Requet
> (PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
> and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
> features. There variables area redundant since we could simply just check
> the global EFR mask.
> 
> Therefore, replace it with a helper function with appropriate name.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h       | 11 +++++++++++
>  drivers/iommu/amd/amd_iommu_types.h |  9 ++++-----
>  drivers/iommu/amd/init.c            | 14 +-------------
>  drivers/iommu/amd/iommu.c           |  2 +-
>  4 files changed, 17 insertions(+), 19 deletions(-)

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index a0b0deb6fbcb..1f707944b23f 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -392,7 +392,7 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
>  	 */
>  	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
>  	    dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
> -		dev_data->iommu_v2 = iommu->is_iommu_v2;
> +		dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();

This doesn't make alot of sense to me, we should not have global
functions and data in drivers. In this case I would expect the driver
to consult the amd_iommu linked to the struct device it is working on
to determine the capability.

Eg just directly test:

	iommu_feature(iommu, FEATURE_GT) && iommu_feature(iommu, FEATURE_PPR)

And remove all the other copies of the iommu_v2

And arguably this can (eventually) be defered to an attach that
requires the gcr3 table.

Also, if the gcr3 table is supported or not should be entirely up to
the HW, policy inputs like "iommu_default_passthrough" and
"force_isolation" are nonsensical here..

Looking in the history this looks like a bodge to cover up the domain
type mismatch during allocation. We need to get to a point where the
device is known during domain allocation so the driver can provide a
v2 page table if the device could possibly use PASID and remove all
this hackery.

Jason

Re: [PATCH v2 10/16] iommu/amd: Modify logic for checking GT and PPR features

[Vasant Hegde]

On 7/28/2023 7:54 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:36:03AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> In order to support v2 page table, IOMMU driver need to check if the
>> hardware can support Guest Translation (GT) and Peripheral Page Requet
>> (PPR) features. Currently, IOMMU driver uses global (amd_iommu_v2_present)
>> and per-iommu (struct amd_iommu.is_iommu_v2) variables to track the
>> features. There variables area redundant since we could simply just check
>> the global EFR mask.
>>
>> Therefore, replace it with a helper function with appropriate name.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu.h       | 11 +++++++++++
>>  drivers/iommu/amd/amd_iommu_types.h |  9 ++++-----
>>  drivers/iommu/amd/init.c            | 14 +-------------
>>  drivers/iommu/amd/iommu.c           |  2 +-
>>  4 files changed, 17 insertions(+), 19 deletions(-)
> 
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index a0b0deb6fbcb..1f707944b23f 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -392,7 +392,7 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
>>  	 */
>>  	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
>>  	    dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
>> -		dev_data->iommu_v2 = iommu->is_iommu_v2;
>> +		dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();
> 
> This doesn't make alot of sense to me, we should not have global
> functions and data in drivers. In this case I would expect the driver
> to consult the amd_iommu linked to the struct device it is working on
> to determine the capability.
> 
> Eg just directly test:
> 
> 	iommu_feature(iommu, FEATURE_GT) && iommu_feature(iommu, FEATURE_PPR)
> 
> And remove all the other copies of the iommu_v2

Yes. Eventually we are removing iommu_v2 variable from both iommu and device
structures.

> 
> And arguably this can (eventually) be defered to an attach that
> requires the gcr3 table.

Yes. Later part of the SVA series does this. Basically init discovers the device
capabilities, attach/detach_device() takes care of enabling/disabling features.
Finally def_domain_type() decides best page table mode for the given device.

-Vasant

> 
> Also, if the gcr3 table is supported or not should be entirely up to
> the HW, policy inputs like "iommu_default_passthrough" and
> "force_isolation" are nonsensical here..
> 
> Looking in the history this looks like a bodge to cover up the domain
> type mismatch during allocation. We need to get to a point where the
> device is known during domain allocation so the driver can provide a
> v2 page table if the device could possibly use PASID and remove all
> this hackery.
> 
> Jason

[PATCH v2 11/16] iommu/amd: Rename ats related variables

[Vasant Hegde]

Remove nested structure and make it as 'ats_{enable/qdep}'.

No functional changes intended.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  6 ++----
 drivers/iommu/amd/iommu.c           | 26 +++++++++++++-------------
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index bbb5ac4f0d39..a066f375ba87 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -816,10 +816,8 @@ struct iommu_dev_data {
 	struct device *dev;
 	u16 devid;			  /* PCI Device ID */
 	bool iommu_v2;			  /* Device can make use of IOMMUv2 */
-	struct {
-		bool enabled;
-		int qdep;
-	} ats;				  /* ATS state */
+	int  ats_qdep;
+	bool ats_enabled;		  /* ATS state */
 	bool pri_tlp;			  /* PASID TLB required for
 					     PPR completions */
 	bool use_vapic;			  /* Enable device to use vapic mode */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 1f707944b23f..5a4170592bf4 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1309,7 +1309,7 @@ static int device_flush_iotlb(struct iommu_dev_data *dev_data,
 	struct iommu_cmd cmd;
 	int qdep;
 
-	qdep     = dev_data->ats.qdep;
+	qdep     = dev_data->ats_qdep;
 	iommu    = rlookup_amd_iommu(dev_data->dev);
 	if (!iommu)
 		return -EINVAL;
@@ -1360,7 +1360,7 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
 			return ret;
 	}
 
-	if (dev_data->ats.enabled)
+	if (dev_data->ats_enabled)
 		ret = device_flush_iotlb(dev_data, 0, ~0UL);
 
 	return ret;
@@ -1393,7 +1393,7 @@ static void __domain_flush_pages(struct protection_domain *domain,
 
 	list_for_each_entry(dev_data, &domain->dev_list, list) {
 
-		if (!dev_data->ats.enabled)
+		if (!dev_data->ats_enabled)
 			continue;
 
 		ret |= device_flush_iotlb(dev_data, address, size);
@@ -1711,7 +1711,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	iommu = rlookup_amd_iommu(dev_data->dev);
 	if (!iommu)
 		return;
-	ats   = dev_data->ats.enabled;
+	ats   = dev_data->ats_enabled;
 
 	/* Update data structures */
 	dev_data->domain = domain;
@@ -1849,14 +1849,14 @@ static int attach_device(struct device *dev,
 			if (pdev_pri_ats_enable(pdev) != 0)
 				goto out;
 
-			dev_data->ats.enabled = true;
-			dev_data->ats.qdep    = pci_ats_queue_depth(pdev);
+			dev_data->ats_enabled = true;
+			dev_data->ats_qdep    = pci_ats_queue_depth(pdev);
 			dev_data->pri_tlp     = pci_prg_resp_pasid_required(pdev);
 		}
 	} else if (amd_iommu_iotlb_sup &&
 		   pci_enable_ats(pdev, PAGE_SHIFT) == 0) {
-		dev_data->ats.enabled = true;
-		dev_data->ats.qdep    = pci_ats_queue_depth(pdev);
+		dev_data->ats_enabled = true;
+		dev_data->ats_qdep    = pci_ats_queue_depth(pdev);
 	}
 
 skip_ats_check:
@@ -1913,10 +1913,10 @@ static void detach_device(struct device *dev)
 
 	if (domain->flags & PD_IOMMUV2_MASK && dev_data->iommu_v2)
 		pdev_iommuv2_disable(to_pci_dev(dev));
-	else if (dev_data->ats.enabled)
+	else if (dev_data->ats_enabled)
 		pci_disable_ats(to_pci_dev(dev));
 
-	dev_data->ats.enabled = false;
+	dev_data->ats_enabled = false;
 
 out:
 	spin_unlock(&dev_data->lock);
@@ -2006,7 +2006,7 @@ static void update_device_table(struct protection_domain *domain)
 		if (!iommu)
 			continue;
 		set_dte_entry(iommu, dev_data->devid, domain,
-			      dev_data->ats.enabled, dev_data->iommu_v2);
+			      dev_data->ats_enabled, dev_data->iommu_v2);
 		clone_aliases(iommu, dev_data->dev);
 	}
 }
@@ -2605,10 +2605,10 @@ static int __flush_pasid(struct protection_domain *domain, u32 pasid,
 		   There might be non-IOMMUv2 capable devices in an IOMMUv2
 		 * domain.
 		 */
-		if (!dev_data->ats.enabled)
+		if (!dev_data->ats_enabled)
 			continue;
 
-		qdep  = dev_data->ats.qdep;
+		qdep  = dev_data->ats_qdep;
 		iommu = rlookup_amd_iommu(dev_data->dev);
 		if (!iommu)
 			continue;
-- 
2.31.1

Re: [PATCH v2 11/16] iommu/amd: Rename ats related variables

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:04AM +0000, Vasant Hegde wrote:
> Remove nested structure and make it as 'ats_{enable/qdep}'.
> 
> No functional changes intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  6 ++----
>  drivers/iommu/amd/iommu.c           | 26 +++++++++++++-------------
>  2 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index bbb5ac4f0d39..a066f375ba87 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -816,10 +816,8 @@ struct iommu_dev_data {
>  	struct device *dev;
>  	u16 devid;			  /* PCI Device ID */
>  	bool iommu_v2;			  /* Device can make use of IOMMUv2 */
> -	struct {
> -		bool enabled;
> -		int qdep;
> -	} ats;				  /* ATS state */
> +	int  ats_qdep;
> +	bool ats_enabled;		  /* ATS state */
>  	bool pri_tlp;			  /* PASID TLB required for
>  					     PPR completions */
>  	bool use_vapic;			  /* Enable device to use vapic mode */

Linus does not like lists of bools like this, you should try to write
it as

u8 ats_enabled:1;
u8 pri_tlp:1;

etc

Otherwise it looks good

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

Jason

Re: [PATCH v2 11/16] iommu/amd: Rename ats related variables

[Vasant Hegde]

Hi Jason,

On 7/28/2023 7:57 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:36:04AM +0000, Vasant Hegde wrote:
>> Remove nested structure and make it as 'ats_{enable/qdep}'.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu_types.h |  6 ++----
>>  drivers/iommu/amd/iommu.c           | 26 +++++++++++++-------------
>>  2 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index bbb5ac4f0d39..a066f375ba87 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -816,10 +816,8 @@ struct iommu_dev_data {
>>  	struct device *dev;
>>  	u16 devid;			  /* PCI Device ID */
>>  	bool iommu_v2;			  /* Device can make use of IOMMUv2 */
>> -	struct {
>> -		bool enabled;
>> -		int qdep;
>> -	} ats;				  /* ATS state */
>> +	int  ats_qdep;
>> +	bool ats_enabled;		  /* ATS state */
>>  	bool pri_tlp;			  /* PASID TLB required for
>>  					     PPR completions */
>>  	bool use_vapic;			  /* Enable device to use vapic mode */
> 
> Linus does not like lists of bools like this, you should try to write
> it as
> 
> u8 ats_enabled:1;
> u8 pri_tlp:1;\

Ack. Will fix it in next version.

Thanks
-Vasant

> 
> etc
> 
> Otherwise it looks good
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Jason

[PATCH v2 12/16] iommu/amd: Add support for different types of PPR handler

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

For AMD IOMMU, the PPR feature is needed to support IO page fault (IOPF).
PPR is enabled per PCI end-point device, and is configured by the PPR bit
in the IOMMU device table entry (i.e DTE[PPR]).

Currently, PPR is enabled for a device when it is initialized for AMD
IOMMU v2API. To support paging for IOPF, the DTE[PPR] bit needs to be
updated when enable/disable IOPF feature.

Introducing struct iommu_dev_data.ppr and enum ppr_handlers to track PPR
setting for each device.

Finally iommu_dev_data.ppr is set only when IOMMU supports PPR. Hence
remove redundant feature support check in set_dte_entry().

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  7 +++++++
 drivers/iommu/amd/iommu.c           | 10 ++++------
 drivers/iommu/amd/iommu_v2.c        | 14 +++++++++++++-
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index a066f375ba87..321d361dfb60 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -803,6 +803,12 @@ struct devid_map {
 	bool cmd_line;
 };
 
+enum ppr_handlers {
+	PPR_HANDLER_NONE,	/* No handler specified */
+	PPR_HANDLER_V2API,	/* IOMMU v2 API ppr handler */
+	PPR_HANDLER_IOPF,	/* IOPF ppr handler */
+};
+
 /*
  * This struct contains device specific data for the IOMMU
  */
@@ -820,6 +826,7 @@ struct iommu_dev_data {
 	bool ats_enabled;		  /* ATS state */
 	bool pri_tlp;			  /* PASID TLB required for
 					     PPR completions */
+	enum ppr_handlers ppr;
 	bool use_vapic;			  /* Enable device to use vapic mode */
 	bool defer_attach;
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 5a4170592bf4..e67c6fae452c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1631,10 +1631,8 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 	if (ats)
 		flags |= DTE_FLAG_IOTLB;
 
-	if (ppr) {
-		if (iommu_feature(iommu, FEATURE_EPHSUP))
-			pte_root |= 1ULL << DEV_ENTRY_PPR;
-	}
+	if (ppr)
+		pte_root |= 1ULL << DEV_ENTRY_PPR;
 
 	if (domain->flags & PD_IOMMUV2_MASK) {
 		u64 gcr3 = iommu_virt_to_phys(domain->gcr3_tbl);
@@ -1727,7 +1725,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
 
 	/* Update device table */
 	set_dte_entry(iommu, dev_data->devid, domain,
-		      ats, dev_data->iommu_v2);
+		      ats, dev_data->ppr);
 	clone_aliases(iommu, dev_data->dev);
 
 	device_flush_dte(dev_data);
@@ -2006,7 +2004,7 @@ static void update_device_table(struct protection_domain *domain)
 		if (!iommu)
 			continue;
 		set_dte_entry(iommu, dev_data->devid, domain,
-			      dev_data->ats_enabled, dev_data->iommu_v2);
+			      dev_data->ats_enabled, dev_data->ppr);
 		clone_aliases(iommu, dev_data->dev);
 	}
 }
diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 9bf114382c00..4148cbf069dc 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -740,6 +740,10 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 	unsigned long flags;
 	int ret, tmp;
 	u32 sbdf;
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+
+	if (!dev_data)
+		return -ENODEV;
 
 	might_sleep();
 
@@ -762,6 +766,9 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 	if (dev_state == NULL)
 		return -ENOMEM;
 
+	/* Enable device for PPR support */
+	dev_data->ppr = PPR_HANDLER_V2API;
+
 	spin_lock_init(&dev_state->lock);
 	init_waitqueue_head(&dev_state->wq);
 	dev_state->pdev  = pdev;
@@ -832,6 +839,8 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 out_free_dev_state:
 	kfree(dev_state);
 
+	dev_data->ppr = PPR_HANDLER_NONE;
+
 	return ret;
 }
 EXPORT_SYMBOL(amd_iommu_init_device);
@@ -841,8 +850,9 @@ void amd_iommu_free_device(struct pci_dev *pdev)
 	struct device_state *dev_state;
 	unsigned long flags;
 	u32 sbdf;
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
 
-	if (!amd_iommu_v2_supported())
+	if (!dev_data || !amd_iommu_v2_supported())
 		return;
 
 	sbdf = get_pci_sbdf_id(pdev);
@@ -859,6 +869,8 @@ void amd_iommu_free_device(struct pci_dev *pdev)
 
 	spin_unlock_irqrestore(&state_lock, flags);
 
+	dev_data->ppr = PPR_HANDLER_NONE;
+
 	put_device_state(dev_state);
 	free_device_state(dev_state);
 }
-- 
2.31.1

Re: [PATCH v2 12/16] iommu/amd: Add support for different types of PPR handler

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:05AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> For AMD IOMMU, the PPR feature is needed to support IO page fault (IOPF).
> PPR is enabled per PCI end-point device, and is configured by the PPR bit
> in the IOMMU device table entry (i.e DTE[PPR]).
> 
> Currently, PPR is enabled for a device when it is initialized for AMD
> IOMMU v2API. To support paging for IOPF, the DTE[PPR] bit needs to be
> updated when enable/disable IOPF feature.
> 
> Introducing struct iommu_dev_data.ppr and enum ppr_handlers to track PPR
> setting for each device.
> 
> Finally iommu_dev_data.ppr is set only when IOMMU supports PPR. Hence
> remove redundant feature support check in set_dte_entry().
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  7 +++++++
>  drivers/iommu/amd/iommu.c           | 10 ++++------
>  drivers/iommu/amd/iommu_v2.c        | 14 +++++++++++++-
>  3 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index a066f375ba87..321d361dfb60 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -803,6 +803,12 @@ struct devid_map {
>  	bool cmd_line;
>  };
>  
> +enum ppr_handlers {
> +	PPR_HANDLER_NONE,	/* No handler specified */
> +	PPR_HANDLER_V2API,	/* IOMMU v2 API ppr handler */
> +	PPR_HANDLER_IOPF,	/* IOPF ppr handler */

This constant is never used, move it to the patch that uses it.

Why are you doing this? It would be much better to hook the GPU driver
into the standard API, what prevents that?

Jason

Re: [PATCH v2 12/16] iommu/amd: Add support for different types of PPR handler

[Vasant Hegde]

Hi Jason,

On 7/28/2023 8:01 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:36:05AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> For AMD IOMMU, the PPR feature is needed to support IO page fault (IOPF).
>> PPR is enabled per PCI end-point device, and is configured by the PPR bit
>> in the IOMMU device table entry (i.e DTE[PPR]).
>>
>> Currently, PPR is enabled for a device when it is initialized for AMD
>> IOMMU v2API. To support paging for IOPF, the DTE[PPR] bit needs to be
>> updated when enable/disable IOPF feature.
>>
>> Introducing struct iommu_dev_data.ppr and enum ppr_handlers to track PPR
>> setting for each device.
>>
>> Finally iommu_dev_data.ppr is set only when IOMMU supports PPR. Hence
>> remove redundant feature support check in set_dte_entry().
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu_types.h |  7 +++++++
>>  drivers/iommu/amd/iommu.c           | 10 ++++------
>>  drivers/iommu/amd/iommu_v2.c        | 14 +++++++++++++-
>>  3 files changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index a066f375ba87..321d361dfb60 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -803,6 +803,12 @@ struct devid_map {
>>  	bool cmd_line;
>>  };
>>  
>> +enum ppr_handlers {
>> +	PPR_HANDLER_NONE,	/* No handler specified */
>> +	PPR_HANDLER_V2API,	/* IOMMU v2 API ppr handler */
>> +	PPR_HANDLER_IOPF,	/* IOPF ppr handler */
> 
> This constant is never used, move it to the patch that uses it.

Sure. I will move IOPF macro to later patch series.

> 
> Why are you doing this? It would be much better to hook the GPU driver
> into the standard API, what prevents that?

Because once we implement IOPF support we will have two different path. Hence
adding variable to track PPR.

For GPUs, yes. We will look into the option of converting them to use generic
SVA interfaces.

-Vasant

Re: [PATCH v2 12/16] iommu/amd: Add support for different types of PPR handler

[Jason Gunthorpe]

On Mon, Jul 31, 2023 at 01:32:05PM +0530, Vasant Hegde wrote:

> >> +enum ppr_handlers {
> >> +	PPR_HANDLER_NONE,	/* No handler specified */
> >> +	PPR_HANDLER_V2API,	/* IOMMU v2 API ppr handler */
> >> +	PPR_HANDLER_IOPF,	/* IOPF ppr handler */
> > 
> > This constant is never used, move it to the patch that uses it.
> 
> Sure. I will move IOPF macro to later patch series.
> 
> > 
> > Why are you doing this? It would be much better to hook the GPU driver
> > into the standard API, what prevents that?
> 
> Because once we implement IOPF support we will have two different path. Hence
> adding variable to track PPR.

You should try very hard to avoid having two different paths.

Jason

Re: [PATCH v2 12/16] iommu/amd: Add support for different types of PPR handler

[Vasant Hegde]

Hi Jason,


On 7/31/2023 5:41 PM, Jason Gunthorpe wrote:
> On Mon, Jul 31, 2023 at 01:32:05PM +0530, Vasant Hegde wrote:
> 
>>>> +enum ppr_handlers {
>>>> +	PPR_HANDLER_NONE,	/* No handler specified */
>>>> +	PPR_HANDLER_V2API,	/* IOMMU v2 API ppr handler */
>>>> +	PPR_HANDLER_IOPF,	/* IOPF ppr handler */
>>>
>>> This constant is never used, move it to the patch that uses it.
>>
>> Sure. I will move IOPF macro to later patch series.
>>
>>>
>>> Why are you doing this? It would be much better to hook the GPU driver
>>> into the standard API, what prevents that?
>>
>> Because once we implement IOPF support we will have two different path. Hence
>> adding variable to track PPR.
> 
> You should try very hard to avoid having two different paths.

Yes. I think eventually we will get their.

-Vasant

[PATCH v2 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities

[Vasant Hegde]

Currently we use struct iommu_dev_data.iommu_v2 to keep track of the device
ATS, PRI, and PASID capabilities. But these capabilities can be enabled
independently (except PRI requires ATS support). Hence, replace
the iommu_v2 variable with a flags variable, which keep track of the device
capabilities.

Device PRI/PASID is shared between PF and any associated VFs (See commit
9bf49e36d718 ("PCI/ATS: Handle sharing of PF PRI Capability with
all VFs")). Hence use pci_pri_supported() and pci_pasid_features()
instead of pci_find_ext_capability() to check device PRI/PASID support.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  2 +-
 drivers/iommu/amd/iommu.c           | 39 +++++++++++++++--------------
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 321d361dfb60..57c74870b17f 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -821,7 +821,7 @@ struct iommu_dev_data {
 	struct protection_domain *domain; /* Domain the device is bound to */
 	struct device *dev;
 	u16 devid;			  /* PCI Device ID */
-	bool iommu_v2;			  /* Device can make use of IOMMUv2 */
+	u32 flags;			  /* Holds AMD_IOMMU_DEVICE_FLAG_<*> */
 	int  ats_qdep;
 	bool ats_enabled;		  /* ATS state */
 	bool pri_tlp;			  /* PASID TLB required for
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index e67c6fae452c..7f67e8991949 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -314,24 +314,25 @@ static struct iommu_group *acpihid_device_group(struct device *dev)
 	return entry->group;
 }
 
-static bool pci_iommuv2_capable(struct pci_dev *pdev)
+static inline bool pdev_pasid_supported(struct iommu_dev_data *dev_data)
 {
-	static const int caps[] = {
-		PCI_EXT_CAP_ID_PRI,
-		PCI_EXT_CAP_ID_PASID,
-	};
-	int i, pos;
+	return (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PASID_SUP);
+}
 
-	if (!pci_ats_supported(pdev))
-		return false;
+static u32 pdev_get_caps(struct pci_dev *pdev)
+{
+	u32 flags = 0;
 
-	for (i = 0; i < 2; ++i) {
-		pos = pci_find_ext_capability(pdev, caps[i]);
-		if (pos == 0)
-			return false;
-	}
+	if (pci_ats_supported(pdev))
+		flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP;
 
-	return true;
+	if (pci_pri_supported(pdev))
+		flags |= AMD_IOMMU_DEVICE_FLAG_PRI_SUP;
+
+	if (pci_pasid_features(pdev) >= 0)
+		flags |= AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
+
+	return flags;
 }
 
 /*
@@ -391,8 +392,8 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
 	 * it'll be forced to go into translation mode.
 	 */
 	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
-	    dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
-		dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();
+	    dev_is_pci(dev) && amd_iommu_gt_ppr_supported()) {
+		dev_data->flags = pdev_get_caps(to_pci_dev(dev));
 	}
 
 	dev_iommu_priv_set(dev, dev_data);
@@ -1843,7 +1844,7 @@ static int attach_device(struct device *dev,
 			goto out;
 		}
 
-		if (dev_data->iommu_v2) {
+		if (pdev_pasid_supported(dev_data)) {
 			if (pdev_pri_ats_enable(pdev) != 0)
 				goto out;
 
@@ -1909,7 +1910,7 @@ static void detach_device(struct device *dev)
 	if (!dev_is_pci(dev))
 		goto out;
 
-	if (domain->flags & PD_IOMMUV2_MASK && dev_data->iommu_v2)
+	if (domain->flags & PD_IOMMUV2_MASK && pdev_pasid_supported(dev_data))
 		pdev_iommuv2_disable(to_pci_dev(dev));
 	else if (dev_data->ats_enabled)
 		pci_disable_ats(to_pci_dev(dev));
@@ -2464,7 +2465,7 @@ static int amd_iommu_def_domain_type(struct device *dev)
 	 *    and require remapping.
 	 *  - SNP is enabled, because it prohibits DTE[Mode]=0.
 	 */
-	if (dev_data->iommu_v2 &&
+	if (pdev_pasid_supported(dev_data) &&
 	    !cc_platform_has(CC_ATTR_MEM_ENCRYPT) &&
 	    !amd_iommu_snp_en) {
 		return IOMMU_DOMAIN_IDENTITY;
-- 
2.31.1

Re: [PATCH v2 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:06AM +0000, Vasant Hegde wrote:
> Currently we use struct iommu_dev_data.iommu_v2 to keep track of the device
> ATS, PRI, and PASID capabilities. But these capabilities can be enabled
> independently (except PRI requires ATS support). Hence, replace
> the iommu_v2 variable with a flags variable, which keep track of the device
> capabilities.
>
> Device PRI/PASID is shared between PF and any associated VFs (See commit
> 9bf49e36d718 ("PCI/ATS: Handle sharing of PF PRI Capability with
> all VFs")). Hence use pci_pri_supported() and pci_pasid_features()
> instead of pci_find_ext_capability() to check device PRI/PASID support.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  2 +-
>  drivers/iommu/amd/iommu.c           | 39 +++++++++++++++--------------
>  2 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 321d361dfb60..57c74870b17f 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -821,7 +821,7 @@ struct iommu_dev_data {
>  	struct protection_domain *domain; /* Domain the device is bound to */
>  	struct device *dev;
>  	u16 devid;			  /* PCI Device ID */
> -	bool iommu_v2;			  /* Device can make use of IOMMUv2 */
> +	u32 flags;			  /* Holds AMD_IOMMU_DEVICE_FLAG_<*> */
>  	int  ats_qdep;
>  	bool ats_enabled;		  /* ATS state */
>  	bool pri_tlp;			  /* PASID TLB required for
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index e67c6fae452c..7f67e8991949 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -314,24 +314,25 @@ static struct iommu_group *acpihid_device_group(struct device *dev)
>  	return entry->group;
>  }
>  
> -static bool pci_iommuv2_capable(struct pci_dev *pdev)
> +static inline bool pdev_pasid_supported(struct iommu_dev_data *dev_data)
>  {
> -	static const int caps[] = {
> -		PCI_EXT_CAP_ID_PRI,
> -		PCI_EXT_CAP_ID_PASID,
> -	};
> -	int i, pos;
> +	return (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PASID_SUP);
> +}
>  
> -	if (!pci_ats_supported(pdev))
> -		return false;
> +static u32 pdev_get_caps(struct pci_dev *pdev)
> +{
> +	u32 flags = 0;
>  
> -	for (i = 0; i < 2; ++i) {
> -		pos = pci_find_ext_capability(pdev, caps[i]);
> -		if (pos == 0)
> -			return false;
> -	}
> +	if (pci_ats_supported(pdev))
> +		flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP;
>  
> -	return true;
> +	if (pci_pri_supported(pdev))
> +		flags |= AMD_IOMMU_DEVICE_FLAG_PRI_SUP;
> +
> +	if (pci_pasid_features(pdev) >= 0)
> +		flags |= AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
> +
> +	return flags;
>  }
>  
>  /*
> @@ -391,8 +392,8 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
>  	 * it'll be forced to go into translation mode.
>  	 */
>  	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
> -	    dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
> -		dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();
> +	    dev_is_pci(dev) && amd_iommu_gt_ppr_supported()) {
> +		dev_data->flags = pdev_get_caps(to_pci_dev(dev));
>  	}

This is even more confusing now. Why would the flags rely on policy knobs??

> @@ -1843,7 +1844,7 @@ static int attach_device(struct device *dev,
>  			goto out;
>  		}
>  
> -		if (dev_data->iommu_v2) {
> +		if (pdev_pasid_supported(dev_data)) {
>  			if (pdev_pri_ats_enable(pdev) != 0)
>  				goto out;

ATS should be usuable without PASID support. ATS on a RID is perfectly
valid and is being used in the real world.
  
> @@ -1909,7 +1910,7 @@ static void detach_device(struct device *dev)
>  	if (!dev_is_pci(dev))
>  		goto out;
>  
> -	if (domain->flags & PD_IOMMUV2_MASK && dev_data->iommu_v2)
> +	if (domain->flags & PD_IOMMUV2_MASK && pdev_pasid_supported(dev_data))
>  		pdev_iommuv2_disable(to_pci_dev(dev));
>  	else if (dev_data->ats_enabled)
>  		pci_disable_ats(to_pci_dev(dev));
> @@ -2464,7 +2465,7 @@ static int amd_iommu_def_domain_type(struct device *dev)
>  	 *    and require remapping.
>  	 *  - SNP is enabled, because it prohibits DTE[Mode]=0.
>  	 */
> -	if (dev_data->iommu_v2 &&
> +	if (pdev_pasid_supported(dev_data) &&
>  	    !cc_platform_has(CC_ATTR_MEM_ENCRYPT) &&
>  	    !amd_iommu_snp_en) {
>  		return IOMMU_DOMAIN_IDENTITY;

This looks like a mess that needs fixing. PASID support should never
be a trigger for an IDENTITY default domain.

If GPUs are broken then match their PCI ID's and do the workaround
only for them.

Jason

Re: [PATCH v2 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities

[Vasant Hegde]

On 7/28/2023 8:08 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:36:06AM +0000, Vasant Hegde wrote:
>> Currently we use struct iommu_dev_data.iommu_v2 to keep track of the device
>> ATS, PRI, and PASID capabilities. But these capabilities can be enabled
>> independently (except PRI requires ATS support). Hence, replace
>> the iommu_v2 variable with a flags variable, which keep track of the device
>> capabilities.
>>
>> Device PRI/PASID is shared between PF and any associated VFs (See commit
>> 9bf49e36d718 ("PCI/ATS: Handle sharing of PF PRI Capability with
>> all VFs")). Hence use pci_pri_supported() and pci_pasid_features()
>> instead of pci_find_ext_capability() to check device PRI/PASID support.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu_types.h |  2 +-
>>  drivers/iommu/amd/iommu.c           | 39 +++++++++++++++--------------
>>  2 files changed, 21 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index 321d361dfb60..57c74870b17f 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -821,7 +821,7 @@ struct iommu_dev_data {
>>  	struct protection_domain *domain; /* Domain the device is bound to */
>>  	struct device *dev;
>>  	u16 devid;			  /* PCI Device ID */
>> -	bool iommu_v2;			  /* Device can make use of IOMMUv2 */
>> +	u32 flags;			  /* Holds AMD_IOMMU_DEVICE_FLAG_<*> */
>>  	int  ats_qdep;
>>  	bool ats_enabled;		  /* ATS state */
>>  	bool pri_tlp;			  /* PASID TLB required for
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index e67c6fae452c..7f67e8991949 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -314,24 +314,25 @@ static struct iommu_group *acpihid_device_group(struct device *dev)
>>  	return entry->group;
>>  }
>>  
>> -static bool pci_iommuv2_capable(struct pci_dev *pdev)
>> +static inline bool pdev_pasid_supported(struct iommu_dev_data *dev_data)
>>  {
>> -	static const int caps[] = {
>> -		PCI_EXT_CAP_ID_PRI,
>> -		PCI_EXT_CAP_ID_PASID,
>> -	};
>> -	int i, pos;
>> +	return (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PASID_SUP);
>> +}
>>  
>> -	if (!pci_ats_supported(pdev))
>> -		return false;
>> +static u32 pdev_get_caps(struct pci_dev *pdev)
>> +{
>> +	u32 flags = 0;
>>  
>> -	for (i = 0; i < 2; ++i) {
>> -		pos = pci_find_ext_capability(pdev, caps[i]);
>> -		if (pos == 0)
>> -			return false;
>> -	}
>> +	if (pci_ats_supported(pdev))
>> +		flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP;
>>  
>> -	return true;
>> +	if (pci_pri_supported(pdev))
>> +		flags |= AMD_IOMMU_DEVICE_FLAG_PRI_SUP;
>> +
>> +	if (pci_pasid_features(pdev) >= 0)
>> +		flags |= AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
>> +
>> +	return flags;
>>  }
>>  
>>  /*
>> @@ -391,8 +392,8 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
>>  	 * it'll be forced to go into translation mode.
>>  	 */
>>  	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
>> -	    dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
>> -		dev_data->iommu_v2 = amd_iommu_gt_ppr_supported();
>> +	    dev_is_pci(dev) && amd_iommu_gt_ppr_supported()) {
>> +		dev_data->flags = pdev_get_caps(to_pci_dev(dev));
>>  	}
> 
> This is even more confusing now. Why would the flags rely on policy knobs??

This patch adds support to track supported flags explicitly. We are not changing
existing behavior. That will be cleaned up with Part3.

> 
>> @@ -1843,7 +1844,7 @@ static int attach_device(struct device *dev,
>>  			goto out;
>>  		}
>>  
>> -		if (dev_data->iommu_v2) {
>> +		if (pdev_pasid_supported(dev_data)) {
>>  			if (pdev_pri_ats_enable(pdev) != 0)
>>  				goto out;
> 
> ATS should be usuable without PASID support. ATS on a RID is perfectly
> valid and is being used in the real world.

That's correct. Current code is bit confusing. We have follow up patch that will
cleanup {attach/detach}_device() code.

>   
>> @@ -1909,7 +1910,7 @@ static void detach_device(struct device *dev)
>>  	if (!dev_is_pci(dev))
>>  		goto out;
>>  
>> -	if (domain->flags & PD_IOMMUV2_MASK && dev_data->iommu_v2)
>> +	if (domain->flags & PD_IOMMUV2_MASK && pdev_pasid_supported(dev_data))
>>  		pdev_iommuv2_disable(to_pci_dev(dev));
>>  	else if (dev_data->ats_enabled)
>>  		pci_disable_ats(to_pci_dev(dev));
>> @@ -2464,7 +2465,7 @@ static int amd_iommu_def_domain_type(struct device *dev)
>>  	 *    and require remapping.
>>  	 *  - SNP is enabled, because it prohibits DTE[Mode]=0.
>>  	 */
>> -	if (dev_data->iommu_v2 &&
>> +	if (pdev_pasid_supported(dev_data) &&
>>  	    !cc_platform_has(CC_ATTR_MEM_ENCRYPT) &&
>>  	    !amd_iommu_snp_en) {
>>  		return IOMMU_DOMAIN_IDENTITY;
> 
> This looks like a mess that needs fixing. PASID support should never
> be a trigger for an IDENTITY default domain.
> 
> If GPUs are broken then match their PCI ID's and do the workaround
> only for them.

I had to dig git history to understand why its implemented like the way it is
now. IIUC it was not for broken GPU. We cannot switch from V1 page table to SVA
(iommu_v2 module). Hence they forced device to be in passthrough mode.

Part3 of SVA series cleans up this code.

-Vasant

Re: [PATCH v2 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities

[Jason Gunthorpe]

On Mon, Jul 31, 2023 at 01:27:49PM +0530, Vasant Hegde wrote:

> I had to dig git history to understand why its implemented like the way it is
> now. IIUC it was not for broken GPU. We cannot switch from V1 page table to SVA
> (iommu_v2 module). Hence they forced device to be in passthrough mode.

That has nothing to do with identity. It means you need to select v2
page table when working with the GPU.

We really need to get this cleaned up properly

Jason

Re: [PATCH v2 13/16] iommu/amd: Introduce iommu_dev_data.flags to track device capabilities

[Vasant Hegde]

Hi Jason,


On 7/31/2023 5:38 PM, Jason Gunthorpe wrote:
> On Mon, Jul 31, 2023 at 01:27:49PM +0530, Vasant Hegde wrote:
> 
>> I had to dig git history to understand why its implemented like the way it is
>> now. IIUC it was not for broken GPU. We cannot switch from V1 page table to SVA
>> (iommu_v2 module). Hence they forced device to be in passthrough mode.
> 
> That has nothing to do with identity. It means you need to select v2
> page table when working with the GPU.

Thats correct. GPU uses IOMMU v2 page table. But we cannot switch from V1 page
table to V2. Also we were not having v2 page table support when they added
iommu_v2 module.


> 
> We really need to get this cleaned up properly

I agree. We are heading to the direction of deprecating iommu_v2 module. And we
will cleanup boot mode domain allocation as well.

-Vasant

[PATCH v2 14/16] iommu/amd: Enable device ATS/PASID/PRI capabilities independently

[Vasant Hegde]

Introduce helper functions to enable/disable device ATS/PASID/PRI
capabilities independently along with the new pasid_enabled and
pri_enabled variables in struct iommu_dev_data to keep track,
which allows attach_device() and detach_device() to be simplified.

Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h       |   4 +
 drivers/iommu/amd/amd_iommu_types.h |   2 +
 drivers/iommu/amd/iommu.c           | 203 ++++++++++++++++------------
 3 files changed, 120 insertions(+), 89 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 0605f02fa711..56996dc2868d 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -54,6 +54,10 @@ int amd_iommu_pc_get_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
 int amd_iommu_pc_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
 			 u8 fxn, u64 *value);
 
+/* Device capabilities */
+int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev);
+void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev);
+
 int amd_iommu_register_ppr_notifier(struct notifier_block *nb);
 int amd_iommu_unregister_ppr_notifier(struct notifier_block *nb);
 void amd_iommu_domain_direct_map(struct iommu_domain *dom);
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 57c74870b17f..445b58c24d20 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -824,6 +824,8 @@ struct iommu_dev_data {
 	u32 flags;			  /* Holds AMD_IOMMU_DEVICE_FLAG_<*> */
 	int  ats_qdep;
 	bool ats_enabled;		  /* ATS state */
+	bool pri_enabled;		  /* PRI state */
+	bool pasid_enabled;		  /* PASID state */
 	bool pri_tlp;			  /* PASID TLB required for
 					     PPR completions */
 	enum ppr_handlers ppr;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 7f67e8991949..3b20d4d97192 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -335,6 +335,113 @@ static u32 pdev_get_caps(struct pci_dev *pdev)
 	return flags;
 }
 
+static inline int pdev_enable_cap_ats(struct pci_dev *pdev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+	int ret = -EINVAL;
+
+	if (dev_data->ats_enabled)
+		return 0;
+
+	if (amd_iommu_iotlb_sup &&
+	    (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_ATS_SUP)) {
+		ret = pci_enable_ats(pdev, PAGE_SHIFT);
+		if (!ret) {
+			dev_data->ats_enabled = true;
+			dev_data->ats_qdep    = pci_ats_queue_depth(pdev);
+		}
+	}
+
+	return ret;
+}
+
+static inline void pdev_disable_cap_ats(struct pci_dev *pdev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+
+	if (dev_data->ats_enabled) {
+		pci_disable_ats(pdev);
+		dev_data->ats_enabled = false;
+	}
+}
+
+int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+	int ret = -EINVAL;
+
+	if (dev_data->pri_enabled)
+		return 0;
+
+	if (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PRI_SUP) {
+		/*
+		 * First reset the PRI state of the device.
+		 * FIXME: Hardcode number of outstanding requests for now
+		 */
+		if (!pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32)) {
+			dev_data->pri_enabled = true;
+			dev_data->pri_tlp     = pci_prg_resp_pasid_required(pdev);
+
+			ret = 0;
+		}
+	}
+
+	return ret;
+}
+
+void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+
+	if (dev_data->pri_enabled) {
+		pci_disable_pri(pdev);
+		dev_data->pri_enabled = false;
+	}
+}
+
+static inline int pdev_enable_cap_pasid(struct pci_dev *pdev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+	int ret = -EINVAL;
+
+	if (dev_data->pasid_enabled)
+		return 0;
+
+	if (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PASID_SUP) {
+		/* Only allow access to user-accessible pages */
+		ret = pci_enable_pasid(pdev, 0);
+		if (!ret)
+			dev_data->pasid_enabled = true;
+	}
+
+	return ret;
+}
+
+static inline void pdev_disable_cap_pasid(struct pci_dev *pdev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(&pdev->dev);
+
+	if (dev_data->pasid_enabled) {
+		pci_disable_pasid(pdev);
+		dev_data->pasid_enabled = false;
+	}
+}
+
+static void pdev_enable_caps(struct pci_dev *pdev)
+{
+	pdev_enable_cap_ats(pdev);
+	pdev_enable_cap_pasid(pdev);
+	amd_iommu_pdev_enable_cap_pri(pdev);
+
+}
+
+static void pdev_disable_caps(struct pci_dev *pdev)
+{
+	pdev_disable_cap_ats(pdev);
+	pdev_disable_cap_pasid(pdev);
+	amd_iommu_pdev_disable_cap_pri(pdev);
+}
+
 /*
  * This function checks if the driver got a valid device from the caller to
  * avoid dereferencing invalid pointers.
@@ -1761,48 +1868,6 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	domain->dev_cnt                 -= 1;
 }
 
-static void pdev_iommuv2_disable(struct pci_dev *pdev)
-{
-	pci_disable_ats(pdev);
-	pci_disable_pri(pdev);
-	pci_disable_pasid(pdev);
-}
-
-static int pdev_pri_ats_enable(struct pci_dev *pdev)
-{
-	int ret;
-
-	/* Only allow access to user-accessible pages */
-	ret = pci_enable_pasid(pdev, 0);
-	if (ret)
-		return ret;
-
-	/* First reset the PRI state of the device */
-	ret = pci_reset_pri(pdev);
-	if (ret)
-		goto out_err_pasid;
-
-	/* Enable PRI */
-	/* FIXME: Hardcode number of outstanding requests for now */
-	ret = pci_enable_pri(pdev, 32);
-	if (ret)
-		goto out_err_pasid;
-
-	ret = pci_enable_ats(pdev, PAGE_SHIFT);
-	if (ret)
-		goto out_err_pri;
-
-	return 0;
-
-out_err_pri:
-	pci_disable_pri(pdev);
-
-out_err_pasid:
-	pci_disable_pasid(pdev);
-
-	return ret;
-}
-
 /*
  * If a device is not yet associated with a domain, this function makes the
  * device visible in the domain
@@ -1811,9 +1876,8 @@ static int attach_device(struct device *dev,
 			 struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data;
-	struct pci_dev *pdev;
 	unsigned long flags;
-	int ret;
+	int ret = 0;
 
 	spin_lock_irqsave(&domain->lock, flags);
 
@@ -1821,45 +1885,13 @@ static int attach_device(struct device *dev,
 
 	spin_lock(&dev_data->lock);
 
-	ret = -EBUSY;
-	if (dev_data->domain != NULL)
+	if (dev_data->domain != NULL) {
+		ret = -EBUSY;
 		goto out;
-
-	if (!dev_is_pci(dev))
-		goto skip_ats_check;
-
-	pdev = to_pci_dev(dev);
-	if (domain->flags & PD_IOMMUV2_MASK) {
-		struct iommu_domain *def_domain = iommu_get_dma_domain(dev);
-
-		ret = -EINVAL;
-
-		/*
-		 * In case of using AMD_IOMMU_V1 page table mode and the device
-		 * is enabling for PPR/ATS support (using v2 table),
-		 * we need to make sure that the domain type is identity map.
-		 */
-		if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
-		    def_domain->type != IOMMU_DOMAIN_IDENTITY) {
-			goto out;
-		}
-
-		if (pdev_pasid_supported(dev_data)) {
-			if (pdev_pri_ats_enable(pdev) != 0)
-				goto out;
-
-			dev_data->ats_enabled = true;
-			dev_data->ats_qdep    = pci_ats_queue_depth(pdev);
-			dev_data->pri_tlp     = pci_prg_resp_pasid_required(pdev);
-		}
-	} else if (amd_iommu_iotlb_sup &&
-		   pci_enable_ats(pdev, PAGE_SHIFT) == 0) {
-		dev_data->ats_enabled = true;
-		dev_data->ats_qdep    = pci_ats_queue_depth(pdev);
 	}
 
-skip_ats_check:
-	ret = 0;
+	if (dev_is_pci(dev))
+		pdev_enable_caps(to_pci_dev(dev));
 
 	do_attach(dev_data, domain);
 
@@ -1907,15 +1939,8 @@ static void detach_device(struct device *dev)
 
 	do_detach(dev_data);
 
-	if (!dev_is_pci(dev))
-		goto out;
-
-	if (domain->flags & PD_IOMMUV2_MASK && pdev_pasid_supported(dev_data))
-		pdev_iommuv2_disable(to_pci_dev(dev));
-	else if (dev_data->ats_enabled)
-		pci_disable_ats(to_pci_dev(dev));
-
-	dev_data->ats_enabled = false;
+	if (dev_is_pci(dev))
+		pdev_disable_caps(to_pci_dev(dev));
 
 out:
 	spin_unlock(&dev_data->lock);
-- 
2.31.1

Re: [PATCH v2 14/16] iommu/amd: Enable device ATS/PASID/PRI capabilities independently

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:07AM +0000, Vasant Hegde wrote:
> Introduce helper functions to enable/disable device ATS/PASID/PRI
> capabilities independently along with the new pasid_enabled and
> pri_enabled variables in struct iommu_dev_data to keep track,
> which allows attach_device() and detach_device() to be simplified.
> 
> Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h       |   4 +
>  drivers/iommu/amd/amd_iommu_types.h |   2 +
>  drivers/iommu/amd/iommu.c           | 203 ++++++++++++++++------------
>  3 files changed, 120 insertions(+), 89 deletions(-)

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

Jason

[PATCH v2 15/16] iommu/amd: Initialize iommu_device->max_pasids

[Vasant Hegde]

Commit 1adf3cc20d69 ("iommu: Add max_pasids field in struct iommu_device")
introduced a variable struct iommu_device.max_pasids to track max
PASIDS supported by each IOMMU.

Let us initialize this field for AMD IOMMU. IOMMU core will use this value
to set max PASIDs per device (see __iommu_probe_device()).

Also remove unused global 'amd_iommu_max_pasid' variable.

Finally current code restricts max PASIDs to 16 bits and calls BUG_ON if
max PASID is more than 16bit. This patch replaces BUG_ON with WARN_ON
as system can continue to work with 16-bit PASID.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  3 ---
 drivers/iommu/amd/init.c            | 10 +++-------
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 445b58c24d20..4c80075c0454 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -895,9 +895,6 @@ extern unsigned amd_iommu_aperture_order;
 /* allocation bitmap for domain ids */
 extern unsigned long *amd_iommu_pd_alloc_bitmap;
 
-/* Smallest max PASID supported by any IOMMU in the system */
-extern u32 amd_iommu_max_pasid;
-
 extern bool amd_iommu_force_isolation;
 
 /* Max levels of glxval supported */
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 1f56478ae74e..6441c81467ab 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -185,8 +185,6 @@ static int amd_iommus_present;
 bool amd_iommu_np_cache __read_mostly;
 bool amd_iommu_iotlb_sup __read_mostly = true;
 
-u32 amd_iommu_max_pasid __read_mostly = ~0;
-
 static bool amd_iommu_pc_present __read_mostly;
 bool amdr_ivrs_remap_support __read_mostly;
 
@@ -2086,16 +2084,14 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 
 	if (iommu_feature(iommu, FEATURE_GT)) {
 		int glxval;
-		u32 max_pasid;
 		u64 pasmax;
 
 		pasmax = iommu->features & FEATURE_PASID_MASK;
 		pasmax >>= FEATURE_PASID_SHIFT;
-		max_pasid  = (1 << (pasmax + 1)) - 1;
-
-		amd_iommu_max_pasid = min(amd_iommu_max_pasid, max_pasid);
+		iommu->iommu.max_pasids = (1 << (pasmax + 1)) - 1;
 
-		BUG_ON(amd_iommu_max_pasid & ~PASID_MASK);
+		WARN_ON(iommu->iommu.max_pasids & ~PASID_MASK);
+		iommu->iommu.max_pasids &= PASID_MASK;
 
 		glxval   = iommu->features & FEATURE_GLXVAL_MASK;
 		glxval >>= FEATURE_GLXVAL_SHIFT;
-- 
2.31.1

Re: [PATCH v2 15/16] iommu/amd: Initialize iommu_device->max_pasids

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:08AM +0000, Vasant Hegde wrote:
> Commit 1adf3cc20d69 ("iommu: Add max_pasids field in struct iommu_device")
> introduced a variable struct iommu_device.max_pasids to track max
> PASIDS supported by each IOMMU.
> 
> Let us initialize this field for AMD IOMMU. IOMMU core will use this value
> to set max PASIDs per device (see __iommu_probe_device()).
> 
> Also remove unused global 'amd_iommu_max_pasid' variable.
> 
> Finally current code restricts max PASIDs to 16 bits and calls BUG_ON if
> max PASID is more than 16bit. This patch replaces BUG_ON with WARN_ON
> as system can continue to work with 16-bit PASID.

>  		pasmax = iommu->features & FEATURE_PASID_MASK;
>  		pasmax >>= FEATURE_PASID_SHIFT;
> -		max_pasid  = (1 << (pasmax + 1)) - 1;

This can be up to (1<<32)-1

> -
> -		amd_iommu_max_pasid = min(amd_iommu_max_pasid, max_pasid);
> +		iommu->iommu.max_pasids = (1 << (pasmax + 1)) - 1;

So why not

   	iommu->iommu.max_pasids = min((1 << (pasmax + 1)) - 1, PASID_MAX)

and forget the WARN_ON?

Jason

Re: [PATCH v2 15/16] iommu/amd: Initialize iommu_device->max_pasids

[Vasant Hegde]

On 7/28/2023 8:16 PM, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 05:36:08AM +0000, Vasant Hegde wrote:
>> Commit 1adf3cc20d69 ("iommu: Add max_pasids field in struct iommu_device")
>> introduced a variable struct iommu_device.max_pasids to track max
>> PASIDS supported by each IOMMU.
>>
>> Let us initialize this field for AMD IOMMU. IOMMU core will use this value
>> to set max PASIDs per device (see __iommu_probe_device()).
>>
>> Also remove unused global 'amd_iommu_max_pasid' variable.
>>
>> Finally current code restricts max PASIDs to 16 bits and calls BUG_ON if
>> max PASID is more than 16bit. This patch replaces BUG_ON with WARN_ON
>> as system can continue to work with 16-bit PASID.
> 
>>  		pasmax = iommu->features & FEATURE_PASID_MASK;
>>  		pasmax >>= FEATURE_PASID_SHIFT;
>> -		max_pasid  = (1 << (pasmax + 1)) - 1;
> 
> This can be up to (1<<32)-1
> 
>> -
>> -		amd_iommu_max_pasid = min(amd_iommu_max_pasid, max_pasid);
>> +		iommu->iommu.max_pasids = (1 << (pasmax + 1)) - 1;
> 
> So why not
> 
>    	iommu->iommu.max_pasids = min((1 << (pasmax + 1)) - 1, PASID_MAX)

Because some older chips had support upto 16bit PASID only.

Looks like I missed to cover this details in the description. I will mentioned
about below commit details in the description.

a919a018cccf ("iommu/amd: Fix logic to determine and checking max PASID"



> 
> and forget the WARN_ON?

I do have WARN_ON and then apply PASID_MASK.

-Vasant

Re: [PATCH v2 15/16] iommu/amd: Initialize iommu_device->max_pasids

[Jason Gunthorpe]

On Mon, Jul 31, 2023 at 12:33:18PM +0530, Vasant Hegde wrote:
> On 7/28/2023 8:16 PM, Jason Gunthorpe wrote:
> > On Fri, Jul 28, 2023 at 05:36:08AM +0000, Vasant Hegde wrote:
> >> Commit 1adf3cc20d69 ("iommu: Add max_pasids field in struct iommu_device")
> >> introduced a variable struct iommu_device.max_pasids to track max
> >> PASIDS supported by each IOMMU.
> >>
> >> Let us initialize this field for AMD IOMMU. IOMMU core will use this value
> >> to set max PASIDs per device (see __iommu_probe_device()).
> >>
> >> Also remove unused global 'amd_iommu_max_pasid' variable.
> >>
> >> Finally current code restricts max PASIDs to 16 bits and calls BUG_ON if
> >> max PASID is more than 16bit. This patch replaces BUG_ON with WARN_ON
> >> as system can continue to work with 16-bit PASID.
> > 
> >>  		pasmax = iommu->features & FEATURE_PASID_MASK;
> >>  		pasmax >>= FEATURE_PASID_SHIFT;
> >> -		max_pasid  = (1 << (pasmax + 1)) - 1;
> > 
> > This can be up to (1<<32)-1
> > 
> >> -
> >> -		amd_iommu_max_pasid = min(amd_iommu_max_pasid, max_pasid);
> >> +		iommu->iommu.max_pasids = (1 << (pasmax + 1)) - 1;
> > 
> > So why not
> > 
> >    	iommu->iommu.max_pasids = min((1 << (pasmax + 1)) - 1, PASID_MAX)
> 
> Because some older chips had support upto 16bit PASID only.

Sure, but I mean why not just directly cap it to PASID_MAX and not
bother with the WARN? If you WARN then a new chip reporting > 16 bits
of PASID support will cause pointless WARN_ONs at boot time even
though the driver will run fine?

Jason

Re: [PATCH v2 15/16] iommu/amd: Initialize iommu_device->max_pasids

[Vasant Hegde]

Hi Jason,


On 7/31/2023 5:37 PM, Jason Gunthorpe wrote:
> On Mon, Jul 31, 2023 at 12:33:18PM +0530, Vasant Hegde wrote:
>> On 7/28/2023 8:16 PM, Jason Gunthorpe wrote:
>>> On Fri, Jul 28, 2023 at 05:36:08AM +0000, Vasant Hegde wrote:
>>>> Commit 1adf3cc20d69 ("iommu: Add max_pasids field in struct iommu_device")
>>>> introduced a variable struct iommu_device.max_pasids to track max
>>>> PASIDS supported by each IOMMU.
>>>>
>>>> Let us initialize this field for AMD IOMMU. IOMMU core will use this value
>>>> to set max PASIDs per device (see __iommu_probe_device()).
>>>>
>>>> Also remove unused global 'amd_iommu_max_pasid' variable.
>>>>
>>>> Finally current code restricts max PASIDs to 16 bits and calls BUG_ON if
>>>> max PASID is more than 16bit. This patch replaces BUG_ON with WARN_ON
>>>> as system can continue to work with 16-bit PASID.
>>>
>>>>  		pasmax = iommu->features & FEATURE_PASID_MASK;
>>>>  		pasmax >>= FEATURE_PASID_SHIFT;
>>>> -		max_pasid  = (1 << (pasmax + 1)) - 1;
>>>
>>> This can be up to (1<<32)-1
>>>
>>>> -
>>>> -		amd_iommu_max_pasid = min(amd_iommu_max_pasid, max_pasid);
>>>> +		iommu->iommu.max_pasids = (1 << (pasmax + 1)) - 1;
>>>
>>> So why not
>>>
>>>    	iommu->iommu.max_pasids = min((1 << (pasmax + 1)) - 1, PASID_MAX)
>>
>> Because some older chips had support upto 16bit PASID only.
> 
> Sure, but I mean why not just directly cap it to PASID_MAX and not
> bother with the WARN? If you WARN then a new chip reporting > 16 bits
> of PASID support will cause pointless WARN_ONs at boot time even
> though the driver will run fine?

Sure. I can get rid of WARN_ON and make it work.

-Vasant

[PATCH v2 16/16] iommu/amd: Simplify amd_iommu_device_info()

[Vasant Hegde]

Let 'iommu_dev_data.flags' track device exec and priv support as well.
So that in amd_iommu_device_info() we don't need to check device
capabilities again.

Also in probe path __iommu_probe_device() updates dev->iommu->max_pasids.
Just use that instead of calculating max PASIDs again

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 41 ++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 3b20d4d97192..54ee5a07f589 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -321,6 +321,7 @@ static inline bool pdev_pasid_supported(struct iommu_dev_data *dev_data)
 
 static u32 pdev_get_caps(struct pci_dev *pdev)
 {
+	int features;
 	u32 flags = 0;
 
 	if (pci_ats_supported(pdev))
@@ -329,9 +330,17 @@ static u32 pdev_get_caps(struct pci_dev *pdev)
 	if (pci_pri_supported(pdev))
 		flags |= AMD_IOMMU_DEVICE_FLAG_PRI_SUP;
 
-	if (pci_pasid_features(pdev) >= 0)
+	features = pci_pasid_features(pdev);
+	if (features >= 0) {
 		flags |= AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
 
+		if (features & PCI_PASID_CAP_EXEC)
+			flags |= AMD_IOMMU_DEVICE_FLAG_EXEC_SUP;
+
+		if (features & PCI_PASID_CAP_PRIV)
+			flags |= AMD_IOMMU_DEVICE_FLAG_PRIV_SUP;
+	}
+
 	return flags;
 }
 
@@ -2806,8 +2815,8 @@ EXPORT_SYMBOL(amd_iommu_complete_ppr);
 int amd_iommu_device_info(struct pci_dev *pdev,
                           struct amd_iommu_device_info *info)
 {
-	int max_pasids;
-	int pos;
+	struct device *dev = &pdev->dev;
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 
 	if (pdev == NULL || info == NULL)
 		return -EINVAL;
@@ -2817,29 +2826,9 @@ int amd_iommu_device_info(struct pci_dev *pdev,
 
 	memset(info, 0, sizeof(*info));
 
-	if (pci_ats_supported(pdev))
-		info->flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP;
-
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
-	if (pos)
-		info->flags |= AMD_IOMMU_DEVICE_FLAG_PRI_SUP;
-
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
-	if (pos) {
-		int features;
-
-		max_pasids = 1 << (9 * (amd_iommu_max_glx_val + 1));
-		max_pasids = min(max_pasids, (1 << 20));
-
-		info->flags |= AMD_IOMMU_DEVICE_FLAG_PASID_SUP;
-		info->max_pasids = min(pci_max_pasids(pdev), max_pasids);
-
-		features = pci_pasid_features(pdev);
-		if (features & PCI_PASID_CAP_EXEC)
-			info->flags |= AMD_IOMMU_DEVICE_FLAG_EXEC_SUP;
-		if (features & PCI_PASID_CAP_PRIV)
-			info->flags |= AMD_IOMMU_DEVICE_FLAG_PRIV_SUP;
-	}
+	info->flags = dev_data->flags;
+	if (dev->iommu)
+		info->max_pasids = dev->iommu->max_pasids;
 
 	return 0;
 }
-- 
2.31.1

Re: [PATCH v2 16/16] iommu/amd: Simplify amd_iommu_device_info()

[Jason Gunthorpe]

On Fri, Jul 28, 2023 at 05:36:09AM +0000, Vasant Hegde wrote:
> Let 'iommu_dev_data.flags' track device exec and priv support as well.
> So that in amd_iommu_device_info() we don't need to check device
> capabilities again.
> 
> Also in probe path __iommu_probe_device() updates dev->iommu->max_pasids.
> Just use that instead of calculating max PASIDs again

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

Jason