1. Patchew
  2. linux
  3. [PATCH v4 0/6] iommu/amd: Use 128-bit cmpxchg operation to update DTE
  4. View patch

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

Suravee Suthikulpanit posted 6 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH v4 3/6] iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers
Posted by Suravee Suthikulpanit 2 months, 2 weeks ago 
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 | 117 +++++++++++++++++++++-----------------
 1 file changed, 65 insertions(+), 52 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 48a721d10f06..12f27061680d 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1947,17 +1947,58 @@ 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;
+	struct dev_table_entry *dte = &get_dev_table(iommu)[dev_data->devid];
 
 	if (gcr3_info && gcr3_info->gcr3_tbl)
 		domid = dev_data->gcr3_info.domid;
@@ -1965,72 +2006,44 @@ static void set_dte_entry(struct amd_iommu *iommu,
 		domid = domain->id;
 
 	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;
+	if (dev_data->ats_enabled)
+		new.data[1] |= DTE_FLAG_IOTLB;
+	else
+		new.data[1] &= ~DTE_FLAG_IOTLB;
 
-		tmp = DTE_GCR3_VAL_C(gcr3) << DTE_GCR3_SHIFT_C;
-		flags    |= tmp;
+	old_domid = new.data[1] & DEV_DOMID_MASK;
+	new.data[1] &= ~DEV_DOMID_MASK;
+	new.data[1] |= domid;
 
-		if (amd_iommu_gpt_level == PAGE_MODE_5_LEVEL) {
-			dev_table[devid].data[2] |=
-				((u64)GUEST_PGTABLE_5_LEVEL << DTE_GPT_LEVEL_SHIFT);
-		}
+	/* Need to preserve DTE[96:106] */
+	new.data[1] |= dte->data[1] & DTE_FLAG_MASK;
 
-		/* GIOV is supported with V2 page table mode only */
-		if (pdom_is_v2_pgtbl_mode(domain))
-			pte_root |= DTE_FLAG_GIOV;
-	}
+	/* Need to preserve interrupt remapping information in DTE[128:255] */
+	new.data128[1] = dte->data128[1];
 
-	flags &= ~DEV_DOMID_MASK;
-	flags |= domid;
+	set_dte_gcr3_table(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;
+	update_dte256(iommu, dev_data, &new);
 
 	/*
 	 * A kdump kernel might be replacing a domain ID that was copied from
-- 
2.34.1
Re: [PATCH v4 3/6] iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers
Posted by Jason Gunthorpe 2 months ago 
On Mon, Sep 16, 2024 at 05:18:02PM +0000, Suravee Suthikulpanit wrote:
> 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 | 117 +++++++++++++++++++++-----------------
>  1 file changed, 65 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 48a721d10f06..12f27061680d 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1947,17 +1947,58 @@ 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;

When does this get called to install a gcr3 table without a v2 domain?

Other than my remark on patch 5 this looks Ok to me and making a
helper function for the gcr3 case is a good step forward.

Suggest you follow up with helper functions for blocking, identity and
v1 as well :) Then it will be really easy to follow.

Thanks,
Jason
Re: [PATCH v4 3/6] iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers
Posted by Suthikulpanit, Suravee 1 month, 3 weeks ago 
On 9/27/2024 2:56 AM, Jason Gunthorpe wrote:
> On Mon, Sep 16, 2024 at 05:18:02PM +0000, Suravee Suthikulpanit wrote:
>> 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 | 117 +++++++++++++++++++++-----------------
>>   1 file changed, 65 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 48a721d10f06..12f27061680d 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -1947,17 +1947,58 @@ 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;
> 
> When does this get called to install a gcr3 table without a v2 domain?

The GCR3 table is also used when we setup v2 table for SVA stuff. In 
such case, we would be setting up w/ PASID. Therefore, the GIOV bit is 
not needed.

> Other than my remark on patch 5 this looks Ok to me and making a
> helper function for the gcr3 case is a good step forward.
> 
> Suggest you follow up with helper functions for blocking, identity and
> v1 as well :) Then it will be really easy to follow.

We will look to simplify the code in the future.

Thanks,
Suravee
Re: [PATCH v4 3/6] iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers
Posted by Jason Gunthorpe 1 month, 3 weeks ago 
On Thu, Oct 03, 2024 at 11:16:19PM +0700, Suthikulpanit, Suravee wrote:

> > > +	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;
> > 
> > When does this get called to install a gcr3 table without a v2 domain?
> 
> The GCR3 table is also used when we setup v2 table for SVA stuff. In such
> case, we would be setting up w/ PASID. Therefore, the GIOV bit is not
> needed.

If I understand the manual right this should be written as:

if (dev_data->domain->type != IDENTITY)
   target->data[0] |= DTE_FLAG_GIOV;

Ie everything on the RID except identity should be translated through
PASID 0 and if PASID 0 is a V2 page table then it will translate and
if PASID 0 is non-valid then it will block?

Identity needs to not use GIOV otherwise it will be blocking?

Jason

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.