[PATCH rc] iommu/amd: Fix GT feature enablement again

[PATCH rc] iommu/amd: Fix GT feature enablement again

[Vasant Hegde]

Current code configures GCR3 even when device is attached to identity
domain. So that we can support SVA with identity domain. This means in
attach device path it updates Guest Translation related bits in DTE.

Commit de111f6b4f6a ("iommu/amd: Enable Guest Translation after reading
IOMMU feature register") missed to enable Control[GT] bit in resume
path. Its causing certain laptop to fail to resume after suspend.

This is because we have inconsistency between between control register
(GT is disabled) and DTE (where we have enabled guest translation related
bits) in resume path. And IOMMU hardware throws ILLEGAL_DEV_TABLE_ENTRY.

Fix it by enabling GT bit in resume path.

Reported-by: Błażej Szczygieł <spaz16@wp.pl>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218975
Fixes: de111f6b4f6a ("iommu/amd: Enable Guest Translation after reading IOMMU feature register")
Tested-by: Błażej Szczygieł <spaz16@wp.pl>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 161248067776..c89d85b54a1a 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2743,6 +2743,7 @@ static void early_enable_iommu(struct amd_iommu *iommu)
 	iommu_enable_command_buffer(iommu);
 	iommu_enable_event_buffer(iommu);
 	iommu_set_exclusion_range(iommu);
+	iommu_enable_gt(iommu);
 	iommu_enable_ga(iommu);
 	iommu_enable_xt(iommu);
 	iommu_enable_irtcachedis(iommu);
-- 
2.31.1

Re: [PATCH rc] iommu/amd: Fix GT feature enablement again

[Jerry Snitselaar]

On Fri, Jun 21, 2024 at 10:15:33AM GMT, Vasant Hegde wrote:
> Current code configures GCR3 even when device is attached to identity
> domain. So that we can support SVA with identity domain. This means in
> attach device path it updates Guest Translation related bits in DTE.
> 
> Commit de111f6b4f6a ("iommu/amd: Enable Guest Translation after reading
> IOMMU feature register") missed to enable Control[GT] bit in resume
> path. Its causing certain laptop to fail to resume after suspend.
> 

Does this bring back the issue that de111f6b4f6a was trying to fix in
relation to getting a bad value in the IVRS table?

> This is because we have inconsistency between between control register
> (GT is disabled) and DTE (where we have enabled guest translation related
> bits) in resume path. And IOMMU hardware throws ILLEGAL_DEV_TABLE_ENTRY.
> 
> Fix it by enabling GT bit in resume path.
> 
> Reported-by: Błażej Szczygieł <spaz16@wp.pl>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218975
> Fixes: de111f6b4f6a ("iommu/amd: Enable Guest Translation after reading IOMMU feature register")
> Tested-by: Błażej Szczygieł <spaz16@wp.pl>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/init.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 161248067776..c89d85b54a1a 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -2743,6 +2743,7 @@ static void early_enable_iommu(struct amd_iommu *iommu)
>  	iommu_enable_command_buffer(iommu);
>  	iommu_enable_event_buffer(iommu);
>  	iommu_set_exclusion_range(iommu);
> +	iommu_enable_gt(iommu);
>  	iommu_enable_ga(iommu);
>  	iommu_enable_xt(iommu);
>  	iommu_enable_irtcachedis(iommu);
> -- 
> 2.31.1
> 

Re: [PATCH rc] iommu/amd: Fix GT feature enablement again

[Vasant Hegde]

Hi Jerry,


On 6/22/2024 6:02 AM, Jerry Snitselaar wrote:
> On Fri, Jun 21, 2024 at 10:15:33AM GMT, Vasant Hegde wrote:
>> Current code configures GCR3 even when device is attached to identity
>> domain. So that we can support SVA with identity domain. This means in
>> attach device path it updates Guest Translation related bits in DTE.
>>
>> Commit de111f6b4f6a ("iommu/amd: Enable Guest Translation after reading
>> IOMMU feature register") missed to enable Control[GT] bit in resume
>> path. Its causing certain laptop to fail to resume after suspend.
>>
> 
> Does this bring back the issue that de111f6b4f6a was trying to fix in
> relation to getting a bad value in the IVRS table?


Actually it will not bring back that issue as I have kept the GT enablement in
iommu_init_pci() as well.

So if we have bad IVRS table, then it makes second attempt in iommu_init_pci()
path. So we are good.


We may have to rework some part of init code.. I will look into it later.


-Vasant


> 
>> This is because we have inconsistency between between control register
>> (GT is disabled) and DTE (where we have enabled guest translation related
>> bits) in resume path. And IOMMU hardware throws ILLEGAL_DEV_TABLE_ENTRY.
>>
>> Fix it by enabling GT bit in resume path.
>>
>> Reported-by: Błażej Szczygieł <spaz16@wp.pl>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218975
>> Fixes: de111f6b4f6a ("iommu/amd: Enable Guest Translation after reading IOMMU feature register")
>> Tested-by: Błażej Szczygieł <spaz16@wp.pl>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/init.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 161248067776..c89d85b54a1a 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -2743,6 +2743,7 @@ static void early_enable_iommu(struct amd_iommu *iommu)
>>  	iommu_enable_command_buffer(iommu);
>>  	iommu_enable_event_buffer(iommu);
>>  	iommu_set_exclusion_range(iommu);
>> +	iommu_enable_gt(iommu);
>>  	iommu_enable_ga(iommu);
>>  	iommu_enable_xt(iommu);
>>  	iommu_enable_irtcachedis(iommu);
>> -- 
>> 2.31.1
>>
> 
> 

Re: [PATCH rc] iommu/amd: Fix GT feature enablement again

[Jerry Snitselaar]

On Fri, Jun 21, 2024 at 10:15:33AM GMT, Vasant Hegde wrote:
> Current code configures GCR3 even when device is attached to identity
> domain. So that we can support SVA with identity domain. This means in
> attach device path it updates Guest Translation related bits in DTE.
> 
> Commit de111f6b4f6a ("iommu/amd: Enable Guest Translation after reading
> IOMMU feature register") missed to enable Control[GT] bit in resume
> path. Its causing certain laptop to fail to resume after suspend.
> 
> This is because we have inconsistency between between control register
> (GT is disabled) and DTE (where we have enabled guest translation related
> bits) in resume path. And IOMMU hardware throws ILLEGAL_DEV_TABLE_ENTRY.
> 
> Fix it by enabling GT bit in resume path.
> 
> Reported-by: Błażej Szczygieł <spaz16@wp.pl>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218975
> Fixes: de111f6b4f6a ("iommu/amd: Enable Guest Translation after reading IOMMU feature register")
> Tested-by: Błażej Szczygieł <spaz16@wp.pl>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

> ---
>  drivers/iommu/amd/init.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 161248067776..c89d85b54a1a 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -2743,6 +2743,7 @@ static void early_enable_iommu(struct amd_iommu *iommu)
>  	iommu_enable_command_buffer(iommu);
>  	iommu_enable_event_buffer(iommu);
>  	iommu_set_exclusion_range(iommu);
> +	iommu_enable_gt(iommu);
>  	iommu_enable_ga(iommu);
>  	iommu_enable_xt(iommu);
>  	iommu_enable_irtcachedis(iommu);
> -- 
> 2.31.1
> 

Re: [PATCH rc] iommu/amd: Fix GT feature enablement again

[Joerg Roedel]

On Fri, Jun 21, 2024 at 10:15:33AM +0000, Vasant Hegde wrote:
>  drivers/iommu/amd/init.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.