From: Ashish Kalra <ashish.kalra@amd.com>

Ciphertext hiding prevents host accesses from reading the ciphertext of
SNP guest private memory. Instead of reading ciphertext, the host reads
will see constant default values (0xff).

Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
ASIDs. All SNP active guests must have an ASID less than or equal to
MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests
(SEV and SEV-ES) must be greater than MAX_SNP_ASID.

This patch-set adds two new module parameters to the KVM module, first
to enable CipherTextHiding support and a user configurable MAX_SNP_ASID
to define the system-wide maximum SNP ASID value. If this value is not set,
then the ASID space is equally divided between SEV-SNP and SEV-ES guests.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/kvm/svm/sev.c | 50 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7a156ba07d1f..a905f755312a 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -58,6 +58,14 @@ static bool sev_es_debug_swap_enabled = true;
 module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
 static u64 sev_supported_vmsa_features;
 
+static bool cipher_text_hiding;
+module_param(cipher_text_hiding, bool, 0444);
+MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
+
+static int max_snp_asid;
+module_param(max_snp_asid, int, 0444);
+MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");
+
 #define AP_RESET_HOLD_NONE		0
 #define AP_RESET_HOLD_NAE_EVENT		1
 #define AP_RESET_HOLD_MSR_PROTO		2
@@ -85,6 +93,8 @@ static DEFINE_MUTEX(sev_bitmap_lock);
 unsigned int max_sev_asid;
 static unsigned int min_sev_asid;
 static unsigned long sev_me_mask;
+static unsigned int snp_max_snp_asid;
+static bool snp_cipher_text_hiding;
 static unsigned int nr_asids;
 static unsigned long *sev_asid_bitmap;
 static unsigned long *sev_reclaim_asid_bitmap;
@@ -171,7 +181,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
 	misc_cg_uncharge(type, sev->misc_cg, 1);
 }
 
-static int sev_asid_new(struct kvm_sev_info *sev)
+static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
 {
 	/*
 	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
@@ -199,6 +209,18 @@ static int sev_asid_new(struct kvm_sev_info *sev)
 
 	mutex_lock(&sev_bitmap_lock);
 
+	/*
+	 * When CipherTextHiding is enabled, all SNP guests must have an
+	 * ASID less than or equal to MAX_SNP_ASID provided on the
+	 * SNP_INIT_EX command and all the SEV-ES guests must have
+	 * an ASID greater than MAX_SNP_ASID.
+	 */
+	if (snp_cipher_text_hiding && sev->es_active) {
+		if (vm_type == KVM_X86_SNP_VM)
+			max_asid = snp_max_snp_asid;
+		else
+			min_asid = snp_max_snp_asid + 1;
+	}
 again:
 	asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid);
 	if (asid > max_asid) {
@@ -438,7 +460,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (vm_type == KVM_X86_SNP_VM)
 		sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
 
-	ret = sev_asid_new(sev);
+	ret = sev_asid_new(sev, vm_type);
 	if (ret)
 		goto e_no_asid;
 
@@ -3005,6 +3027,18 @@ void __init sev_hardware_setup(void)
 	if (!sev_es_enabled)
 		goto out;
 
+	if (cipher_text_hiding && is_sev_snp_ciphertext_hiding_supported()) {
+		/* Do sanity checks on user-defined MAX_SNP_ASID */
+		if (max_snp_asid >= edx) {
+			pr_info("max_snp_asid module parameter is not valid, limiting to %d\n",
+				 edx - 1);
+			max_snp_asid = edx - 1;
+		}
+		snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2;
+		snp_cipher_text_hiding = true;
+		pr_info("SEV-SNP CipherTextHiding feature support enabled\n");
+	}
+
 	/*
 	 * SEV-ES requires MMIO caching as KVM doesn't have access to the guest
 	 * instruction stream, i.e. can't emulate in response to a #NPF and
@@ -3040,14 +3074,18 @@ void __init sev_hardware_setup(void)
 								       "unusable" :
 								       "disabled",
 			min_sev_asid, max_sev_asid);
-	if (boot_cpu_has(X86_FEATURE_SEV_ES))
+	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
+		if (snp_max_snp_asid >= (min_sev_asid - 1))
+			sev_es_supported = false;
 		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
 			str_enabled_disabled(sev_es_supported),
-			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+			min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
+							      0, min_sev_asid - 1);
+	}
 	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
 		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
 			str_enabled_disabled(sev_snp_supported),
-			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+			min_sev_asid > 1 ? 1 : 0, snp_max_snp_asid ? : min_sev_asid - 1);
 
 	sev_enabled = sev_supported;
 	sev_es_enabled = sev_es_supported;
@@ -3068,6 +3106,8 @@ void __init sev_hardware_setup(void)
 	 * Do both SNP and SEV initialization at KVM module load.
 	 */
 	init_args.probe = true;
+	init_args.cipher_text_hiding_en = snp_cipher_text_hiding;
+	init_args.snp_max_snp_asid = snp_max_snp_asid;
 	sev_platform_init(&init_args);
 }
 
