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. > > 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. > > 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. 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. Thanks, Ashish > > Thanks, > > Kim
