[PATCH v2 0/2] KVM: SVM: small tweaks for sev_hardware_setup

[PATCH v2 0/2] KVM: SVM: small tweaks for sev_hardware_setup

[Alexander Mikhalitsyn]

KVM: SVM: enhance info printk's in SEV init

Let's print available ASID ranges for SEV/SEV-ES guests.
This information can be useful for system administrator
to debug if SEV/SEV-ES fails to enable.

There are a few reasons.
SEV:
- NPT is disabled (module parameter)
- CPU lacks some features (sev, decodeassists)
- Maximum SEV ASID is 0

SEV-ES:
- mmio_caching is disabled (module parameter)
- CPU lacks sev_es feature
- Minimum SEV ASID value is 1 (can be adjusted in BIOS/UEFI)

==
   
KVM: SVM: free sev_*asid_bitmap init if SEV init fails

If misc_cg_set_capacity() fails for some reason then we have
a memleak for sev_reclaim_asid_bitmap/sev_asid_bitmap. It's
not a case right now, because misc_cg_set_capacity() just can't
fail and check inside it is always successful.

But let's fix that for code consistency.

Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stéphane Graber <stgraber@ubuntu.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

Alexander Mikhalitsyn (2):
  KVM: SVM: free sev_*asid_bitmap init if SEV init fails
  KVM: SVM: enhance info printk's in SEV init

 arch/x86/kvm/svm/sev.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] KVM: SVM: free sev_*asid_bitmap init if SEV init fails

[Alexander Mikhalitsyn]

If misc_cg_set_capacity() fails for some reason then we have
a memleak for sev_reclaim_asid_bitmap/sev_asid_bitmap. It's
not a case right now, because misc_cg_set_capacity() just can't
fail and check inside it is always successful.

But let's fix that for code consistency.

Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stéphane Graber <stgraber@ubuntu.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 arch/x86/kvm/svm/sev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 69ae5e1b3120..cc832a8d1bca 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2216,8 +2216,13 @@ void __init sev_hardware_setup(void)
 	}
 
 	sev_asid_count = max_sev_asid - min_sev_asid + 1;
-	if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count))
+	if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) {
+		bitmap_free(sev_reclaim_asid_bitmap);
+		sev_reclaim_asid_bitmap = NULL;
+		bitmap_free(sev_asid_bitmap);
+		sev_asid_bitmap = NULL;
 		goto out;
+	}
 
 	pr_info("SEV supported: %u ASIDs\n", sev_asid_count);
 	sev_supported = true;
-- 
2.34.1

Re: [PATCH v2 1/2] KVM: SVM: free sev_*asid_bitmap init if SEV init fails

[Sean Christopherson]

On Mon, May 22, 2023, Alexander Mikhalitsyn wrote:
> If misc_cg_set_capacity() fails for some reason then we have
> a memleak for sev_reclaim_asid_bitmap/sev_asid_bitmap. It's
> not a case right now, because misc_cg_set_capacity() just can't
> fail and check inside it is always successful.
> 
> But let's fix that for code consistency.
> 
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: St�phane Graber <stgraber@ubuntu.com>
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---
>  arch/x86/kvm/svm/sev.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 69ae5e1b3120..cc832a8d1bca 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2216,8 +2216,13 @@ void __init sev_hardware_setup(void)
>  	}
>  
>  	sev_asid_count = max_sev_asid - min_sev_asid + 1;
> -	if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count))
> +	if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) {
> +		bitmap_free(sev_reclaim_asid_bitmap);
> +		sev_reclaim_asid_bitmap = NULL;
> +		bitmap_free(sev_asid_bitmap);
> +		sev_asid_bitmap = NULL;
>  		goto out;
> +	}

Blech, didn't look close enough at v1.  I think I'd rather yell and continue on.
If misc_cg_set_capacity() were to fail, debugging would be unnecessarily painful,
and at least as things stand today, there's nothing userspace can do to remedy
the problem except by manually disabling SEV and/or SEV-ES.

---
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 6 Jun 2023 11:34:28 -0700
Subject: [PATCH] KVM: SVM: WARN, but continue, if misc_cg_set_capacity() fails

WARN and continue if misc_cg_set_capacity() fails, as the only scenario
in which it can fail is if the specified resource is invalid, which should
never happen when CONFIG_KVM_AMD_SEV=y.  Deliberately not bailing "fixes"
a theoretical bug where KVM would leak the ASID bitmaps on failure, which
again can't happen.

If the impossible should happen, the end result is effectively the same
with respect to SEV and SEV-ES (they are unusable), while continuing on
has the advantage of letting KVM load, i.e. userspace can still run
non-SEV guests.

