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@xxxxxxx>
>>
>> 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@xxxxxxx>
>> ---
>> 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 ?
