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

On 8/18/2025 2:38 PM, Kim Phillips wrote:

On 8/18/25 2:16 PM, Kalra, Ashish wrote:

quoted

On 8/16/2025 3:39 AM, Herbert Xu wrote:

quoted

On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:

quoted

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/ (local)

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