Reported-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index d65578d8784d..07756b7348ae 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2216,9 +2216,7 @@ void __init sev_hardware_setup(void)
 	}
 
 	sev_asid_count = max_sev_asid - min_sev_asid + 1;
-	if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count))
-		goto out;
-
+	WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count));
 	sev_supported = true;
 
 	/* SEV-ES support requested? */
@@ -2243,9 +2241,7 @@ void __init sev_hardware_setup(void)
 		goto out;
 
 	sev_es_asid_count = min_sev_asid - 1;
-	if (misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count))
-		goto out;
-
+	WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
 	sev_es_supported = true;
 
 out:

base-commit: 6d1bc9754b04075d938b47cf7f7800814b8911a7
-- 

Re: [PATCH v2 1/2] KVM: SVM: free sev_*asid_bitmap init if SEV init fails

[Aleksandr Mikhalitsyn]

On Tue, Jun 6, 2023 at 8:45 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, May 22, 2023, Alexander Mikhalitsyn wrote:
> > If misc_cg_set_capacity() fails for some reason then we have
> > a memleak for sev_reclaim_asid_bitmap/sev_asid_bitmap. It's
> > not a case right now, because misc_cg_set_capacity() just can't
> > fail and check inside it is always successful.
> >
> > But let's fix that for code consistency.
> >
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: St�phane Graber <stgraber@ubuntu.com>
> > Cc: kvm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> >  arch/x86/kvm/svm/sev.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 69ae5e1b3120..cc832a8d1bca 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2216,8 +2216,13 @@ void __init sev_hardware_setup(void)
> >       }
> >
> >       sev_asid_count = max_sev_asid - min_sev_asid + 1;
> > -     if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count))
> > +     if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) {
> > +             bitmap_free(sev_reclaim_asid_bitmap);
> > +             sev_reclaim_asid_bitmap = NULL;
> > +             bitmap_free(sev_asid_bitmap);
> > +             sev_asid_bitmap = NULL;
> >               goto out;
> > +     }
>
> Blech, didn't look close enough at v1.  I think I'd rather yell and continue on.
> If misc_cg_set_capacity() were to fail, debugging would be unnecessarily painful,
> and at least as things stand today, there's nothing userspace can do to remedy
> the problem except by manually disabling SEV and/or SEV-ES.

Hi Sean,

I agree with your point. Let's go this way!

Kind regards,
Alex

>
> ---
> From: Sean Christopherson <seanjc@google.com>
> Date: Tue, 6 Jun 2023 11:34:28 -0700
> Subject: [PATCH] KVM: SVM: WARN, but continue, if misc_cg_set_capacity() fails
>
> WARN and continue if misc_cg_set_capacity() fails, as the only scenario
> in which it can fail is if the specified resource is invalid, which should
> never happen when CONFIG_KVM_AMD_SEV=y.  Deliberately not bailing "fixes"
> a theoretical bug where KVM would leak the ASID bitmaps on failure, which
> again can't happen.
>
> If the impossible should happen, the end result is effectively the same
> with respect to SEV and SEV-ES (they are unusable), while continuing on
> has the advantage of letting KVM load, i.e. userspace can still run
> non-SEV guests.
>
> Reported-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index d65578d8784d..07756b7348ae 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2216,9 +2216,7 @@ void __init sev_hardware_setup(void)
>         }
>
>         sev_asid_count = max_sev_asid - min_sev_asid + 1;
> -       if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count))
> -               goto out;
> -
> +       WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count));
>         sev_supported = true;
>
>         /* SEV-ES support requested? */
> @@ -2243,9 +2241,7 @@ void __init sev_hardware_setup(void)
>                 goto out;
>
>         sev_es_asid_count = min_sev_asid - 1;
> -       if (misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count))
> -               goto out;
> -
> +       WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
>         sev_es_supported = true;
>
>  out:
>
> base-commit: 6d1bc9754b04075d938b47cf7f7800814b8911a7
> --
>

[PATCH v2 2/2] KVM: SVM: enhance info printk's in SEV init

[Alexander Mikhalitsyn]

Let's print available ASID ranges for SEV/SEV-ES guests.
This information can be useful for system administrator
to debug if SEV/SEV-ES fails to enable.

There are a few reasons.
SEV:
- NPT is disabled (module parameter)
- CPU lacks some features (sev, decodeassists)
- Maximum SEV ASID is 0

SEV-ES:
- mmio_caching is disabled (module parameter)
- CPU lacks sev_es feature
- Minimum SEV ASID value is 1 (can be adjusted in BIOS/UEFI)

Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stéphane Graber <stgraber@ubuntu.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
v2:
	- print only the ASID ranges according to Sean's suggestion
