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@xxxxxxx/ >>> >>> 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 >
