Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

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

On 8/12/2025 11:45 AM, Kim Phillips wrote:
> On 8/12/25 9:40 AM, Kalra, Ashish wrote:
>> On 8/12/2025 7:06 AM, Kim Phillips wrote:
>>>   arch/x86/kvm/svm/sev.c | 47 ++++++++++++++++++-----------------------------
>>>   1 file changed, 18 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 7ac0f0f25e68..57c6e4717e51 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -2970,42 +2970,29 @@ static bool is_sev_snp_initialized(void)
>>>
>>>   static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>>>   {
>>> -       unsigned int ciphertext_hiding_asid_nr = 0;
>>> -
>>> -       if (!ciphertext_hiding_asids[0])
>>> -               return false;
>>> -
>>> -       if (!sev_is_snp_ciphertext_hiding_supported()) {
>>> +       if (ciphertext_hiding_asids[0] && !sev_is_snp_ciphertext_hiding_supported()) {
>>>                  pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
>>>                  return false;
>>>          }
>>>
>> This is incorrect, if ciphertext_hiding_asids module parameter is never specified, user will always
>> get a warning of an invalid ciphertext_hiding_asids module parameter.
>>
>> When this module parameter is optional why should the user get a warning about an invalid module parameter.
> 
> Ack, sorry, new diff below that fixes this.
> 
>> Again, why do we want to do all these checks below if this module parameter has not been specified by
>> the user ?
> 
> Not sure what you mean by 'below' here (assuming in the resulting code), but, in general, there are less checks with this diff than the original v7 code.
> 
>>> -       if (isdigit(ciphertext_hiding_asids[0])) {
>>> -               if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
>>> -                       goto invalid_parameter;
>>> -
>>> -               /* Do sanity check on user-defined ciphertext_hiding_asids */
>>> -               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>>> -                       pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
>>> -                               ciphertext_hiding_asid_nr, min_sev_asid);
>> A *combined* error message such as this:
>> "invalid ciphertext_hiding_asids XXX or !(0 < XXX < minimum SEV ASID 100)"
>>
>> is going to be really confusing to the user.
>>
>> It is much simpler for user to understand if the error/warning is:
>> "Module parameter ciphertext_hiding_asids XXX exceeds or equals minimum SEV ASID YYY"
>> OR
>> "Module parameter ciphertext_hiding_asids XXX invalid"
> 
> I tend to disagree. If, e.g., the user sets ciphertext_hiding_asids=100, they see:
> 
>      kvm_amd: invalid ciphertext_hiding_asids "100" or !(0 < 100 < minimum SEV ASID 100)
> 
> which the user can easily unmistakably and quickly deduce that the problem is the latter - not the former - condition that has the problem.
> 
> The original v7 code in that same case would emit:
> 
> kvm_amd: Module parameter ciphertext_hiding_asids (100) exceeds or equals minimum SEV ASID (100)
> 
> ...to which the user would ask themselves "What's wrong with equalling the minimum SEV ASID (100)"?

I disagree, the documentation mentions clearly that: 
For SEV-ES/SEV-SNP guests the maximum ASID available is MIN_SEV_ASID - 1.

Which the above message conveys quite clearly.

> 
> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).

> 
> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
> 
>      kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
> 
> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.

This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!

And how can user input of 0x1, result in max_snp_asid == 99 ?

This is the issue with combining the checks and emitting a combined error message:

Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information : 
!(0 < 99 < minimum SEV ASID 100)

The original message is much simpler to understand and correct too: 
Module parameter ciphertext_hiding_asids (-1) invalid

> 
> But all this is nothing compared to the added simplicity resulting from making the change to the original v7 code.

I disagree, combining checks and emitting a combined error message is going to be more confusing to the user as the above case of (ciphertext_hiding_asids=0x1) shows.

Thanks,
Ashish

> 
> New diff from original v7 below:
> 
>  arch/x86/kvm/svm/sev.c | 42 +++++++++++++++++-------------------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7ac0f0f25e68..a879ea5f53f2 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2970,8 +2970,6 @@ static bool is_sev_snp_initialized(void)
> 
>  static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>  {
> -       unsigned int ciphertext_hiding_asid_nr = 0;
> -
>         if (!ciphertext_hiding_asids[0])
>                 return false;
> 
> @@ -2980,32 +2978,24 @@ static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>                 return false;
>         }
> 
> -       if (isdigit(ciphertext_hiding_asids[0])) {
> -               if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
> -                       goto invalid_parameter;
> -
> -               /* Do sanity check on user-defined ciphertext_hiding_asids */
> -               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> -                       pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
> -                               ciphertext_hiding_asid_nr, min_sev_asid);
> -                       return false;
> -               }
> -       } else if (!strcmp(ciphertext_hiding_asids, "max")) {
> -               ciphertext_hiding_asid_nr = min_sev_asid - 1;
> -       }
> -
> -       if (ciphertext_hiding_asid_nr) {
> -               max_snp_asid = ciphertext_hiding_asid_nr;
> +       if (!strcmp(ciphertext_hiding_asids, "max")) {
> +               max_snp_asid = min_sev_asid - 1;
>                 min_sev_es_asid = max_snp_asid + 1;
> -               pr_info("SEV-SNP ciphertext hiding enabled\n");
> -
>                 return true;
>         }
> 
> -invalid_parameter:
> -       pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> -               ciphertext_hiding_asids);
> -       return false;
> +       /* Do sanity check on user-defined ciphertext_hiding_asids */
> +       if (kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid) ||
> +           !max_snp_asid || max_snp_asid >= min_sev_asid) {
> +               pr_warn("invalid ciphertext_hiding_asids \"%s\" or !(0 < %u < minimum SEV ASID %u)\n",
> +                       ciphertext_hiding_asids, max_snp_asid, min_sev_asid);
> +               max_snp_asid = min_sev_asid - 1;
> +               return false;
> +       }
> +
> +       min_sev_es_asid = max_snp_asid + 1;
> +
> +       return true;
>  }
> 
>  void __init sev_hardware_setup(void)
> @@ -3122,8 +3112,10 @@ void __init sev_hardware_setup(void)
>                  * ASID range into separate SEV-ES and SEV-SNP ASID ranges with
>                  * the SEV-SNP ASID starting at 1.
>                  */
> -               if (check_and_enable_sev_snp_ciphertext_hiding())
> +               if (check_and_enable_sev_snp_ciphertext_hiding()) {
> +                       pr_info("SEV-SNP ciphertext hiding enabled\n");
>                         init_args.max_snp_asid = max_snp_asid;
> +               }
>                 if (sev_platform_init(&init_args))
>                         sev_supported = sev_es_supported = sev_snp_supported = false;
>                 else if (sev_snp_supported)
> 
> Thanks,
> 
> Kim
>