linux-kernel - Re: [PATCH 2/2] KVM: SVM: add some info prints to SEV init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZGe9I+S78pQ/RPs7@google.com>
Date:   Fri, 19 May 2023 11:17:07 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
Cc:     pbonzini@...hat.com,
        "Stéphane Graber" <stgraber@...ntu.com>,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] KVM: SVM: add some info prints to SEV init

On Tue, Apr 04, 2023, Alexander Miqqqqkhalitsyn wrote:
> Let's add a few pr_info's to sev_hardware_setup to make SEV/SEV-ES
> enabling a little bit handier for users. Right now it's too hard
> to guess why SEV/SEV-ES are failing to enable.

Hmm, I'm somewhat torn, but I'm against taking this patch, at least not in its
current form.  I appreciated that determining why KVM isn't enabling SEV/SEV-ES
is annoying, but there's very little actionable information provided here that
isn't also super obvious.  I also don't want to start us down a slippery slope
of printing out messages every time KVM doesn't enable a feature.

If someone tries to enable SEV and doesn't check that their CPU supports SEV,
then IMO that's on them.  Ditto for SEV-ES.

The NPT thing is mildly interesting, but practically speaking I don't expect that
to ever be a hindrace for generic enabling.  Ditto for MMIO caching.

The decode assists check is (a) completely unactionable for the vast, vast majority
of users and (b) is a WARN_ON_ONCE() condition.

The ASID stuff is by far the most interesting, but that's also quite interesting
for when SEV and SEV-ES _are_ fully supported.

So if we want to provide the user more info, I'd prefer to do something like the
below, which I think would be more helpful and would avoid my slippery slope
concerns.

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c25aeb550cd9..eb4c6e3812d9 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2216,7 +2216,6 @@ void __init sev_hardware_setup(void)
        if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count))
                goto out;

-       pr_info("SEV supported: %u ASIDs\n", sev_asid_count);
        sev_supported = true;

        /* SEV-ES support requested? */
@@ -2243,11 +2242,16 @@ void __init sev_hardware_setup(void)
        sev_es_asid_count = min_sev_asid - 1;
        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", ...);
+       if (boot_cpu_has(X86_FEATURE_SEV_ES))
+               pr_info("SEV-ES %s (ASIDs %u - %u)\n",
+                       sev_es_supported ? "enabled" : "disabled", ...);
+
        sev_enabled = sev_supported;
        sev_es_enabled = sev_es_supported;
 #endif