Re: [PATCH 11/13] KVM: SVM: Drop redundant svm_sev_enabled() helper

[Sean Christopherson]

On Mon, Jan 11, 2021, Tom Lendacky wrote:
> On 1/8/21 6:47 PM, Sean Christopherson wrote:
> > Replace calls to svm_sev_enabled() with direct checks on sev_enabled, or
> > in the case of svm_mem_enc_op, simply drop the call to svm_sev_enabled().
> > This effectively replaces checks against a valid max_sev_asid with checks
> > against sev_enabled.  sev_enabled is forced off by sev_hardware_setup()
> > if max_sev_asid is invalid, all call sites are guaranteed to run after
> > sev_hardware_setup(), and all of the checks care about SEV being fully
> > enabled (as opposed to intentionally handling the scenario where
> > max_sev_asid is valid but SEV enabling fails due to OOM).
> > 
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> >   arch/x86/kvm/svm/sev.c | 6 +++---
> >   arch/x86/kvm/svm/svm.h | 5 -----
> >   2 files changed, 3 insertions(+), 8 deletions(-)
> > 
> 
> With CONFIG_KVM=y, CONFIG_KVM_AMD=y and CONFIG_CRYPTO_DEV_CCP_DD=m, I get
> the following build warning:

...

> In function ‘bitmap_zero’,
>     inlined from ‘__sev_recycle_asids’ at arch/x86/kvm/svm/sev.c:92:2,
>     inlined from ‘sev_asid_new’ at arch/x86/kvm/svm/sev.c:113:16,
>     inlined from ‘sev_guest_init’ at arch/x86/kvm/svm/sev.c:195:9:
> ./include/linux/bitmap.h:238:2: warning: argument 1 null where non-null expected [-Wnonnull]
>   238 |  memset(dst, 0, len);
>       |  ^~~~~~~~~~~~~~~~~~~

Ah, because that config "silently" disables CONFIG_KVM_AMD_SEV.  The warning
pops up because svm_sev_enabled() included !IS_ENABLED(CONFIG_KVM_AMD_SEV) and
that was enough for the compiler to understand that svm_mem_enc_op() was a nop.

That being said, unless I'm missing something, this is a false positive the
compiler's part, e.g. the warning occurs even if sev_enabled is false be default,
i.e. CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=n.

Anyways, I'm leaning towards "fixing" this by defining sev_enabled and
sev_es_enabled to false if CONFIG_KVM_AMD_SEV=n.  It'd be a worthwhile change to
condition the default values on CONFIG_KVM_AMD_SEV anyways, so it'd kill two
birds with one stone.  Long term, I'm tempted to exporing conditioning all of
sev.c on CONFIG_KVM_AMD_SEV=y, but there are just enough functions exposed via
svm.h that make me think it wouldn't be worth the effort.

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 1b9174a49b65..7e14514dd083 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -28,12 +28,17 @@
 #define __ex(x) __kvm_handle_fault_on_reboot(x)

 /* enable/disable SEV support */
+#ifdef CONFIG_KVM_AMD_SEV
 static bool sev_enabled = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT);
 module_param_named(sev, sev_enabled, bool, 0444);

 /* enable/disable SEV-ES support */
 static bool sev_es_enabled = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT);
 module_param_named(sev_es, sev_es_enabled, bool, 0444);
+#else
+#define sev_enabled false
+#define sev_es_enabled false
+#endif /* CONFIG_KVM_AMD_SEV */

 static u8 sev_enc_bit;
 static int sev_flush_asids(void);
@@ -1253,11 +1258,12 @@ void sev_vm_destroy(struct kvm *kvm)

 void __init sev_hardware_setup(void)
 {
+#ifdef CONFIG_KVM_AMD_SEV
        unsigned int eax, ebx, ecx, edx;
        bool sev_es_supported = false;
        bool sev_supported = false;

-       if (!IS_ENABLED(CONFIG_KVM_AMD_SEV) || !sev_enabled)
+       if (!sev_enabled)
                goto out;

        /* Does the CPU support SEV? */
@@ -1310,6 +1316,7 @@ void __init sev_hardware_setup(void)
 out:
        sev_enabled = sev_supported;
        sev_es_enabled = sev_es_supported;
+#endif
 }

 void sev_hardware_teardown(void)