Hello Tom,
On 6/3/2025 11:26 AM, Tom Lendacky wrote:
> On 5/19/25 19:02, 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).
>>
>> The SEV ASID space is basically split into legacy SEV and SEV-ES+.
>> CipherTextHiding further partitions the SEV-ES+ ASID space into SEV-ES
>
> s/CipherTextHiding/Ciphertext hiding/
>
>> and SEV-SNP.
>>
>> Add new module parameter to the KVM module to enable CipherTextHiding
>
> Ditto.
Ok.
>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>> ---
>> .../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=
>
> s/ns_asids/asids/
>
> I'm not sure that the "nr" adds anything here.
>
Ok.
>> + [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.
>> +
>
> Ditto on Dave's comments.
>
Ok.
>> 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.
>
> I think it is already understood that the ASID space is split between SEV
> and SEV-ES/SEV-SNP guests. So something like this maybe?
>
> The ciphertext hiding feature partions the joint SEV-ES/SEV-SNP ASID range
> into separate SEV-ES and SEV-SNP ASID ranges with teh SEV-SNP ASID range
> starting at 1.
>
Yes that sounds better.
>> + */
>> + 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;
>
> So specifying a number greater than min_sev_asid will result in enabling
> ciphertext hiding and no SEV-ES guests allowed even though you report that
> the number is invalid?
>
Well, the user specified a non-zero ciphertext_hiding_nr_asids, so the intent is to enable ciphertext hiding and therefore
sanitize the user specified parameter and enable ciphertext hiding.
> I think the message can be worded better to convey what happens.
>
> "Requested ciphertext hiding ASIDs (%u) exceeds minimum SEV ASID (%u), setting ciphertext hiding ASID range to the maximum value (%u)\n"
>
> Or something a little more concise.
>
Ok.
>> + }
>> +
>> + min_sev_es_asid = ciphertext_hiding_nr_asids == -1 ? (min_sev_asid - 1) / 2 :
>
> Should this be (min_sev_asid - 1) / 2 + 1 ?
>
> Take min_sev_asid = 3, that means min_sev_es_asid would be 2 and
> max_snp_asid would be 1, right?
Yes.
>
> And if you set min_sev_es_asid before first you favor SEV-ES.
>
Ok.
> So should you do:
>
> max_snp_asid = ciphertext_hiding_asids != -1 ? : (min_sev_asid - 1) / 2 + 1;
> min_sev_es_asid = max_snp_asid + 1;
>
Considering the above example again, this will still be incorrect as with above change:
Taking, min_sev_asid == 3, max_snp_asid = 2 and min_sev_asid = 3.
So i believe it should be :
max_snp_asid = ciphertext_hiding_asids != -1 ? : (min_sev_asid - 1) / 2;
min_sev_es_asid = max_snp_asid + 1;
>
>> + 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");
>
> "SEV-SNP ciphertext hiding enabled\n"
>
> No need to use the CipherTextHiding nomenclature everywhere.
>
Ok.
Thanks,
Ashish
> Thanks,
> Tom
>
>> + }
>> +
>> 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);
>> }
>>
