Message-ID: <ff8408bb-b110-4930-b914-98afe605c112@amd.com>
Date: Fri, 25 Apr 2025 14:46:02 -0500
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, hpa@...or.com, herbert@...dor.apana.org.au,
 x86@...nel.org, john.allen@....com, davem@...emloft.net,
 thomas.lendacky@....com, michael.roth@....com, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org
Subject: Re: [PATCH v3 4/4] KVM: SVM: Add SEV-SNP CipherTextHiding support

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@....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@....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
>>

Powered by blists - more mailing lists