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

[Kim Phillips <kim.phillips@amd.com>]

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)"?

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.

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

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