[PATCH v2 rc] iommu/amd: Do not enable SNP when V2 page table is enabled

[PATCH v2 rc] iommu/amd: Do not enable SNP when V2 page table is enabled

[Vasant Hegde]

DTE[Mode]=0 is not supported when SNP is enabled in the host. That means
to support SNP, IOMMU must be configured with V1 page table (See IOMMU
spec [1] for the details). If user passes kernel command line to configure
IOMMU domains with v2 page table (amd_iommu=pgtbl_v2) then disable SNP.

[1] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/specifications/48882_IOMMU.pdf

Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
Changes in v2:
  - Rebased on top of v6.9-rc3 and resolved the conflict.

-Vasant

 drivers/iommu/amd/init.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 005981c2f96a..0d9b69765099 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3215,28 +3215,29 @@ static void iommu_snp_enable(void)
 		return;
 	/*
 	 * The SNP support requires that IOMMU must be enabled, and is
-	 * not configured in the passthrough mode.
+	 * configured with V1 page table (DTE[Mode] = 0 is not supported).
 	 */
 	if (no_iommu || iommu_default_passthrough()) {
 		pr_err("SNP: IOMMU disabled or configured in passthrough mode, SNP cannot be supported.\n");
-		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
-		return;
+		goto disable_snp;
+	}
+
+	if (amd_iommu_pgtable != AMD_IOMMU_V1) {
+		pr_warn("SNP: IOMMU is configured with V2 page table mode, SNP cannot be supported.\n");
+		goto disable_snp;
 	}
 
 	amd_iommu_snp_en = check_feature(FEATURE_SNP);
 	if (!amd_iommu_snp_en) {
 		pr_err("SNP: IOMMU SNP feature not enabled, SNP cannot be supported.\n");
-		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
-		return;
+		goto disable_snp;
 	}
 
 	pr_info("IOMMU SNP support enabled.\n");
+	return;
 
-	/* Enforce IOMMU v1 pagetable when SNP is enabled. */
-	if (amd_iommu_pgtable != AMD_IOMMU_V1) {
-		pr_warn("Forcing use of AMD IOMMU v1 page table due to SNP.\n");
-		amd_iommu_pgtable = AMD_IOMMU_V1;
-	}
+disable_snp:
+	cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
 #endif
 }
 
-- 
2.31.1

Re: [PATCH v2 rc] iommu/amd: Do not enable SNP when V2 page table is enabled

[Tom Lendacky]

On 4/9/24 00:13, Vasant Hegde wrote:
> DTE[Mode]=0 is not supported when SNP is enabled in the host. That means
> to support SNP, IOMMU must be configured with V1 page table (See IOMMU
> spec [1] for the details). If user passes kernel command line to configure
> IOMMU domains with v2 page table (amd_iommu=pgtbl_v2) then disable SNP.

Might be nice to say that you are doing as the user asked by not forcing 
the pagetables to v1.

One minor comment below, otherwise:

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> 
> [1] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/specifications/48882_IOMMU.pdf
> 
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> Changes in v2:
>    - Rebased on top of v6.9-rc3 and resolved the conflict.
> 
> -Vasant
> 
>   drivers/iommu/amd/init.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 005981c2f96a..0d9b69765099 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3215,28 +3215,29 @@ static void iommu_snp_enable(void)
>   		return;
>   	/*
>   	 * The SNP support requires that IOMMU must be enabled, and is
> -	 * not configured in the passthrough mode.
> +	 * configured with V1 page table (DTE[Mode] = 0 is not supported).
>   	 */
>   	if (no_iommu || iommu_default_passthrough()) {
>   		pr_err("SNP: IOMMU disabled or configured in passthrough mode, SNP cannot be supported.\n");
> -		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
> -		return;
> +		goto disable_snp;
> +	}
> +
> +	if (amd_iommu_pgtable != AMD_IOMMU_V1) {
> +		pr_warn("SNP: IOMMU is configured with V2 page table mode, SNP cannot be supported.\n");

In the previous check, pr_err() is used, while here pr_warn() is used. 
Should be consistent. Not sure if that means changing the pr_err() to 
pr_warn() or making this pr_warn() a pr_err().

Thanks,
Tom

> +		goto disable_snp;
>   	}
>   
>   	amd_iommu_snp_en = check_feature(FEATURE_SNP);
>   	if (!amd_iommu_snp_en) {
>   		pr_err("SNP: IOMMU SNP feature not enabled, SNP cannot be supported.\n");
> -		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
> -		return;
> +		goto disable_snp;
>   	}
>   
>   	pr_info("IOMMU SNP support enabled.\n");
> +	return;
>   
> -	/* Enforce IOMMU v1 pagetable when SNP is enabled. */
> -	if (amd_iommu_pgtable != AMD_IOMMU_V1) {
> -		pr_warn("Forcing use of AMD IOMMU v1 page table due to SNP.\n");
> -		amd_iommu_pgtable = AMD_IOMMU_V1;
> -	}
> +disable_snp:
> +	cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
>   #endif
>   }
>   

Re: [PATCH v2 rc] iommu/amd: Do not enable SNP when V2 page table is enabled

[Vasant Hegde]

Tom,


On 4/9/2024 8:06 PM, Tom Lendacky wrote:
> On 4/9/24 00:13, Vasant Hegde wrote:
>> DTE[Mode]=0 is not supported when SNP is enabled in the host. That means
>> to support SNP, IOMMU must be configured with V1 page table (See IOMMU
>> spec [1] for the details). If user passes kernel command line to 
>> configure
>> IOMMU domains with v2 page table (amd_iommu=pgtbl_v2) then disable SNP.
> 
> Might be nice to say that you are doing as the user asked by not forcing 
> the pagetables to v1.

Sure. I will update the description.


> 
> One minor comment below, otherwise:
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
>>
>> [1] 
>> https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/specifications/48882_IOMMU.pdf
>>
>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>> Cc: Michael Roth <michael.roth@amd.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> Changes in v2:
>>    - Rebased on top of v6.9-rc3 and resolved the conflict.
>>
>> -Vasant
>>
>>   drivers/iommu/amd/init.c | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 005981c2f96a..0d9b69765099 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -3215,28 +3215,29 @@ static void iommu_snp_enable(void)
>>           return;
>>       /*
>>        * The SNP support requires that IOMMU must be enabled, and is
>> -     * not configured in the passthrough mode.
>> +     * configured with V1 page table (DTE[Mode] = 0 is not supported).
>>        */
>>       if (no_iommu || iommu_default_passthrough()) {
>>           pr_err("SNP: IOMMU disabled or configured in passthrough 
>> mode, SNP cannot be supported.\n");
>> -        cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
>> -        return;
>> +        goto disable_snp;
>> +    }
>> +
>> +    if (amd_iommu_pgtable != AMD_IOMMU_V1) {
>> +        pr_warn("SNP: IOMMU is configured with V2 page table mode, 
>> SNP cannot be supported.\n");
> 
> In the previous check, pr_err() is used, while here pr_warn() is used. 
> Should be consistent. Not sure if that means changing the pr_err() to 
> pr_warn() or making this pr_warn() a pr_err().

Looks like we had this inconsistency from beginning. May be we should 
convert it to warning. I will post separate fix for that.


-Vasant