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.
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 <[email protected]>
---
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);
+
#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);
+ goto out;
+ }
+
/* Minimum ASID value that should be used for SEV guest */
min_sev_asid = edx;
sev_me_mask = 1UL << (ebx & 0x3f);
@@ -2215,7 +2229,8 @@ void __init sev_hardware_setup(void)
goto out;
}
- sev_asid_count = max_sev_asid - min_sev_asid + 1;
+ sev_asid_count = (requested_sev_asids) ? max_sev_asid - min_sev_asid + 1 :
+ requested_sev_asids;
WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count));
sev_supported = true;
@@ -2237,10 +2252,11 @@ void __init sev_hardware_setup(void)
goto out;
/* Has the system been allocated ASIDs for SEV-ES? */
- if (min_sev_asid == 1)
+ if (max_sev_asid - sev_asid_count <= 1)
goto out;
- sev_es_asid_count = min_sev_asid - 1;
+ sev_es_asid_count = (requested_sev_es_asids) ? min_sev_asid - 1 :
+ requested_sev_es_asids;
WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
sev_es_supported = true;
@@ -2248,11 +2264,13 @@ void __init sev_hardware_setup(void)
if (boot_cpu_has(X86_FEATURE_SEV))
pr_info("SEV %s (ASIDs %u - %u)\n",
sev_supported ? "enabled" : "disabled",
- min_sev_asid, max_sev_asid);
+ min_sev_asid, sev_asid_count);
if (boot_cpu_has(X86_FEATURE_SEV_ES))
pr_info("SEV-ES %s (ASIDs %u - %u)\n",
sev_es_supported ? "enabled" : "disabled",
- min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
+ sev_asid_count,
+ (requested_sev_es_asids) ? 0 :
+ sev_asid_count + sev_es_asid_count);
sev_enabled = sev_supported;
sev_es_enabled = sev_es_supported;
--
2.41.0
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.
"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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
>
On 2023-10-10 14: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.
>
> "fun"?
>
> Also, please use the full 72 columns for your changelog.
>
Alright.
>> 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 <[email protected]>
>> ---
>> 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.
I could think of setting both cgroup caps to the maximum
number of asids and then check the code in the module to make
sure anytime a sev/sev_es asid is reserved both cgroups get
updated to reflect the remaining asids, or even better, just
use only one cgroup to keep track of them. That way the parameters
become redundant, would any of these ideas work for you? Do you,
or anybody else, have better ideas or preferences in this topic?
>> +
>> #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?
Good question, I'll address it in v2.
Thanks!
José.
On 2023-10-10 14:40, Paolo Bonzini wrote:
> 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?
I'll check if I can use cpuid info to improve this
for v2, I just noticed my cpu advertises sev_es but
the kernel doesn't activate it despite of the kvm_amd
parameter being set, so I tried to fix it this way, but
any comments or ideas for improvement are welcomed.
Thanks!
José.
On Tue, Oct 10, 2023 at 5:22 AM José Pekkarinen
<[email protected]> 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.
> 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.
>
All this patch does is introduce an error case right? Because if the
BIOS hasn't actually configured those SEV-ES asids and KVM tries to
use an SEV as an SEV-ES asid commands to the ASP will fail, right?
What happens when you try to create an SEV-ES VM with this patch, when
the BIOS hasn't allocated any SEV-ES asids?
On 2023-10-10 19:49, Peter Gonda wrote:
> On Tue, Oct 10, 2023 at 5:22 AM José Pekkarinen
> <[email protected]> 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.
>> 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.
>>
>
> All this patch does is introduce an error case right? Because if the
> BIOS hasn't actually configured those SEV-ES asids and KVM tries to
> use an SEV as an SEV-ES asid commands to the ASP will fail, right?
>
> What happens when you try to create an SEV-ES VM with this patch, when
> the BIOS hasn't allocated any SEV-ES asids?
It still doesn't enable SEV-ES since the cpu exposes
min_sev_asids as 1, and there is a check to bail out in
the hardware setup function, so definitely this is not
fixing anything. I may not being understanding something
here though, since my BIOS doesn't seem to have any options
nor hints about SEV-ES, so I'm not quite sure it really
does something to provide the functionality. For the records
it is a Supermicro H11SSL-NC.
Thanks!
José.