-- 
2.34.1

On Tue, Apr 22, 2025, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Ciphertext hiding prevents host accesses from reading the ciphertext of
> SNP guest private memory. Instead of reading ciphertext, the host reads
> will see constant default values (0xff).
> 
> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
> ASIDs.

Uh, no.  The only "host" ASID is '0'.

> All SNP active guests must have an ASID less than or equal to MAX_SNP_ASID
> provided to the SNP_INIT_EX command. All SEV-legacy guests (SEV and SEV-ES)
> must be greater than MAX_SNP_ASID.

This is misleading, arguably wrong.  The ASID space is already split into legacy+SEV and
SEV-ES+.  CTH further splits the SEV-ES+ space into SEV-ES and SEV-SNP+.
> 
> This patch-set adds two new module parameters to the KVM module, first

No "This patch".

> to enable CipherTextHiding support and a user configurable MAX_SNP_ASID
> to define the system-wide maximum SNP ASID value. If this value is not set,
> then the ASID space is equally divided between SEV-SNP and SEV-ES guests.

This quite, and I suspect completely useless for every production use case.  I
also *really* dislike max_snp_asid.  More below.

> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  arch/x86/kvm/svm/sev.c | 50 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7a156ba07d1f..a905f755312a 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -58,6 +58,14 @@ static bool sev_es_debug_swap_enabled = true;
>  module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>  static u64 sev_supported_vmsa_features;
>  
> +static bool cipher_text_hiding;
> +module_param(cipher_text_hiding, bool, 0444);
> +MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
> +
> +static int max_snp_asid;
> +module_param(max_snp_asid, int, 0444);
> +MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");

I'd much, much prefer proper document in Documentation/admin-guide/kernel-parameters.txt.
The basic gist of the params is self-explanatory, but how all of this works is not.

And max_snp_asid is extremely misleading.  Pretty much any reader is going to expect
it to do what it says: set the max SNP ASID.  But unless cipher_text_hiding is
enabled, which it's not by default, the param does absolutely nothing.

To address both problems, can we somehow figure out a way to use a single param?
The hardest part is probably coming up with a name.  E.g.

  static int ciphertext_hiding_nr_asids;
  module_param(ciphertext_hiding_nr_asids, int, 0444);

Then a non-zero value means "enable CipherTexthiding", and effects the ASID carve-out.
If we wanted to support the 50/50 split, we would use '-1' as an "auto" flag,
i.e. enable CipherTexthiding and split the SEV-ES+ ASIDs.  Though to be honest,
I'd prefer to avoid that unless it's actually useful.

Ha!  And I'm doubling down on that suggestion, because this code is wrong:

	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
		if (snp_max_snp_asid >= (min_sev_asid - 1))
			sev_es_supported = false;
		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
			str_enabled_disabled(sev_es_supported),
			min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
							      0, min_sev_asid - 1);
	}

A non-zero snp_max_snp_asid shouldn't break SEV-ES if CipherTextHiding isn't supported.

>  #define AP_RESET_HOLD_NONE		0
>  #define AP_RESET_HOLD_NAE_EVENT		1
>  #define AP_RESET_HOLD_MSR_PROTO		2
> @@ -85,6 +93,8 @@ static DEFINE_MUTEX(sev_bitmap_lock);
>  unsigned int max_sev_asid;
>  static unsigned int min_sev_asid;
>  static unsigned long sev_me_mask;
> +static unsigned int snp_max_snp_asid;
> +static bool snp_cipher_text_hiding;
>  static unsigned int nr_asids;
>  static unsigned long *sev_asid_bitmap;
>  static unsigned long *sev_reclaim_asid_bitmap;
> @@ -171,7 +181,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
>  	misc_cg_uncharge(type, sev->misc_cg, 1);
>  }
>  
> -static int sev_asid_new(struct kvm_sev_info *sev)
> +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
>  {
>  	/*
>  	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
> @@ -199,6 +209,18 @@ static int sev_asid_new(struct kvm_sev_info *sev)
>  
>  	mutex_lock(&sev_bitmap_lock);
>  
> +	/*
> +	 * When CipherTextHiding is enabled, all SNP guests must have an
> +	 * ASID less than or equal to MAX_SNP_ASID provided on the

Wrap at ~80, not

> +	 * SNP_INIT_EX command and all the SEV-ES guests must have
> +	 * an ASID greater than MAX_SNP_ASID.

Please don't referense MAX_SNP_ASID.  The reader doesn't need to know what the
PSP calls its parameter.  What matters is the concept, and to a lesser extent
KVM's param.

> +	 */
> +	if (snp_cipher_text_hiding && sev->es_active) {
> +		if (vm_type == KVM_X86_SNP_VM)
> +			max_asid = snp_max_snp_asid;
> +		else
> +			min_asid = snp_max_snp_asid + 1;
> +	}

Irrespective of the module params, I would much prefer to have a max_snp_asid
param that is kept up-to-date regardless of whether or not CipherTextHiding is
enabled.   Then you don't need a comment here, only a big fat comment in the code
that configures the min/max ASIDs, which is going to be a gnarly comment no matter
what we do.  Oh, and this should be done before the

	if (min_asid > max_asid)
		return -ENOTTY;

sanity check.

And then drop the mix of ternary operators and if statements, and just do:

	unsigned int min_asid, max_asid, asid;
	bool retry = true;
	int ret;

	if (vm_type == KVM_X86_SNP_VM) {
		min_asid = min_snp_asid;
		max_asid = max_snp_asid;
	} else if (sev->es_active) {
		min_asid = min_sev_es_asid;
		max_asid = max_sev_es_asid;
	} else {
		min_asid = min_sev_asid;
		max_asid = max_sev_asid;
	}

	/*
	 * The min ASID can end up larger than the max if basic SEV support is
	 * effectively disabled by disallowing use of ASIDs for SEV guests.
	 * Ditto for SEV-ES guests when CipherTextHiding is enabled.
	 */
	if (min_asid > max_asid)
		return -ENOTTY;

> @@ -3040,14 +3074,18 @@ void __init sev_hardware_setup(void)
>  								       "unusable" :
>  								       "disabled",
>  			min_sev_asid, max_sev_asid);
> -	if (boot_cpu_has(X86_FEATURE_SEV_ES))
> +	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
> +		if (snp_max_snp_asid >= (min_sev_asid - 1))
> +			sev_es_supported = false;
>  		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
>  			str_enabled_disabled(sev_es_supported),
> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
> +			min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
> +							      0, min_sev_asid - 1);
> +	}
>  	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
>  		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
>  			str_enabled_disabled(sev_snp_supported),
> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
> +			min_sev_asid > 1 ? 1 : 0, snp_max_snp_asid ? : min_sev_asid - 1);

Mixing in snp_max_snp_asid pretty much makes this is unreadable.  Please rework
this code to generate {min,max}_{sev,sev_es,snp,}_asid (add prep patches if
necessary).  I don't care terribly if ternary operators are used, but please
don't chain them.

>  
>  	sev_enabled = sev_supported;
>  	sev_es_enabled = sev_es_supported;
> @@ -3068,6 +3106,8 @@ void __init sev_hardware_setup(void)
>  	 * Do both SNP and SEV initialization at KVM module load.
>  	 */
>  	init_args.probe = true;
> +	init_args.cipher_text_hiding_en = snp_cipher_text_hiding;
> +	init_args.snp_max_snp_asid = snp_max_snp_asid;
>  	sev_platform_init(&init_args);
>  }
>  
> -- 
> 2.34.1
>

Hello Sean,

On 4/23/2025 4:15 PM, Sean Christopherson wrote:
> On Tue, Apr 22, 2025, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> Ciphertext hiding prevents host accesses from reading the ciphertext of
>> SNP guest private memory. Instead of reading ciphertext, the host reads
>> will see constant default values (0xff).
>>
>> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
>> ASIDs.
> 
> Uh, no.  The only "host" ASID is '0'.
> 
>> All SNP active guests must have an ASID less than or equal to MAX_SNP_ASID
>> provided to the SNP_INIT_EX command. All SEV-legacy guests (SEV and SEV-ES)
>> must be greater than MAX_SNP_ASID.
> 
> This is misleading, arguably wrong.  The ASID space is already split into legacy+SEV and
> SEV-ES+.  CTH further splits the SEV-ES+ space into SEV-ES and SEV-SNP+.
>>

But the above statement is practically correct, once CTH is enabled, 
SNP guests must have ASIDs less than or equal to MAX_SNP_ASID and SEV and SEV-ES
have to use ASIDs greater than MAX_SNP_ASID. 

And yes, CTH basically splits the SEV-ES ASID space further into SEV-ES and SEV-SNP.

>> This patch-set adds two new module parameters to the KVM module, first
> 
> No "This patch".
> 
>> to enable CipherTextHiding support and a user configurable MAX_SNP_ASID
>> to define the system-wide maximum SNP ASID value. If this value is not set,
>> then the ASID space is equally divided between SEV-SNP and SEV-ES guests.
> 

What i really mean is that if CTH support is enabled and this MAX_SNP_ASID
is not defined by the user then the ASID space is equally divided between SNP and SEV-ES.

> This quite, and I suspect completely useless for every production use case.  I
> also *really* dislike max_snp_asid.  More below.
> 
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>  arch/x86/kvm/svm/sev.c | 50 +++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 45 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 7a156ba07d1f..a905f755312a 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -58,6 +58,14 @@ static bool sev_es_debug_swap_enabled = true;
>>  module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>>  static u64 sev_supported_vmsa_features;
>>  
>> +static bool cipher_text_hiding;
>> +module_param(cipher_text_hiding, bool, 0444);
>> +MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
>> +
>> +static int max_snp_asid;
>> +module_param(max_snp_asid, int, 0444);
>> +MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");
> 
> I'd much, much prefer proper document in Documentation/admin-guide/kernel-parameters.txt.
> The basic gist of the params is self-explanatory, but how all of this works is not.
> 
> And max_snp_asid is extremely misleading.  Pretty much any reader is going to expect
> it to do what it says: set the max SNP ASID.  But unless cipher_text_hiding is
> enabled, which it's not by default, the param does absolutely nothing.

Yes, that's what i said above. 

But i do agree it is confusing and misleading.

> 
> To address both problems, can we somehow figure out a way to use a single param?
> The hardest part is probably coming up with a name.  E.g.
> 
>   static int ciphertext_hiding_nr_asids;
>   module_param(ciphertext_hiding_nr_asids, int, 0444);
> 
> Then a non-zero value means "enable CipherTexthiding", and effects the ASID carve-out.
> If we wanted to support the 50/50 split, we would use '-1' as an "auto" flag,
> i.e. enable CipherTexthiding and split the SEV-ES+ ASIDs.

Ok, that makes sense.

Right, split the SEV-ES+ ASID space between SNP and SEV-ES.  

