[PATCH v3 00/13] iommu/amd: SVA Support (part 3) - refactor support for GCR3 table

[PATCH v3 00/13] iommu/amd: SVA Support (part 3) - refactor support for GCR3 table

[Vasant Hegde]

This is part 3 of the 4-part series to introduce Share Virtual
Address (SVA) support, which focuses on refactoring GCR3 table.
This moves GCR3 related information from protection domain
structure to iommu_dev_data (per device) structure.

It contains the following enhancements:

* Patch 1 - 2:
  Cleanup and introduce helper function

* Patch 3 - 12:
  Introduce per device GCR3 table, per device domain ID, invalidation based
  on per device and code refactoring

* Patch 13:
  Remove unused variable from protection domain structure

This patch series is based on top of Improve TLB invalidation logic [1] :
  [1] https://lore.kernel.org/linux-iommu/20231006101624.5912-1-vasant.hegde@amd.com/T/#t

This is also available at github :
  https://github.com/AMDESE/linux/tree/iommu_sva_part3_v3_v6.6_rc5


Changes from v2 -> v3:
  - Dropped "iommu/amd: Use struct protection_domain in helper functions" as flush related
    functions are already fixed and set_gcr3 related functions is reworked in this series.
  - Removed moving to_pdomain() function from this series. It will be introduced
    in SVA series where it will be used.
  - Added per device domain ID and per device invalidation functions
  - Added functions to flush TLB for the given device
  - Added Review-by tags

v2 : https://lore.kernel.org/linux-iommu/20230816174031.634453-1-vasant.hegde@amd.com/T/#t

Changes from v1 - v2:
  - Dropped iommu_v2 module related support as newly introduced Part2
    removed iommu_v2 module.
  - Moved Patch 'Use struct protection_domain in helper functions' from Part1 to
    Part3
  - Updated get_amd_iommu_from_dev() to retrieve iommu from device structure
  - Removed 'PD_MODE_PT'

v1 : https://lore.kernel.org/linux-iommu/20230808100232.5977-1-vasant.hegde@amd.com/T/#t

Thank you,
Vasant / Suravee


Suravee Suthikulpanit (8):
  iommu/amd: Introduce get_amd_iommu_from_dev()
  iommu/amd: Introduce struct protection_domain.pd_mode
  iommu/amd: Introduce per-device GCR3 table
  iommu/amd: Refactor helper function for setting / clearing GCR3
  iommu/amd: Refactor helper function for attaching / detaching device
  iommu/amd: Refactor protection_domain helper functions
  iommu/amd: Refactor GCR3 table helper functions
  iommu/amd: Remove unused GCR3 table parameters from struct protection_domain

Vasant Hegde (5):
  iommu/amd: Pass struct iommu_dev_data to set_dte_entry()
  iommu/amd: Use protection_domain.flags to check page table mode
  iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue
  iommu/amd: Add support for device based flush TLB
  iommu/amd: Rearrange GCR3 table setup code

 drivers/iommu/amd/amd_iommu.h       |  27 +-
 drivers/iommu/amd/amd_iommu_types.h |  28 +-
 drivers/iommu/amd/io_pgtable_v2.c   |  21 +-
 drivers/iommu/amd/iommu.c           | 438 ++++++++++++++++++----------
 include/linux/iommu.h               |  13 +
 5 files changed, 336 insertions(+), 191 deletions(-)

-- 
2.31.1

[PATCH v3 01/13] iommu/amd: Pass struct iommu_dev_data to set_dte_entry()

[Vasant Hegde]

Pass iommu_dev_data structure instead of passing indivisual variables.

No functional changes intended.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2e90f7807168..4f1b356adb8f 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -77,8 +77,8 @@ struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
 
-static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
-			  struct protection_domain *domain, bool ats, bool ppr);
+static void set_dte_entry(struct amd_iommu *iommu,
+			  struct iommu_dev_data *dev_data);
 
 /****************************************************************************
  *
@@ -1648,8 +1648,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->ppr);
+		set_dte_entry(iommu, dev_data);
 		clone_aliases(iommu, dev_data->dev);
 	}
 }
@@ -1799,12 +1798,14 @@ static int setup_gcr3_table(struct protection_domain *domain, int pasids)
 	return 0;
 }
 
-static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
-			  struct protection_domain *domain, bool ats, bool ppr)
+static void set_dte_entry(struct amd_iommu *iommu,
+			  struct iommu_dev_data *dev_data)
 {
 	u64 pte_root = 0;
 	u64 flags = 0;
 	u32 old_domid;
+	u16 devid = dev_data->devid;
+	struct protection_domain *domain = dev_data->domain;
 	struct dev_table_entry *dev_table = get_dev_table(iommu);
 
 	if (domain->iop.mode != PAGE_MODE_NONE)
@@ -1824,10 +1825,10 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 
 	flags = dev_table[devid].data[1];
 
-	if (ats)
+	if (dev_data->ats_enabled)
 		flags |= DTE_FLAG_IOTLB;
 
-	if (ppr)
+	if (dev_data->ppr)
 		pte_root |= 1ULL << DEV_ENTRY_PPR;
 
 	if (domain->flags & PD_IOMMUV2_MASK) {
@@ -1900,12 +1901,10 @@ static void do_attach(struct iommu_dev_data *dev_data,
 		      struct protection_domain *domain)
 {
 	struct amd_iommu *iommu;
-	bool ats;
 
 	iommu = rlookup_amd_iommu(dev_data->dev);
 	if (!iommu)
 		return;
-	ats   = dev_data->ats_enabled;
 
 	/* Update data structures */
 	dev_data->domain = domain;
@@ -1920,8 +1919,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	domain->dev_cnt                 += 1;
 
 	/* Update device table */
-	set_dte_entry(iommu, dev_data->devid, domain,
-		      ats, dev_data->ppr);
+	set_dte_entry(iommu, dev_data);
 	clone_aliases(iommu, dev_data->dev);
 
 	device_flush_dte(dev_data);
-- 
2.31.1

Re: [PATCH v3 01/13] iommu/amd: Pass struct iommu_dev_data to set_dte_entry()

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:40PM +0000, Vasant Hegde wrote:
> Pass iommu_dev_data structure instead of passing indivisual variables.
> 
> No functional changes intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)

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

Jason

[PATCH v3 02/13] iommu/amd: Introduce get_amd_iommu_from_dev()

[Vasant Hegde]

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

And replace rlookup_amd_iommu() with the new helper function where
applicable to avoid unnecessary loop to look up struct amd_iommu from
struct device.

Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h | 14 ++++++++++++++
 drivers/iommu/amd/iommu.c     | 20 ++++++++++----------
 include/linux/iommu.h         | 13 +++++++++++++
 3 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 38b3f4562f3b..b2071ebc73b5 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -150,6 +150,20 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
 	return page ? page_address(page) : NULL;
 }
 
+/*
+ * This must be called after device probe completes. During probe
+ * use rlookup_amd_iommu() get the iommu.
+ */
+static inline struct amd_iommu *get_amd_iommu_from_dev(struct device *dev)
+{
+	struct iommu_device *iommu = iommu_get_iommu_dev(dev);
+
+	if (!iommu)
+		return NULL;
+
+	return container_of(iommu, struct amd_iommu, iommu);
+}
+
 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/iommu.c b/drivers/iommu/amd/iommu.c
index 4f1b356adb8f..eedfa341085c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1408,7 +1408,7 @@ static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
 	bool gn = is_pasid_valid(pasid);
 
 	qdep     = dev_data->ats_qdep;
-	iommu    = rlookup_amd_iommu(dev_data->dev);
+	iommu    = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
 		return -EINVAL;
 
@@ -1436,7 +1436,7 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
 	u16 alias;
 	int ret;
 
-	iommu = rlookup_amd_iommu(dev_data->dev);
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
 		return -EINVAL;
 
@@ -1644,7 +1644,7 @@ static void update_device_table(struct protection_domain *domain)
 	struct iommu_dev_data *dev_data;
 
 	list_for_each_entry(dev_data, &domain->dev_list, list) {
-		struct amd_iommu *iommu = rlookup_amd_iommu(dev_data->dev);
+		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
 
 		if (!iommu)
 			continue;
@@ -1676,7 +1676,7 @@ int amd_iommu_complete_ppr(struct pci_dev *pdev, u32 pasid,
 	struct iommu_cmd cmd;
 
 	dev_data = dev_iommu_priv_get(&pdev->dev);
-	iommu    = rlookup_amd_iommu(&pdev->dev);
+	iommu    = get_amd_iommu_from_dev(&pdev->dev);
 	if (!iommu)
 		return -ENODEV;
 
@@ -1902,7 +1902,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
 {
 	struct amd_iommu *iommu;
 
-	iommu = rlookup_amd_iommu(dev_data->dev);
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
 		return;
 
@@ -1930,7 +1930,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	struct protection_domain *domain = dev_data->domain;
 	struct amd_iommu *iommu;
 
-	iommu = rlookup_amd_iommu(dev_data->dev);
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
 		return;
 
@@ -2072,7 +2072,7 @@ static void amd_iommu_release_device(struct device *dev)
 	if (!check_device(dev))
 		return;
 
-	iommu = rlookup_amd_iommu(dev);
+	iommu = get_amd_iommu_from_dev(dev);
 	if (!iommu)
 		return;
 
@@ -2283,7 +2283,7 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 {
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	struct protection_domain *domain = to_pdomain(dom);
-	struct amd_iommu *iommu = rlookup_amd_iommu(dev);
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
 	int ret;
 
 	/*
@@ -2435,7 +2435,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
 		return;
 
 	devid = PCI_SBDF_TO_DEVID(sbdf);
-	iommu = rlookup_amd_iommu(dev);
+	iommu = get_amd_iommu_from_dev(dev);
 	if (!iommu)
 		return;
 	pci_seg = iommu->pci_seg;
@@ -2769,7 +2769,7 @@ static int set_remap_table_entry_alias(struct pci_dev *pdev, u16 alias,
 {
 	struct irq_remap_table *table = data;
 	struct amd_iommu_pci_seg *pci_seg;
-	struct amd_iommu *iommu = rlookup_amd_iommu(&pdev->dev);
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(&pdev->dev);
 
 	if (!iommu)
 		return -EINVAL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d52d0826cae1..25d9e428797c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -559,6 +559,19 @@ static inline struct iommu_device *dev_to_iommu_device(struct device *dev)
 	return (struct iommu_device *)dev_get_drvdata(dev);
 }
 
+/**
+ * iommu_get_iommu_dev - Get iommu_device for a device
+ * @dev: an end-point device
+ *
+ * Note that this function must be called from the iommu_ops
+ * to retrieve the iommu_device for a device, which the core code
+ * guarentees it will not invoke the op without an attached iommu.
+ */
+static inline struct iommu_device *iommu_get_iommu_dev(struct device *dev)
+{
+	return dev->iommu->iommu_dev;
+}
+
 static inline void iommu_iotlb_gather_init(struct iommu_iotlb_gather *gather)
 {
 	*gather = (struct iommu_iotlb_gather) {
-- 
2.31.1

Re: [PATCH v3 02/13] iommu/amd: Introduce get_amd_iommu_from_dev()

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:41PM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> And replace rlookup_amd_iommu() with the new helper function where
> applicable to avoid unnecessary loop to look up struct amd_iommu from
> struct device.
> 
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h | 14 ++++++++++++++
>  drivers/iommu/amd/iommu.c     | 20 ++++++++++----------
>  include/linux/iommu.h         | 13 +++++++++++++
>  3 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index 38b3f4562f3b..b2071ebc73b5 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -150,6 +150,20 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
>  	return page ? page_address(page) : NULL;
>  }
>  
> +/*
> + * This must be called after device probe completes. During probe
> + * use rlookup_amd_iommu() get the iommu.
> + */
> +static inline struct amd_iommu *get_amd_iommu_from_dev(struct device *dev)
> +{
> +	struct iommu_device *iommu = iommu_get_iommu_dev(dev);
> +
> +	if (!iommu)
> +		return NULL;

This shouldn't be done. See the comment for iommu_get_iommu_dev(). If
you are calling this outside an op context it is broken and the if
won't save it.

Ideally you'd put these calls only at the top of functions
implementing ops and then pass either the amd_iommu or iommu_dev_data
pointers down the call chain.

> +	return container_of(iommu, struct amd_iommu, iommu);
> +}
> +
>  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/iommu.c b/drivers/iommu/amd/iommu.c
> index 4f1b356adb8f..eedfa341085c 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1408,7 +1408,7 @@ static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
>  	bool gn = is_pasid_valid(pasid);
>
>  	qdep     = dev_data->ats_qdep;
> -	iommu    = rlookup_amd_iommu(dev_data->dev);
> +	iommu    = get_amd_iommu_from_dev(dev_data->dev);
>  	if (!iommu)
>  		return -EINVAL;

Eg here we have an iommu_dev_data which must mean the device is probed
and iommu is valid.

> +/**
> + * iommu_get_iommu_dev - Get iommu_device for a device
> + * @dev: an end-point device
> + *
> + * Note that this function must be called from the iommu_ops
> + * to retrieve the iommu_device for a device, which the core code
> + * guarentees it will not invoke the op without an attached iommu.
                                                  ^^^^^^^^^^^^^^^^^^

Means the function never returns NULL.

Jason

Re: [PATCH v3 02/13] iommu/amd: Introduce get_amd_iommu_from_dev()

[Vasant Hegde]

On 11/5/2023 11:35 PM, Jason Gunthorpe wrote:
> On Fri, Oct 13, 2023 at 03:16:41PM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> And replace rlookup_amd_iommu() with the new helper function where
>> applicable to avoid unnecessary loop to look up struct amd_iommu from
>> struct device.
>>
>> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu.h | 14 ++++++++++++++
>>  drivers/iommu/amd/iommu.c     | 20 ++++++++++----------
>>  include/linux/iommu.h         | 13 +++++++++++++
>>  3 files changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index 38b3f4562f3b..b2071ebc73b5 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -150,6 +150,20 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
>>  	return page ? page_address(page) : NULL;
>>  }
>>  
>> +/*
>> + * This must be called after device probe completes. During probe
>> + * use rlookup_amd_iommu() get the iommu.
>> + */
>> +static inline struct amd_iommu *get_amd_iommu_from_dev(struct device *dev)
>> +{
>> +	struct iommu_device *iommu = iommu_get_iommu_dev(dev);
>> +
>> +	if (!iommu)
>> +		return NULL;
> 
> This shouldn't be done. See the comment for iommu_get_iommu_dev(). If
> you are calling this outside an op context it is broken and the if
> won't save it.
> 

Fixed.

> Ideally you'd put these calls only at the top of functions
> implementing ops and then pass either the amd_iommu or iommu_dev_data
> pointers down the call chain.
> 
>> +	return container_of(iommu, struct amd_iommu, iommu);
>> +}
>> +
>>  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/iommu.c b/drivers/iommu/amd/iommu.c
>> index 4f1b356adb8f..eedfa341085c 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -1408,7 +1408,7 @@ static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
>>  	bool gn = is_pasid_valid(pasid);
>>
>>  	qdep     = dev_data->ats_qdep;
>> -	iommu    = rlookup_amd_iommu(dev_data->dev);
>> +	iommu    = get_amd_iommu_from_dev(dev_data->dev);
>>  	if (!iommu)
>>  		return -EINVAL;
> 
> Eg here we have an iommu_dev_data which must mean the device is probed
> and iommu is valid.

Fixed.

-Vasant

[PATCH v3 03/13] iommu/amd: Introduce struct protection_domain.pd_mode

[Vasant Hegde]

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

This enum variable is used to track the type of page table used by the
protection domain. It will replace the protection_domain.flags in
subsequent series.

Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 6 ++++++
 drivers/iommu/amd/iommu.c           | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index ba40e5066300..5785bd51540f 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -538,6 +538,11 @@ struct amd_io_pgtable {
 	u64			*pgd;		/* v2 pgtable pgd pointer */
 };
 
+enum protection_domain_mode {
+	PD_MODE_V1 = 1,
+	PD_MODE_V2,
+};
+
 /*
  * This structure contains generic data for  IOMMU protection domains
  * independent of their use.
@@ -553,6 +558,7 @@ struct protection_domain {
 	int nid;		/* Node ID */
 	u64 *gcr3_tbl;		/* Guest CR3 table */
 	unsigned long flags;	/* flags to find out type of domain */
+	enum protection_domain_mode pd_mode; /* Track page table type */
 	unsigned dev_cnt;	/* devices assigned to this domain */
 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
 };
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index eedfa341085c..ff52beb83b74 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2148,6 +2148,7 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 			return -ENOMEM;
 	}
 
+	domain->pd_mode = PD_MODE_V1;
 	amd_iommu_domain_set_pgtable(domain, pt_root, mode);
 
 	return 0;
@@ -2156,6 +2157,7 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 static int protection_domain_init_v2(struct protection_domain *domain)
 {
 	domain->flags |= PD_GIOV_MASK;
+	domain->pd_mode = PD_MODE_V2;
 
 	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
 
-- 
2.31.1

Re: [PATCH v3 03/13] iommu/amd: Introduce struct protection_domain.pd_mode

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:42PM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> This enum variable is used to track the type of page table used by the
> protection domain. It will replace the protection_domain.flags in
> subsequent series.
> 
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 6 ++++++
>  drivers/iommu/amd/iommu.c           | 2 ++
>  2 files changed, 8 insertions(+)

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

At least in smmuv3 it is very close to not needing kind of indication
at all, the only remaining tests are under ops which could be replaced
by per-mode ops.

It seems like a saner design to start a the very top of call chains
knowing what is required below.

Jason

[PATCH v3 04/13] iommu/amd: Introduce per-device GCR3 table

[Vasant Hegde]

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

AMD IOMMU GCR3 table is indexed by PASID. Each entry stores guest CR3
register value, which is an address to the root of guest IO page table.
The GCR3 table can be programmed per-device. However, Linux AMD IOMMU
driver currently managing the table on a per-domain basis.

PASID is a device feature. When SVA is enabled it will bind PASID to
device, not domain. Hence it makes sense to have per device GCR3 table.

Introduce struct iommu_dev_data.gcr3_tbl_info to keep track of GCR3 table
configuration. This will eventually replaces gcr3 related variables in
protection_domain structure.

Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 5785bd51540f..3b1ef1030687 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -530,6 +530,13 @@ struct amd_irte_ops;
 #define io_pgtable_cfg_to_data(x) \
 	container_of((x), struct amd_io_pgtable, pgtbl_cfg)
 
+struct gcr3_tbl_info {
+	u64	*gcr3_tbl;	/* Guest CR3 table */
+	int	glx;		/* Number of levels for GCR3 table */
+	u32	pasid_cnt;	/* Track attached PASIDs */
+	bool	giov;		/* GIOV bit support */
+};
+
 struct amd_io_pgtable {
 	struct io_pgtable_cfg	pgtbl_cfg;
 	struct io_pgtable	iop;
@@ -810,6 +817,7 @@ struct iommu_dev_data {
 	struct list_head list;		  /* For domain->dev_list */
 	struct llist_node dev_data_list;  /* For global dev_data_list */
 	struct protection_domain *domain; /* Domain the device is bound to */
+	struct gcr3_tbl_info gcr3_info;   /* Per-device GCR3 table */
 	struct device *dev;
 	u16 devid;			  /* PCI Device ID */
 
-- 
2.31.1

[PATCH v3 05/13] iommu/amd: Use protection_domain.flags to check page table mode

[Vasant Hegde]

Page table mode (v1, v2 or pt) is per domain property. Recently we have
enhanced protection_domain.pd_mode to track per domain page table mode.
Use that variable to check the page table mode instead of global
'amd_iommu_pgtable' in {map/unmap}_pages path.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index ff52beb83b74..85d73555c1f9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2336,7 +2336,7 @@ static int amd_iommu_map_pages(struct iommu_domain *dom, unsigned long iova,
 	int prot = 0;
 	int ret = -EINVAL;
 
-	if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
+	if ((domain->pd_mode == PD_MODE_V1) &&
 	    (domain->iop.mode == PAGE_MODE_NONE))
 		return -EINVAL;
 
@@ -2382,7 +2382,7 @@ static size_t amd_iommu_unmap_pages(struct iommu_domain *dom, unsigned long iova
 	struct io_pgtable_ops *ops = &domain->iop.iop.ops;
 	size_t r;
 
-	if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
+	if ((domain->pd_mode == PD_MODE_V1) &&
 	    (domain->iop.mode == PAGE_MODE_NONE))
 		return 0;
 
-- 
2.31.1

[PATCH v3 06/13] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

With v1 page table (stage-2), the AMD IOMMU spec states that the hardware
must use the domain ID to tag its internal translation caches. I/O devices
with different v1 page tables must be given different domain IDs. I/O
devices that share the same v1 page table __may__ be given the same domain
ID. This domain ID management policy is currently implemented by the AMD
IOMMU driver. In this case, only the domain ID is needed when issuing the
INVALIDATE_IOMMU_PAGES command to invalidate the IOMMU translation cache
(TLB).

With v2 page table (stage-1), the hardware uses domain ID and PASID as
parameters to tag and issue the INVALIDATE_IOMMU_PAGES command. Since the
GCR3 table is setup per-device, and there is no guarantee for PASID to be
unique across multiple devices, the same PASID for different devices could
have different v2 page tables. In such case, if multiple devices share the
same domain ID, IOMMU translation cache for these devices would be polluted
due to TLB aliasing.

Hence, avoid the TLB aliasing issue by allocating unique domain ID for each
device even when multiple devices are sharing the same v1 page table.
Please note that this workaround would result in multiple
INVALIDATE_IOMMU_PAGES commands (one per domain id) when unmapping a
translation on the shared v1 page table.

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

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 3b1ef1030687..e836438a9318 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -831,6 +831,8 @@ struct iommu_dev_data {
 	u8 ppr          :1;		  /* Enable device PPR support */
 	bool use_vapic;			  /* Enable device to use vapic mode */
 	bool defer_attach;
+	/* Per device domain ID. Used with V2 page table */
+	u16 domid;
 
 	struct ratelimit_state rs;        /* Ratelimit IOPF messages */
 };
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 85d73555c1f9..09762332f681 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -102,6 +102,14 @@ static inline bool is_pasid_valid(ioasid_t pasid)
 	return (pasid != IOMMU_PASID_INVALID) ? true : false;
 }
 
+/*
+ * Allocate per device domain ID when protection domain is in V2 page table
+ */
+static inline bool domain_id_is_per_dev(struct protection_domain *pdom)
+{
+	return (pdom && pdom->pd_mode != PD_MODE_V1);
+}
+
 static inline int get_acpihid_device_id(struct device *dev,
 					struct acpihid_map_entry **entry)
 {
@@ -1418,6 +1426,19 @@ static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
 	return iommu_queue_command(iommu, &cmd);
 }
 
+/* Flush IOMMU TLB for the given device */
+static int device_flush_tlb_range(struct iommu_dev_data *dev_data,
+				  ioasid_t pasid, u64 address, size_t size)
+{
+	struct iommu_cmd cmd;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
+	bool gn = is_pasid_valid(pasid);
+
+	build_inv_iommu_pages(&cmd, address, size, dev_data->domid, pasid, gn);
+
+	return iommu_queue_command(iommu, &cmd);
+}
+
 static int device_flush_dte_alias(struct pci_dev *pdev, u16 alias, void *data)
 {
 	struct amd_iommu *iommu = data;
@@ -1523,11 +1544,25 @@ static int domain_flush_tlb_range(struct protection_domain *pdom,
 static void __domain_flush_pages(struct protection_domain *pdom,
 				 ioasid_t pasid, u64 address, size_t size)
 {
-	int ret;
+	struct iommu_dev_data *dev_data;
+	int ret = 0;
 
-	ret = domain_flush_tlb_range(pdom, pasid, address, size);
+	if (domain_id_is_per_dev(pdom)) {
+		list_for_each_entry(dev_data, &pdom->dev_list, list) {
+			ret |= device_flush_tlb_range(dev_data,
+						      pasid, address, size);
 
-	ret |= domain_flush_dev_iotlb_range(pdom, pasid, address, size);
+			if (!dev_data->ats_enabled)
+				continue;
+
+			ret |= device_flush_iotlb_range(dev_data,
+							pasid, address, size);
+		}
+	} else {
+		ret = domain_flush_tlb_range(pdom, pasid, address, size);
+
+		ret |= domain_flush_dev_iotlb_range(pdom, pasid, address, size);
+	}
 
 	WARN_ON(ret);
 }
@@ -1805,9 +1840,15 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	u64 flags = 0;
 	u32 old_domid;
 	u16 devid = dev_data->devid;
+	u16 domid;
 	struct protection_domain *domain = dev_data->domain;
 	struct dev_table_entry *dev_table = get_dev_table(iommu);
 
+	if (domain_id_is_per_dev(domain))
+		domid = dev_data->domid;
+	else
+		domid = domain->id;
+
 	if (domain->iop.mode != PAGE_MODE_NONE)
 		pte_root = iommu_virt_to_phys(domain->iop.root);
 
@@ -1820,7 +1861,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	 * When SNP is enabled, Only set TV bit when IOMMU
 	 * page translation is in use.
 	 */
-	if (!amd_iommu_snp_en || (domain->id != 0))
+	if (!amd_iommu_snp_en || (domid != 0))
 		pte_root |= DTE_FLAG_TV;
 
 	flags = dev_table[devid].data[1];
@@ -1866,7 +1907,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	}
 
 	flags &= ~DEV_DOMID_MASK;
-	flags |= domain->id;
+	flags |= domid;
 
 	old_domid = dev_table[devid].data[1] & DEV_DOMID_MASK;
 	dev_table[devid].data[1]  = flags;
@@ -1918,6 +1959,10 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	domain->dev_iommu[iommu->index] += 1;
 	domain->dev_cnt                 += 1;
 
+	/* Allocate per device domain ID */
+	if (domain_id_is_per_dev(domain))
+		dev_data->domid = domain_id_alloc();
+
 	/* Update device table */
 	set_dte_entry(iommu, dev_data);
 	clone_aliases(iommu, dev_data->dev);
@@ -1949,6 +1994,10 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	/* decrease reference counters - needs to happen after the flushes */
 	domain->dev_iommu[iommu->index] -= 1;
 	domain->dev_cnt                 -= 1;
+
+	/* Free per device domain ID */
+	if (domain_id_is_per_dev(domain))
+		domain_id_free(dev_data->domid);
 }
 
 /*
-- 
2.31.1

Re: [PATCH v3 06/13] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:45PM +0000, Vasant Hegde wrote:
> With v1 page table (stage-2), the AMD IOMMU spec states that the hardware
> must use the domain ID to tag its internal translation caches. I/O devices
> with different v1 page tables must be given different domain IDs. I/O
> devices that share the same v1 page table __may__ be given the same domain
> ID. This domain ID management policy is currently implemented by the AMD
> IOMMU driver. In this case, only the domain ID is needed when issuing the
> INVALIDATE_IOMMU_PAGES command to invalidate the IOMMU translation cache
> (TLB).
> 
> With v2 page table (stage-1), the hardware uses domain ID and PASID as
> parameters to tag and issue the INVALIDATE_IOMMU_PAGES command. Since the
> GCR3 table is setup per-device, and there is no guarantee for PASID to be
> unique across multiple devices, the same PASID for different devices could
> have different v2 page tables. In such case, if multiple devices share the
> same domain ID, IOMMU translation cache for these devices would be polluted
> due to TLB aliasing.
> 
> Hence, avoid the TLB aliasing issue by allocating unique domain ID for each
> device even when multiple devices are sharing the same v1 page table.
> Please note that this workaround would result in multiple
> INVALIDATE_IOMMU_PAGES commands (one per domain id) when unmapping a
> translation on the shared v1 page table.

It is worth pointing out that this is a shortcut to implementing a
more complete solution where the domain ID can be shared right up until a
PASID is used.


> @@ -1418,6 +1426,19 @@ static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
>  	return iommu_queue_command(iommu, &cmd);
>  }
>  
> +/* Flush IOMMU TLB for the given device */
> +static int device_flush_tlb_range(struct iommu_dev_data *dev_data,
> +				  ioasid_t pasid, u64 address, size_t size)
> +{
> +	struct iommu_cmd cmd;
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
> +	bool gn = is_pasid_valid(pasid);

Again it seems obfuscating to ecode the table type in the
pasid.

> @@ -1523,11 +1544,25 @@ static int domain_flush_tlb_range(struct protection_domain *pdom,
>  static void __domain_flush_pages(struct protection_domain *pdom,
>  				 ioasid_t pasid, u64 address, size_t size)
>  {
> -	int ret;
> +	struct iommu_dev_data *dev_data;
> +	int ret = 0;
>  
> -	ret = domain_flush_tlb_range(pdom, pasid, address, size);
> +	if (domain_id_is_per_dev(pdom)) {
> +		list_for_each_entry(dev_data, &pdom->dev_list, list) {
> +			ret |= device_flush_tlb_range(dev_data,
> +						      pasid, address, size);
>  
> -	ret |= domain_flush_dev_iotlb_range(pdom, pasid, address, size);
> +			if (!dev_data->ats_enabled)
> +				continue;
> +
> +			ret |= device_flush_iotlb_range(dev_data,
> +							pasid, address, size);
> +		}
> +	} else {
> +		ret = domain_flush_tlb_range(pdom, pasid, address, size);
> +
> +		ret |= domain_flush_dev_iotlb_range(pdom, pasid, address, size);
> +	}
>  
>  	WARN_ON(ret);
>  }

I feel like this has become pretty complicated.

I think this driver really suffers from not having the right
data structures to handle everything cleanly.

In smmuv3 I added a 'master_domain' structure that linked the PCI
device to the iommu_domain. ie when attach is done you'd create a new
master_domain that essentially stores the parameters required to do
invalidation.

For what is going on here I would say to do that an then put the
"domain id" inside the "master_domain". Decide when the domain is
attached if the domain id should by taken from the iommu_domain
(device does not support PASID) or from the device (device does
support PASID)

Then you don't need all this logic to try to figure out what the cache
tag is later on. And you can get build up to removing the
protection_domain->iommu madness and more properly integrate the pci
alias stuff too.

Doing this datastructure change was a big step that made all the rest
of the PASID/SVA/etc stuff I did in smmuv3 flow nicely and logically

Jason

Re: [PATCH v3 06/13] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

On 11/5/2023 11:46 PM, Jason Gunthorpe wrote:
> On Fri, Oct 13, 2023 at 03:16:45PM +0000, Vasant Hegde wrote:
>> With v1 page table (stage-2), the AMD IOMMU spec states that the hardware
>> must use the domain ID to tag its internal translation caches. I/O devices
>> with different v1 page tables must be given different domain IDs. I/O
>> devices that share the same v1 page table __may__ be given the same domain
>> ID. This domain ID management policy is currently implemented by the AMD
>> IOMMU driver. In this case, only the domain ID is needed when issuing the
>> INVALIDATE_IOMMU_PAGES command to invalidate the IOMMU translation cache
>> (TLB).
>>
>> With v2 page table (stage-1), the hardware uses domain ID and PASID as
>> parameters to tag and issue the INVALIDATE_IOMMU_PAGES command. Since the
>> GCR3 table is setup per-device, and there is no guarantee for PASID to be
>> unique across multiple devices, the same PASID for different devices could
>> have different v2 page tables. In such case, if multiple devices share the
>> same domain ID, IOMMU translation cache for these devices would be polluted
>> due to TLB aliasing.
>>
>> Hence, avoid the TLB aliasing issue by allocating unique domain ID for each
>> device even when multiple devices are sharing the same v1 page table.
>> Please note that this workaround would result in multiple
>> INVALIDATE_IOMMU_PAGES commands (one per domain id) when unmapping a
>> translation on the shared v1 page table.
> 
> It is worth pointing out that this is a shortcut to implementing a
> more complete solution where the domain ID can be shared right up until a
> PASID is used.

Let me see if I can rewrite the description bit.


> 
> 
>> @@ -1418,6 +1426,19 @@ static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
>>  	return iommu_queue_command(iommu, &cmd);
>>  }
>>  
>> +/* Flush IOMMU TLB for the given device */
>> +static int device_flush_tlb_range(struct iommu_dev_data *dev_data,
>> +				  ioasid_t pasid, u64 address, size_t size)
>> +{
>> +	struct iommu_cmd cmd;
>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
>> +	bool gn = is_pasid_valid(pasid);
> 
> Again it seems obfuscating to ecode the table type in the
> pasid.


It depends on how invalidation series goes.

> 
>> @@ -1523,11 +1544,25 @@ static int domain_flush_tlb_range(struct protection_domain *pdom,
>>  static void __domain_flush_pages(struct protection_domain *pdom,
>>  				 ioasid_t pasid, u64 address, size_t size)
>>  {
>> -	int ret;
>> +	struct iommu_dev_data *dev_data;
>> +	int ret = 0;
>>  
>> -	ret = domain_flush_tlb_range(pdom, pasid, address, size);
>> +	if (domain_id_is_per_dev(pdom)) {
>> +		list_for_each_entry(dev_data, &pdom->dev_list, list) {
>> +			ret |= device_flush_tlb_range(dev_data,
>> +						      pasid, address, size);
>>  
>> -	ret |= domain_flush_dev_iotlb_range(pdom, pasid, address, size);
>> +			if (!dev_data->ats_enabled)
>> +				continue;
>> +
>> +			ret |= device_flush_iotlb_range(dev_data,
>> +							pasid, address, size);
>> +		}
>> +	} else {
>> +		ret = domain_flush_tlb_range(pdom, pasid, address, size);
>> +
>> +		ret |= domain_flush_dev_iotlb_range(pdom, pasid, address, size);
>> +	}
>>  
>>  	WARN_ON(ret);
>>  }
> 
> I feel like this has become pretty complicated.


I don't think its so complicated. All we do is extra check to get the domain ID
and its necessary as domain ID allocation depends on page table type.

Also we have centralized check in domain_id_is_per_dev() so that its easy to
tweak it later.

> 
> I think this driver really suffers from not having the right
> data structures to handle everything cleanly.
> 
> In smmuv3 I added a 'master_domain' structure that linked the PCI
> device to the iommu_domain. ie when attach is done you'd create a new
> master_domain that essentially stores the parameters required to do
> invalidation.
> 
> For what is going on here I would say to do that an then put the
> "domain id" inside the "master_domain". Decide when the domain is
> attached if the domain id should by taken from the iommu_domain
> (device does not support PASID) or from the device (device does
> support PASID)

We still have single domain concept (at least until we introduce vIOMMU). All we
are changing is how we allocate domain ID.

Having another domain for each device just to keep invalidation info is
complicates things. Also IMO its unnecessary.

-Vasant



> 
> Then you don't need all this logic to try to figure out what the cache
> tag is later on. And you can get build up to removing the
> protection_domain->iommu madness and more properly integrate the pci
> alias stuff too.
> 
> Doing this datastructure change was a big step that made all the rest
> of the PASID/SVA/etc stuff I did in smmuv3 flow nicely and logically
> 
> Jason

Re: [PATCH v3 06/13] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe]

On Mon, Nov 06, 2023 at 06:09:47PM +0530, Vasant Hegde wrote:

> > I think this driver really suffers from not having the right
> > data structures to handle everything cleanly.
> > 
> > In smmuv3 I added a 'master_domain' structure that linked the PCI
> > device to the iommu_domain. ie when attach is done you'd create a new
> > master_domain that essentially stores the parameters required to do
> > invalidation.
> > 
> > For what is going on here I would say to do that an then put the
> > "domain id" inside the "master_domain". Decide when the domain is
> > attached if the domain id should by taken from the iommu_domain
> > (device does not support PASID) or from the device (device does
> > support PASID)
> 
> We still have single domain concept (at least until we introduce
> vIOMMU). All we are changing is how we allocate domain ID.
>
> Having another domain for each device just to keep invalidation info is
> complicates things. Also IMO its unnecessary.

It is not another domain, it is cleaning up the mess of keeping track
of what caches need to be invalidated for a single domain.

Today we have this:

struct protection_domain {
	struct list_head dev_list; /* List of all devices in this domain */
	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */

(and I'm sorry, but using a global array of iommus and this dev_iommu
thing is an insane design)

Now this adds a new concept domain_id_is_per_dev(), and it still
doesn't support PASID properly!

Instead write it like this:

struct attachment {
    struct list_head attachments_item;
    struct amd_iommu *iommu;
    struct iommu_dev_data *device;
    ioasid_t pasid
}

struct protection_domain {
    struct list_head attachments;


Where every ops->attach_dev allocates a new struct attachment and
threads it on the liked list of the protection_domain.

Then the invalidation logic become completely straightforward, no
confusing mess:

invalidate_iotlb_v1:
   list_for_each_iommu(elm, domain->attachments)
       build_cmd_v1_invalidation(&cmd, domain->cache_tag,  ...);
       iommu_queue_command(elm->iommu, &cmd);

invalidate_iotlb_v2:
   list_for_each_iommu(elm, domain->attachments)
       build_cmd_v2_invalidation(&cmd, device->gcr3_cache_tag, elm->pasid, ...);
       iommu_queue_command(elm->iommu, &cmd);

invalidate_ats:
   list_for_each(elm, domain->attachments)
       if (!elm->device->ats enabled)
             continue
       build_cmd_atc_invalidation(&cmd, elm->device, elm->pasid, ...);
       iommu_queue_command(elm->iommu, &cmd);

Where list_for_each_iommu de-duplicates the iommus from the sorted
list, Michael had a series that showed how to do this for SMMU.

Basically you precalculate exactly the invalidations required and
store it in a list associated with the domain. When it is time to do
an invalidation then you just walk the list and do exactly what it
says.

Jason

Re: [PATCH v3 06/13] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

On 11/6/2023 7:06 PM, Jason Gunthorpe wrote:
> On Mon, Nov 06, 2023 at 06:09:47PM +0530, Vasant Hegde wrote:
> 
>>> I think this driver really suffers from not having the right
>>> data structures to handle everything cleanly.
>>>
>>> In smmuv3 I added a 'master_domain' structure that linked the PCI
>>> device to the iommu_domain. ie when attach is done you'd create a new
>>> master_domain that essentially stores the parameters required to do
>>> invalidation.
>>>
>>> For what is going on here I would say to do that an then put the
>>> "domain id" inside the "master_domain". Decide when the domain is
>>> attached if the domain id should by taken from the iommu_domain
>>> (device does not support PASID) or from the device (device does
>>> support PASID)
>>
>> We still have single domain concept (at least until we introduce
>> vIOMMU). All we are changing is how we allocate domain ID.
>>
>> Having another domain for each device just to keep invalidation info is
>> complicates things. Also IMO its unnecessary.
> 
> It is not another domain, it is cleaning up the mess of keeping track
> of what caches need to be invalidated for a single domain.
> 
> Today we have this:
> 
> struct protection_domain {
> 	struct list_head dev_list; /* List of all devices in this domain */
> 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
> 
> (and I'm sorry, but using a global array of iommus and this dev_iommu
> thing is an insane design)

Devices behind different IOMMU can be attached to same domain (like VFIO case).
We do need to track the IOMMUs and as part of invalidation we have a requirement
to send `completion` command to each IOMMU. So this links domain to IOMMUs.

Array is not a best thing here. I have it in my TODO list to change this to
xarray or something. But that's after SVA series as we are already making too
many changes to fundamental data structure in this series.

> 
> Now this adds a new concept domain_id_is_per_dev(), and it still
> doesn't support PASID properly!
> 
> Instead write it like this:
> 
> struct attachment {
>     struct list_head attachments_item;
>     struct amd_iommu *iommu;
>     struct iommu_dev_data *device;
>     ioasid_t pasid
> }
> 
> struct protection_domain {
>     struct list_head attachments;
> 
> 
> Where every ops->attach_dev allocates a new struct attachment and
> threads it on the liked list of the protection_domain.


We support PASID only in V2 page table mode. V1 does not have PASID stuff. So we
just have list of devices in the domain. Then each device has PASID table (that
what this series does).

Also as mentioned above we have the requirement of `completion wait` call for
each IOMMU. Hence we track the IOMMU list. Having it per device like above
increases completion wait calls which is not good. IMO above changes
unnecessarily complicates stuff.

> 
> Then the invalidation logic become completely straightforward, no
> confusing mess:
> 
> invalidate_iotlb_v1:
>    list_for_each_iommu(elm, domain->attachments)
>        build_cmd_v1_invalidation(&cmd, domain->cache_tag,  ...);
>        iommu_queue_command(elm->iommu, &cmd);

Ours is domain based invalidation. So our flushing logic is
	Flush IOMMU TLB for each IOMMUs
	If device has ATS
		Flush device IOTLB

	For each IOMMU (dev_iommu list)
		call completion wait
		

> 
> invalidate_iotlb_v2:
>    list_for_each_iommu(elm, domain->attachments)
>        build_cmd_v2_invalidation(&cmd, device->gcr3_cache_tag, elm->pasid, ...);
>        iommu_queue_command(elm->iommu, &cmd);

From driver point of view, fundamentally V2 invalidation is not too different as
we have single invalidation command. All we need is few extra param to tell its
guest page table invalidation with PASID.



> 
> invalidate_ats:
>    list_for_each(elm, domain->attachments)
>        if (!elm->device->ats enabled)
>              continue
>        build_cmd_atc_invalidation(&cmd, elm->device, elm->pasid, ...);
>        iommu_queue_command(elm->iommu, &cmd);


Driver already does this.

> 
> Where list_for_each_iommu de-duplicates the iommus from the sorted
> list, Michael had a series that showed how to do this for SMMU.
> 
> Basically you precalculate exactly the invalidations required and
> store it in a list associated with the domain. When it is time to do
> an invalidation then you just walk the list and do exactly what it
> says.

I think we have most things already in protection domain. Only extra check we
have is checking `pdom->dev_iommu[i]` which will be fixed separately.

-Vasant

Re: [PATCH v3 06/13] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe]

On Tue, Nov 07, 2023 at 11:00:18AM +0530, Vasant Hegde wrote:
> 
> 
> On 11/6/2023 7:06 PM, Jason Gunthorpe wrote:
> > On Mon, Nov 06, 2023 at 06:09:47PM +0530, Vasant Hegde wrote:
> > 
> >>> I think this driver really suffers from not having the right
> >>> data structures to handle everything cleanly.
> >>>
> >>> In smmuv3 I added a 'master_domain' structure that linked the PCI
> >>> device to the iommu_domain. ie when attach is done you'd create a new
> >>> master_domain that essentially stores the parameters required to do
> >>> invalidation.
> >>>
> >>> For what is going on here I would say to do that an then put the
> >>> "domain id" inside the "master_domain". Decide when the domain is
> >>> attached if the domain id should by taken from the iommu_domain
> >>> (device does not support PASID) or from the device (device does
> >>> support PASID)
> >>
> >> We still have single domain concept (at least until we introduce
> >> vIOMMU). All we are changing is how we allocate domain ID.
> >>
> >> Having another domain for each device just to keep invalidation info is
> >> complicates things. Also IMO its unnecessary.
> > 
> > It is not another domain, it is cleaning up the mess of keeping track
> > of what caches need to be invalidated for a single domain.
> > 
> > Today we have this:
> > 
> > struct protection_domain {
> > 	struct list_head dev_list; /* List of all devices in this domain */
> > 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
> > 
> > (and I'm sorry, but using a global array of iommus and this dev_iommu
> > thing is an insane design)
> 
> Devices behind different IOMMU can be attached to same domain (like VFIO case).
> We do need to track the IOMMUs and as part of invalidation we have a requirement
> to send `completion` command to each IOMMU. So this links domain to IOMMUs.

I understand how it works.

> Array is not a best thing here. I have it in my TODO list to change this to
> xarray or something. But that's after SVA series as we are already making too
> many changes to fundamental data structure in this series.

'unsigned dev_iommu' is the problem not the array.
 
> > Now this adds a new concept domain_id_is_per_dev(), and it still
> > doesn't support PASID properly!
> > 
> > Instead write it like this:
> > 
> > struct attachment {
> >     struct list_head attachments_item;
> >     struct amd_iommu *iommu;
> >     struct iommu_dev_data *device;
> >     ioasid_t pasid
> > }
> > 
> > struct protection_domain {
> >     struct list_head attachments;
> > 
> > 
> > Where every ops->attach_dev allocates a new struct attachment and
> > threads it on the liked list of the protection_domain.
> 
> 
> We support PASID only in V2 page table mode. V1 does not have PASID stuff. So we
> just have list of devices in the domain. Then each device has PASID table (that
> what this series does).

Doesn't matter, v1 uses the dev_iommu and the point is to consolidate
alll of this.

> Also as mentioned above we have the requirement of `completion wait` call for
> each IOMMU. Hence we track the IOMMU list. Having it per device like above
> increases completion wait calls which is not good. IMO above changes
> unnecessarily complicates stuff.

It is not per device, it is still done per-iommu. I wrote:

    list_for_each_iommu(elm, domain->attachments)
        build_cmd_v1_invalidation(&cmd, domain->cache_tag,  ...);
        iommu_queue_command(elm->iommu, &cmd);

Which is the same work as iterating over protection_domain->dev_iommu,
the iommus are extracted from the device list which is needed anyhow
for ATS, PASID, and V2. So just use it everywhere.

> > Then the invalidation logic become completely straightforward, no
> > confusing mess:
> > 
> > invalidate_iotlb_v1:
> >    list_for_each_iommu(elm, domain->attachments)
> >        build_cmd_v1_invalidation(&cmd, domain->cache_tag,  ...);
> >        iommu_queue_command(elm->iommu, &cmd);
> 
> Ours is domain based invalidation. So our flushing logic is

> 	Flush IOMMU TLB for each IOMMUs
> 	If device has ATS
> 		Flush device IOTLB
> 
> 	For each IOMMU (dev_iommu list)
> 		call completion wait

This is what I wrote.

> > invalidate_iotlb_v2:
> >    list_for_each_iommu(elm, domain->attachments)
> >        build_cmd_v2_invalidation(&cmd, device->gcr3_cache_tag, elm->pasid, ...);
> >        iommu_queue_command(elm->iommu, &cmd);
> 
> From driver point of view, fundamentally V2 invalidation is not too different as
> we have single invalidation command. All we need is few extra param to tell its
> guest page table invalidation with PASID.

From a SW perspective it is totally different because V1 invalidates a
single domain id per IOMMU and V2 invalidates a domain_id&PASID for
every device.

> > invalidate_ats:
> >    list_for_each(elm, domain->attachments)
> >        if (!elm->device->ats enabled)
> >              continue
> >        build_cmd_atc_invalidation(&cmd, elm->device, elm->pasid, ...);
> >        iommu_queue_command(elm->iommu, &cmd);
> 
> 
> Driver already does this.

The SVA series had code like this, I'm saying you need to generalize
it.

> > Where list_for_each_iommu de-duplicates the iommus from the sorted
> > list, Michael had a series that showed how to do this for SMMU.
> > 
> > Basically you precalculate exactly the invalidations required and
> > store it in a list associated with the domain. When it is time to do
> > an invalidation then you just walk the list and do exactly what it
> > says.
> 
> I think we have most things already in protection domain. Only extra check we
> have is checking `pdom->dev_iommu[i]` which will be fixed separately.

You have it but it is not structured in a logical way, that is why
this series has introduced nonsensical things like a PASID for a
domain, encoding the V1/v2 state ina PASID/etc, and then did a half
version of this list anyway to make SVA work.

Bring the list from the SVA series into this series, use it
consistently, remove the weird stuff and then it will make sense.

Do not have a list *and* a bunch of weird stuff, that is moving
further away from what it needs to look like..

Jason

Re: [PATCH v3 06/13] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

On 11/7/2023 6:51 PM, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2023 at 11:00:18AM +0530, Vasant Hegde wrote:
>>
>>
>> On 11/6/2023 7:06 PM, Jason Gunthorpe wrote:
>>> On Mon, Nov 06, 2023 at 06:09:47PM +0530, Vasant Hegde wrote:
>>>
>>>>> I think this driver really suffers from not having the right
>>>>> data structures to handle everything cleanly.
>>>>>
>>>>> In smmuv3 I added a 'master_domain' structure that linked the PCI
>>>>> device to the iommu_domain. ie when attach is done you'd create a new
>>>>> master_domain that essentially stores the parameters required to do
>>>>> invalidation.
>>>>>
>>>>> For what is going on here I would say to do that an then put the
>>>>> "domain id" inside the "master_domain". Decide when the domain is
>>>>> attached if the domain id should by taken from the iommu_domain
>>>>> (device does not support PASID) or from the device (device does
>>>>> support PASID)
>>>>
>>>> We still have single domain concept (at least until we introduce
>>>> vIOMMU). All we are changing is how we allocate domain ID.
>>>>
>>>> Having another domain for each device just to keep invalidation info is
>>>> complicates things. Also IMO its unnecessary.
>>>
>>> It is not another domain, it is cleaning up the mess of keeping track
>>> of what caches need to be invalidated for a single domain.
>>>
>>> Today we have this:
>>>
>>> struct protection_domain {
>>> 	struct list_head dev_list; /* List of all devices in this domain */
>>> 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
>>>
>>> (and I'm sorry, but using a global array of iommus and this dev_iommu
>>> thing is an insane design)
>>
>> Devices behind different IOMMU can be attached to same domain (like VFIO case).
>> We do need to track the IOMMUs and as part of invalidation we have a requirement
>> to send `completion` command to each IOMMU. So this links domain to IOMMUs.
> 
> I understand how it works.

Great.

> 
>> Array is not a best thing here. I have it in my TODO list to change this to
>> xarray or something. But that's after SVA series as we are already making too
>> many changes to fundamental data structure in this series.
> 
> 'unsigned dev_iommu' is the problem not the array.

I know. I will fix this after SVA series.

>  
>>> Now this adds a new concept domain_id_is_per_dev(), and it still
>>> doesn't support PASID properly!
>>>
>>> Instead write it like this:
>>>
>>> struct attachment {
>>>     struct list_head attachments_item;
>>>     struct amd_iommu *iommu;
>>>     struct iommu_dev_data *device;
>>>     ioasid_t pasid
>>> }
>>>
>>> struct protection_domain {
>>>     struct list_head attachments;
>>>
>>>
>>> Where every ops->attach_dev allocates a new struct attachment and
>>> threads it on the liked list of the protection_domain.
>>
>>
>> We support PASID only in V2 page table mode. V1 does not have PASID stuff. So we
>> just have list of devices in the domain. Then each device has PASID table (that
>> what this series does).
> 
> Doesn't matter, v1 uses the dev_iommu and the point is to consolidate
> alll of this.
> 
>> Also as mentioned above we have the requirement of `completion wait` call for
>> each IOMMU. Hence we track the IOMMU list. Having it per device like above
>> increases completion wait calls which is not good. IMO above changes
>> unnecessarily complicates stuff.
> 
> It is not per device, it is still done per-iommu. I wrote:
> 
>     list_for_each_iommu(elm, domain->attachments)
>         build_cmd_v1_invalidation(&cmd, domain->cache_tag,  ...);
>         iommu_queue_command(elm->iommu, &cmd);
> 
> Which is the same work as iterating over protection_domain->dev_iommu,
> the iommus are extracted from the device list which is needed anyhow
> for ATS, PASID, and V2. So just use it everywhere.
> 
>>> Then the invalidation logic become completely straightforward, no
>>> confusing mess:
>>>
>>> invalidate_iotlb_v1:
>>>    list_for_each_iommu(elm, domain->attachments)
>>>        build_cmd_v1_invalidation(&cmd, domain->cache_tag,  ...);
>>>        iommu_queue_command(elm->iommu, &cmd);
>>
>> Ours is domain based invalidation. So our flushing logic is
> 
>> 	Flush IOMMU TLB for each IOMMUs
>> 	If device has ATS
>> 		Flush device IOTLB
>>
>> 	For each IOMMU (dev_iommu list)
>> 		call completion wait
> 
> This is what I wrote.
> 
>>> invalidate_iotlb_v2:
>>>    list_for_each_iommu(elm, domain->attachments)
>>>        build_cmd_v2_invalidation(&cmd, device->gcr3_cache_tag, elm->pasid, ...);
>>>        iommu_queue_command(elm->iommu, &cmd);
>>
>> From driver point of view, fundamentally V2 invalidation is not too different as
>> we have single invalidation command. All we need is few extra param to tell its
>> guest page table invalidation with PASID.
> 
> From a SW perspective it is totally different because V1 invalidates a
> single domain id per IOMMU and V2 invalidates a domain_id&PASID for
> every device.

Right. I'd prefer to do it in steps. I would like to limit this series to
current form. (GCR3 movement and domain ID). Then will have separate series for:
  - reworking protection domain structure
  - Handle domain allocation path (adding BLOCKED domain, some cleanup etc)
  - Fix attach/detach path
  - Other enhancements like PASID table expansion support, per-device-domain-ID
improvement

-Vasant

> 
>>> invalidate_ats:
>>>    list_for_each(elm, domain->attachments)
>>>        if (!elm->device->ats enabled)
>>>              continue
>>>        build_cmd_atc_invalidation(&cmd, elm->device, elm->pasid, ...);
>>>        iommu_queue_command(elm->iommu, &cmd);
>>
>>
>> Driver already does this.
> 
> The SVA series had code like this, I'm saying you need to generalize
> it.
> 
>>> Where list_for_each_iommu de-duplicates the iommus from the sorted
>>> list, Michael had a series that showed how to do this for SMMU.
>>>
>>> Basically you precalculate exactly the invalidations required and
>>> store it in a list associated with the domain. When it is time to do
>>> an invalidation then you just walk the list and do exactly what it
>>> says.
>>
>> I think we have most things already in protection domain. Only extra check we
>> have is checking `pdom->dev_iommu[i]` which will be fixed separately.
> 
> You have it but it is not structured in a logical way, that is why
> this series has introduced nonsensical things like a PASID for a
> domain, encoding the V1/v2 state ina PASID/etc, and then did a half
> version of this list anyway to make SVA work.
> 
> Bring the list from the SVA series into this series, use it
> consistently, remove the weird stuff and then it will make sense.
> 
> Do not have a list *and* a bunch of weird stuff, that is moving
> further away from what it needs to look like..
> 
> Jason

[PATCH v3 07/13] iommu/amd: Add support for device based flush TLB

[Vasant Hegde]

Add support to flush TLB/IOTLB for the given device.

These functions will be used in subsequent patches where we will
introduce per device GCR3 table and SVA support.

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

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index b2071ebc73b5..1f8dde87a287 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -63,6 +63,10 @@ void amd_iommu_domain_update(struct protection_domain *domain);
 void amd_iommu_domain_flush_complete(struct protection_domain *domain);
 void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
 				  u64 address, size_t size);
+void amd_iommu_dev_flush_pasid_pages(struct iommu_dev_data *dev_data,
+				     ioasid_t pasid, u64 address, size_t size);
+void amd_iommu_dev_flush_pasid_all(struct iommu_dev_data *dev_data,
+				   ioasid_t pasid);
 
 int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
 			      unsigned long cr3);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 09762332f681..0c44f3ed0c92 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1633,6 +1633,22 @@ static void amd_iommu_domain_flush_all(struct protection_domain *pdom)
 					    CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
 }
 
+void amd_iommu_dev_flush_pasid_pages(struct iommu_dev_data *dev_data,
+				     ioasid_t pasid, u64 address, size_t size)
+{
+	device_flush_tlb_range(dev_data, pasid, address, size);
+
+	if (dev_data->ats_enabled)
+		device_flush_iotlb_range(dev_data, pasid, address, size);
+}
+
+void amd_iommu_dev_flush_pasid_all(struct iommu_dev_data *dev_data,
+				   ioasid_t pasid)
+{
+	amd_iommu_dev_flush_pasid_pages(dev_data, pasid, 0,
+					CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
+}
+
 void amd_iommu_domain_flush_complete(struct protection_domain *domain)
 {
 	int i;
-- 
2.31.1

[PATCH v3 08/13] iommu/amd: Rearrange GCR3 table setup code

[Vasant Hegde]

Consolidate GCR3 table related code in one place so that its easy
to maintain.

Note that this patch doesn't move __set_gcr3/__clear_gcr3. We are moving
GCR3 table from per domain to per device. Following series will rework
these functions. During that time I will move these functions as well.

No functional changes intended.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 0c44f3ed0c92..27486b905b6b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1849,6 +1849,38 @@ static int setup_gcr3_table(struct protection_domain *domain, int pasids)
 	return 0;
 }
 
+static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
+{
+	int index;
+	u64 *pte;
+
+	while (true) {
+
+		index = (pasid >> (9 * level)) & 0x1ff;
+		pte   = &root[index];
+
+		if (level == 0)
+			break;
+
+		if (!(*pte & GCR3_VALID)) {
+			if (!alloc)
+				return NULL;
+
+			root = (void *)get_zeroed_page(GFP_ATOMIC);
+			if (root == NULL)
+				return NULL;
+
+			*pte = iommu_virt_to_phys(root) | GCR3_VALID;
+		}
+
+		root = iommu_phys_to_virt(*pte & PAGE_MASK);
+
+		level -= 1;
+	}
+
+	return pte;
+}
+
 static void set_dte_entry(struct amd_iommu *iommu,
 			  struct iommu_dev_data *dev_data)
 {
@@ -2632,38 +2664,6 @@ const struct iommu_ops amd_iommu_ops = {
 	}
 };
 
-static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
-{
-	int index;
-	u64 *pte;
-
-	while (true) {
-
-		index = (pasid >> (9 * level)) & 0x1ff;
-		pte   = &root[index];
-
-		if (level == 0)
-			break;
-
-		if (!(*pte & GCR3_VALID)) {
-			if (!alloc)
-				return NULL;
-
-			root = (void *)get_zeroed_page(GFP_ATOMIC);
-			if (root == NULL)
-				return NULL;
-
-			*pte = iommu_virt_to_phys(root) | GCR3_VALID;
-		}
-
-		root = iommu_phys_to_virt(*pte & PAGE_MASK);
-
-		level -= 1;
-	}
-
-	return pte;
-}
-
 static int __set_gcr3(struct protection_domain *domain, u32 pasid,
 		      unsigned long cr3)
 {
-- 
2.31.1

Re: [PATCH v3 08/13] iommu/amd: Rearrange GCR3 table setup code

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:47PM +0000, Vasant Hegde wrote:
> Consolidate GCR3 table related code in one place so that its easy
> to maintain.
> 
> Note that this patch doesn't move __set_gcr3/__clear_gcr3. We are moving
> GCR3 table from per domain to per device. Following series will rework
> these functions. During that time I will move these functions as well.
> 
> No functional changes intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 64 +++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)

Same remark as before about just moving code..

Jason

[PATCH v3 09/13] iommu/amd: Refactor helper function for setting / clearing GCR3

[Vasant Hegde]

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

Refactor GCR3 helper functions in preparation to use per device
GCR3 table.
  * Use new per device GCR3 table to set/clear the gcr3 entries.

  * Add internal functions which will be used by subsequent patches
    to set/clear default gcr3 entries.

  * Remove per domain default GCR3 setup during v2 page table allocation.
    Subsequent patch will add support to setup default gcr3 while
    attaching device to domain.

  * Remove amd_iommu_domain_update() from V2 page table path as device
    detach path will take care of updating the domain.

  * Consolidate GCR3 table related code in one place so that its easy
    to maintain.

  * Rename functions to reflect its usage.

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     |   9 +-
 drivers/iommu/amd/io_pgtable_v2.c |  21 +----
 drivers/iommu/amd/iommu.c         | 133 ++++++++++++++++--------------
 3 files changed, 77 insertions(+), 86 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 1f8dde87a287..1fceaf4a8229 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -52,6 +52,11 @@ int amd_iommu_pc_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
 int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev);
 void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev);
 
+/* GCR3 setup */
+int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data,
+		       ioasid_t pasid, unsigned long gcr3);
+int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid);
+
 /* TLB flush */
 /*
  * This function flushes all internal caches of
@@ -68,10 +73,6 @@ void amd_iommu_dev_flush_pasid_pages(struct iommu_dev_data *dev_data,
 void amd_iommu_dev_flush_pasid_all(struct iommu_dev_data *dev_data,
 				   ioasid_t pasid);
 
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
-			      unsigned long cr3);
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid);
-
 #ifdef CONFIG_IRQ_REMAP
 int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
 #else
diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index 6d69ba60744f..93489d2db4e8 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -350,38 +350,26 @@ static const struct iommu_flush_ops v2_flush_ops = {
 
 static void v2_free_pgtable(struct io_pgtable *iop)
 {
-	struct protection_domain *pdom;
 	struct amd_io_pgtable *pgtable = container_of(iop, struct amd_io_pgtable, iop);
 
-	pdom = container_of(pgtable, struct protection_domain, iop);
-	if (!(pdom->flags & PD_IOMMUV2_MASK))
+	if (!pgtable || !pgtable->pgd)
 		return;
 
-	/* Clear gcr3 entry */
-	amd_iommu_domain_clear_gcr3(&pdom->domain, 0);
-
-	/* Make changes visible to IOMMUs */
-	amd_iommu_domain_update(pdom);
-
 	/* Free page table */
 	free_pgtable(pgtable->pgd, get_pgtable_level());
+	pgtable->pgd = NULL;
 }
 
 static struct io_pgtable *v2_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
 {
 	struct amd_io_pgtable *pgtable = io_pgtable_cfg_to_data(cfg);
 	struct protection_domain *pdom = (struct protection_domain *)cookie;
-	int ret;
 	int ias = IOMMU_IN_ADDR_BIT_SIZE;
 
 	pgtable->pgd = alloc_pgtable_page(pdom->nid, GFP_ATOMIC);
 	if (!pgtable->pgd)
 		return NULL;
 
-	ret = amd_iommu_domain_set_gcr3(&pdom->domain, 0, iommu_virt_to_phys(pgtable->pgd));
-	if (ret)
-		goto err_free_pgd;
-
 	if (get_pgtable_level() == PAGE_MODE_5_LEVEL)
 		ias = 57;
 
@@ -395,11 +383,6 @@ static struct io_pgtable *v2_alloc_pgtable(struct io_pgtable_cfg *cfg, void *coo
 	cfg->tlb           = &v2_flush_ops;
 
 	return &pgtable->iop;
-
-err_free_pgd:
-	free_pgtable_page(pgtable->pgd);
-
-	return NULL;
 }
 
 struct io_pgtable_init_fns io_pgtable_amd_iommu_v2_init_fns = {
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 27486b905b6b..1ef6cbf71926 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1849,10 +1849,13 @@ static int setup_gcr3_table(struct protection_domain *domain, int pasids)
 	return 0;
 }
 
-static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
+static u64 *__get_gcr3_pte(struct gcr3_tbl_info *gcr3_info,
+			   ioasid_t pasid, bool alloc)
 {
 	int index;
 	u64 *pte;
+	u64 *root = gcr3_info->gcr3_tbl;
+	int level = gcr3_info->glx;
 
 	while (true) {
 
@@ -1881,6 +1884,72 @@ static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
 	return pte;
 }
 
+static int __set_gcr3(struct iommu_dev_data *dev_data,
+		      ioasid_t pasid, unsigned long gcr3)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	u64 *pte;
+
+	lockdep_assert_held(&dev_data->lock);
+
+	pte = __get_gcr3_pte(gcr3_info, pasid, true);
+	if (pte == NULL)
+		return -ENOMEM;
+
+	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
+	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
+
+	return 0;
+}
+
+int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid,
+		       unsigned long gcr3)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	int ret;
+
+	spin_lock(&dev_data->lock);
+
+	ret = __set_gcr3(dev_data, pasid, gcr3);
+	if (!ret)
+		gcr3_info->pasid_cnt++;
+
+	spin_unlock(&dev_data->lock);
+	return ret;
+}
+
+static int __clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	u64 *pte;
+
+	lockdep_assert_held(&dev_data->lock);
+
+	pte = __get_gcr3_pte(gcr3_info, pasid, false);
+	if (pte == NULL)
+		return -EINVAL;
+
+	*pte = 0;
+	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
+
+	return 0;
+}
+
+int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	int ret;
+
+	spin_lock(&dev_data->lock);
+
+	ret = __clear_gcr3(dev_data, pasid);
+	if (!ret)
+		gcr3_info->pasid_cnt--;
+
+	spin_unlock(&dev_data->lock);
+	return ret;
+}
+
 static void set_dte_entry(struct amd_iommu *iommu,
 			  struct iommu_dev_data *dev_data)
 {
@@ -2664,68 +2733,6 @@ const struct iommu_ops amd_iommu_ops = {
 	}
 };
 
-static int __set_gcr3(struct protection_domain *domain, u32 pasid,
-		      unsigned long cr3)
-{
-	u64 *pte;
-
-	if (domain->iop.mode != PAGE_MODE_NONE)
-		return -EINVAL;
-
-	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, true);
-	if (pte == NULL)
-		return -ENOMEM;
-
-	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
-
-	domain_flush_pages(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
-	return 0;
-}
-
-static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
-{
-	u64 *pte;
-
-	if (domain->iop.mode != PAGE_MODE_NONE)
-		return -EINVAL;
-
-	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, false);
-	if (pte == NULL)
-		return 0;
-
-	*pte = 0;
-
-	domain_flush_pages(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
-	return 0;
-}
-
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
-			      unsigned long cr3)
-{
-	struct protection_domain *domain = to_pdomain(dom);
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&domain->lock, flags);
-	ret = __set_gcr3(domain, pasid, cr3);
-	spin_unlock_irqrestore(&domain->lock, flags);
-
-	return ret;
-}
-
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid)
-{
-	struct protection_domain *domain = to_pdomain(dom);
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&domain->lock, flags);
-	ret = __clear_gcr3(domain, pasid);
-	spin_unlock_irqrestore(&domain->lock, flags);
-
-	return ret;
-}
-
 #ifdef CONFIG_IRQ_REMAP
 
 /*****************************************************************************
-- 
2.31.1

Re: [PATCH v3 09/13] iommu/amd: Refactor helper function for setting / clearing GCR3

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:48PM +0000, Vasant Hegde wrote:
> +static int __set_gcr3(struct iommu_dev_data *dev_data,
> +		      ioasid_t pasid, unsigned long gcr3)
> +{
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +	u64 *pte;
> +
> +	lockdep_assert_held(&dev_data->lock);
> +
> +	pte = __get_gcr3_pte(gcr3_info, pasid, true);
> +	if (pte == NULL)
> +		return -ENOMEM;
> +
> +	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
> +	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
> +
> +	return 0;
> +}
> +
> +int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid,
> +		       unsigned long gcr3)
> +{
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +	int ret;
> +
> +	spin_lock(&dev_data->lock);
> +
> +	ret = __set_gcr3(dev_data, pasid, gcr3);
> +	if (!ret)
> +		gcr3_info->pasid_cnt++;
> +
> +	spin_unlock(&dev_data->lock);

I didn't check carefully but most likely this lock is not needed.

All the attach/detach ops which are the only thing that should alter
the GCR3 table are already called under the group->mutex.

I've been thinking about exporting a lockdep assertion for this, but
drivers already rely on this propery.

Previously when the domain was holding the gcr3 it did need a
dedicated lock.

Jason

Re: [PATCH v3 09/13] iommu/amd: Refactor helper function for setting / clearing GCR3

[Vasant Hegde]

On 11/6/2023 10:21 PM, Jason Gunthorpe wrote:
> On Fri, Oct 13, 2023 at 03:16:48PM +0000, Vasant Hegde wrote:
>> +static int __set_gcr3(struct iommu_dev_data *dev_data,
>> +		      ioasid_t pasid, unsigned long gcr3)
>> +{
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +	u64 *pte;
>> +
>> +	lockdep_assert_held(&dev_data->lock);
>> +
>> +	pte = __get_gcr3_pte(gcr3_info, pasid, true);
>> +	if (pte == NULL)
>> +		return -ENOMEM;
>> +
>> +	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
>> +	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
>> +
>> +	return 0;
>> +}
>> +
>> +int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid,
>> +		       unsigned long gcr3)
>> +{
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +	int ret;
>> +
>> +	spin_lock(&dev_data->lock);
>> +
>> +	ret = __set_gcr3(dev_data, pasid, gcr3);
>> +	if (!ret)
>> +		gcr3_info->pasid_cnt++;
>> +
>> +	spin_unlock(&dev_data->lock);
> 
> I didn't check carefully but most likely this lock is not needed.
> 
> All the attach/detach ops which are the only thing that should alter
> the GCR3 table are already called under the group->mutex.

Yeah. We can switch to group lock check when its exported.

-Vasant

> 
> I've been thinking about exporting a lockdep assertion for this, but
> drivers already rely on this propery.
> 
> Previously when the domain was holding the gcr3 it did need a
> dedicated lock.
> 
> Jason

[PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Vasant Hegde]

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

To use the new helper function for setting up GCR3 table.

If system is booted with V2 page table then setup default GCR3 with
domain GCR3 pointer. So that all devices in the domain uses same page
table for translation. Also return page table setup status from
do_attach() function.

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 | 53 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 1ef6cbf71926..cb24bfaa9dd0 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2055,14 +2055,37 @@ static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
 	amd_iommu_apply_erratum_63(iommu, devid);
 }
 
-static void do_attach(struct iommu_dev_data *dev_data,
-		      struct protection_domain *domain)
+static int _init_gcr3_tbl(struct iommu_dev_data *dev_data)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+
+	/* By default, GCR3 is set to support non-PASID devices. */
+	gcr3_info->giov = true;
+
+	/*
+	 * By default, setup GCR3 table to support MAX PASIDs
+	 * support by the IOMMU HW.
+	 */
+	return setup_gcr3_table(dev_data->domain, -1);
+}
+
+static inline void _destroy_gcr3_tbl(struct iommu_dev_data *dev_data)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+
+	gcr3_info->giov = false;
+	free_gcr3_table(dev_data->domain);
+}
+
+static int do_attach(struct iommu_dev_data *dev_data,
+		     struct protection_domain *domain)
 {
 	struct amd_iommu *iommu;
+	int ret = 0;
 
 	iommu = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
-		return;
+		return -EINVAL;
 
 	/* Update data structures */
 	dev_data->domain = domain;
@@ -2080,11 +2103,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	if (domain_id_is_per_dev(domain))
 		dev_data->domid = domain_id_alloc();
 
+	/* Init GCR3 table */
+	if (domain->pd_mode == PD_MODE_V2) {
+		ret = _init_gcr3_tbl(dev_data);
+		if (ret)
+			return ret;
+
+		ret = __set_gcr3(dev_data, 0,
+				 iommu_virt_to_phys(domain->iop.pgd));
+		if (ret) {
+			_destroy_gcr3_tbl(dev_data);
+			return ret;
+		}
+	}
+
 	/* Update device table */
 	set_dte_entry(iommu, dev_data);
 	clone_aliases(iommu, dev_data->dev);
 
 	device_flush_dte(dev_data);
+
+	return ret;
 }
 
 static void do_detach(struct iommu_dev_data *dev_data)
@@ -2096,6 +2135,12 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	if (!iommu)
 		return;
 
+	/* Clear GCR3 table */
+	if (domain->pd_mode == PD_MODE_V2) {
+		__clear_gcr3(dev_data, 0);
+		_destroy_gcr3_tbl(dev_data);
+	}
+
 	/* Update data structures */
 	dev_data->domain = NULL;
 	list_del(&dev_data->list);
@@ -2142,7 +2187,7 @@ static int attach_device(struct device *dev,
 	if (dev_is_pci(dev))
 		pdev_enable_caps(to_pci_dev(dev));
 
-	do_attach(dev_data, domain);
+	ret = do_attach(dev_data, domain);
 
 out:
 	spin_unlock(&dev_data->lock);
-- 
2.31.1

Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:49PM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> To use the new helper function for setting up GCR3 table.
> 
> If system is booted with V2 page table then setup default GCR3 with
> domain GCR3 pointer. 

Lets stop talking about "booted with V2 page table" - "If the domain
uses the V2 format then setup a GCR3 table in the device to point to it"

> +static int _init_gcr3_tbl(struct iommu_dev_data *dev_data)
> +{
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +
> +	/* By default, GCR3 is set to support non-PASID devices. */
> +	gcr3_info->giov = true;

I admit I find it really hard to read the AMD spec here.. In terms of
the SW model what modes are actually supportable by HW?

No PASIDs being used:
 RID domain=IDENTITY - yes [V=0]
 RID domain=BLOCKED - yes [V=1, TV=1, GV=0, mode == 0]
 RID domain=v1 - Yes [V=1, TV=1, GV=0, mode != 0]
 RID domain=v2 - Yes [V=1, TV=0, GV=1, GIOV=1]

Some kind of PASID in use:
 RID domain=v2 & PASID - Yes [V=1, TV=0, GV=1, GIOV=1]
 RID domain=V1 & PASID - No??
 RID domain=IDENTITY & PASID - ?? [V=1, TV=1 GV=1, mode=0, GIOV=0] (Section 2.2.7.1?)
 RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 

Did I get it right? If so GIOV should ultimately be deduced based on
what domain the RID has?

Look at how the SMMUv3 stuff ended up. Their STE is the same purpose
as the AMD DTE. There are alot of combinations here, it was hard to
make a code flow that was clean. It turned out pretty good when the
DTE was generated in the ops->attach based on a calculation of exactly
what the current configuration is, because we already know what we are
in alot of detail at that point.

eg we know if we are attaching an identity domain and PASIDs are in
use that a single specific DTE should be created. So just call a
function directly to get the required DTE.

IOW - I'm not sure it really makes logical sense to store giov in
gcr3_info.

> +static int do_attach(struct iommu_dev_data *dev_data,
> +		     struct protection_domain *domain)
>  {
>  	struct amd_iommu *iommu;
> +	int ret = 0;
>  
>  	iommu = get_amd_iommu_from_dev(dev_data->dev);
>  	if (!iommu)
> -		return;
> +		return -EINVAL;

iommu can't be null here, have a dev_data.

>  	dev_data->domain = domain;
> @@ -2080,11 +2103,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
>  	if (domain_id_is_per_dev(domain))
>  		dev_data->domid = domain_id_alloc();

At some point this is the wrong place to put this, the domain ID is
logically associated with the gcr3 table, it should never be used if
there is no gcr table allocated, and it should be freed once the gcr3
table is freed.

> +	/* Init GCR3 table */
> +	if (domain->pd_mode == PD_MODE_V2) {

Is there a case where domain_id_is_per_dev() but we are attaching a v1
table?

Jason

Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Vasant Hegde]

On 11/6/2023 10:59 PM, Jason Gunthorpe wrote:
> On Fri, Oct 13, 2023 at 03:16:49PM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> To use the new helper function for setting up GCR3 table.
>>
>> If system is booted with V2 page table then setup default GCR3 with
>> domain GCR3 pointer. 
> 
> Lets stop talking about "booted with V2 page table" - "If the domain
> uses the V2 format then setup a GCR3 table in the device to point to it"
> 
>> +static int _init_gcr3_tbl(struct iommu_dev_data *dev_data)
>> +{
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +
>> +	/* By default, GCR3 is set to support non-PASID devices. */
>> +	gcr3_info->giov = true;
> 
> I admit I find it really hard to read the AMD spec here.. In terms of

Sorry. I can't help.

> the SW model what modes are actually supportable by HW?
> 
> No PASIDs being used:
>  RID domain=IDENTITY - yes [V=0]

That's still valid. DTE[V]=1

>  RID domain=BLOCKED - yes [V=1, TV=1, GV=0, mode == 0]

May be TV=1 is valid here, but I am not too sure. I am yet to explore this one.
Rest of the flags are fine.

>  RID domain=v1 - Yes [V=1, TV=1, GV=0, mode != 0]
>  RID domain=v2 - Yes [V=1, TV=0, GV=1, GIOV=1]

TV=1. Rest of the flags are fine.


> 
> Some kind of PASID in use:
>  RID domain=v2 & PASID - Yes [V=1, TV=0, GV=1, GIOV=1]

Correct.

>  RID domain=V1 & PASID - No??

Correct. No PASID support.

>  RID domain=IDENTITY & PASID - ?? [V=1, TV=1 GV=1, mode=0, GIOV=0] (Section 2.2.7.1?)

This is supported.

>  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 

I haven't thought this scenario. Why do we even need this case?

> 
> Did I get it right? If so GIOV should ultimately be deduced based on
> what domain the RID has?
> 
> Look at how the SMMUv3 stuff ended up. Their STE is the same purpose
> as the AMD DTE. There are alot of combinations here, it was hard to
> make a code flow that was clean. It turned out pretty good when the
> DTE was generated in the ops->attach based on a calculation of exactly
> what the current configuration is, because we already know what we are
> in alot of detail at that point.
> 
> eg we know if we are attaching an identity domain and PASIDs are in
> use that a single specific DTE should be created. So just call a
> function directly to get the required DTE.
> 
> IOW - I'm not sure it really makes logical sense to store giov in
> gcr3_info.

We had a choice of having a giov flag inside gcr3_info as it tells how to
configure GCR3 related bits in DTE -OR- having a extra logic to calculate it
every time. I can be calculated.


> 
>> +static int do_attach(struct iommu_dev_data *dev_data,
>> +		     struct protection_domain *domain)
>>  {
>>  	struct amd_iommu *iommu;
>> +	int ret = 0;
>>  
>>  	iommu = get_amd_iommu_from_dev(dev_data->dev);
>>  	if (!iommu)
>> -		return;
>> +		return -EINVAL;
> 
> iommu can't be null here, have a dev_data.

Fixed.

> 
>>  	dev_data->domain = domain;
>> @@ -2080,11 +2103,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
>>  	if (domain_id_is_per_dev(domain))
>>  		dev_data->domid = domain_id_alloc();
> 
> At some point this is the wrong place to put this, the domain ID is
> logically associated with the gcr3 table, it should never be used if
> there is no gcr table allocated, and it should be freed once the gcr3
> table is freed.

Domain ID is decided based on page table type (and may be based on PASID later).
 That's why I have a function to decide whether to allocate ID or not and it
should be done in this path only. So that we can configure DTE.



> 
>> +	/* Init GCR3 table */
>> +	if (domain->pd_mode == PD_MODE_V2) {
> 
> Is there a case where domain_id_is_per_dev() but we are attaching a v1
> table?

Not today.

-Vasant

Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Jason Gunthorpe]

On Tue, Nov 07, 2023 at 11:25:49AM +0530, Vasant Hegde wrote:

> >  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 
> 
> I haven't thought this scenario. Why do we even need this case?

It is triggerable by iommufd, it has to be supported.

You understand I'm going to insist that *all* of this is fixed before
you can progress with any new iommufd features, right? We must have
drivers implement the APIs correctly when they are exposed to
userspace. I sent patches fixing SMMUv3, we will have to make the same
fixing for AMD and Intel too.

> > Did I get it right? If so GIOV should ultimately be deduced based on
> > what domain the RID has?
> > 
> > Look at how the SMMUv3 stuff ended up. Their STE is the same purpose
> > as the AMD DTE. There are alot of combinations here, it was hard to
> > make a code flow that was clean. It turned out pretty good when the
> > DTE was generated in the ops->attach based on a calculation of exactly
> > what the current configuration is, because we already know what we are
> > in alot of detail at that point.
> > 
> > eg we know if we are attaching an identity domain and PASIDs are in
> > use that a single specific DTE should be created. So just call a
> > function directly to get the required DTE.
> > 
> > IOW - I'm not sure it really makes logical sense to store giov in
> > gcr3_info.
> 
> We had a choice of having a giov flag inside gcr3_info as it tells how to
> configure GCR3 related bits in DTE -OR- having a extra logic to calculate it
> every time. I can be calculated.

The codepath that determines the DTE will know already what the value
should be. Again look at how SMMUv3 turned out, the GIOV is very
similar to S1DSS.

> >>  	dev_data->domain = domain;
> >> @@ -2080,11 +2103,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
> >>  	if (domain_id_is_per_dev(domain))
> >>  		dev_data->domid = domain_id_alloc();
> > 
> > At some point this is the wrong place to put this, the domain ID is
> > logically associated with the gcr3 table, it should never be used if
> > there is no gcr table allocated, and it should be freed once the gcr3
> > table is freed.
> 
> Domain ID is decided based on page table type (and may be based on PASID later).
>  That's why I have a function to decide whether to allocate ID or not and it
> should be done in this path only. So that we can configure DTE.

The page table type determines if the GCR3 table is loaded, the domain
ID is needed if the GCR3 table is loaded.

Logically the domain ID is part of the GCR3 table.

Jason

Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Vasant Hegde]

On 11/7/2023 6:58 PM, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2023 at 11:25:49AM +0530, Vasant Hegde wrote:
> 
>>>  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 
>>
>> I haven't thought this scenario. Why do we even need this case?
> 
> It is triggerable by iommufd, it has to be supported.

You mean PASID support with BLOCKED domain?

> 
> You understand I'm going to insist that *all* of this is fixed before
> you can progress with any new iommufd features, right? We must have

I have seen those part of code and it makes sense to have BLOCKED domain
implemented. Its in our TODO list. I intended to look into domain allocation
path more closely once we finish SVA series.

> drivers implement the APIs correctly when they are exposed to
> userspace. I sent patches fixing SMMUv3, we will have to make the same
> fixing for AMD and Intel too.
> 
>>> Did I get it right? If so GIOV should ultimately be deduced based on
>>> what domain the RID has?
>>>
>>> Look at how the SMMUv3 stuff ended up. Their STE is the same purpose
>>> as the AMD DTE. There are alot of combinations here, it was hard to
>>> make a code flow that was clean. It turned out pretty good when the
>>> DTE was generated in the ops->attach based on a calculation of exactly
>>> what the current configuration is, because we already know what we are
>>> in alot of detail at that point.
>>>
>>> eg we know if we are attaching an identity domain and PASIDs are in
>>> use that a single specific DTE should be created. So just call a
>>> function directly to get the required DTE.
>>>
>>> IOW - I'm not sure it really makes logical sense to store giov in
>>> gcr3_info.
>>
>> We had a choice of having a giov flag inside gcr3_info as it tells how to
>> configure GCR3 related bits in DTE -OR- having a extra logic to calculate it
>> every time. I can be calculated.
> 
> The codepath that determines the DTE will know already what the value
> should be. Again look at how SMMUv3 turned out, the GIOV is very
> similar to S1DSS.

I have removed `giov` variable. Instead I have page table check.

> 
>>>>  	dev_data->domain = domain;
>>>> @@ -2080,11 +2103,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
>>>>  	if (domain_id_is_per_dev(domain))
>>>>  		dev_data->domid = domain_id_alloc();
>>>
>>> At some point this is the wrong place to put this, the domain ID is
>>> logically associated with the gcr3 table, it should never be used if
>>> there is no gcr table allocated, and it should be freed once the gcr3
>>> table is freed.
>>
>> Domain ID is decided based on page table type (and may be based on PASID later).
>>  That's why I have a function to decide whether to allocate ID or not and it
>> should be done in this path only. So that we can configure DTE.
> 
> The page table type determines if the GCR3 table is loaded, the domain
> ID is needed if the GCR3 table is loaded.

For now I'd prefer to keep it in do_attach/detach path. I want to rework
per-device-domain-ID after SVA series (i.e. allocate per-device-domain-ID only
when its needed).

-Vasant

Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Jason Gunthorpe]

On Thu, Nov 23, 2023 at 11:09:20PM +0530, Vasant Hegde wrote:
> 
> 
> On 11/7/2023 6:58 PM, Jason Gunthorpe wrote:
> > On Tue, Nov 07, 2023 at 11:25:49AM +0530, Vasant Hegde wrote:
> > 
> >>>  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 
> >>
> >> I haven't thought this scenario. Why do we even need this case?
> > 
> > It is triggerable by iommufd, it has to be supported.
> 
> You mean PASID support with BLOCKED domain?

Yes, that is API possible.

> I have seen those part of code and it makes sense to have BLOCKED domain
> implemented. Its in our TODO list. I intended to look into domain allocation
> path more closely once we finish SVA series.

Great!
 
> > The page table type determines if the GCR3 table is loaded, the domain
> > ID is needed if the GCR3 table is loaded.
> 
> For now I'd prefer to keep it in do_attach/detach path. I want to rework
> per-device-domain-ID after SVA series (i.e. allocate per-device-domain-ID only
> when its needed).

Yeah, that sounds OK

Jason

Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Vasant Hegde]

Jason,


On 11/30/2023 11:25 PM, Jason Gunthorpe wrote:
> On Thu, Nov 23, 2023 at 11:09:20PM +0530, Vasant Hegde wrote:
>>
>>
>> On 11/7/2023 6:58 PM, Jason Gunthorpe wrote:
>>> On Tue, Nov 07, 2023 at 11:25:49AM +0530, Vasant Hegde wrote:
>>>
>>>>>  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 
>>>>
>>>> I haven't thought this scenario. Why do we even need this case?
>>>
>>> It is triggerable by iommufd, it has to be supported.
>>
>> You mean PASID support with BLOCKED domain?
> 
> Yes, that is API possible.

This will be tricky as we set DTE flags per device, not per PASID. This needs
more investigation. We will implement BLOCKING domain first then we can look
into PASID with BLOCKED domain support.

-Vasant

Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 11:11:24AM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 11/30/2023 11:25 PM, Jason Gunthorpe wrote:
> > On Thu, Nov 23, 2023 at 11:09:20PM +0530, Vasant Hegde wrote:
> >>
> >>
> >> On 11/7/2023 6:58 PM, Jason Gunthorpe wrote:
> >>> On Tue, Nov 07, 2023 at 11:25:49AM +0530, Vasant Hegde wrote:
> >>>
> >>>>>  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 
> >>>>
> >>>> I haven't thought this scenario. Why do we even need this case?
> >>>
> >>> It is triggerable by iommufd, it has to be supported.
> >>
> >> You mean PASID support with BLOCKED domain?
> > 
> > Yes, that is API possible.
> 
> This will be tricky as we set DTE flags per device, not per PASID. This needs
> more investigation. We will implement BLOCKING domain first then we can look
> into PASID with BLOCKED domain support.

You can look at how the smmu stuff ended up for some inspiration, it
is quite similar.

It is what I keep saying about properly structuring things so the DTE
content is decided high up and then flowed down to programming
logic. Then you don't get stuck with "we set DTE flags based on the
device" which is not actually workable.

Jason

Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Vasant Hegde]

Jason,


On 12/12/2023 8:29 PM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 11:11:24AM +0530, Vasant Hegde wrote:
>> Jason,
>>
>>
>> On 11/30/2023 11:25 PM, Jason Gunthorpe wrote:
>>> On Thu, Nov 23, 2023 at 11:09:20PM +0530, Vasant Hegde wrote:
>>>>
>>>>
>>>> On 11/7/2023 6:58 PM, Jason Gunthorpe wrote:
>>>>> On Tue, Nov 07, 2023 at 11:25:49AM +0530, Vasant Hegde wrote:
>>>>>
>>>>>>>  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 
>>>>>>
>>>>>> I haven't thought this scenario. Why do we even need this case?
>>>>>
>>>>> It is triggerable by iommufd, it has to be supported.
>>>>
>>>> You mean PASID support with BLOCKED domain?
>>>
>>> Yes, that is API possible.
>>
>> This will be tricky as we set DTE flags per device, not per PASID. This needs
>> more investigation. We will implement BLOCKING domain first then we can look
>> into PASID with BLOCKED domain support.
> 
> You can look at how the smmu stuff ended up for some inspiration, it
> is quite similar.
> 
> It is what I keep saying about properly structuring things so the DTE
> content is decided high up and then flowed down to programming
> logic. Then you don't get stuck with "we set DTE flags based on the
> device" which is not actually workable.

Agreed. That's the ultimiate goal. Just that we want to do it in stages so that
its easy to manage. I have posted v4 last week. Please take a look.

[1]
https://lore.kernel.org/linux-iommu/20231212085224.6985-1-vasant.hegde@amd.com/]


-Vasant

[PATCH v3 11/13] iommu/amd: Refactor protection_domain helper functions

[Vasant Hegde]

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

To removes the code to setup GCR3 table, and only handle domain
create / destroy, since GCR3 is no longer part of a domain.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index cb24bfaa9dd0..302ae7ed6d7c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2335,9 +2335,6 @@ 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);
 
@@ -2365,15 +2362,10 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 	return 0;
 }
 
-static int protection_domain_init_v2(struct protection_domain *domain)
+static int protection_domain_init_v2(struct protection_domain *pdom)
 {
-	domain->flags |= PD_GIOV_MASK;
-	domain->pd_mode = PD_MODE_V2;
-
-	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
-
-	if (setup_gcr3_table(domain, 1))
-		return -ENOMEM;
+	pdom->pd_mode = PD_MODE_V2;
+	pdom->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
 
 	return 0;
 }
-- 
2.31.1

Re: [PATCH v3 11/13] iommu/amd: Refactor protection_domain helper functions

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:50PM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> To removes the code to setup GCR3 table, and only handle domain
> create / destroy, since GCR3 is no longer part of a domain.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)

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

[PATCH v3 12/13] iommu/amd: Refactor GCR3 table helper functions

[Vasant Hegde]

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

To use the new per-device struct gcr3_tbl_info. Also modify
set_dte_entry() to use new per device GCR3 table.

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 | 57 +++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 302ae7ed6d7c..988341ec499d 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1801,16 +1801,26 @@ static void free_gcr3_tbl_level2(u64 *tbl)
 	}
 }
 
-static void free_gcr3_table(struct protection_domain *domain)
+static void free_gcr3_table(struct iommu_dev_data *dev_data)
 {
-	if (domain->glx == 2)
-		free_gcr3_tbl_level2(domain->gcr3_tbl);
-	else if (domain->glx == 1)
-		free_gcr3_tbl_level1(domain->gcr3_tbl);
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
+
+	if (gcr3_info->glx == 2)
+		free_gcr3_tbl_level2(gcr3_info->gcr3_tbl);
+	else if (gcr3_info->glx == 1)
+		free_gcr3_tbl_level1(gcr3_info->gcr3_tbl);
 	else
-		BUG_ON(domain->glx != 0);
+		WARN_ON_ONCE(gcr3_info->glx != 0);
+
+	gcr3_info->glx = 0;
+
+	set_dte_entry(iommu, dev_data);
+	clone_aliases(iommu, dev_data->dev);
+	device_flush_dte(dev_data);
 
-	free_page((unsigned long)domain->gcr3_tbl);
+	free_page((unsigned long)gcr3_info->gcr3_tbl);
+	gcr3_info->gcr3_tbl = NULL;
 }
 
 /*
@@ -1829,22 +1839,28 @@ static int get_gcr3_levels(int pasids)
 	return levels ? (DIV_ROUND_UP(levels, 9) - 1) : 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)
+static int setup_gcr3_table(struct iommu_dev_data *dev_data, int pasids)
 {
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
 	int levels = get_gcr3_levels(pasids);
 
 	if (levels > amd_iommu_max_glx_val)
 		return -EINVAL;
 
-	domain->gcr3_tbl = alloc_pgtable_page(domain->nid, GFP_ATOMIC);
-	if (domain->gcr3_tbl == NULL)
+	if (gcr3_info->gcr3_tbl)
+		return -EBUSY;
+
+	gcr3_info->gcr3_tbl = alloc_pgtable_page(dev_to_node(dev_data->dev),
+						 GFP_ATOMIC);
+	if (gcr3_info->gcr3_tbl == NULL)
 		return -ENOMEM;
 
-	domain->glx      = levels;
-	domain->flags   |= PD_IOMMUV2_MASK;
+	gcr3_info->glx = levels;
 
-	amd_iommu_domain_update(domain);
+	set_dte_entry(iommu, dev_data);
+	clone_aliases(iommu, dev_data->dev);
+	device_flush_dte(dev_data);
 
 	return 0;
 }
@@ -1960,6 +1976,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	u16 domid;
 	struct protection_domain *domain = dev_data->domain;
 	struct dev_table_entry *dev_table = get_dev_table(iommu);
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
 
 	if (domain_id_is_per_dev(domain))
 		domid = dev_data->domid;
@@ -1989,9 +2006,9 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	if (dev_data->ppr)
 		pte_root |= 1ULL << DEV_ENTRY_PPR;
 
-	if (domain->flags & PD_IOMMUV2_MASK) {
-		u64 gcr3 = iommu_virt_to_phys(domain->gcr3_tbl);
-		u64 glx  = domain->glx;
+	if (gcr3_info && gcr3_info->gcr3_tbl) {
+		u64 gcr3 = iommu_virt_to_phys(gcr3_info->gcr3_tbl);
+		u64 glx  = gcr3_info->glx;
 		u64 tmp;
 
 		pte_root |= DTE_FLAG_GV;
@@ -2019,7 +2036,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 				((u64)GUEST_PGTABLE_5_LEVEL << DTE_GPT_LEVEL_SHIFT);
 		}
 
-		if (domain->flags & PD_GIOV_MASK)
+		if (gcr3_info->giov)
 			pte_root |= DTE_FLAG_GIOV;
 	}
 
@@ -2066,7 +2083,7 @@ static int _init_gcr3_tbl(struct iommu_dev_data *dev_data)
 	 * By default, setup GCR3 table to support MAX PASIDs
 	 * support by the IOMMU HW.
 	 */
-	return setup_gcr3_table(dev_data->domain, -1);
+	return setup_gcr3_table(dev_data, -1);
 }
 
 static inline void _destroy_gcr3_tbl(struct iommu_dev_data *dev_data)
@@ -2074,7 +2091,7 @@ static inline void _destroy_gcr3_tbl(struct iommu_dev_data *dev_data)
 	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
 
 	gcr3_info->giov = false;
-	free_gcr3_table(dev_data->domain);
+	free_gcr3_table(dev_data);
 }
 
 static int do_attach(struct iommu_dev_data *dev_data,
-- 
2.31.1

Re: [PATCH v3 12/13] iommu/amd: Refactor GCR3 table helper functions

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:51PM +0000, Vasant Hegde wrote:
> -static void free_gcr3_table(struct protection_domain *domain)
> +static void free_gcr3_table(struct iommu_dev_data *dev_data)
>  {
> -	if (domain->glx == 2)
> -		free_gcr3_tbl_level2(domain->gcr3_tbl);
> -	else if (domain->glx == 1)
> -		free_gcr3_tbl_level1(domain->gcr3_tbl);
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
> +
> +	if (gcr3_info->glx == 2)
> +		free_gcr3_tbl_level2(gcr3_info->gcr3_tbl);
> +	else if (gcr3_info->glx == 1)
> +		free_gcr3_tbl_level1(gcr3_info->gcr3_tbl);
>  	else
> -		BUG_ON(domain->glx != 0);
> +		WARN_ON_ONCE(gcr3_info->glx != 0);
> +
> +	gcr3_info->glx = 0;
> +
> +	set_dte_entry(iommu, dev_data);
> +	clone_aliases(iommu, dev_data->dev);
> +	device_flush_dte(dev_data);

This looks super wonky. We free some of the gcr3 memory and *then*
update the DTE? Is that ordered properly?

As per my prior email the caller should have already updated the DTE
because it attached some non-gcr3 needing thing. Once the new DTE is
in place then free_gcr3_table() should simply just free the allocated
memory.

Commingling DTE manipulation inside the gcr3 layer is not good layering.

You should be moving to a direction where the ops->attach_dev does
exactly one update to the DTE. It loads the new correct value of the
DTE that attach_dev is asking to create. All this repeated touching of
the DTE during the attach_dev flow is sort of a functional bug, or at
least a sub-optimal implementation of the API.

> -/* 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)
> +static int setup_gcr3_table(struct iommu_dev_data *dev_data, int pasids)
>  {
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
>  	int levels = get_gcr3_levels(pasids);
>  
>  	if (levels > amd_iommu_max_glx_val)
>  		return -EINVAL;
>  
> -	domain->gcr3_tbl = alloc_pgtable_page(domain->nid, GFP_ATOMIC);
> -	if (domain->gcr3_tbl == NULL)
> +	if (gcr3_info->gcr3_tbl)
> +		return -EBUSY;
> +
> +	gcr3_info->gcr3_tbl = alloc_pgtable_page(dev_to_node(dev_data->dev),
> +						 GFP_ATOMIC);
> +	if (gcr3_info->gcr3_tbl == NULL)
>  		return -ENOMEM;

Why is this in an atomic context? I can understand that the domain was
because it used a spinlock, but this should now be locked by the group
mutex which is not atomic.

Jason

Re: [PATCH v3 12/13] iommu/amd: Refactor GCR3 table helper functions

[Vasant Hegde]

On 11/6/2023 11:10 PM, Jason Gunthorpe wrote:
> On Fri, Oct 13, 2023 at 03:16:51PM +0000, Vasant Hegde wrote:
>> -static void free_gcr3_table(struct protection_domain *domain)
>> +static void free_gcr3_table(struct iommu_dev_data *dev_data)
>>  {
>> -	if (domain->glx == 2)
>> -		free_gcr3_tbl_level2(domain->gcr3_tbl);
>> -	else if (domain->glx == 1)
>> -		free_gcr3_tbl_level1(domain->gcr3_tbl);
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
>> +
>> +	if (gcr3_info->glx == 2)
>> +		free_gcr3_tbl_level2(gcr3_info->gcr3_tbl);
>> +	else if (gcr3_info->glx == 1)
>> +		free_gcr3_tbl_level1(gcr3_info->gcr3_tbl);
>>  	else
>> -		BUG_ON(domain->glx != 0);
>> +		WARN_ON_ONCE(gcr3_info->glx != 0);
>> +
>> +	gcr3_info->glx = 0;
>> +
>> +	set_dte_entry(iommu, dev_data);
>> +	clone_aliases(iommu, dev_data->dev);
>> +	device_flush_dte(dev_data);
> 
> This looks super wonky. We free some of the gcr3 memory and *then*
> update the DTE? Is that ordered properly?

Its not. I have fixed it.

> 
> As per my prior email the caller should have already updated the DTE
> because it attached some non-gcr3 needing thing. Once the new DTE is
> in place then free_gcr3_table() should simply just free the allocated
> memory.

We make sure its detached before attaching to new domain.

> 
> Commingling DTE manipulation inside the gcr3 layer is not good layering.

Its not GCR3 entry update path. Its removing GCR3 completely which needs to be
reflected in DTE. So it makes sense to have a DTE flush. Having it here is
causing extra DTE flush. I have already moved this to do_detach() and
amd_iommu_gcr3_uninit().

> 
> You should be moving to a direction where the ops->attach_dev does
> exactly one update to the DTE. It loads the new correct value of the
> DTE that attach_dev is asking to create. All this repeated touching of
> the DTE during the attach_dev flow is sort of a functional bug, or at
> least a sub-optimal implementation of the API.

We are not touching DTE repeatedly. We do need to detach device (so touch DTE)
and then attach device to domain (another touch).

I am not confident to make change like above (i. e. just attaching device to new
domain and then destroying old domain related data) in this series. Those
improvements can be looked into it later.

> 
>> -/* 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)
>> +static int setup_gcr3_table(struct iommu_dev_data *dev_data, int pasids)
>>  {
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
>>  	int levels = get_gcr3_levels(pasids);
>>  
>>  	if (levels > amd_iommu_max_glx_val)
>>  		return -EINVAL;
>>  
>> -	domain->gcr3_tbl = alloc_pgtable_page(domain->nid, GFP_ATOMIC);
>> -	if (domain->gcr3_tbl == NULL)
>> +	if (gcr3_info->gcr3_tbl)
>> +		return -EBUSY;
>> +
>> +	gcr3_info->gcr3_tbl = alloc_pgtable_page(dev_to_node(dev_data->dev),
>> +						 GFP_ATOMIC);
>> +	if (gcr3_info->gcr3_tbl == NULL)
>>  		return -ENOMEM;
> 
> Why is this in an atomic context? I can understand that the domain was
> because it used a spinlock, but this should now be locked by the group
> mutex which is not atomic.

We still hold device lock. It will be fixed after switch to group mutex lock.

-Vasant

Re: [PATCH v3 12/13] iommu/amd: Refactor GCR3 table helper functions

[Jason Gunthorpe]

On Tue, Nov 07, 2023 at 11:43:10AM +0530, Vasant Hegde wrote:
> > You should be moving to a direction where the ops->attach_dev does
> > exactly one update to the DTE. It loads the new correct value of the
> > DTE that attach_dev is asking to create. All this repeated touching of
> > the DTE during the attach_dev flow is sort of a functional bug, or at
> > least a sub-optimal implementation of the API.
> 
> We are not touching DTE repeatedly. We do need to detach device (so touch DTE)
> and then attach device to domain (another touch).

Twice is repeatedly. Again look at how smmuv3 turned out, there is
exactly *ONE* DTE update per op callback.
 
> I am not confident to make change like above (i. e. just attaching device to new
> domain and then destroying old domain related data) in this series. Those
> improvements can be looked into it later.

Sure, but you need to start organizing the code to work like this with
the proper layers and division of work.

The best advice I can give you is to make it look more like SMMUv3
because that is the only example that solves *everything* If each
series moves things closer to that then you'll be in a better position
to fix everything eventually.

Jason

Re: [PATCH v3 12/13] iommu/amd: Refactor GCR3 table helper functions

[Vasant Hegde]

Jason,


On 11/7/2023 7:01 PM, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2023 at 11:43:10AM +0530, Vasant Hegde wrote:
>>> You should be moving to a direction where the ops->attach_dev does
>>> exactly one update to the DTE. It loads the new correct value of the
>>> DTE that attach_dev is asking to create. All this repeated touching of
>>> the DTE during the attach_dev flow is sort of a functional bug, or at
>>> least a sub-optimal implementation of the API.
>>
>> We are not touching DTE repeatedly. We do need to detach device (so touch DTE)
>> and then attach device to domain (another touch).
> 
> Twice is repeatedly. Again look at how smmuv3 turned out, there is
> exactly *ONE* DTE update per op callback.

I have fixed setup/free gcr3 table path. As mentioned I have added cleaning up
do_attach/detach() path to TODO list. That will be outside this series.

>  
>> I am not confident to make change like above (i. e. just attaching device to new
>> domain and then destroying old domain related data) in this series. Those
>> improvements can be looked into it later.
> 
> Sure, but you need to start organizing the code to work like this with
> the proper layers and division of work.

Ok.

-Vasant

Re: [PATCH v3 12/13] iommu/amd: Refactor GCR3 table helper functions

[Jason Gunthorpe]

On Thu, Nov 23, 2023 at 10:53:23PM +0530, Vasant Hegde wrote:
> On 11/7/2023 7:01 PM, Jason Gunthorpe wrote:
> > On Tue, Nov 07, 2023 at 11:43:10AM +0530, Vasant Hegde wrote:
> >>> You should be moving to a direction where the ops->attach_dev does
> >>> exactly one update to the DTE. It loads the new correct value of the
> >>> DTE that attach_dev is asking to create. All this repeated touching of
> >>> the DTE during the attach_dev flow is sort of a functional bug, or at
> >>> least a sub-optimal implementation of the API.
> >>
> >> We are not touching DTE repeatedly. We do need to detach device (so touch DTE)
> >> and then attach device to domain (another touch).
> > 
> > Twice is repeatedly. Again look at how smmuv3 turned out, there is
> > exactly *ONE* DTE update per op callback.
> 
> I have fixed setup/free gcr3 table path. As mentioned I have added cleaning up
> do_attach/detach() path to TODO list. That will be outside this series.

Sure, I hope to look next week

Thanks,
Jason

[PATCH v3 13/13] iommu/amd: Remove unused GCR3 table parameters from struct protection_domain

[Vasant Hegde]

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

Since they are moved to struct iommu_dev_data, and the driver has been
ported to use them.

Also remove V2 page table check in __flush_pasid() as this gets called
only when we have PASIDs enabled.

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 | 12 ------------
 drivers/iommu/amd/iommu.c           |  2 +-
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index e836438a9318..da94dca1eb92 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -442,15 +442,6 @@
 
 #define MAX_DOMAIN_ID 65536
 
-/* Protection domain flags */
-#define PD_DMA_OPS_MASK		BIT(0) /* domain used for dma_ops */
-#define PD_DEFAULT_MASK		BIT(1) /* domain is a default dma_ops
-					      domain for an IOMMU */
-#define PD_PASSTHROUGH_MASK	BIT(2) /* domain has no page
-					      translation */
-#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
@@ -561,10 +552,7 @@ struct protection_domain {
 	struct amd_io_pgtable iop;
 	spinlock_t lock;	/* mostly used to lock the page table*/
 	u16 id;			/* the domain id written to the device table */
-	int glx;		/* Number of levels for GCR3 table */
 	int nid;		/* Node ID */
-	u64 *gcr3_tbl;		/* Guest CR3 table */
-	unsigned long flags;	/* flags to find out type of domain */
 	enum protection_domain_mode pd_mode; /* Track page table type */
 	unsigned dev_cnt;	/* devices assigned to this domain */
 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 988341ec499d..d6e9f1ebd1ea 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -92,7 +92,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
  */
 static inline ioasid_t pdom_get_default_pasid(struct protection_domain *pdom)
 {
-	return (pdom && (pdom->flags & PD_IOMMUV2_MASK)) ?
+	return (pdom && (pdom->pd_mode == PD_MODE_V2)) ?
 		IOMMU_NO_PASID : IOMMU_PASID_INVALID;
 }
 
-- 
2.31.1

Re: [PATCH v3 13/13] iommu/amd: Remove unused GCR3 table parameters from struct protection_domain

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 03:16:52PM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Since they are moved to struct iommu_dev_data, and the driver has been
> ported to use them.
> 
> Also remove V2 page table check in __flush_pasid() as this gets called
> only when we have PASIDs enabled.
> 
> 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 | 12 ------------
>  drivers/iommu/amd/iommu.c           |  2 +-
>  2 files changed, 1 insertion(+), 13 deletions(-)

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

Jason