| Message ID | 20210109004714.1341275-4-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: Misc SEV cleanups | expand |
Sean Christopherson <seanjc@google.com> 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@google.com> > --- > 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? 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? > + > 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);
On 1/8/21 6:47 PM, Sean Christopherson wrote: > 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@google.com> > --- > 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); > + > 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(); I believe the reason for the original if statement was similar to: 853c110982ea ("KVM: x86: support CONFIG_KVM_AMD=y with CONFIG_CRYPTO_DEV_CCP_DD=m") But with the removal of sev_platform_status() from sev_hardware_setup(), I think it's ok to call sev_hardware_setup() no matter what now. Thanks, Tom > > 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); >
On 1/11/21 4:42 AM, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@google.com> 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@google.com> >> --- >> 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); >
Tom Lendacky <thomas.lendacky@amd.com> writes: > On 1/11/21 4:42 AM, Vitaly Kuznetsov wrote: >> Sean Christopherson <seanjc@google.com> 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@google.com> >>> --- >>> 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? > Yes, as one may expect the result will be that SEV-ES guests work and plain SEV don't.
On Mon, Jan 11, 2021, Vitaly Kuznetsov wrote: > Tom Lendacky <thomas.lendacky@amd.com> writes: > > > On 1/11/21 4:42 AM, Vitaly Kuznetsov wrote: > >> Sean Christopherson <seanjc@google.com> 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@google.com> > >>> --- > >>> 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? > > > > Yes, as one may expect the result will be that SEV-ES guests work and > plain SEV don't. KVM doesn't issue messages when it overrides other module params due to disable requirements, e.g. ept=0 unrestricted_guest=1 is roughly equivalent. Not that what KVM currently does is right, but at least it's consistent. :-) And on the other hand, I think it's reasonable to expect that specifying only sev=0 is sufficient to disable both SEV and SEV-ES, e.g. to turn them off when they're enabled by default.
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); + 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);
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@google.com> --- 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(-)