> Though to be honest,
> I'd prefer to avoid that unless it's actually useful.
> 
> Ha!  And I'm doubling down on that suggestion, because this code is wrong:

Where ?

> 
> 	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
> 		if (snp_max_snp_asid >= (min_sev_asid - 1))
> 			sev_es_supported = false;

SEV-ES is disabled if SNP is using all ASIDs upto min_sev_asid - 1.

> 		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
> 			str_enabled_disabled(sev_es_supported),
> 			min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
> 							      0, min_sev_asid - 1);
> 	}
> 
> A non-zero snp_max_snp_asid shouldn't break SEV-ES if CipherTextHiding isn't supported.

I don't see above where SEV-ES is broken if snp_max_snp_asid is non-zero and CTH is enabled ?

If snp_max_snp_asid == min_sev_asid-1, then SEV-ES is going to be disabled, right ?

> 
>>  #define AP_RESET_HOLD_NONE		0
>>  #define AP_RESET_HOLD_NAE_EVENT		1
>>  #define AP_RESET_HOLD_MSR_PROTO		2
>> @@ -85,6 +93,8 @@ static DEFINE_MUTEX(sev_bitmap_lock);
>>  unsigned int max_sev_asid;
>>  static unsigned int min_sev_asid;
>>  static unsigned long sev_me_mask;
>> +static unsigned int snp_max_snp_asid;
>> +static bool snp_cipher_text_hiding;
>>  static unsigned int nr_asids;
>>  static unsigned long *sev_asid_bitmap;
>>  static unsigned long *sev_reclaim_asid_bitmap;
>> @@ -171,7 +181,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
>>  	misc_cg_uncharge(type, sev->misc_cg, 1);
>>  }
>>  
>> -static int sev_asid_new(struct kvm_sev_info *sev)
>> +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
>>  {
>>  	/*
>>  	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
>> @@ -199,6 +209,18 @@ static int sev_asid_new(struct kvm_sev_info *sev)
>>  
>>  	mutex_lock(&sev_bitmap_lock);
>>  
>> +	/*
>> +	 * When CipherTextHiding is enabled, all SNP guests must have an
>> +	 * ASID less than or equal to MAX_SNP_ASID provided on the
> 
> Wrap at ~80, not
> 
>> +	 * SNP_INIT_EX command and all the SEV-ES guests must have
>> +	 * an ASID greater than MAX_SNP_ASID.
> 
> Please don't referense MAX_SNP_ASID.  The reader doesn't need to know what the
> PSP calls its parameter.  What matters is the concept, and to a lesser extent
> KVM's param.
>

Ok.

>> +	 */
>> +	if (snp_cipher_text_hiding && sev->es_active) {
>> +		if (vm_type == KVM_X86_SNP_VM)
>> +			max_asid = snp_max_snp_asid;
>> +		else
>> +			min_asid = snp_max_snp_asid + 1;
>> +	}
> 
> Irrespective of the module params, I would much prefer to have a max_snp_asid
> param that is kept up-to-date regardless of whether or not CipherTextHiding is
> enabled. 

param ?

From what i see with your suggestions below, you are suggesting adding new
{min,max}snp/sev_es/sev_asid to track min and max ASIDs for SNP, SEV-ES
and SEV separately. 

> Then you don't need a comment here, only a big fat comment in the code
> that configures the min/max ASIDs, which is going to be a gnarly comment no matter
> what we do.  Oh, and this should be done before the
> 
> 	if (min_asid > max_asid)
> 		return -ENOTTY;
> 
> sanity check.
> 
> And then drop the mix of ternary operators and if statements, and just do:
> 
> 	unsigned int min_asid, max_asid, asid;
> 	bool retry = true;
> 	int ret;
> 
> 	if (vm_type == KVM_X86_SNP_VM) {
> 		min_asid = min_snp_asid;
> 		max_asid = max_snp_asid;
> 	} else if (sev->es_active) {
> 		min_asid = min_sev_es_asid;
> 		max_asid = max_sev_es_asid;
> 	} else {
> 		min_asid = min_sev_asid;
> 		max_asid = max_sev_asid;
> 	}
> 
> 	/*
> 	 * The min ASID can end up larger than the max if basic SEV support is
> 	 * effectively disabled by disallowing use of ASIDs for SEV guests.
> 	 * Ditto for SEV-ES guests when CipherTextHiding is enabled.
> 	 */

Ok, makes sense.

> 	if (min_asid > max_asid)
> 		return -ENOTTY;
> 
>> @@ -3040,14 +3074,18 @@ void __init sev_hardware_setup(void)
>>  								       "unusable" :
>>  								       "disabled",
>>  			min_sev_asid, max_sev_asid);
>> -	if (boot_cpu_has(X86_FEATURE_SEV_ES))
>> +	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
>> +		if (snp_max_snp_asid >= (min_sev_asid - 1))
>> +			sev_es_supported = false;
>>  		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
>>  			str_enabled_disabled(sev_es_supported),
>> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
>> +			min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
>> +							      0, min_sev_asid - 1);
>> +	}
>>  	if (boot_cpu_has(X86_FEATURE_SEV_SNP))
>>  		pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
>>  			str_enabled_disabled(sev_snp_supported),
>> -			min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
>> +			min_sev_asid > 1 ? 1 : 0, snp_max_snp_asid ? : min_sev_asid - 1);
> 
> Mixing in snp_max_snp_asid pretty much makes this is unreadable.  Please rework
> this code to generate {min,max}_{sev,sev_es,snp,}_asid (add prep patches if
> necessary).  I don't care terribly if ternary operators are used, but please
> don't chain them.
> 

