Message-ID: <76ac003b-2502-459a-bf94-55f7b15e2e1b@amd.com>
Date: Mon, 18 Aug 2025 18:58:08 -0500
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Kim Phillips <kim.phillips@....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/2025 6:23 PM, Kim Phillips wrote:
> 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.
> 

All the above are your opinions and i don't agree with them and my patch is bug-free and simple and easy to understand and works perfectly, and i am not making any more changes to it.

Thanks,
Ashish

>> 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