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

[Kim Phillips <kim.phillips@amd.com>]

On 7/7/25 1:16 AM, Kalra, Ashish wrote:
> 
> On 7/2/2025 5:43 PM, Kalra, Ashish wrote:
>> 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).
>>
> 
> Also do note that the message above is printing the exact values of min_sev_es_asid and max_sev_es_asid, as they have been computed.
> 
> And it adds that SEV-ES is now unusable as now min_sev_es_asid > max_sev_es_asid.

Right, it'd be nice if that were made clearer to the user, too:

min_sev_es_asid 100 > max_sev_es_asid 99

>>> 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.
>>
> 
> And i do need to fix this case for ciphertext_hiding_asids==0, i.e., ciphertext hiding feature is not enabled, as the above is not functioning correctly.

Right, it's not just messaging, SNP should still be enabled when ciphertext_hiding_asids==0,
whereas this is not the case with this patchset.


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

Please also consider this.

Thanks,

Kim