Re: [PATCH v5 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

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

Hello Kim,

On 7/2/2025 4:46 PM, Kim Phillips wrote:
> Hi Ashish,
> 
> I can confirm that this v5 series fixes v4's __sev_do_cmd_locked
> assertion failure problem, thanks.  More comments inline:
> 
> On 7/1/25 3:16 PM, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Extra From: line not necessary.
> 
>> @@ -2913,10 +2921,46 @@ static bool is_sev_snp_initialized(void)
>>       return initialized;
>>   }
>>   +static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>> +{
>> +    unsigned int ciphertext_hiding_asid_nr = 0;
>> +
>> +    if (!sev_is_snp_ciphertext_hiding_supported()) {
>> +        pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported or enabled\n");
>> +        return false;
>> +    }
>> +
>> +    if (isdigit(ciphertext_hiding_asids[0])) {
>> +        if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) {
>> +            pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>> +                ciphertext_hiding_asids);
>> +            return false;
>> +        }
>> +        /* Do sanity checks on user-defined ciphertext_hiding_asids */
>> +        if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>> +            pr_warn("Requested ciphertext hiding ASIDs (%u) exceeds or equals minimum SEV ASID (%u)\n",
>> +                ciphertext_hiding_asid_nr, min_sev_asid);
>> +            return false;
>> +        }
>> +    } else if (!strcmp(ciphertext_hiding_asids, "max")) {
>> +        ciphertext_hiding_asid_nr = min_sev_asid - 1;
>> +    } else {
>> +        pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>> +            ciphertext_hiding_asids);
>> +        return false;
>> +    }
> 
> This code can be made much simpler if all the invalid
> cases were combined to emit a single pr_warn().
> 

There definitely has to be a different pr_warn() for the sanity check case and invalid parameter cases and sanity check has to be done if the
specified parameter is an unsigned int, so the check needs to be done separately.

I can definitely add a branch just for the invalid cases.

>> @@ -3036,7 +3090,9 @@ void __init sev_hardware_setup(void)
>>               min_sev_asid, max_sev_asid);
>>       if (boot_cpu_has(X86_FEATURE_SEV_ES))
>>           pr_info("SEV-ES %s (ASIDs %u - %u)\n",
>> -            str_enabled_disabled(sev_es_supported),
>> +            sev_es_supported ? min_sev_es_asid < min_sev_asid ? "enabled" :
>> +                                        "unusable" :
>> +                                        "disabled",
>>               min_sev_es_asid, max_sev_es_asid);
>>       if (boot_cpu_has(X86_FEATURE_SEV_SNP))
>>           pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
> 
> If I set ciphertext_hiding_asids=99, I get the new 'unusable':
> 
> kvm_amd: SEV-SNP ciphertext hiding enabled
> ...
> kvm_amd: SEV enabled (ASIDs 100 - 1006)
> kvm_amd: SEV-ES unusable (ASIDs 100 - 99)
> kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
> 
> Ok.

Which is correct. 

This is similar to the SEV case where min_sev_asid can be greater than max_sev_asid and that also emits similarly : 
SEV unusable (ASIDs 1007 - 1006) (this is an example of that case).

> 
> Now, if I set ciphertext_hiding_asids=0, I get:
> 
> kvm_amd: SEV-SNP ciphertext hiding enabled
> ...
> kvm_amd: SEV enabled (ASIDs 100 - 1006)
> kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
> kvm_amd: SEV-SNP enabled (ASIDs 1 - 0)
> 
> ..where SNP is unusable this time, yet it's not flagged as such.
> 

Actually SNP still needs to be usable/enabled in this case, as specifying ciphertext_hiding_asids=0 is same as specifying that ciphertext hiding feature should
not be enabled, so code-wise this is behaving correctly, but messaging needs to be fixed, which i will fix.

Thanks,
Ashish

> If there's no difference between "unusable" and not enabled, then
> I think it's better to keep the not enabled messaging behaviour
> and just not emit the line at all:  It's confusing to see the
> invalid "100 - 99" and "1 - 0" ranges.
> 
> Thanks,
> 
> Kim