Re: [PATCH] kvm/sev: make SEV/SEV-ES asids configurable

[José Pekkarinen]

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 <jose.pekkarinen@xxxxxxxxxxx>
> > ---
> >  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é.