Message-ID: <a16f1420-fe20-4c3c-9b75-806b1da22336@amd.com>
Date: Mon, 18 Aug 2025 18:23:53 -0500
From: Kim Phillips <kim.phillips@....com>
To: "Kalra, Ashish" <ashish.kalra@....com>, Herbert Xu
	<herbert@...dor.apana.org.au>
CC: <Neeraj.Upadhyay@....com>, <aik@....com>, <akpm@...ux-foundation.org>,
	<ardb@...nel.org>, <arnd@...db.de>, <bp@...en8.de>, <corbet@....net>,
	<dave.hansen@...ux.intel.com>, <davem@...emloft.net>, <hpa@...or.com>,
	<john.allen@....com>, <kvm@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<michael.roth@....com>, <mingo@...hat.com>, <nikunj@....com>,
	<paulmck@...nel.org>, <pbonzini@...hat.com>, <rostedt@...dmis.org>,
	<seanjc@...gle.com>, <tglx@...utronix.de>, <thomas.lendacky@....com>,
	<x86@...nel.org>
Subject: Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

On 8/18/25 3:39 PM, Kalra, Ashish wrote:
> On 8/18/2025 2:38 PM, Kim Phillips wrote:
>> On 8/18/25 2:16 PM, Kalra, Ashish wrote:
>>> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>>>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>>>> Hi Herbert, can you please merge patches 1-5.
>>>>>
>>>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>>>> for patches 6 & 7.
>>>> These patches will be at the base of the cryptodev tree for 6.17
>>>> so it could be pulled into another tree without any risks.
>>>>
>>>> Cheers,
>>> Thanks Herbert for pulling in patches 1-5.
>>>
>>> Paolo, can you please merge patches 6 and 7 into the KVM tree.
>> Hi Ashish,
>>
>> I have pending comments on patch 7:
>>
>> https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/
>>
>> If still not welcome, can you say why you think:
>>
>> 1. The ciphertext_hiding_asid_nr variable is necessary
> I prefer safe coding, and i don't want to update max_snp_asid, until i am sure there are no sanity
> check failures and that's why i prefer using a *temp* variable and then updating max_snp_asid when i
> am sure all sanity checks have been done.
>
> Otherwise, in your case you are updating max_snp_asid and then rolling it back in case of sanity check
> failures, i don't like that.

Item (1):

The rollback is in a single place, and the extra variable's existence 
can be avoided, or, at least have 'temp' somewhere in its name.

FWIW, any "Safe coding" practices should have been performed prior to 
the submission of the patch, IMO.

>> 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check  redundant?
> isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid
> calling kstrtoint() if the parameter is not a number.

Item (2):

This is module initialization code, it's better optimized for 
readability than for performance.  As a reader of the code, I'm 
constantly wondering why the redundancy exists, and am sure it is made 
objectively easier to read if the isdigit() check were removed.

>> 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed.
> This is for understandable code flow :
>
> 1). Check module parameter is set by user.
> 2). Check ciphertext_hiding_feature enabled.
> 3). Check if parameter is numeric.
>      Sanity checks on numeric parameter
>      If checks fail goto invalid_parameter
> 4). Check if parameter is the string "max".
> 5). Set max_snp_asid and min_sev_es_asid.
> 6). Fall-through to invalid parameter.
> invalid_parameter:
>
> This is overall a more understandable code flow.

Item (3):

That's not how your original v7 flows, but I do now see the non-obvious 
fall-through from the else if (...'max'...).  I believe I am not alone 
in missing that, and that a comment would have helped. Also, the 'else' 
isn't required

Flow readability-wise, comparing the two, after the two common if()s, 
your original v7 goes:

{
...
     if (isdigit() {
         if (kstrtoint())
             goto invalid_parameter
         if (temporary variable >= min_sev_asid) {
             pr_warn()
             return false;
     } else if (..."max"...) {
         temporary variable = ...
         /* non-obvious fallthrough to invalid_parameter iff 
temporary_variable == 0 */
     }

     if (temporary variable) {
         max_snp_asid = ...
         min_sev_es_asid = ...
         pr_info(..."enabled"...)
         return true;
     }

invalid_parameter:
     pr_warn()
     return false;
}

vs the result of my latest diff:

{
...
     if (..."max"...) {
         max_snp_asid =
         min_sev_es_asid = ...
         return true;
     }

     if (kstrtoint()) {
         pr_warn()
         return false
     }

     if (max_snp_asid < 1 || >= min_sev_asid) {
         pr_warn()
         max_snp_asid = /* rollback */
         return false;
     }

     min_sev_es_asid = ...

     return true;
}

So, just from an outright flow perspective, I believe my latest diff is 
objectively easier to follow.

> Again, this is just a module parameter checking function and not something which will affect runtime performance by eliminating a single temporary variable or jump label.
With this statement, you self-contradict your rationale to keep your 
version of the above to Item (2): "isdigit() is a MACRO compared to 
kstrtoint() call, it is more optimal to do an inline check and avoid 
calling kstrtoint() if the parameter is not a number". If not willing to 
take my latest diff as-is, I really would like to see:

Item (1)'s variable get a temporary-sounding name,
item (2)'s the isdigit() check (and thus a whole indentation level) 
removed, and
item (3)'s flow reconsidered given the (IMO objective) readability 
enhancement.

Thanks,

Kim

Powered by blists - more mailing lists