Message-ID: <96022875-5a6f-4192-b1eb-40f389b4859f@amd.com>
Date: Tue, 12 Aug 2025 13:40:13 -0500
From: Kim Phillips <kim.phillips@....com>
To: "Kalra, Ashish" <ashish.kalra@....com>, Tom Lendacky
	<thomas.lendacky@....com>, <corbet@....net>, <seanjc@...gle.com>,
	<pbonzini@...hat.com>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
	<hpa@...or.com>, <john.allen@....com>, <herbert@...dor.apana.org.au>,
	<davem@...emloft.net>, <akpm@...ux-foundation.org>, <rostedt@...dmis.org>,
	<paulmck@...nel.org>
CC: <nikunj@....com>, <Neeraj.Upadhyay@....com>, <aik@....com>,
	<ardb@...nel.org>, <michael.roth@....com>, <arnd@...db.de>,
	<linux-doc@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>
Subject: Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

On 8/12/25 1:29 PM, Kalra, Ashish wrote:
>
> On 8/12/2025 11:45 AM, Kim Phillips wrote:
>> On 8/12/25 9:40 AM, Kalra, Ashish wrote:
>>> On 8/12/2025 7:06 AM, Kim Phillips wrote:
>>>>    arch/x86/kvm/svm/sev.c | 47 ++++++++++++++++++-----------------------------
>>>>    1 file changed, 18 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>> index 7ac0f0f25e68..57c6e4717e51 100644
>>>> --- a/arch/x86/kvm/svm/sev.c
>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>> @@ -2970,42 +2970,29 @@ static bool is_sev_snp_initialized(void)
>>>>
>>>>    static bool check_and_enable_sev_snp_ciphertext_hiding(void)
>>>>    {
>>>> -       unsigned int ciphertext_hiding_asid_nr = 0;
>>>> -
>>>> -       if (!ciphertext_hiding_asids[0])
>>>> -               return false;
>>>> -
>>>> -       if (!sev_is_snp_ciphertext_hiding_supported()) {
>>>> +       if (ciphertext_hiding_asids[0] && !sev_is_snp_ciphertext_hiding_supported()) {
>>>>                   pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
>>>>                   return false;
>>>>           }
>>>>
>>> This is incorrect, if ciphertext_hiding_asids module parameter is never specified, user will always
>>> get a warning of an invalid ciphertext_hiding_asids module parameter.
>>>
>>> When this module parameter is optional why should the user get a warning about an invalid module parameter.
>> Ack, sorry, new diff below that fixes this.
>>
>>> Again, why do we want to do all these checks below if this module parameter has not been specified by
>>> the user ?
>> Not sure what you mean by 'below' here (assuming in the resulting code), but, in general, there are less checks with this diff than the original v7 code.
>>
>>>> -       if (isdigit(ciphertext_hiding_asids[0])) {
>>>> -               if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
>>>> -                       goto invalid_parameter;
>>>> -
>>>> -               /* Do sanity check on user-defined ciphertext_hiding_asids */
>>>> -               if (ciphertext_hiding_asid_nr >= min_sev_asid) {
>>>> -                       pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
>>>> -                               ciphertext_hiding_asid_nr, min_sev_asid);
>>> A *combined* error message such as this:
>>> "invalid ciphertext_hiding_asids XXX or !(0 < XXX < minimum SEV ASID 100)"
>>>
>>> is going to be really confusing to the user.
>>>
>>> It is much simpler for user to understand if the error/warning is:
>>> "Module parameter ciphertext_hiding_asids XXX exceeds or equals minimum SEV ASID YYY"
>>> OR
>>> "Module parameter ciphertext_hiding_asids XXX invalid"
>> I tend to disagree. If, e.g., the user sets ciphertext_hiding_asids=100, they see:
>>
>>       kvm_amd: invalid ciphertext_hiding_asids "100" or !(0 < 100 < minimum SEV ASID 100)
>>
>> which the user can easily unmistakably and quickly deduce that the problem is the latter - not the former - condition that has the problem.
>>
>> The original v7 code in that same case would emit:
>>
>> kvm_amd: Module parameter ciphertext_hiding_asids (100) exceeds or equals minimum SEV ASID (100)
>>
>> ...to which the user would ask themselves "What's wrong with equalling the minimum SEV ASID (100)"?
> I disagree, the documentation mentions clearly that:
> For SEV-ES/SEV-SNP guests the maximum ASID available is MIN_SEV_ASID - 1.
>
> Which the above message conveys quite clearly.

The point of clear error messaging is to avoid the user having to 
(re-)read the documentation.

>
>> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).
>> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
>>
>>       kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>
>> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.
> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!

Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.

> And how can user input of 0x1, result in max_snp_asid == 99 ?

It doesn't, again, the 0x is the invalid part.

> This is the issue with combining the checks and emitting a combined error message:
>
> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information :
> !(0 < 99 < minimum SEV ASID 100)

It's not, it says it's *OR* that condition.

> The original message is much simpler to understand and correct too:
> Module parameter ciphertext_hiding_asids (-1) invalid

Which is wildly different from any possible derivation of 0x1.

>> But all this is nothing compared to the added simplicity resulting from making the change to the original v7 code.
> I disagree, combining checks and emitting a combined error message is going to be more confusing to the user as the above case of (ciphertext_hiding_asids=0x1) shows.

I don't, but nevertheless, it can still be differentiated and still be 
cleaner code than the original v7...

> Thanks,
> Ashish

Thanks,

Kim

Powered by blists - more mailing lists