[PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Alejandro Jimenez]

When updating the page table root field on the DTE, avoid overwriting any
bits that are already set. The earlier call to make_clear_dte() writes
default values that all DTEs must have set (currently DTE[V]), and those
must be preserved.

Currently this doesn't cause problems since the page table root update is
the first field that is set after make_clear_dte() is called, and
DTE_FLAG_V is set again later along with the permission bits (IR/IW).
Remove this redundant assignment too.

Fixes: fd5dff9de4be ("iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers")
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.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 20177e18eb0d..a8c17b602c95 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2052,12 +2052,12 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	make_clear_dte(dev_data, dte, &new);
 
 	if (domain->iop.mode != PAGE_MODE_NONE)
-		new.data[0] = iommu_virt_to_phys(domain->iop.root);
+		new.data[0] |= iommu_virt_to_phys(domain->iop.root);
 
 	new.data[0] |= (domain->iop.mode & DEV_ENTRY_MODE_MASK)
 		    << DEV_ENTRY_MODE_SHIFT;
 
-	new.data[0] |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_V;
+	new.data[0] |= DTE_FLAG_IR | DTE_FLAG_IW;
 
 	/*
 	 * When SNP is enabled, we can only support TV=1 with non-zero domain ID.

base-commit: 1f37ef785d54b27e304c189038697a845259408a
-- 
2.39.3

Re: [PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Srivastava, Dheeraj Kumar]

Hi,

On 1/7/2025 12:44 AM, Alejandro Jimenez wrote:
> When updating the page table root field on the DTE, avoid overwriting any
> bits that are already set. The earlier call to make_clear_dte() writes
> default values that all DTEs must have set (currently DTE[V]), and those
> must be preserved.
> 
> Currently this doesn't cause problems since the page table root update is
> the first field that is set after make_clear_dte() is called, and
> DTE_FLAG_V is set again later along with the permission bits (IR/IW).
> Remove this redundant assignment too.
> 
> Fixes: fd5dff9de4be ("iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

LGTM. Please feel free to add...

Reviewed-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>

Thanks
Dheeraj

> ---
>   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 20177e18eb0d..a8c17b602c95 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2052,12 +2052,12 @@ static void set_dte_entry(struct amd_iommu *iommu,
>   	make_clear_dte(dev_data, dte, &new);
>   
>   	if (domain->iop.mode != PAGE_MODE_NONE)
> -		new.data[0] = iommu_virt_to_phys(domain->iop.root);
> +		new.data[0] |= iommu_virt_to_phys(domain->iop.root);
>   
>   	new.data[0] |= (domain->iop.mode & DEV_ENTRY_MODE_MASK)
>   		    << DEV_ENTRY_MODE_SHIFT;
>   
> -	new.data[0] |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_V;
> +	new.data[0] |= DTE_FLAG_IR | DTE_FLAG_IW;
>   
>   	/*
>   	 * When SNP is enabled, we can only support TV=1 with non-zero domain ID.
> 
> base-commit: 1f37ef785d54b27e304c189038697a845259408a

Re: [PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Vasant Hegde]

Hi Alejandro, Suravee,



On 1/7/2025 12:44 AM, Alejandro Jimenez wrote:
> When updating the page table root field on the DTE, avoid overwriting any
> bits that are already set. The earlier call to make_clear_dte() writes
> default values that all DTEs must have set (currently DTE[V]), and those
> must be preserved.
> 
> Currently this doesn't cause problems since the page table root update is
> the first field that is set after make_clear_dte() is called, and
> DTE_FLAG_V is set again later along with the permission bits (IR/IW).
> Remove this redundant assignment too.

Good catch!

> > Fixes: fd5dff9de4be ("iommu/amd: Modify set_dte_entry() to use 256-bit DTE
helpers")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.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 20177e18eb0d..a8c17b602c95 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2052,12 +2052,12 @@ static void set_dte_entry(struct amd_iommu *iommu,
>  	make_clear_dte(dev_data, dte, &new);

This fix is fine. But with all changes we have done in this code does, do we
really need make_clear_dte()?

How about removing `make_clear_dte()` and moving DTE_FLAG_V to
write_dte_lower128() ?

-Vasant

Re: [PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Arun Kodilkar, Sairaj]

On 1/7/2025 5:45 PM, Vasant Hegde wrote:
Hi vasant,
> 
> On 1/7/2025 12:44 AM, Alejandro Jimenez wrote:
>> When updating the page table root field on the DTE, avoid overwriting any
>> bits that are already set. The earlier call to make_clear_dte() writes
>> default values that all DTEs must have set (currently DTE[V]), and those
>> must be preserved.
>>
>> Currently this doesn't cause problems since the page table root update is
>> the first field that is set after make_clear_dte() is called, and
>> DTE_FLAG_V is set again later along with the permission bits (IR/IW).
>> Remove this redundant assignment too.
> 
> Good catch!
> 
>>> Fixes: fd5dff9de4be ("iommu/amd: Modify set_dte_entry() to use 256-bit DTE
> helpers")
>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.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 20177e18eb0d..a8c17b602c95 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2052,12 +2052,12 @@ static void set_dte_entry(struct amd_iommu *iommu,
>>   	make_clear_dte(dev_data, dte, &new);
> 
> This fix is fine. But with all changes we have done in this code does, do we
> really need make_clear_dte()?
> 
> How about removing `make_clear_dte()` and moving DTE_FLAG_V to
> write_dte_lower128() ?
> 

I dont think moving DTE_FLAG_V to write_dte_lower128() is a good
idea, because the order in which the function update_dte256()
assigns upper and lower 128 bits depends on this flag.

Regards
Sairaj Kodilkar

> -Vasant
> 
> 
> 

Re: [PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Jason Gunthorpe]

On Tue, Jan 07, 2025 at 05:45:38PM +0530, Vasant Hegde wrote:
> > @@ -2052,12 +2052,12 @@ static void set_dte_entry(struct amd_iommu *iommu,
> >  	make_clear_dte(dev_data, dte, &new);
> 
> This fix is fine. But with all changes we have done in this code does, do we
> really need make_clear_dte()?

> How about removing `make_clear_dte()` and moving DTE_FLAG_V to
> write_dte_lower128() ?

The V flag should just be set by the functions building the DTE,
write_dte_lower() should accept a fully formed dte as a matter of
layering.

I'm hopefull Suravee will come with something like this:

https://lore.kernel.org/linux-iommu/20241016142237.GP3559746@nvidia.com/

And then I'd expect to drop make_clear_dte() and sprinkle the V into
the individual settings that already have bitflags for entry 0.

The one place outside that call chain using make_clear_dte() is
clear_dte_entry() which should call a set_dte_blocked() with the above
break up.

Then the set_dte() functions are called by the attach functions and
you can get hitless replace.

Jason

Re: [PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Alejandro Jimenez]

On 1/7/25 13:01, Jason Gunthorpe wrote:
> On Tue, Jan 07, 2025 at 05:45:38PM +0530, Vasant Hegde wrote:
>>> @@ -2052,12 +2052,12 @@ static void set_dte_entry(struct amd_iommu *iommu,
>>>   	make_clear_dte(dev_data, dte, &new);
>>
>> This fix is fine. But with all changes we have done in this code does, do we
>> really need make_clear_dte()?
> 
>> How about removing `make_clear_dte()` and moving DTE_FLAG_V to
>> write_dte_lower128() ?
> 
> The V flag should just be set by the functions building the DTE,
> write_dte_lower() should accept a fully formed dte as a matter of
> layering.
> 
> I'm hopefull Suravee will come with something like this:
> 
> https://lore.kernel.org/linux-iommu/20241016142237.GP3559746@nvidia.com/
> 

Suravee did say he would implement the suggestions above in the series for
nested translation support, but that might take a while considering where
we are in the cycle. Given that this fix is straightforward and has been
reviewed by Dheeraj, are there any concerns with merging it in its current
form?

Otherwise I can implement and test the more extensive changes that Jason
provided above, and hopefully Suravee's work on nested support can easily
merge with those once ready.

Alejandro

Re: [PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Jason Gunthorpe]

On Wed, Feb 26, 2025 at 11:19:46AM -0500, Alejandro Jimenez wrote:
> On 1/7/25 13:01, Jason Gunthorpe wrote:
> > On Tue, Jan 07, 2025 at 05:45:38PM +0530, Vasant Hegde wrote:
> > > > @@ -2052,12 +2052,12 @@ static void set_dte_entry(struct amd_iommu *iommu,
> > > >   	make_clear_dte(dev_data, dte, &new);
> > > 
> > > This fix is fine. But with all changes we have done in this code does, do we
> > > really need make_clear_dte()?
> > 
> > > How about removing `make_clear_dte()` and moving DTE_FLAG_V to
> > > write_dte_lower128() ?
> > 
> > The V flag should just be set by the functions building the DTE,
> > write_dte_lower() should accept a fully formed dte as a matter of
> > layering.
> > 
> > I'm hopefull Suravee will come with something like this:
> > 
> > https://lore.kernel.org/linux-iommu/20241016142237.GP3559746@nvidia.com/
> > 
> 
> Suravee did say he would implement the suggestions above in the series for
> nested translation support, but that might take a while considering where
> we are in the cycle. Given that this fix is straightforward and has been
> reviewed by Dheeraj, are there any concerns with merging it in its current
> form?

+1 to take the fix

Jason

Re: [PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Vasant Hegde]

On 2/26/2025 9:49 PM, Alejandro Jimenez wrote:
> 
> 
> On 1/7/25 13:01, Jason Gunthorpe wrote:
>> On Tue, Jan 07, 2025 at 05:45:38PM +0530, Vasant Hegde wrote:
>>>> @@ -2052,12 +2052,12 @@ static void set_dte_entry(struct amd_iommu *iommu,
>>>>       make_clear_dte(dev_data, dte, &new);
>>>
>>> This fix is fine. But with all changes we have done in this code does, do we
>>> really need make_clear_dte()?
>>
>>> How about removing `make_clear_dte()` and moving DTE_FLAG_V to
>>> write_dte_lower128() ?
>>
>> The V flag should just be set by the functions building the DTE,
>> write_dte_lower() should accept a fully formed dte as a matter of
>> layering.
>>
>> I'm hopefull Suravee will come with something like this:
>>
>> https://lore.kernel.org/linux-iommu/20241016142237.GP3559746@nvidia.com/
>>
> 
> Suravee did say he would implement the suggestions above in the series for
> nested translation support, but that might take a while considering where
> we are in the cycle. Given that this fix is straightforward and has been
> reviewed by Dheeraj, are there any concerns with merging it in its current
> form?

Ack. I think its fine to pull current patch for now.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH 1/1] iommu/amd: Preserve default DTE fields when updating Host Page Table Root

[Joerg Roedel]

On Mon, Jan 06, 2025 at 07:14:13PM +0000, Alejandro Jimenez wrote:
> When updating the page table root field on the DTE, avoid overwriting any
> bits that are already set. The earlier call to make_clear_dte() writes
> default values that all DTEs must have set (currently DTE[V]), and those
> must be preserved.
> 
> Currently this doesn't cause problems since the page table root update is
> the first field that is set after make_clear_dte() is called, and
> DTE_FLAG_V is set again later along with the permission bits (IR/IW).
> Remove this redundant assignment too.
> 
> Fixes: fd5dff9de4be ("iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  drivers/iommu/amd/iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.