On 8/12/25 6:30 PM, Kim Phillips wrote:

On 8/12/25 2:38 PM, Kalra, Ashish wrote:

quoted

On 8/12/2025 2:11 PM, Kim Phillips wrote:

quoted

On 8/12/25 1:52 PM, Kalra, Ashish wrote:

quoted

On 8/12/2025 1:40 PM, Kim Phillips wrote:

quoted

quoted

quoted

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!

Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.

quoted

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

It doesn't, again, the 0x is the invalid part.

quoted

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)

It's not, it says it's *OR* that condition.

To me this is wrong as
!(0 < 99 < minimum SEV ASID 100) is simply not a correct statement!

The diff I provided emits exactly this:

kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum 
SEV ASID 100)


which means *EITHER*:

invalid ciphertext_hiding_asids "0x1"

*OR*

!(0 < 99 < minimum SEV ASID 100)

but since the latter is 'true', the user is pointed to the former
"0x1" as being the interpretation problem.

Would adding the word "Either" help?:

kvm_amd: Either invalid ciphertext_hiding_asids "0x1", or !(0 < 99 < 
minimum SEV ASID 100)

?

No, i simply won't put an invalid expression out there:

!(0 < 99 < minimum SEV ASID 100)

When not quoted out of context, it's not an invalid expression (in the 
99 case) because it's preceded with the word "or:"

..., or !(0 < 99 < minimum SEV ASID 100)

quoted

quoted

If not, feel free to separate them: the code is still much cleaner.

Separating the checks will make the code not very different from the 
original function, so i am going to keep the original code.

Take a look at the example diff below, then.  It's still less, simpler 
code because it eliminates:

1. the unnecessary ciphertext_hiding_asid_nr variable

2. the redundant isdigit(ciphertext_hiding_asids[0])) check
and 3. the 'invalid_parameter:' label referenced by only one goto 
statement. 

Re-posting, since I believe the previous email's diff got mangled:

  arch/x86/kvm/svm/sev.c | 44 ++++++++++++++++++++------------------------
  1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7ac0f0f25e68..1b9702500c73 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,28 @@ 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 (!strcmp(ciphertext_hiding_asids, "max")) {
+               max_snp_asid = min_sev_asid - 1;
+               min_sev_es_asid = max_snp_asid + 1;
+               return true;
         }

-       if (ciphertext_hiding_asid_nr) {
-               max_snp_asid = ciphertext_hiding_asid_nr;
-               min_sev_es_asid = max_snp_asid + 1;
-               pr_info("SEV-SNP ciphertext hiding enabled\n");
+       if (kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid)) {
+               pr_warn("ciphertext_hiding_asids \"%s\" is not an 
integer or 'max'\n", ciphertext_hiding_asids);
+               return false;
+       }

-               return true;
+       /* Do sanity check on user-defined ciphertext_hiding_asids */
+       if (max_snp_asid < 1 || max_snp_asid >= min_sev_asid) {
+               pr_warn("!(0 < ciphertext_hiding_asids %u < minimum SEV 
ASID %u)\n",
+                       max_snp_asid, min_sev_asid);
+               max_snp_asid = min_sev_asid - 1;
+               return false;
         }

-invalid_parameter:
-       pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
-               ciphertext_hiding_asids);
-       return false;
+       min_sev_es_asid = max_snp_asid + 1;
+
+       return true;
  }

  void __init sev_hardware_setup(void)

@@ -3122,8 +3116,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)