* [PATCH 1/2] kvm:x86/svm: enable SVM lock if host supports it @ 2013-04-21 8:42 prasadjoshi.linux 2013-04-21 8:42 ` [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR prasadjoshi.linux 0 siblings, 1 reply; 5+ messages in thread From: prasadjoshi.linux @ 2013-04-21 8:42 UTC (permalink / raw) To: prasadjoshi.linux; +Cc: kvm, joro, mtosatti, gleb From: Prasad Joshi <prasadjoshi.linux@gmail.com> SVM lock features allows software from preventing update to EFER.SVME. Enable the SVM lock in guest if it is supported on host machine. Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com> --- arch/x86/kvm/svm.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index e1b1ce2..fcdfdea 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -4031,6 +4031,10 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry) if (boot_cpu_has(X86_FEATURE_NRIPS)) entry->edx |= SVM_FEATURE_NRIP; + /* support SVM Lock if host supports it */ + if (boot_cpu_has(X86_FEATURE_SVML)) + entry->edx |= SVM_FEATURE_SVML; + /* Support NPT for the guest if enabled */ if (npt_enabled) entry->edx |= SVM_FEATURE_NPT; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR 2013-04-21 8:42 [PATCH 1/2] kvm:x86/svm: enable SVM lock if host supports it prasadjoshi.linux @ 2013-04-21 8:42 ` prasadjoshi.linux 2013-04-21 9:19 ` Gleb Natapov 0 siblings, 1 reply; 5+ messages in thread From: prasadjoshi.linux @ 2013-04-21 8:42 UTC (permalink / raw) To: prasadjoshi.linux; +Cc: kvm, joro, mtosatti, gleb From: Prasad Joshi <prasadjoshi.linux@gmail.com> Write only SVM_KEY can be used to create a password protected mechanism to clear VM_CR.LOCK. This key can only be set when VM_CR.LOCK is unset. Once this key is set, a write to it compares the written value with already set key. The VM_CR.LOCK is unset only if the key matches. All reads from this MSR must return 0. Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com> --- arch/x86/include/uapi/asm/msr-index.h | 1 + arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h index 892ce40..eda8346 100644 --- a/arch/x86/include/uapi/asm/msr-index.h +++ b/arch/x86/include/uapi/asm/msr-index.h @@ -170,6 +170,7 @@ #define MSR_AMD64_PATCH_LEVEL 0x0000008b #define MSR_AMD64_TSC_RATIO 0xc0000104 +#define MSR_AMD64_SVM_KEY 0xc0010118 #define MSR_AMD64_NB_CFG 0xc001001f #define MSR_AMD64_PATCH_LOADER 0xc0010020 #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140 diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index fcdfdea..73b2dc2 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -150,6 +150,8 @@ struct vcpu_svm { struct nested_state nested; + u64 svm_key; /* Write only SVM Key */ + bool nmi_singlestep; unsigned int3_injected; @@ -3079,6 +3081,13 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data) case MSR_VM_CR: *data = svm->nested.vm_cr_msr; break; + case MSR_AMD64_SVM_KEY: + /* + * SVM Key is password protected mechanism to manage + * VM_CR.LOCK and therefore reading it must always return 0. + */ + *data = 0; + break; case MSR_IA32_UCODE_REV: *data = 0x01000065; break; @@ -3132,6 +3141,26 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) return 0; } +static int svm_set_svm_key(struct kvm_vcpu *vcpu, u64 data) +{ + struct vcpu_svm *svm = to_svm(vcpu); + + if (!boot_cpu_has(X86_FEATURE_SVML)) + return 1; + + if (!(svm->nested.vm_cr_msr & SVM_VM_CR_SVM_LOCK_MASK) && data) { + /* vm_cr.lock not set: set the password key */ + svm->svm_key = data; + } else { + /* vm_cr.lock is set */ + if (data && svm->svm_key == data) { + /* password key matched: reset the lock */ + svm->nested.vm_cr_msr &= ~SVM_VM_CR_SVM_LOCK_MASK; + } + } + return 0; +} + static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) { struct vcpu_svm *svm = to_svm(vcpu); @@ -3191,6 +3220,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) break; case MSR_VM_CR: return svm_set_vm_cr(vcpu, data); + case MSR_AMD64_SVM_KEY: + return svm_set_svm_key(vcpu, data); case MSR_VM_IGNNE: vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data); break; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR 2013-04-21 8:42 ` [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR prasadjoshi.linux @ 2013-04-21 9:19 ` Gleb Natapov 2013-04-22 3:54 ` Prasad Joshi 0 siblings, 1 reply; 5+ messages in thread From: Gleb Natapov @ 2013-04-21 9:19 UTC (permalink / raw) To: prasadjoshi.linux; +Cc: kvm, joro, mtosatti On Sun, Apr 21, 2013 at 02:12:21PM +0530, prasadjoshi.linux@gmail.com wrote: > From: Prasad Joshi <prasadjoshi.linux@gmail.com> > > Write only SVM_KEY can be used to create a password protected mechanism > to clear VM_CR.LOCK. > > This key can only be set when VM_CR.LOCK is unset. Once this key is set, > a write to it compares the written value with already set key. The > VM_CR.LOCK is unset only if the key matches. All reads from this MSR > must return 0. > > Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com> > --- > arch/x86/include/uapi/asm/msr-index.h | 1 + > arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h > index 892ce40..eda8346 100644 > --- a/arch/x86/include/uapi/asm/msr-index.h > +++ b/arch/x86/include/uapi/asm/msr-index.h > @@ -170,6 +170,7 @@ > > #define MSR_AMD64_PATCH_LEVEL 0x0000008b > #define MSR_AMD64_TSC_RATIO 0xc0000104 > +#define MSR_AMD64_SVM_KEY 0xc0010118 > #define MSR_AMD64_NB_CFG 0xc001001f > #define MSR_AMD64_PATCH_LOADER 0xc0010020 > #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140 > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index fcdfdea..73b2dc2 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -150,6 +150,8 @@ struct vcpu_svm { > > struct nested_state nested; > > + u64 svm_key; /* Write only SVM Key */ > + > bool nmi_singlestep; > > unsigned int3_injected; > @@ -3079,6 +3081,13 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data) > case MSR_VM_CR: > *data = svm->nested.vm_cr_msr; > break; > + case MSR_AMD64_SVM_KEY: > + /* > + * SVM Key is password protected mechanism to manage > + * VM_CR.LOCK and therefore reading it must always return 0. > + */ > + *data = 0; > + break; > case MSR_IA32_UCODE_REV: > *data = 0x01000065; > break; > @@ -3132,6 +3141,26 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) > return 0; > } > > +static int svm_set_svm_key(struct kvm_vcpu *vcpu, u64 data) > +{ > + struct vcpu_svm *svm = to_svm(vcpu); > + > + if (!boot_cpu_has(X86_FEATURE_SVML)) > + return 1; > + Since the feature is emulated why do you care if host cpu supports it? > + if (!(svm->nested.vm_cr_msr & SVM_VM_CR_SVM_LOCK_MASK) && data) { > + /* vm_cr.lock not set: set the password key */ > + svm->svm_key = data; > + } else { > + /* vm_cr.lock is set */ > + if (data && svm->svm_key == data) { > + /* password key matched: reset the lock */ > + svm->nested.vm_cr_msr &= ~SVM_VM_CR_SVM_LOCK_MASK; > + } > + } That's not enough. First of all KVM does not currently checks SVMDIS before allowing setting of EFER_SVME. Second svm_set_vm_cr() erroneously ignores writes into VM_CR SVMDIS/LOCK if SVMDIS is set, while it should check LOCK. Third when write is done by a userspace during reset the key check should be ignored. > + return 0; > +} > + > static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > { > struct vcpu_svm *svm = to_svm(vcpu); > @@ -3191,6 +3220,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > break; > case MSR_VM_CR: > return svm_set_vm_cr(vcpu, data); > + case MSR_AMD64_SVM_KEY: > + return svm_set_svm_key(vcpu, data); > case MSR_VM_IGNNE: > vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data); > break; > -- > 1.7.10.4 -- Gleb. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR 2013-04-21 9:19 ` Gleb Natapov @ 2013-04-22 3:54 ` Prasad Joshi 2013-04-22 8:45 ` Gleb Natapov 0 siblings, 1 reply; 5+ messages in thread From: Prasad Joshi @ 2013-04-22 3:54 UTC (permalink / raw) To: Gleb Natapov; +Cc: kvm, joro, mtosatti On Sun, Apr 21, 2013 at 2:49 PM, Gleb Natapov <gleb@redhat.com> wrote: > On Sun, Apr 21, 2013 at 02:12:21PM +0530, prasadjoshi.linux@gmail.com wrote: >> From: Prasad Joshi <prasadjoshi.linux@gmail.com> >> >> Write only SVM_KEY can be used to create a password protected mechanism >> to clear VM_CR.LOCK. >> >> This key can only be set when VM_CR.LOCK is unset. Once this key is set, >> a write to it compares the written value with already set key. The >> VM_CR.LOCK is unset only if the key matches. All reads from this MSR >> must return 0. >> >> Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com> >> --- >> arch/x86/include/uapi/asm/msr-index.h | 1 + >> arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++ >> 2 files changed, 32 insertions(+) >> >> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h >> index 892ce40..eda8346 100644 >> --- a/arch/x86/include/uapi/asm/msr-index.h >> +++ b/arch/x86/include/uapi/asm/msr-index.h >> @@ -170,6 +170,7 @@ >> >> #define MSR_AMD64_PATCH_LEVEL 0x0000008b >> #define MSR_AMD64_TSC_RATIO 0xc0000104 >> +#define MSR_AMD64_SVM_KEY 0xc0010118 >> #define MSR_AMD64_NB_CFG 0xc001001f >> #define MSR_AMD64_PATCH_LOADER 0xc0010020 >> #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140 >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index fcdfdea..73b2dc2 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -150,6 +150,8 @@ struct vcpu_svm { >> >> struct nested_state nested; >> >> + u64 svm_key; /* Write only SVM Key */ >> + >> bool nmi_singlestep; >> >> unsigned int3_injected; >> @@ -3079,6 +3081,13 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data) >> case MSR_VM_CR: >> *data = svm->nested.vm_cr_msr; >> break; >> + case MSR_AMD64_SVM_KEY: >> + /* >> + * SVM Key is password protected mechanism to manage >> + * VM_CR.LOCK and therefore reading it must always return 0. >> + */ >> + *data = 0; >> + break; >> case MSR_IA32_UCODE_REV: >> *data = 0x01000065; >> break; >> @@ -3132,6 +3141,26 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) >> return 0; >> } >> >> +static int svm_set_svm_key(struct kvm_vcpu *vcpu, u64 data) >> +{ >> + struct vcpu_svm *svm = to_svm(vcpu); >> + >> + if (!boot_cpu_has(X86_FEATURE_SVML)) >> + return 1; >> + > Since the feature is emulated why do you care if host cpu supports it? Thanks Gleb. I changed this to checking SVML feature on the guest CPU. Something like this should work if (!(cpuid_edx(SVM_CPUID_FUNC) & SVM_FEATURE_SVML)) return 1; > >> + if (!(svm->nested.vm_cr_msr & SVM_VM_CR_SVM_LOCK_MASK) && data) { >> + /* vm_cr.lock not set: set the password key */ >> + svm->svm_key = data; >> + } else { >> + /* vm_cr.lock is set */ >> + if (data && svm->svm_key == data) { >> + /* password key matched: reset the lock */ >> + svm->nested.vm_cr_msr &= ~SVM_VM_CR_SVM_LOCK_MASK; >> + } >> + } > That's not enough. First of all KVM does not currently checks > SVMDIS before allowing setting of EFER_SVME. Second svm_set_vm_cr() erroneously > ignores writes into VM_CR SVMDIS/LOCK if SVMDIS is set, while it should > check LOCK. I was aware of these changes (BUGs), however thought they must go in another patch. Any ways, I will include them in the next patch series. > Third when write is done by a userspace during reset the > key check should be ignored. I missed this part. I think, CPU reset must set svm_key to 0 and reset the VM_CR.LOCK. Both of these can be part of init_vmcb() which is called during CPU reset. It should ensure that the no key is registered during CPU reset. Thanks. -- Prasad ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR 2013-04-22 3:54 ` Prasad Joshi @ 2013-04-22 8:45 ` Gleb Natapov 0 siblings, 0 replies; 5+ messages in thread From: Gleb Natapov @ 2013-04-22 8:45 UTC (permalink / raw) To: Prasad Joshi; +Cc: kvm, joro, mtosatti On Mon, Apr 22, 2013 at 09:24:12AM +0530, Prasad Joshi wrote: > On Sun, Apr 21, 2013 at 2:49 PM, Gleb Natapov <gleb@redhat.com> wrote: > > On Sun, Apr 21, 2013 at 02:12:21PM +0530, prasadjoshi.linux@gmail.com wrote: > >> From: Prasad Joshi <prasadjoshi.linux@gmail.com> > >> > >> Write only SVM_KEY can be used to create a password protected mechanism > >> to clear VM_CR.LOCK. > >> > >> This key can only be set when VM_CR.LOCK is unset. Once this key is set, > >> a write to it compares the written value with already set key. The > >> VM_CR.LOCK is unset only if the key matches. All reads from this MSR > >> must return 0. > >> > >> Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com> > >> --- > >> arch/x86/include/uapi/asm/msr-index.h | 1 + > >> arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++ > >> 2 files changed, 32 insertions(+) > >> > >> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h > >> index 892ce40..eda8346 100644 > >> --- a/arch/x86/include/uapi/asm/msr-index.h > >> +++ b/arch/x86/include/uapi/asm/msr-index.h > >> @@ -170,6 +170,7 @@ > >> > >> #define MSR_AMD64_PATCH_LEVEL 0x0000008b > >> #define MSR_AMD64_TSC_RATIO 0xc0000104 > >> +#define MSR_AMD64_SVM_KEY 0xc0010118 > >> #define MSR_AMD64_NB_CFG 0xc001001f > >> #define MSR_AMD64_PATCH_LOADER 0xc0010020 > >> #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140 > >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > >> index fcdfdea..73b2dc2 100644 > >> --- a/arch/x86/kvm/svm.c > >> +++ b/arch/x86/kvm/svm.c > >> @@ -150,6 +150,8 @@ struct vcpu_svm { > >> > >> struct nested_state nested; > >> > >> + u64 svm_key; /* Write only SVM Key */ > >> + > >> bool nmi_singlestep; > >> > >> unsigned int3_injected; > >> @@ -3079,6 +3081,13 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data) > >> case MSR_VM_CR: > >> *data = svm->nested.vm_cr_msr; > >> break; > >> + case MSR_AMD64_SVM_KEY: > >> + /* > >> + * SVM Key is password protected mechanism to manage > >> + * VM_CR.LOCK and therefore reading it must always return 0. > >> + */ > >> + *data = 0; > >> + break; > >> case MSR_IA32_UCODE_REV: > >> *data = 0x01000065; > >> break; > >> @@ -3132,6 +3141,26 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) > >> return 0; > >> } > >> > >> +static int svm_set_svm_key(struct kvm_vcpu *vcpu, u64 data) > >> +{ > >> + struct vcpu_svm *svm = to_svm(vcpu); > >> + > >> + if (!boot_cpu_has(X86_FEATURE_SVML)) > >> + return 1; > >> + > > Since the feature is emulated why do you care if host cpu supports it? > > Thanks Gleb. I changed this to checking SVML feature on the guest CPU. > > Something like this should work > > if (!(cpuid_edx(SVM_CPUID_FUNC) & SVM_FEATURE_SVML)) > return 1; > This still checks host cpu. You need to use kvm_find_cpuid_entry(). > > > >> + if (!(svm->nested.vm_cr_msr & SVM_VM_CR_SVM_LOCK_MASK) && data) { > >> + /* vm_cr.lock not set: set the password key */ > >> + svm->svm_key = data; > >> + } else { > >> + /* vm_cr.lock is set */ > >> + if (data && svm->svm_key == data) { > >> + /* password key matched: reset the lock */ > >> + svm->nested.vm_cr_msr &= ~SVM_VM_CR_SVM_LOCK_MASK; > >> + } > >> + } > > That's not enough. First of all KVM does not currently checks > > SVMDIS before allowing setting of EFER_SVME. Second svm_set_vm_cr() erroneously > > ignores writes into VM_CR SVMDIS/LOCK if SVMDIS is set, while it should > > check LOCK. > > I was aware of these changes (BUGs), however thought they must go in > another patch. It should go to another patch, but send them together with the series that implement LOCK feature. Without the fixes LOCK feature does not work. > Any ways, I will include them in the next patch series. > Thanks. > > Third when write is done by a userspace during reset the > > key check should be ignored. > > I missed this part. I think, CPU reset must set svm_key to 0 and reset > the VM_CR.LOCK. Both of these can be part of init_vmcb() which is > called during CPU reset. It should ensure that the no key is > registered during CPU reset. init_vmcb() is called as part of kvm_vcpu_reset(). kvm_vcpu_reset() is called during vcpu creation and when vcpu receives INIT signal (if irqchip is in kernel). INIT signal does not cause LOCK to be unset, otherwise it would have been pointless. Power-up reset is emulated by userspace configuring vcpu with reset state, for that userspase should be able to set any value there. -- Gleb. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-04-22 8:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-21 8:42 [PATCH 1/2] kvm:x86/svm: enable SVM lock if host supports it prasadjoshi.linux 2013-04-21 8:42 ` [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR prasadjoshi.linux 2013-04-21 9:19 ` Gleb Natapov 2013-04-22 3:54 ` Prasad Joshi 2013-04-22 8:45 ` Gleb Natapov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox