Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <5c19c422-41ad-430b-664c-15f3e2087922@redhat.com>
Date:   Tue, 10 Oct 2023 13:40:54 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] kvm/sev: make SEV/SEV-ES asids configurable
To:     Greg KH <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Jos=c3=a9_Pekkarinen?= <jose.pekkarinen@foxhound.fi>
Cc:     seanjc@google.com, skhan@linuxfoundation.org,
        dave.hansen@linux.intel.com, kvm@vger.kernel.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, mingo@redhat.com, bp@alien8.de,
        hpa@zytor.com, tglx@linutronix.de,
        linux-kernel-mentees@lists.linuxfoundation.org
References: <20231010100441.30950-1-jose.pekkarinen@foxhound.fi>
 <2023101050-scuff-overstay-9b43@gregkh>
Content-Language: en-US
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <2023101050-scuff-overstay-9b43@gregkh>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 10/10/23 13:35, Greg KH wrote:
> On Tue, Oct 10, 2023 at 01:04:39PM +0300, José Pekkarinen wrote:
>> There are bioses that doesn't allow to configure the
>> number of asids allocated for SEV/SEV-ES, for those
>> cases, the default behaviour allocates all the asids
>> for SEV, leaving no room for SEV-ES to have some fun.

In addition to what Greg pointed out (and there are many more cases that 
have to be checked for errors, including possible overflows), why is it 
correct to just ignore what's in CPUID?

Paolo

> "fun"?
> 
> Also, please use the full 72 columns for your changelog.
> 
>> If the user request SEV-ES to be enabled, it will
>> find the kernel just run out of resources and ignored
>> user request. This following patch will address this
>> issue by making the number of asids for SEV/SEV-ES
>> configurable over kernel module parameters.
>>
>> Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
>> ---
>>   arch/x86/kvm/svm/sev.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 07756b7348ae..68a63b42d16a 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -51,9 +51,18 @@
>>   static bool sev_enabled = true;
>>   module_param_named(sev, sev_enabled, bool, 0444);
>>   
>> +/* nr of asids requested for SEV */
>> +static unsigned int requested_sev_asids;
>> +module_param_named(sev_asids, requested_sev_asids, uint, 0444);
>> +
>>   /* enable/disable SEV-ES support */
>>   static bool sev_es_enabled = true;
>>   module_param_named(sev_es, sev_es_enabled, bool, 0444);
>> +
>> +/* nr of asids requested for SEV-ES */
>> +static unsigned int requested_sev_es_asids;
>> +module_param_named(sev_es_asids, requested_sev_asids, uint, 0444);
> 
> Why more module parameters?  Why can't this "just work" properly without
> forcing a user to make manual changes?  This isn't the 1990's anymore.
> 
> 
>> +
>>   #else
>>   #define sev_enabled false
>>   #define sev_es_enabled false
>> @@ -2194,6 +2203,11 @@ void __init sev_hardware_setup(void)
>>   	if (!max_sev_asid)
>>   		goto out;
>>   
>> +	if (requested_sev_asids + requested_sev_es_asids > max_sev_asid) {
>> +		pr_info("SEV asids requested more than available: %u ASIDs\n", max_sev_asid);
> 
> Why isn't this an error?
> 
> thanks,
> 
> greg k-h
>