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           | 28 ++++++++++++++++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index fea7544f8c55..db3ee094a144 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 DTE_INTR_MASK		(~GENMASK_ULL(55, 52))
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 676742d6f19a..2df679eb61c9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2086,19 +2086,31 @@ 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.
+	 */
+	new.data[0] = DTE_FLAG_V;
 
-	/* remove entry from the device table seen by the hardware */
-	dev_table[devid].data[0]  = DTE_FLAG_V;
+	/* Apply erratum 63 */
+	if (FIELD_GET(DTE_SYSMGT_MASK, dte->data[1]) == 0x01)
+		new.data[0] |= BIT_ULL(DEV_ENTRY_IW);
 
 	if (!amd_iommu_snp_en)
-		dev_table[devid].data[0] |= DTE_FLAG_TV;
+		new.data[0] |= DTE_FLAG_TV;
+
+	/* Need to preserve DTE[96:106] */
+	new.data[1] = dte->data[1] & DTE_FLAG_MASK;
 
-	dev_table[devid].data[1] &= DTE_FLAG_MASK;
+	/* Need to preserve interrupt remapping information in DTE[128:255] */
+	new.data128[1] = dte->data128[1];
 
-	amd_iommu_apply_erratum_63(iommu, devid);
+	update_dte256(iommu, dev_data, &new);
 }
 
 /* Update and flush DTE for the given device */
@@ -2109,7 +2121,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

On Mon, Sep 16, 2024 at 05:18:04PM +0000, Suravee Suthikulpanit wrote:

> -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.
> +	 */
> +	new.data[0] = DTE_FLAG_V;
>  
> -	/* remove entry from the device table seen by the hardware */
> -	dev_table[devid].data[0]  = DTE_FLAG_V;
> +	/* Apply erratum 63 */
> +	if (FIELD_GET(DTE_SYSMGT_MASK, dte->data[1]) == 0x01)
> +		new.data[0] |= BIT_ULL(DEV_ENTRY_IW);
>  
>  	if (!amd_iommu_snp_en)
> -		dev_table[devid].data[0] |= DTE_FLAG_TV;
> +		new.data[0] |= DTE_FLAG_TV;
> +
> +	/* Need to preserve DTE[96:106] */
> +	new.data[1] = dte->data[1] & DTE_FLAG_MASK;
>  
> -	dev_table[devid].data[1] &= DTE_FLAG_MASK;
> +	/* Need to preserve interrupt remapping information in DTE[128:255] */
> +	new.data128[1] = dte->data128[1];

It doesn't, update_dte256() does this automatically. Just leave it
zero here.

> -	amd_iommu_apply_erratum_63(iommu, devid);
> +	update_dte256(iommu, dev_data, &new);
>  }

I suggest you change this slightly so the flow is more like

make_clear_dte(..., struct dev_table_entry *entry) {..}

Which would have most of the above. Then:

clear_dte_entry()
{
    struct dev_table_entry target;

    make_clear_dte(.., &target);
    update_dte256(iommu, dev_data, &new);
}

And then in the prior patches you can write like:

static void make_dte_gcr3_table(struct amd_iommu *iommu,
                              struct iommu_dev_data *dev_data,
                              struct dev_table_entry *target)
{
    make_clear_dte(.., &target);
    ...
}

And drop all the wild masking:

+       /* 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;

Since make_clear_dte() already zero'd these fields.

Jason

On 9/27/2024 2:54 AM, Jason Gunthorpe wrote:
> On Mon, Sep 16, 2024 at 05:18:04PM +0000, Suravee Suthikulpanit wrote:
>
> ....
>
> I suggest you change this slightly so the flow is more like
> 
> make_clear_dte(..., struct dev_table_entry *entry) {..}
> 
> Which would have most of the above. Then:
> 
> clear_dte_entry()
> {
>      struct dev_table_entry target;
> 
>      make_clear_dte(.., &target);
>      update_dte256(iommu, dev_data, &new);
> }
> 
> And then in the prior patches you can write like:
> 
> static void make_dte_gcr3_table(struct amd_iommu *iommu,
>                                struct iommu_dev_data *dev_data,
>                                struct dev_table_entry *target)
> {
>      make_clear_dte(.., &target);
>      ...
> }
> 
> And drop all the wild masking:
> 
> +       /* 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;
> 
> Since make_clear_dte() already zero'd these fields.

Thanks for suggestion. I'll clean this up.

Suravee