Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

["Kalra, Ashish" <ashish.kalra@amd.com>]

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. 

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