[PATCH v3 0/5] iommu/amd: Use 128-bit cmpxchg operation to update DTE

[PATCH v3 0/5] iommu/amd: Use 128-bit cmpxchg operation to update DTE

[Suravee Suthikulpanit]

This series modifies current implementation to use 128-bit cmpxchg to update DTE
when needed as specified in the AMD I/O Virtualization Techonology (IOMMU)
Specification.

Changes in V3:
  * Patch 2:
    - Consolidate patch 2 and 3
    - Change rw_semaphore to spin_lock
  * Patch 3: Expand locking across 256-bit DTE read and update
  * Patch 4: Fix clear_dte_entry()
  * Patch 5: Consolidate amd_iommu_set_dirty_tracking() and set_dte_irq_entry() fixes

v2: https://lore.kernel.org/lkml/20240829180726.5022-1-suravee.suthikulpanit@amd.com/
v1 :https://lore.kernel.org/lkml/e937e26f-038a-6d01-76a9-76c86760ca4a@gmail.com/T/

Thanks,
Suravee

Suravee Suthikulpanit (5):
  iommu/amd: Disable AMD IOMMU if CMPXCHG16B feature is not supported
  iommu/amd: Introduce helper functions to access and update 256-bit DTE
  iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers
  iommu/amd: Modify clear_dte_entry() to avoid in-place update
  iommu/amd: Do not update DTE in-place in amd_iommu_set_dirty_tracking
    and set_dte_irq_entry

 drivers/iommu/amd/amd_iommu_types.h |   8 +-
 drivers/iommu/amd/init.c            |  23 +--
 drivers/iommu/amd/iommu.c           | 272 +++++++++++++++++++---------
 3 files changed, 201 insertions(+), 102 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] iommu/amd: Disable AMD IOMMU if CMPXCHG16B feature is not supported

[Suravee Suthikulpanit]

According to the AMD IOMMU spec, the IOMMU reads the entire DTE either
in two 128-bit transactions or a single 256-bit transaction. It is
recommended to update DTE using 128-bit operation followed by an
INVALIDATE_DEVTAB_ENTYRY command when the IV=1b or V=1b.

According to the AMD BIOS and Kernel Developer's Guide (BDKG) dated back
to family 10h Processor [1], which is the first introduction of AMD IOMMU,
AMD processor always has CPUID Fn0000_0001_ECX[CMPXCHG16B]=1.
Therefore, it is safe to assume cmpxchg128 is available with all AMD
processor w/ IOMMU.

In addition, the CMPXCHG16B feature has already been checked separately
before enabling the GA, XT, and GAM modes. Consolidate the detection logic,
and fail the IOMMU initialization if the feature is not supported.

[1] https://www.amd.com/content/dam/amd/en/documents/archived-tech-docs/programmer-references/31116.pdf

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 6b15ce09e78d..983c09898a10 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1762,13 +1762,8 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h,
 		else
 			iommu->mmio_phys_end = MMIO_CNTR_CONF_OFFSET;
 
-		/*
-		 * Note: GA (128-bit IRTE) mode requires cmpxchg16b supports.
-		 * GAM also requires GA mode. Therefore, we need to
-		 * check cmpxchg16b support before enabling it.
-		 */
-		if (!boot_cpu_has(X86_FEATURE_CX16) ||
-		    ((h->efr_attr & (0x1 << IOMMU_FEAT_GASUP_SHIFT)) == 0))
+		/* GAM requires GA mode. */
+		if ((h->efr_attr & (0x1 << IOMMU_FEAT_GASUP_SHIFT)) == 0)
 			amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY;
 		break;
 	case 0x11:
@@ -1778,13 +1773,8 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h,
 		else
 			iommu->mmio_phys_end = MMIO_CNTR_CONF_OFFSET;
 
-		/*
-		 * Note: GA (128-bit IRTE) mode requires cmpxchg16b supports.
-		 * XT, GAM also requires GA mode. Therefore, we need to
-		 * check cmpxchg16b support before enabling them.
-		 */
-		if (!boot_cpu_has(X86_FEATURE_CX16) ||
-		    ((h->efr_reg & (0x1 << IOMMU_EFR_GASUP_SHIFT)) == 0)) {
+		/* XT and GAM require GA mode. */
+		if ((h->efr_reg & (0x1 << IOMMU_EFR_GASUP_SHIFT)) == 0) {
 			amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY;
 			break;
 		}
@@ -3049,6 +3039,11 @@ static int __init early_amd_iommu_init(void)
 		return -EINVAL;
 	}
 