---
 arch/x86/kvm/svm/sev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index cc832a8d1bca..fff63d1f2a34 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2224,7 +2224,6 @@ void __init sev_hardware_setup(void)
 		goto out;
 	}
 
-	pr_info("SEV supported: %u ASIDs\n", sev_asid_count);
 	sev_supported = true;
 
 	/* SEV-ES support requested? */
@@ -2252,10 +2251,16 @@ void __init sev_hardware_setup(void)
 	if (misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count))
 		goto out;
 
-	pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count);
 	sev_es_supported = true;
 
 out:
+	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);
+	if (boot_cpu_has(X86_FEATURE_SEV_ES))
+		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
+			sev_es_supported ? "enabled" : "disabled", 1, min_sev_asid - 1);
+
 	sev_enabled = sev_supported;
 	sev_es_enabled = sev_es_supported;
 #endif
-- 
2.34.1

Re: [PATCH v2 2/2] KVM: SVM: enhance info printk's in SEV init

[Sean Christopherson]

On Mon, May 22, 2023, Alexander Mikhalitsyn wrote:
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index cc832a8d1bca..fff63d1f2a34 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2224,7 +2224,6 @@ void __init sev_hardware_setup(void)
>  		goto out;
>  	}
>  
> -	pr_info("SEV supported: %u ASIDs\n", sev_asid_count);
>  	sev_supported = true;
>  
>  	/* SEV-ES support requested? */
> @@ -2252,10 +2251,16 @@ void __init sev_hardware_setup(void)
>  	if (misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count))
>  		goto out;
>  
> -	pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count);
>  	sev_es_supported = true;
>  
>  out:
> +	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);
> +	if (boot_cpu_has(X86_FEATURE_SEV_ES))
> +		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
> +			sev_es_supported ? "enabled" : "disabled", 1, min_sev_asid - 1);

The min should print '0' if min_sev_asid<=1, otherwise the output will be

	SEV-ES disabled (ASIDs 1 - 0)

which is confusing.  That would also align with what gets printed out for SEV
when it's not supported at all (min==max=0).

No need for v3, I'll fixup when applying.

Re: [PATCH v2 2/2] KVM: SVM: enhance info printk's in SEV init

[Aleksandr Mikhalitsyn]

On Tue, Jun 6, 2023 at 8:46 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, May 22, 2023, Alexander Mikhalitsyn wrote:
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index cc832a8d1bca..fff63d1f2a34 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2224,7 +2224,6 @@ void __init sev_hardware_setup(void)
> >               goto out;
> >       }
> >
> > -     pr_info("SEV supported: %u ASIDs\n", sev_asid_count);
> >       sev_supported = true;
> >
> >       /* SEV-ES support requested? */
> > @@ -2252,10 +2251,16 @@ void __init sev_hardware_setup(void)
> >       if (misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count))
> >               goto out;
> >
> > -     pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count);
> >       sev_es_supported = true;
> >
> >  out:
> > +     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);
> > +     if (boot_cpu_has(X86_FEATURE_SEV_ES))
> > +             pr_info("SEV-ES %s (ASIDs %u - %u)\n",
> > +                     sev_es_supported ? "enabled" : "disabled", 1, min_sev_asid - 1);
>
> The min should print '0' if min_sev_asid<=1, otherwise the output will be
>
>         SEV-ES disabled (ASIDs 1 - 0)
>
> which is confusing.  That would also align with what gets printed out for SEV
> when it's not supported at all (min==max=0).
>
> No need for v3, I'll fixup when applying.

Got it. Ok!

Thanks again for looking into that.

Kind regards,
Alex

Re: [PATCH v2 0/2] KVM: SVM: small tweaks for sev_hardware_setup

[Sean Christopherson]

On Mon, 22 May 2023 18:12:46 +0200, Alexander Mikhalitsyn wrote:
> KVM: SVM: enhance info printk's in SEV init
> 
> Let's print available ASID ranges for SEV/SEV-ES guests.
> This information can be useful for system administrator
> to debug if SEV/SEV-ES fails to enable.
> 
> There are a few reasons.
> SEV:
> - NPT is disabled (module parameter)
> - CPU lacks some features (sev, decodeassists)
> - Maximum SEV ASID is 0
> 
> [...]

Applied [2/2] to kvm-x86 svm, with the minor fixup.  I'll post a formal patch
for the alternative approach to "fixing" the leak, I can always circle back to
this series if this approach is preferred.

Thanks!

[2/2] KVM: SVM: enhance info printk's in SEV init
      https://github.com/kvm-x86/linux/commit/6d1bc9754b04

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes