Re: [PATCH v4 5/5] KVM: SEV: Add SEV-SNP CipherTextHiding support

["Kalra, Ashish" <ashish.kalra@amd.com>]

On 6/5/2025 1:32 AM, Alexey Kardashevskiy wrote:
> On 20/5/25 10:02, 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).
>>
>> The SEV ASID space is basically split into legacy SEV and SEV-ES+.
>> CipherTextHiding further partitions the SEV-ES+ ASID space into SEV-ES
>> and SEV-SNP.
>>
>> Add new module parameter to the KVM module to enable CipherTextHiding
>> support and a user configurable system-wide maximum SNP ASID value. If
>> the module parameter value is -1 then the ASID space is equally
>> divided between SEV-SNP and SEV-ES guests.
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>   .../admin-guide/kernel-parameters.txt         | 10 ++++++
>>   arch/x86/kvm/svm/sev.c                        | 31 +++++++++++++++++++
>>   2 files changed, 41 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 1e5e76bba9da..2cddb2b5c59d 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -2891,6 +2891,16 @@
>>               (enabled). Disable by KVM if hardware lacks support
>>               for NPT.
>>   +    kvm-amd.ciphertext_hiding_nr_asids=
>> +            [KVM,AMD] Enables SEV-SNP CipherTextHiding feature and
>> +            controls show many ASIDs are available for SEV-SNP guests.
>> +            The ASID space is basically split into legacy SEV and
>> +            SEV-ES+. CipherTextHiding feature further splits the
>> +            SEV-ES+ ASID space into SEV-ES and SEV-SNP.
>> +            If the value is -1, then it is used as an auto flag
>> +            and splits the ASID space equally between SEV-ES and
>> +            SEV-SNP ASIDs.
> 
> 
> Why in halves? 0 or max would make sense and I'd think the user wants all SEV-ES+ VMs be hidden by default so I'd name the parameter as no_hiding_nr_asids and make the default value of zero mean "every SEV-ES+ is hidden".
>

Actually 'max' sounds nice, but we still want to name it ciphertext_hiding_asids or something and keep the value relative to it, so probably it can be 
a number or 'max'.

Thanks,
Ashish
 
> Or there is a downside of hiding all VMs?
> 
> 
>> +
>>       kvm-arm.mode=
>>               [KVM,ARM,EARLY] Select one of KVM/arm64's modes of
>>               operation.
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 383db1da8699..68dcb13d98f2 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -59,6 +59,10 @@ 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 int ciphertext_hiding_nr_asids;
>> +module_param(ciphertext_hiding_nr_asids, int, 0444);
>> +MODULE_PARM_DESC(max_snp_asid, "  Number of ASIDs available for SEV-SNP guests when CipherTextHiding is enabled");
>> +
>>   #define AP_RESET_HOLD_NONE        0
>>   #define AP_RESET_HOLD_NAE_EVENT        1
>>   #define AP_RESET_HOLD_MSR_PROTO        2
>> @@ -200,6 +204,9 @@ static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
>>       /*
>>        * 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.
>> +     * Similarly for SEV-ES guests the min ASID can end up larger than the
>> +     * max when CipherTextHiding is enabled, effectively disabling SEV-ES
>> +     * support.
>>        */
>>         if (min_asid > max_asid)
>> @@ -2955,6 +2962,7 @@ void __init sev_hardware_setup(void)
>>   {
>>       unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
>>       struct sev_platform_init_args init_args = {0};
>> +    bool snp_cipher_text_hiding = false;
>>       bool sev_snp_supported = false;
>>       bool sev_es_supported = false;
>>       bool sev_supported = false;
>> @@ -3052,6 +3060,27 @@ void __init sev_hardware_setup(void)
>>       if (min_sev_asid == 1)
>>           goto out;
>>   +    /*
>> +     * The ASID space is basically split into legacy SEV and SEV-ES+.
>> +     * CipherTextHiding feature further partitions the SEV-ES+ ASID space
>> +     * into ASIDs for SEV-ES and SEV-SNP guests.
>> +     */
>> +    if (ciphertext_hiding_nr_asids && sev_is_snp_ciphertext_hiding_supported()) {
>> +        /* Do sanity checks on user-defined ciphertext_hiding_nr_asids */
>> +        if (ciphertext_hiding_nr_asids != -1 &&
>> +            ciphertext_hiding_nr_asids >= min_sev_asid) {
>> +            pr_info("ciphertext_hiding_nr_asids module parameter invalid, limiting SEV-SNP ASIDs to %d\n",
>> +                 min_sev_asid);
>> +            ciphertext_hiding_nr_asids = min_sev_asid - 1;
>> +        }
>> +
>> +        min_sev_es_asid = ciphertext_hiding_nr_asids == -1 ? (min_sev_asid - 1) / 2 :
>> +                  ciphertext_hiding_nr_asids + 1;
>> +        max_snp_asid = min_sev_es_asid - 1;
>> +        snp_cipher_text_hiding = true;
>> +        pr_info("SEV-SNP CipherTextHiding feature support enabled\n");
> 
> 
> Can do "init_args.snp_max_snp_asid = max_snp_asid;" here (as max_snp_asid seems to not change between here and next hunk) and drop snp_cipher_text_hiding. Thanks,
> 
>> +    }
>> +
>>       sev_es_asid_count = min_sev_asid - 1;
>>       WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
>>       sev_es_supported = true;
>> @@ -3092,6 +3121,8 @@ void __init sev_hardware_setup(void)
>>        * Do both SNP and SEV initialization at KVM module load.
>>        */
>>       init_args.probe = true;
>> +    if (snp_cipher_text_hiding)
>> +        init_args.snp_max_snp_asid = max_snp_asid;
>>       sev_platform_init(&init_args);
>>   }
>>   
>