Ok.

Thanks,
Ashish

>>  
>>  	sev_enabled = sev_supported;
>>  	sev_es_enabled = sev_es_supported;
>> @@ -3068,6 +3106,8 @@ void __init sev_hardware_setup(void)
>>  	 * Do both SNP and SEV initialization at KVM module load.
>>  	 */
>>  	init_args.probe = true;
>> +	init_args.cipher_text_hiding_en = snp_cipher_text_hiding;
>> +	init_args.snp_max_snp_asid = snp_max_snp_asid;
>>  	sev_platform_init(&init_args);
>>  }
>>  
>> -- 
>> 2.34.1
>>

On Fri, Apr 25, 2025, Ashish Kalra wrote:
> On 4/23/2025 4:15 PM, Sean Christopherson wrote:
> 
> > 
> > 	if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
> > 		if (snp_max_snp_asid >= (min_sev_asid - 1))
> > 			sev_es_supported = false;
> 
> SEV-ES is disabled if SNP is using all ASIDs upto min_sev_asid - 1.
> 
> > 		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
> > 			str_enabled_disabled(sev_es_supported),
> > 			min_sev_asid > 1 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
> > 							      0, min_sev_asid - 1);
> > 	}
> > 
> > A non-zero snp_max_snp_asid shouldn't break SEV-ES if CipherTextHiding isn't supported.
> 
> I don't see above where SEV-ES is broken if snp_max_snp_asid is non-zero and
> CTH is enabled ?

Please read what I wrote.  I did not say it's broken if CTH is enabled.  I said
it's broken if CTH isn't supported, i.e. is disabled.

snp_max_snp_asid isn't sanitized if CTH is unsupported or disabled by userspace,
and so KVM will compute the wrong min_sev_asid if snp_max_snp_asid is non-zero,
even though snp_max_snp_asid has no bearing on reality.

> >> +	 */
> >> +	if (snp_cipher_text_hiding && sev->es_active) {
> >> +		if (vm_type == KVM_X86_SNP_VM)
> >> +			max_asid = snp_max_snp_asid;
> >> +		else
> >> +			min_asid = snp_max_snp_asid + 1;
> >> +	}
> > 
> > Irrespective of the module params, I would much prefer to have a max_snp_asid
> > param that is kept up-to-date regardless of whether or not CipherTextHiding is
> > enabled. 
> 
> param ?

Sorry, s/param/variable.  Doesn't need to be user visible.