Re: [PATCH 03/13] KVM: SVM: Move SEV module params/variables to sev.c

[Tom Lendacky]

On 1/11/21 4:42 AM, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@xxxxxxxxxx> writes:
>
> > Unconditionally invoke sev_hardware_setup() when configuring SVM and
> > handle clearing the module params/variable 'sev' and 'sev_es' in
> > sev_hardware_setup().  This allows making said variables static within
> > sev.c and reduces the odds of a collision with guest code, e.g. the guest
> > side of things has already laid claim to 'sev_enabled'.
> >
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> >   arch/x86/kvm/svm/sev.c | 11 +++++++++++
> >   arch/x86/kvm/svm/svm.c | 15 +--------------
> >   arch/x86/kvm/svm/svm.h |  2 --
> >   3 files changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 0eeb6e1b803d..8ba93b8fa435 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -27,6 +27,14 @@
> >   
> >   #define __ex(x) __kvm_handle_fault_on_reboot(x)
> >   
> > +/* enable/disable SEV support */
> > +static int sev = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT);
> > +module_param(sev, int, 0444);
> > +
> > +/* enable/disable SEV-ES support */
> > +static int sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT);
> > +module_param(sev_es, int, 0444);
>
> Two stupid questions (and not really related to your patch) for
> self-eduacation if I may:
>
> 1) Why do we rely on CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT (which
> sound like it control the guest side of things) to set defaults here?

I thought it was a review comment, but I'm not able to find it now.

Brijesh probably remembers better than me.

> 2) It appears to be possible to do 'modprobe kvm_amd sev=0 sev_es=1' and
> this looks like a bogus configuration, should we make an effort to
> validate the correctness upon module load?

This will still result in an overall sev=0 sev_es=0. Is the question just 
about issuing a message based on the initial values specified?

Thanks,
Tom

> > +
> >   static u8 sev_enc_bit;
> >   static int sev_flush_asids(void);
> >   static DECLARE_RWSEM(sev_deactivate_lock);
> > @@ -1249,6 +1257,9 @@ void __init sev_hardware_setup(void)
> >   	bool sev_es_supported = false;
> >   	bool sev_supported = false;
> >   
> > +	if (!IS_ENABLED(CONFIG_KVM_AMD_SEV) || !sev)
> > +		goto out;
> > +
> >   	/* Does the CPU support SEV? */
> >   	if (!boot_cpu_has(X86_FEATURE_SEV))
> >   		goto out;
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index ccf52c5531fb..f89f702b2a58 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -189,14 +189,6 @@ module_param(vls, int, 0444);
> >   static int vgif = true;
> >   module_param(vgif, int, 0444);
> >   
> > -/* enable/disable SEV support */
> > -int sev = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT);
> > -module_param(sev, int, 0444);
> > -
> > -/* enable/disable SEV-ES support */
> > -int sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT);
> > -module_param(sev_es, int, 0444);
> > -
> >   bool __read_mostly dump_invalid_vmcb;
> >   module_param(dump_invalid_vmcb, bool, 0644);
> >   
> > @@ -976,12 +968,7 @@ static __init int svm_hardware_setup(void)
> >   		kvm_enable_efer_bits(EFER_SVME | EFER_LMSLE);
> >   	}
> >   
> > -	if (IS_ENABLED(CONFIG_KVM_AMD_SEV) && sev) {
> > -		sev_hardware_setup();
> > -	} else {
> > -		sev = false;
> > -		sev_es = false;
> > -	}
> > +	sev_hardware_setup();
> >   
> >   	svm_adjust_mmio_mask();
> >   
> > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> > index 0fe874ae5498..8e169835f52a 100644
> > --- a/arch/x86/kvm/svm/svm.h
> > +++ b/arch/x86/kvm/svm/svm.h
> > @@ -408,8 +408,6 @@ static inline bool gif_set(struct vcpu_svm *svm)
> >   #define MSR_CR3_LONG_MBZ_MASK			0xfff0000000000000U
> >   #define MSR_INVALID				0xffffffffU
> >   
> > -extern int sev;
> > -extern int sev_es;
> >   extern bool dump_invalid_vmcb;
> >   
> >   u32 svm_msrpm_offset(u32 msr);