+	if (!boot_cpu_has(X86_FEATURE_CX16)) {
+		pr_err("Failed to initialize. The CMPXCHG16B feature is required.\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * Validate checksum here so we don't need to do it when
 	 * we actually parse the table
-- 
2.34.1

Re: [PATCH v3 1/5] iommu/amd: Disable AMD IOMMU if CMPXCHG16B feature is not supported

[Jason Gunthorpe]

On Fri, Sep 06, 2024 at 12:13:04PM +0000, Suravee Suthikulpanit wrote:
> According to the AMD IOMMU spec, the IOMMU reads the entire DTE either
> in two 128-bit transactions or a single 256-bit transaction. 

.. if two 128-bit transaction on the read side is possible then you
need flushing! :(

For instance this:

  IOMMU         CPU
Read [0]    
              Write [0]
              Write [1]
Read [1]

Will result in the iommu seeing torn incorrect data - the Guest paging
mode may not match the page table pointer, or the VIOMMU data may
become mismatched to the host translation.

Avoiding flushing is only possible if the full 256 bits are read
atomically.

> It is recommended to update DTE using 128-bit operation followed by
> an INVALIDATE_DEVTAB_ENTYRY command when the IV=1b or V=1b.

This advice only works when going from non-valid to valid.

> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  drivers/iommu/amd/init.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)

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

Jason

Re: [PATCH v3 1/5] iommu/amd: Disable AMD IOMMU if CMPXCHG16B feature is not supported

[Jason Gunthorpe]

On Fri, Sep 06, 2024 at 01:38:18PM -0300, Jason Gunthorpe wrote:
> On Fri, Sep 06, 2024 at 12:13:04PM +0000, Suravee Suthikulpanit wrote:
> > According to the AMD IOMMU spec, the IOMMU reads the entire DTE either
> > in two 128-bit transactions or a single 256-bit transaction. 
> 
> .. if two 128-bit transaction on the read side is possible then you
> need flushing! :(
> 
> For instance this:
> 
>   IOMMU         CPU
> Read [0]    
>               Write [0]
>               Write [1]
> Read [1]
> 
> Will result in the iommu seeing torn incorrect data - the Guest paging
> mode may not match the page table pointer, or the VIOMMU data may
> become mismatched to the host translation.
> 
> Avoiding flushing is only possible if the full 256 bits are read
> atomically.

Also, please think about what qemu does when paravirtualizing
this. qemu will read the DTE entry using the CPU.

For your above remark it should be reading using two 128 bit loads.

However, it doesn't seem to be doing that:

static bool amdvi_get_dte(AMDVIState *s, int devid, uint64_t *entry)
{
    uint32_t offset = devid * AMDVI_DEVTAB_ENTRY_SIZE;

    if (dma_memory_read(&address_space_memory, s->devtab + offset, entry,
                        AMDVI_DEVTAB_ENTRY_SIZE, MEMTXATTRS_UNSPECIFIED)) {


The dma_memory_read eventually boils down to memcpy()

So qemu looks wrong to me.

Jason

Re: [PATCH v3 1/5] iommu/amd: Disable AMD IOMMU if CMPXCHG16B feature is not supported

[Suthikulpanit, Suravee]

On 9/9/2024 10:16 PM, Jason Gunthorpe wrote:
> On Fri, Sep 06, 2024 at 01:38:18PM -0300, Jason Gunthorpe wrote:
>> On Fri, Sep 06, 2024 at 12:13:04PM +0000, Suravee Suthikulpanit wrote:
>>> According to the AMD IOMMU spec, the IOMMU reads the entire DTE either
>>> in two 128-bit transactions or a single 256-bit transaction.
>>
>> .. if two 128-bit transaction on the read side is possible then you
>> need flushing! :(
>>
>> For instance this:
>>
>>    IOMMU         CPU
>> Read [0]
>>                Write [0]
>>                Write [1]
>> Read [1]
>>
>> Will result in the iommu seeing torn incorrect data - the Guest paging
>> mode may not match the page table pointer, or the VIOMMU data may
>> become mismatched to the host translation.
>>
>> Avoiding flushing is only possible if the full 256 bits are read
>> atomically.
> 
> Also, please think about what qemu does when paravirtualizing
> this. qemu will read the DTE entry using the CPU.
> 
> For your above remark it should be reading using two 128 bit loads.
> 
> However, it doesn't seem to be doing that:
> 
> static bool amdvi_get_dte(AMDVIState *s, int devid, uint64_t *entry)
> {
>      uint32_t offset = devid * AMDVI_DEVTAB_ENTRY_SIZE;
> 
>      if (dma_memory_read(&address_space_memory, s->devtab + offset, entry,
>                          AMDVI_DEVTAB_ENTRY_SIZE, MEMTXATTRS_UNSPECIFIED)) {
> 
> 
> The dma_memory_read eventually boils down to memcpy()
> 
> So qemu looks wrong to me.
> 
> Jason

Thanks for pointing out. Let me check QEMU and see how to update the code.

Thanks,
Suravee

Re: [PATCH v3 1/5] iommu/amd: Disable AMD IOMMU if CMPXCHG16B feature is not supported

[Suthikulpanit, Suravee]

On 9/6/2024 11:38 PM, Jason Gunthorpe wrote:
> On Fri, Sep 06, 2024 at 12:13:04PM +0000, Suravee Suthikulpanit wrote:
>> According to the AMD IOMMU spec, the IOMMU reads the entire DTE either
>> in two 128-bit transactions or a single 256-bit transaction.
> 
> .. if two 128-bit transaction on the read side is possible then you
> need flushing! :(
> 
> For instance this:
> 
>    IOMMU         CPU
> Read [0]
>                Write [0]
>                Write [1]
> Read [1]
> 
> Will result in the iommu seeing torn incorrect data - the Guest paging
> mode may not match the page table pointer, or the VIOMMU data may
> become mismatched to the host translation.
> 
> Avoiding flushing is only possible if the full 256 bits are read
> atomically.

I have verified with the hardware designer, and they have now confirmed 
that the IOMMU hardware has always been implemented with 256-bit read. 
The next revision of the IOMMU spec will be updated to correctly 
describe this part. Therefore, I will update the commit message and 
implement the code accordingly.

>> It is recommended to update DTE using 128-bit operation followed by
>> an INVALIDATE_DEVTAB_ENTYRY command when the IV=1b or V=1b.
> 
> This advice only works when going from non-valid to valid.

Actually, if we change the DTE when IV=1 or V=1, we would need to 
invalidate as well.

>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   drivers/iommu/amd/init.c | 23 +++++++++--------------
>>   1 file changed, 9 insertions(+), 14 deletions(-)
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Jason

Thanks,
Suravee

Re: [PATCH v3 1/5] iommu/amd: Disable AMD IOMMU if CMPXCHG16B feature is not supported

[Jason Gunthorpe]

On Mon, Sep 16, 2024 at 11:11:46PM +0700, Suthikulpanit, Suravee wrote:

> > Avoiding flushing is only possible if the full 256 bits are read
> > atomically.
> 
> I have verified with the hardware designer, and they have now confirmed that
> the IOMMU hardware has always been implemented with 256-bit read. The next
> revision of the IOMMU spec will be updated to correctly describe this part.
> Therefore, I will update the commit message and implement the code
> accordingly.

That is certainly convenient, except qemu doesn't follow that spec :\

Jason

[PATCH v3 2/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE

[Suravee Suthikulpanit]

The current implementation does not follow 128-bit write requirement
to update DTE as specified in the AMD I/O Virtualization Techonology
(IOMMU) Specification.

Therefore, modify the struct dev_table_entry to contain union of u128 data
array, and introduce two helper functions:

  * update_dte256() to update DTE using two 128-bit cmpxchg
    operations to update 256-bit DTE with the modified structure.

  * get_dte256() to copy 256-bit DTE to the provided structrue.

In addition, when access/update 256-bit DTE, it needs to be locked
to prevent cmpxchg128 failure and data tearing. Therefore, introduce
a per-DTE spin_lock struct dev_data.dte_lock to provide synchronization
across get_dte256() and update_dte256().

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  6 ++-
 drivers/iommu/amd/iommu.c           | 80 +++++++++++++++++++++++++----
 2 files changed, 76 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index c9f9a598eb82..1836da2d9e60 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -833,6 +833,7 @@ struct devid_map {
 struct iommu_dev_data {
 	/*Protect against attach/detach races */
 	spinlock_t lock;
+	spinlock_t dte_lock;              /* DTE lock for 256-bit access */
 
 	struct list_head list;		  /* For domain->dev_list */
 	struct llist_node dev_data_list;  /* For global dev_data_list */
@@ -883,7 +884,10 @@ extern struct amd_iommu *amd_iommus[MAX_IOMMUS];
  * Structure defining one entry in the device table
  */
 struct dev_table_entry {
-	u64 data[4];
+	union {
+		u64 data[4];
+		u128 data128[2];
+	};
 };
 
 /*
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 87c5385ce3f2..b994e7837306 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -85,6 +85,45 @@ static void set_dte_entry(struct amd_iommu *iommu,
  *
  ****************************************************************************/
 
+static void update_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
+			  struct dev_table_entry *new)
+{
+	struct dev_table_entry *dev_table = get_dev_table(iommu);
+	struct dev_table_entry *ptr = &dev_table[dev_data->devid];
+	struct dev_table_entry old;
+	u128 tmp;
+
+	lockdep_assert_held(&dev_data->dte_lock);
+
+	old.data128[0] = ptr->data128[0];
+	old.data128[1] = ptr->data128[1];
+
+	tmp = cmpxchg128(&ptr->data128[1], old.data128[1], new->data128[1]);
+	if (tmp == old.data128[1]) {
+		if (!try_cmpxchg128(&ptr->data128[0], &old.data128[0], new->data128[0])) {
+			/* Restore hi 128-bit */
+			cmpxchg128(&ptr->data128[1], new->data128[1], tmp);
+			pr_err("%s: Failed. devid=%#x, dte=%016llx:%016llx:%016llx:%016llx\n",
+			       __func__, dev_data->devid, new->data[0], new->data[1],
+			       new->data[2], new->data[3]);
+		}
+	}
+}
+
+static void get_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
+		      struct dev_table_entry *dte)
+{
+	struct dev_table_entry *ptr;
+	struct dev_table_entry *dev_table = get_dev_table(iommu);
+
+	lockdep_assert_held(&dev_data->dte_lock);
+
+	ptr = &dev_table[dev_data->devid];
+
+	dte->data128[0] = ptr->data128[0];
+	dte->data128[1] = ptr->data128[1];
+}
+
 static inline bool pdom_is_v2_pgtbl_mode(struct protection_domain *pdom)
 {
 	return (pdom && (pdom->pd_mode == PD_MODE_V2));
@@ -205,6 +244,7 @@ static struct iommu_dev_data *alloc_dev_data(struct amd_iommu *iommu, u16 devid)
 		return NULL;
 
 	spin_lock_init(&dev_data->lock);
+	spin_lock_init(&dev_data->dte_lock);
 	dev_data->devid = devid;
 	ratelimit_default_init(&dev_data->rs);
 
@@ -232,9 +272,11 @@ static struct iommu_dev_data *search_dev_data(struct amd_iommu *iommu, u16 devid
 
 static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
 {
+	struct dev_table_entry dte;
 	struct amd_iommu *iommu;
-	struct dev_table_entry *dev_table;
+	struct iommu_dev_data *dev_data, *alias_data;
 	u16 devid = pci_dev_id(pdev);
+	int ret;
 
 	if (devid == alias)
 		return 0;
@@ -243,13 +285,28 @@ static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
 	if (!iommu)
 		return 0;
 
-	amd_iommu_set_rlookup_table(iommu, alias);
-	dev_table = get_dev_table(iommu);
-	memcpy(dev_table[alias].data,
-	       dev_table[devid].data,
-	       sizeof(dev_table[alias].data));
+	/* Get DTE for pdev */
+	dev_data = dev_iommu_priv_get(&pdev->dev);
+	if (!dev_data)
+		return -EINVAL;
 
-	return 0;
+	spin_lock(&dev_data->dte_lock);
+	get_dte256(iommu, dev_data, &dte);
+	spin_unlock(&dev_data->dte_lock);
+
+	/* Setup for alias */
+	alias_data = search_dev_data(iommu, alias);
+	if (!alias_data) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	spin_lock(&alias_data->dte_lock);
+	update_dte256(iommu, alias_data, &dte);
+	amd_iommu_set_rlookup_table(iommu, alias);
+	spin_unlock(&alias_data->dte_lock);
+out:
+	return ret;
 }
 
 static void clone_aliases(struct amd_iommu *iommu, struct device *dev)
@@ -583,10 +640,15 @@ static void amd_iommu_uninit_device(struct device *dev)
 static void dump_dte_entry(struct amd_iommu *iommu, u16 devid)
 {
 	int i;
-	struct dev_table_entry *dev_table = get_dev_table(iommu);
+	struct dev_table_entry dte;
+	struct iommu_dev_data *dev_data = find_dev_data(iommu, devid);
+
+	spin_lock(&dev_data->dte_lock);
+	get_dte256(iommu, dev_data, &dte);
+	spin_unlock(&dev_data->dte_lock);
 
 	for (i = 0; i < 4; ++i)
-		pr_err("DTE[%d]: %016llx\n", i, dev_table[devid].data[i]);
+		pr_err("DTE[%d]: %016llx\n", i, dte.data[i]);
 }
 
 static void dump_command(unsigned long phys_addr)
-- 
2.34.1

Re: [PATCH v3 2/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE

[Jacob Pan]

Hi Suravee,

On Fri, 6 Sep 2024 12:13:05 +0000
Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:

> The current implementation does not follow 128-bit write requirement
> to update DTE as specified in the AMD I/O Virtualization Techonology
> (IOMMU) Specification.
> 
> Therefore, modify the struct dev_table_entry to contain union of u128
> data array, and introduce two helper functions:
> 
>   * update_dte256() to update DTE using two 128-bit cmpxchg
>     operations to update 256-bit DTE with the modified structure.
> 
>   * get_dte256() to copy 256-bit DTE to the provided structrue.
> 
> In addition, when access/update 256-bit DTE, it needs to be locked
> to prevent cmpxchg128 failure and data tearing. Therefore, introduce
> a per-DTE spin_lock struct dev_data.dte_lock to provide
> synchronization across get_dte256() and update_dte256().
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  6 ++-
>  drivers/iommu/amd/iommu.c           | 80
> +++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 10
> deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h
> b/drivers/iommu/amd/amd_iommu_types.h index
> c9f9a598eb82..1836da2d9e60 100644 ---
> a/drivers/iommu/amd/amd_iommu_types.h +++
> b/drivers/iommu/amd/amd_iommu_types.h @@ -833,6 +833,7 @@ struct
> devid_map { struct iommu_dev_data {
>  	/*Protect against attach/detach races */
>  	spinlock_t lock;
> +	spinlock_t dte_lock;              /* DTE lock for 256-bit
> access */ 
>  	struct list_head list;		  /* For
> domain->dev_list */ struct llist_node dev_data_list;  /* For global
> dev_data_list */ @@ -883,7 +884,10 @@ extern struct amd_iommu
> *amd_iommus[MAX_IOMMUS];
>   * Structure defining one entry in the device table
>   */
>  struct dev_table_entry {
> -	u64 data[4];
> +	union {
> +		u64 data[4];
> +		u128 data128[2];
> +	};
>  };
>  
>  /*
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 87c5385ce3f2..b994e7837306 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -85,6 +85,45 @@ static void set_dte_entry(struct amd_iommu *iommu,
>   *
>   ****************************************************************************/
>  
> +static void update_dte256(struct amd_iommu *iommu, struct
> iommu_dev_data *dev_data,
> +			  struct dev_table_entry *new)
> +{
> +	struct dev_table_entry *dev_table = get_dev_table(iommu);
> +	struct dev_table_entry *ptr = &dev_table[dev_data->devid];
> +	struct dev_table_entry old;
> +	u128 tmp;
> +
> +	lockdep_assert_held(&dev_data->dte_lock);
> +
> +	old.data128[0] = ptr->data128[0];
> +	old.data128[1] = ptr->data128[1];
> +
> +	tmp = cmpxchg128(&ptr->data128[1], old.data128[1],
> new->data128[1]);
> +	if (tmp == old.data128[1]) {
If you are able to deal with the failure, why not use try_cmpxchg128
for the hi 128 bit also? It is more efficient.

> +		if (!try_cmpxchg128(&ptr->data128[0],
> &old.data128[0], new->data128[0])) {
> +			/* Restore hi 128-bit */
> +			cmpxchg128(&ptr->data128[1],
> new->data128[1], tmp);
> +			pr_err("%s: Failed. devid=%#x,
> dte=%016llx:%016llx:%016llx:%016llx\n",
> +			       __func__, dev_data->devid,
> new->data[0], new->data[1],
> +			       new->data[2], new->data[3]);
> +		}
> +	}
> +}
> +
> +static void get_dte256(struct amd_iommu *iommu, struct
> iommu_dev_data *dev_data,
> +		      struct dev_table_entry *dte)
> +{
> +	struct dev_table_entry *ptr;
> +	struct dev_table_entry *dev_table = get_dev_table(iommu);
> +
> +	lockdep_assert_held(&dev_data->dte_lock);
> +
> +	ptr = &dev_table[dev_data->devid];
> +
> +	dte->data128[0] = ptr->data128[0];
> +	dte->data128[1] = ptr->data128[1];
> +}
> +
>  static inline bool pdom_is_v2_pgtbl_mode(struct protection_domain
> *pdom) {
>  	return (pdom && (pdom->pd_mode == PD_MODE_V2));
> @@ -205,6 +244,7 @@ static struct iommu_dev_data
> *alloc_dev_data(struct amd_iommu *iommu, u16 devid) return NULL;
>  
>  	spin_lock_init(&dev_data->lock);
> +	spin_lock_init(&dev_data->dte_lock);
>  	dev_data->devid = devid;
>  	ratelimit_default_init(&dev_data->rs);
>  
> @@ -232,9 +272,11 @@ static struct iommu_dev_data
> *search_dev_data(struct amd_iommu *iommu, u16 devid 
>  static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
>  {
> +	struct dev_table_entry dte;
>  	struct amd_iommu *iommu;
> -	struct dev_table_entry *dev_table;
> +	struct iommu_dev_data *dev_data, *alias_data;
>  	u16 devid = pci_dev_id(pdev);
> +	int ret;
>  
>  	if (devid == alias)
>  		return 0;
> @@ -243,13 +285,28 @@ static int clone_alias(struct pci_dev *pdev,
> u16 alias, void *data) if (!iommu)
>  		return 0;
>  
> -	amd_iommu_set_rlookup_table(iommu, alias);
> -	dev_table = get_dev_table(iommu);
> -	memcpy(dev_table[alias].data,
> -	       dev_table[devid].data,
> -	       sizeof(dev_table[alias].data));
> +	/* Get DTE for pdev */
> +	dev_data = dev_iommu_priv_get(&pdev->dev);
> +	if (!dev_data)
> +		return -EINVAL;
>  
> -	return 0;
> +	spin_lock(&dev_data->dte_lock);
> +	get_dte256(iommu, dev_data, &dte);
> +	spin_unlock(&dev_data->dte_lock);
> +
> +	/* Setup for alias */
> +	alias_data = search_dev_data(iommu, alias);
> +	if (!alias_data) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	spin_lock(&alias_data->dte_lock);
> +	update_dte256(iommu, alias_data, &dte);
> +	amd_iommu_set_rlookup_table(iommu, alias);
> +	spin_unlock(&alias_data->dte_lock);
> +out:
> +	return ret;
>  }
>  
>  static void clone_aliases(struct amd_iommu *iommu, struct device
> *dev) @@ -583,10 +640,15 @@ static void
> amd_iommu_uninit_device(struct device *dev) static void
> dump_dte_entry(struct amd_iommu *iommu, u16 devid) {
>  	int i;
> -	struct dev_table_entry *dev_table = get_dev_table(iommu);
> +	struct dev_table_entry dte;
> +	struct iommu_dev_data *dev_data = find_dev_data(iommu,
> devid); +
> +	spin_lock(&dev_data->dte_lock);
> +	get_dte256(iommu, dev_data, &dte);
> +	spin_unlock(&dev_data->dte_lock);
>  
>  	for (i = 0; i < 4; ++i)
> -		pr_err("DTE[%d]: %016llx\n", i,
> dev_table[devid].data[i]);
> +		pr_err("DTE[%d]: %016llx\n", i, dte.data[i]);
>  }
>  
>  static void dump_command(unsigned long phys_addr)

Re: [PATCH v3 2/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE

[Jason Gunthorpe]

On Fri, Sep 06, 2024 at 12:13:05PM +0000, Suravee Suthikulpanit wrote:

> +static void update_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
> +			  struct dev_table_entry *new)
> +{
> +	struct dev_table_entry *dev_table = get_dev_table(iommu);
> +	struct dev_table_entry *ptr = &dev_table[dev_data->devid];
> +	struct dev_table_entry old;
> +	u128 tmp;
> +
> +	lockdep_assert_held(&dev_data->dte_lock);
> +
> +	old.data128[0] = ptr->data128[0];
> +	old.data128[1] = ptr->data128[1];
> +
> +	tmp = cmpxchg128(&ptr->data128[1], old.data128[1], new->data128[1]);
> +	if (tmp == old.data128[1]) {
> +		if (!try_cmpxchg128(&ptr->data128[0], &old.data128[0], new->data128[0])) {
> +			/* Restore hi 128-bit */
> +			cmpxchg128(&ptr->data128[1], new->data128[1], tmp);

Like I said before, you can't fix this. Just go fowards. Keeping an
old DTE will UAF things, that is much worse than just forcing a new
DTE and loosing some interrupt settings.

> @@ -243,13 +285,28 @@ static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
>  	if (!iommu)
>  		return 0;
>  
> -	amd_iommu_set_rlookup_table(iommu, alias);
> -	dev_table = get_dev_table(iommu);
> -	memcpy(dev_table[alias].data,
> -	       dev_table[devid].data,
> -	       sizeof(dev_table[alias].data));
> +	/* Get DTE for pdev */
> +	dev_data = dev_iommu_priv_get(&pdev->dev);
> +	if (!dev_data)
> +		return -EINVAL;
>  
> -	return 0;
> +	spin_lock(&dev_data->dte_lock);
> +	get_dte256(iommu, dev_data, &dte);
> +	spin_unlock(&dev_data->dte_lock);

You can't unlock after reading the DTE and the relock it to program
it. The interrupt data can have changed while unlocked.

Put the lock inside update_dte256() and force the interrupt bits
under the lock.

Something like this is what I'm expecting:

static void write_upper(struct dev_table_entry *ptr, struct dev_table_entry *new)
{
	struct dev_table_entry old;

	lockdep_assert_held(&dev_data->dte_lock);

	do {
		old->data128[1] = ptr->data128[1];
		new->data64[2] &= ~INTR_MASK;
		new->data64[2] |= old->data64[2] & INTR_MASK;
	} while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1],
				 new->data128[1]));
}

static void write_lower(struct dev_table_entry *ptr, struct dev_table_entry *new)
{
	struct dev_table_entry old;

	do {
		old->data128[0] = ptr->data128[0];
	} while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0],
				 new->data128[0]));
}

static void update_dte256(struct amd_iommu *iommu,
			  struct iommu_dev_data *dev_data,
			  struct dev_table_entry *new)
{

	spin_lock(&dev_data->dte_lock);
	if (!(ptr->data64[0] & V)) {
		write_upper(ptr, new);
		/* NO FLUSH assume V=0 never cached */
		write_lower(ptr, new);
		/* FLUSH */
	} else if (!(new->data64[0] & V) {
		write_lower(ptr, new);
		/* FLUSH */
		write_upper(ptr, new);
		/* NO FLUSH assume V=0 never cached */
	} else {
		/* both are valid */
		if (FIELD_GET(ptr->data[2], GUEST_PAGING_MODE) ==
		    FIELD_GET(new->data[2], GUEST_PAGING_MODE)) {
			/* Upper doesn't change */
			write_upper(ptr, new);
			write_lower(ptr, new);
			/* FLUSH */
		else if (old has no guest page table) {
			write_upper(ptr, new);
			/* FLUSH */
			write_lower(ptr, new);
			/* FLUSH */
		else if (new has no guest page table) {
			write_lower(ptr, new);
			/* FLUSH */
			write_upper(ptr, new);
			/* FLUSH */
		} else {
			struct dev_table_entry clear = {};

			write_lower(ptr, &clear);
			/* FLUSH to set V=0 */
			write_upper(ptr, new);
			/* NO FLUSH assume V=0 never cached */
			write_lower(ptr, new);
			/* FLUSH */
		}
	}

	spin_unlock(&dev_data->dte_lock);
}

And it probably needs more logic to accomodate the VIOMMU valid bits
in the 2nd 128.

Jason

Re: [PATCH v3 2/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE

[Suthikulpanit, Suravee]

On 9/7/2024 12:00 AM, Jason Gunthorpe wrote:
> On Fri, Sep 06, 2024 at 12:13:05PM +0000, Suravee Suthikulpanit wrote:
> 
>> +static void update_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
>> +			  struct dev_table_entry *new)
>> +{
>> +	struct dev_table_entry *dev_table = get_dev_table(iommu);
>> +	struct dev_table_entry *ptr = &dev_table[dev_data->devid];
>> +	struct dev_table_entry old;
>> +	u128 tmp;
>> +
>> +	lockdep_assert_held(&dev_data->dte_lock);
>> +
>> +	old.data128[0] = ptr->data128[0];
>> +	old.data128[1] = ptr->data128[1];
>> +
>> +	tmp = cmpxchg128(&ptr->data128[1], old.data128[1], new->data128[1]);
>> +	if (tmp == old.data128[1]) {
>> +		if (!try_cmpxchg128(&ptr->data128[0], &old.data128[0], new->data128[0])) {
>> +			/* Restore hi 128-bit */
>> +			cmpxchg128(&ptr->data128[1], new->data128[1], tmp);
> 
> Like I said before, you can't fix this. Just go fowards. Keeping an
> old DTE will UAF things, that is much worse than just forcing a new
> DTE and loosing some interrupt settings.
> 
>> @@ -243,13 +285,28 @@ static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
>>   	if (!iommu)
>>   		return 0;
>>   
>> -	amd_iommu_set_rlookup_table(iommu, alias);
>> -	dev_table = get_dev_table(iommu);
>> -	memcpy(dev_table[alias].data,
>> -	       dev_table[devid].data,
>> -	       sizeof(dev_table[alias].data));
>> +	/* Get DTE for pdev */
>> +	dev_data = dev_iommu_priv_get(&pdev->dev);
>> +	if (!dev_data)
>> +		return -EINVAL;
>>   
>> -	return 0;
>> +	spin_lock(&dev_data->dte_lock);
>> +	get_dte256(iommu, dev_data, &dte);
>> +	spin_unlock(&dev_data->dte_lock);
> 
> You can't unlock after reading the DTE and the relock it to program
> it. The interrupt data can have changed while unlocked.
> 
> Put the lock inside update_dte256() and force the interrupt bits
> under the lock.
> 
> Something like this is what I'm expecting:
> 
> static void write_upper(struct dev_table_entry *ptr, struct dev_table_entry *new)
> {
> 	struct dev_table_entry old;
> 
> 	lockdep_assert_held(&dev_data->dte_lock);
> 
> 	do {
> 		old->data128[1] = ptr->data128[1];
> 		new->data64[2] &= ~INTR_MASK;
> 		new->data64[2] |= old->data64[2] & INTR_MASK;
> 	} while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1],
> 				 new->data128[1]));
> }
> 
> static void write_lower(struct dev_table_entry *ptr, struct dev_table_entry *new)
> {
> 	struct dev_table_entry old;
> 
> 	do {
> 		old->data128[0] = ptr->data128[0];
> 	} while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0],
> 				 new->data128[0]));
> }
> 
> static void update_dte256(struct amd_iommu *iommu,
> 			  struct iommu_dev_data *dev_data,
> 			  struct dev_table_entry *new)
> {
> 
> 	spin_lock(&dev_data->dte_lock);
> 	if (!(ptr->data64[0] & V)) {
> 		write_upper(ptr, new);
> 		/* NO FLUSH assume V=0 never cached */
> 		write_lower(ptr, new);
> 		/* FLUSH */
> 	} else if (!(new->data64[0] & V) {
> 		write_lower(ptr, new);
> 		/* FLUSH */
> 		write_upper(ptr, new);
> 		/* NO FLUSH assume V=0 never cached */
> 	} else {
> 		/* both are valid */
> 		if (FIELD_GET(ptr->data[2], GUEST_PAGING_MODE) ==
> 		    FIELD_GET(new->data[2], GUEST_PAGING_MODE)) {
> 			/* Upper doesn't change */
> 			write_upper(ptr, new);
> 			write_lower(ptr, new);
> 			/* FLUSH */
> 		else if (old has no guest page table) {
> 			write_upper(ptr, new);
> 			/* FLUSH */
> 			write_lower(ptr, new);
> 			/* FLUSH */
> 		else if (new has no guest page table) {
> 			write_lower(ptr, new);
> 			/* FLUSH */
> 			write_upper(ptr, new);
> 			/* FLUSH */
> 		} else {
> 			struct dev_table_entry clear = {};
> 
> 			write_lower(ptr, &clear);
> 			/* FLUSH to set V=0 */
> 			write_upper(ptr, new);
> 			/* NO FLUSH assume V=0 never cached */
> 			write_lower(ptr, new);
> 			/* FLUSH */
> 		}
> 	}
> 
> 	spin_unlock(&dev_data->dte_lock);
> }

Got your point. I will update based on your suggestion here.

> And it probably needs more logic to accomodate the VIOMMU valid bits
> in the 2nd 128.

I'll take care of this when enable the HW-vIOMMU stuff.

Thanks,
Suravee

Re: [PATCH v3 2/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE

[Uros Bizjak]

On Fri, Sep 6, 2024 at 5:53 PM Jacob Pan <jacob.pan@linux.microsoft.com> wrote:

> > +static void update_dte256(struct amd_iommu *iommu, struct
> > iommu_dev_data *dev_data,
> > +                       struct dev_table_entry *new)
> > +{
> > +     struct dev_table_entry *dev_table = get_dev_table(iommu);
> > +     struct dev_table_entry *ptr = &dev_table[dev_data->devid];
> > +     struct dev_table_entry old;
> > +     u128 tmp;
> > +
> > +     lockdep_assert_held(&dev_data->dte_lock);
> > +
> > +     old.data128[0] = ptr->data128[0];
> > +     old.data128[1] = ptr->data128[1];
> > +
> > +     tmp = cmpxchg128(&ptr->data128[1], old.data128[1], new->data128[1]);
> > +     if (tmp == old.data128[1]) {
> If you are able to deal with the failure, why not use try_cmpxchg128
> for the hi 128 bit also? It is more efficient.

Indeed, just write:

if (try_cmpxchg128(&ptr->data128[1], &old.data128[1], new->data128[1])) {

to substitute the last two lines above.

Please also note that try_cmpxchg128() updates its second argument on failure.

Uros.

Re: [PATCH v3 2/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE

[kernel test robot]

Hi Suravee,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on joro-iommu/next v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Suravee-Suthikulpanit/iommu-amd-Disable-AMD-IOMMU-if-CMPXCHG16B-feature-is-not-supported/20240906-201533
base:   linus/master
patch link:    https://lore.kernel.org/r/20240906121308.5013-3-suravee.suthikulpanit%40amd.com
patch subject: [PATCH v3 2/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240907/202409072100.WaB846Yg-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409072100.WaB846Yg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409072100.WaB846Yg-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iommu/amd/iommu.c:299:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     299 |         if (!alias_data) {
         |             ^~~~~~~~~~~
   drivers/iommu/amd/iommu.c:309:9: note: uninitialized use occurs here
     309 |         return ret;
         |                ^~~
   drivers/iommu/amd/iommu.c:299:2: note: remove the 'if' if its condition is always true
     299 |         if (!alias_data) {
         |         ^~~~~~~~~~~~~~~~
   drivers/iommu/amd/iommu.c:279:9: note: initialize the variable 'ret' to silence this warning
     279 |         int ret;
         |                ^
         |                 = 0
   1 warning generated.


vim +299 drivers/iommu/amd/iommu.c

   272	
   273	static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
   274	{
   275		struct dev_table_entry dte;
   276		struct amd_iommu *iommu;
   277		struct iommu_dev_data *dev_data, *alias_data;
   278		u16 devid = pci_dev_id(pdev);
   279		int ret;
   280	
   281		if (devid == alias)
   282			return 0;
   283	
   284		iommu = rlookup_amd_iommu(&pdev->dev);
   285		if (!iommu)
   286			return 0;
   287	
   288		/* Get DTE for pdev */
   289		dev_data = dev_iommu_priv_get(&pdev->dev);
   290		if (!dev_data)
   291			return -EINVAL;
   292	
   293		spin_lock(&dev_data->dte_lock);
   294		get_dte256(iommu, dev_data, &dte);
   295		spin_unlock(&dev_data->dte_lock);
   296	
   297		/* Setup for alias */
   298		alias_data = search_dev_data(iommu, alias);
 > 299		if (!alias_data) {
   300			ret = -EINVAL;
   301			goto out;
   302		}
   303	
   304		spin_lock(&alias_data->dte_lock);
   305		update_dte256(iommu, alias_data, &dte);
   306		amd_iommu_set_rlookup_table(iommu, alias);
   307		spin_unlock(&alias_data->dte_lock);
   308	out:
   309		return ret;
   310	}
   311	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 3/5] iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers

[Suravee Suthikulpanit]

Also, the set_dte_entry() is used to program several DTE fields (e.g.
stage1 table, stage2 table, domain id, and etc.), which is difficult
to keep track with current implementation.

Therefore, separate logic for setting up the GCR3 Table Root Pointer,
GIOV, GV, GLX, and GuestPagingMode into another helper function
set_dte_gcr3_table().

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/iommu.c | 123 ++++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 53 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b994e7837306..f18ae6c077f4 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1913,16 +1913,56 @@ int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
 	return ret;
 }
 
+static void set_dte_gcr3_table(struct amd_iommu *iommu,
+			       struct iommu_dev_data *dev_data,
+			       struct dev_table_entry *target)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	u64 tmp, gcr3;
+
+	if (!gcr3_info->gcr3_tbl)
+		return;
+
+	pr_debug("%s: devid=%#x, glx=%#x, gcr3_tbl=%#llx\n",
+		 __func__, dev_data->devid, gcr3_info->glx,
+		 (unsigned long long)gcr3_info->gcr3_tbl);
+
+	tmp = gcr3_info->glx;
+	target->data[0] |= (tmp & DTE_GLX_MASK) << DTE_GLX_SHIFT;
+	if (pdom_is_v2_pgtbl_mode(dev_data->domain))
+		target->data[0] |= DTE_FLAG_GIOV;
+	target->data[0] |= DTE_FLAG_GV;
+
+	/* First mask out possible old values for GCR3 table */
+	tmp = DTE_GCR3_VAL_A(~0ULL) << DTE_GCR3_SHIFT_A;
+	target->data[0] &= ~tmp;
+	tmp = DTE_GCR3_VAL_B(~0ULL) << DTE_GCR3_SHIFT_B;
+	tmp |= DTE_GCR3_VAL_C(~0ULL) << DTE_GCR3_SHIFT_C;
+	target->data[1] &= ~tmp;
+
+	gcr3 = iommu_virt_to_phys(gcr3_info->gcr3_tbl);
+
+	/* Encode GCR3 table into DTE */
+	tmp = DTE_GCR3_VAL_A(gcr3) << DTE_GCR3_SHIFT_A;
+	target->data[0] |= tmp;
+	tmp = DTE_GCR3_VAL_B(gcr3) << DTE_GCR3_SHIFT_B;
+	tmp |= DTE_GCR3_VAL_C(gcr3) << DTE_GCR3_SHIFT_C;
+	target->data[1] |= tmp;
+
+	/* Mask out old values for GuestPagingMode */
+	target->data[2] &= ~(0x3ULL << DTE_GPT_LEVEL_SHIFT);
+	/* Guest page table can only support 4 and 5 levels  */
+	if (amd_iommu_gpt_level == PAGE_MODE_5_LEVEL)
+		target->data[2] |= ((u64)GUEST_PGTABLE_5_LEVEL << DTE_GPT_LEVEL_SHIFT);
+}
+
 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;
 	u16 domid;
+	u32 old_domid;
+	struct dev_table_entry new;
 	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 (gcr3_info && gcr3_info->gcr3_tbl)
@@ -1930,73 +1970,50 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	else
 		domid = domain->id;
 
+
+	spin_lock(&dev_data->dte_lock);
+	/*
+	 * Need to preserve the certain fields in DTE because it contains
+	 * interrupt-remapping and other settings, which might be
+	 * programmed earlier by other code.
+	 */
+	get_dte256(iommu, dev_data, &new);
+
 	if (domain->iop.mode != PAGE_MODE_NONE)
-		pte_root = iommu_virt_to_phys(domain->iop.root);
+		new.data[0] = iommu_virt_to_phys(domain->iop.root);
 
-	pte_root |= (domain->iop.mode & DEV_ENTRY_MODE_MASK)
+	new.data[0] |= (domain->iop.mode & DEV_ENTRY_MODE_MASK)
 		    << DEV_ENTRY_MODE_SHIFT;
 
-	pte_root |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_V;
+	new.data[0] |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_V;
 
 	/*
 	 * When SNP is enabled, Only set TV bit when IOMMU
 	 * page translation is in use.
 	 */
 	if (!amd_iommu_snp_en || (domid != 0))
-		pte_root |= DTE_FLAG_TV;
-
-	flags = dev_table[devid].data[1];
-
-	if (dev_data->ats_enabled)
-		flags |= DTE_FLAG_IOTLB;
+		new.data[0] |= DTE_FLAG_TV;
 
 	if (dev_data->ppr)
-		pte_root |= 1ULL << DEV_ENTRY_PPR;
+		new.data[0] |= 1ULL << DEV_ENTRY_PPR;
 
 	if (domain->dirty_tracking)
-		pte_root |= DTE_FLAG_HAD;
-
-	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;
-		pte_root |= (glx & DTE_GLX_MASK) << DTE_GLX_SHIFT;
-
-		/* First mask out possible old values for GCR3 table */
-		tmp = DTE_GCR3_VAL_B(~0ULL) << DTE_GCR3_SHIFT_B;
-		flags    &= ~tmp;
+		new.data[0] |= DTE_FLAG_HAD;
 
-		tmp = DTE_GCR3_VAL_C(~0ULL) << DTE_GCR3_SHIFT_C;
-		flags    &= ~tmp;
-
-		/* Encode GCR3 table into DTE */
-		tmp = DTE_GCR3_VAL_A(gcr3) << DTE_GCR3_SHIFT_A;
-		pte_root |= tmp;
-
-		tmp = DTE_GCR3_VAL_B(gcr3) << DTE_GCR3_SHIFT_B;
-		flags    |= tmp;
-
-		tmp = DTE_GCR3_VAL_C(gcr3) << DTE_GCR3_SHIFT_C;
-		flags    |= tmp;
+	if (dev_data->ats_enabled)
+		new.data[1] |= DTE_FLAG_IOTLB;
+	else
+		new.data[1] &= ~DTE_FLAG_IOTLB;
 
-		if (amd_iommu_gpt_level == PAGE_MODE_5_LEVEL) {
-			dev_table[devid].data[2] |=
-				((u64)GUEST_PGTABLE_5_LEVEL << DTE_GPT_LEVEL_SHIFT);
-		}
+	old_domid = new.data[1] & DEV_DOMID_MASK;
+	new.data[1] &= ~DEV_DOMID_MASK;
+	new.data[1] |= domid;
 
-		/* GIOV is supported with V2 page table mode only */
-		if (pdom_is_v2_pgtbl_mode(domain))
-			pte_root |= DTE_FLAG_GIOV;
-	}
+	set_dte_gcr3_table(iommu, dev_data, &new);
 
-	flags &= ~DEV_DOMID_MASK;
-	flags |= domid;
+	update_dte256(iommu, dev_data, &new);
 
-	old_domid = dev_table[devid].data[1] & DEV_DOMID_MASK;
-	dev_table[devid].data[1]  = flags;
-	dev_table[devid].data[0]  = pte_root;
+	spin_unlock(&dev_data->dte_lock);
 
 	/*
 	 * A kdump kernel might be replacing a domain ID that was copied from
-- 
2.34.1

[PATCH v3 4/5] iommu/amd: Modify clear_dte_entry() to avoid in-place update

[Suravee Suthikulpanit]

Lock DTE and copy value to a temporary storage before update using
cmpxchg128.

Also, refactor the function to simplify logic for applying erratum 63.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  2 ++
 drivers/iommu/amd/iommu.c           | 27 ++++++++++++++++++++-------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 1836da2d9e60..81a994471a30 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -425,6 +425,8 @@
 
 #define DTE_GPT_LEVEL_SHIFT	54
 
+#define DTE_SYSMGT_MASK		GENMASK_ULL(41, 40)
+
 #define GCR3_VALID		0x01ULL
 
 #define IOMMU_PAGE_MASK (((1ULL << 52) - 1) & ~0xfffULL)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index f18ae6c077f4..15eb816d4313 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2025,19 +2025,32 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	}
 }
 
-static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
+static void clear_dte_entry(struct amd_iommu *iommu, struct iommu_dev_data *dev_data)
 {
-	struct dev_table_entry *dev_table = get_dev_table(iommu);
+	struct dev_table_entry new;
+	struct dev_table_entry *dte = &get_dev_table(iommu)[dev_data->devid];
+
+	/*
+	 * Need to preserve DTE[96:106] because certain fields are
+	 * programmed using value in IVRS table from early init phase.
+	 */
+	spin_lock(&dev_data->dte_lock);
+	get_dte256(iommu, dev_data, &new);
 
 	/* remove entry from the device table seen by the hardware */
-	dev_table[devid].data[0]  = DTE_FLAG_V;
+	new.data[0] = DTE_FLAG_V;
 
 	if (!amd_iommu_snp_en)
-		dev_table[devid].data[0] |= DTE_FLAG_TV;
+		new.data[0] |= DTE_FLAG_TV;
 
-	dev_table[devid].data[1] &= DTE_FLAG_MASK;
+	new.data[1] &= DTE_FLAG_MASK;
 
-	amd_iommu_apply_erratum_63(iommu, devid);
+	/* Apply erratum 63 */
+	if (FIELD_GET(DTE_SYSMGT_MASK, new.data[1]) == 0x01)
+		new.data[0] |= BIT_ULL(DEV_ENTRY_IW);
+
+	WARN_ON(!try_cmpxchg128(&dte->data128[0], &dte->data128[0], new.data128[0]));
+	spin_unlock(&dev_data->dte_lock);
 }
 
 /* Update and flush DTE for the given device */
@@ -2048,7 +2061,7 @@ void amd_iommu_dev_update_dte(struct iommu_dev_data *dev_data, bool set)
 	if (set)
 		set_dte_entry(iommu, dev_data);
 	else
-		clear_dte_entry(iommu, dev_data->devid);
+		clear_dte_entry(iommu, dev_data);
 
 	clone_aliases(iommu, dev_data->dev);
 	device_flush_dte(dev_data);
-- 
2.34.1

Re: [PATCH v3 4/5] iommu/amd: Modify clear_dte_entry() to avoid in-place update

[Jason Gunthorpe]

On Fri, Sep 06, 2024 at 12:13:07PM +0000, Suravee Suthikulpanit wrote:
> Lock DTE and copy value to a temporary storage before update using
> cmpxchg128.
> 
> Also, refactor the function to simplify logic for applying erratum 63.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  2 ++
>  drivers/iommu/amd/iommu.c           | 27 ++++++++++++++++++++-------
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 1836da2d9e60..81a994471a30 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -425,6 +425,8 @@
>  
>  #define DTE_GPT_LEVEL_SHIFT	54
>  
> +#define DTE_SYSMGT_MASK		GENMASK_ULL(41, 40)
> +
>  #define GCR3_VALID		0x01ULL
>  
>  #define IOMMU_PAGE_MASK (((1ULL << 52) - 1) & ~0xfffULL)
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index f18ae6c077f4..15eb816d4313 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2025,19 +2025,32 @@ static void set_dte_entry(struct amd_iommu *iommu,
>  	}
>  }
>  
> -static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
> +static void clear_dte_entry(struct amd_iommu *iommu, struct iommu_dev_data *dev_data)
>  {
> -	struct dev_table_entry *dev_table = get_dev_table(iommu);
> +	struct dev_table_entry new;
> +	struct dev_table_entry *dte = &get_dev_table(iommu)[dev_data->devid];
> +
> +	/*
> +	 * Need to preserve DTE[96:106] because certain fields are
> +	 * programmed using value in IVRS table from early init phase.
> +	 */
> +	spin_lock(&dev_data->dte_lock);
> +	get_dte256(iommu, dev_data, &new);

I think there is no point in the get?

	struct dev_table_entry new = {}

	new.data[0] = DTE_FLAG_V;
	if (FIELD_GET(DTE_SYSMGT_MASK, old.data[1]) == 0x01)
		new.data[0] |= BIT_ULL(DEV_ENTRY_IW);

	new.data[1] = old.data[1] & DTE_FLAG_MASK
	new.data[2..4] = 0

That is pretty clear and simple

> -	dev_table[devid].data[1] &= DTE_FLAG_MASK;
> +	new.data[1] &= DTE_FLAG_MASK;

Would be nice if DTE_FLAG_MASK was broken into fields someday..

> -	amd_iommu_apply_erratum_63(iommu, devid);
> +	/* Apply erratum 63 */
> +	if (FIELD_GET(DTE_SYSMGT_MASK, new.data[1]) == 0x01)
> +		new.data[0] |= BIT_ULL(DEV_ENTRY_IW);
> +
> +	WARN_ON(!try_cmpxchg128(&dte->data128[0], &dte->data128[0], new.data128[0]));

As before this has to move forward, we can't fail to clear the DTE, it
will open a UAF

This should also clear the top 128 bits, so I would call the
update_dte256 directly?

Jason

[PATCH v3 5/5] iommu/amd: Do not update DTE in-place in amd_iommu_set_dirty_tracking and set_dte_irq_entry

[Suravee Suthikulpanit]

Update the DTE in place could result in data tearing since the AMD IOMMU
hardware reads the entire DTE either in two 128-bit transactions or
a single 256-bit transaction.

In case of updating only single 64-bit, it is sufficient to lock use
WRITE_ONCE() beaucse it is writing to memory read back by HW.

Also, lock the DTE before updating.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/iommu.c | 42 ++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 15eb816d4313..e9cfe9238e83 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2753,12 +2753,12 @@ static int amd_iommu_set_dirty_tracking(struct iommu_domain *domain,
 					bool enable)
 {
 	struct protection_domain *pdomain = to_pdomain(domain);
-	struct dev_table_entry *dev_table;
+	struct dev_table_entry *dte;
 	struct iommu_dev_data *dev_data;
 	bool domain_flush = false;
 	struct amd_iommu *iommu;
 	unsigned long flags;
-	u64 pte_root;
+	u64 new;
 
 	spin_lock_irqsave(&pdomain->lock, flags);
 	if (!(pdomain->dirty_tracking ^ enable)) {
@@ -2767,16 +2767,15 @@ static int amd_iommu_set_dirty_tracking(struct iommu_domain *domain,
 	}
 
 	list_for_each_entry(dev_data, &pdomain->dev_list, list) {
+		spin_lock(&dev_data->dte_lock);
 		iommu = get_amd_iommu_from_dev_data(dev_data);
-
-		dev_table = get_dev_table(iommu);
-		pte_root = dev_table[dev_data->devid].data[0];
-
-		pte_root = (enable ? pte_root | DTE_FLAG_HAD :
-				     pte_root & ~DTE_FLAG_HAD);
+		dte = &get_dev_table(iommu)[dev_data->devid];
+		new = dte->data[0];
+		new = (enable ? new | DTE_FLAG_HAD : new & ~DTE_FLAG_HAD);
+		WRITE_ONCE(dte->data[0], new);
+		spin_unlock(&dev_data->dte_lock);
 
 		/* Flush device DTE */
-		dev_table[dev_data->devid].data[0] = pte_root;
 		device_flush_dte(dev_data);
 		domain_flush = true;
 	}
@@ -3042,17 +3041,24 @@ static void iommu_flush_irt_and_complete(struct amd_iommu *iommu, u16 devid)
 static void set_dte_irq_entry(struct amd_iommu *iommu, u16 devid,
 			      struct irq_remap_table *table)
 {
-	u64 dte;
-	struct dev_table_entry *dev_table = get_dev_table(iommu);
+	u64 new;
+	struct dev_table_entry *dte = &get_dev_table(iommu)[devid];
+	struct iommu_dev_data *dev_data = search_dev_data(iommu, devid);
+
+	if (dev_data)
+		spin_lock(&dev_data->dte_lock);
+
+	new = dte->data[2];
+	new &= ~DTE_IRQ_PHYS_ADDR_MASK;
+	new |= iommu_virt_to_phys(table->table);
+	new |= DTE_IRQ_REMAP_INTCTL;
+	new |= DTE_INTTABLEN;
+	new |= DTE_IRQ_REMAP_ENABLE;
 
-	dte	= dev_table[devid].data[2];
-	dte	&= ~DTE_IRQ_PHYS_ADDR_MASK;
-	dte	|= iommu_virt_to_phys(table->table);
-	dte	|= DTE_IRQ_REMAP_INTCTL;
-	dte	|= DTE_INTTABLEN;
-	dte	|= DTE_IRQ_REMAP_ENABLE;
+	WRITE_ONCE(dte->data[2], new);
 
-	dev_table[devid].data[2] = dte;
+	if (dev_data)
+		spin_unlock(&dev_data->dte_lock);
 }
 
 static struct irq_remap_table *get_irq_table(struct amd_iommu *iommu, u16 devid)
-- 
2.34.1