kvm:x86/svm: enable SVM lock if host supports it
PATCH 1/2 kvm:x86/svm: enable SVM lock if host supports it
From: Prasad Joshi <[email protected]>
SVM lock features allows software from preventing update to EFER.SVME. Enable the SVM lock in guest if it is supported on host machine.
--- arch/x86/kvm/svm.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/x86/kvm/svm.c +4 -0
@@ -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 ;
Signature · 1 line
1.7.10.4
PATCH 2/2 kvm/svm: emulate SVM_KEY MSR
From: Prasad Joshi <[email protected]>
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.
--- arch/x86/include/uapi/asm/msr-index.h | 1 + arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)
arch/x86/include/uapi/asm/msr-index.h +1 -0
@@ -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
arch/x86/kvm/svm.c +31 -0
@@ -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 ;
Signature · 1 line
1.7.10.4
Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR
Quoted 56 lines · On Sun, Apr 21, 2013 at 02:12:21PM +0530, [email protected] wrote:
From: Prasad Joshi <[email protected]>
Write only
SVM_KEYcan 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 <[email protected]>--- arch/x86/include/uapi/asm/msr-index.h | 1 + arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)arch/x86/include/uapi/asm/msr-index.h +1
-0@@ -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 0xc0010140arch/x86/kvm/svm.c +9
-0@@ -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 ;
arch/x86/kvm/svm.c static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) · line 3150
Quoted 10 lines
arch/x86/kvm/svm.c +7
-0@@ -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?
arch/x86/kvm/svm.c static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) · line 3160
Quoted 10 lines
arch/x86/kvm/svm.c +10
-0+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.
Quoted 17 lines
arch/x86/kvm/svm.c +5
-0+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 ;Signature · 1 line
1.7.10.4
--
Gleb.
Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR
Quoted 60 lines · Gleb Natapov
Quoted 59 lines · On Sun, Apr 21, 2013 at 02:12:21PM +0530, [email protected] wrote:
From: Prasad Joshi <[email protected]>
Write only
SVM_KEYcan 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 <[email protected]>--- arch/x86/include/uapi/asm/msr-index.h | 1 + arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)arch/x86/include/uapi/asm/msr-index.h +1
-0@@ -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 0xc0010140arch/x86/kvm/svm.c +9
-0@@ -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 ; }
Quoted 8 lines
arch/x86/kvm/svm.c static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) · line 3150
Quoted 7 lines
arch/x86/kvm/svm.c +7
-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;
Quoted 11 lines
arch/x86/kvm/svm.c static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) · line 3160
Quoted 11 lines
arch/x86/kvm/svm.c +10
-0+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 ; + } + }
Quoted 4 lines
That's not enough. First of all KVM does not currently checks SVMDIS before allowing setting of
EFER_SVME. Secondsvm_set_vm_cr()erroneously ignores writes intoVM_CRSVMDIS/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.
Quoted 2 lines
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
Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR
Quoted 70 lines · Prasad Joshi
Quoted 60 lines · Gleb Natapov
Quoted 59 lines · On Sun, Apr 21, 2013 at 02:12:21PM +0530, [email protected] wrote:
From: Prasad Joshi <[email protected]>
Write only
SVM_KEYcan 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 <[email protected]>--- arch/x86/include/uapi/asm/msr-index.h | 1 + arch/x86/kvm/svm.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)arch/x86/include/uapi/asm/msr-index.h +1
-0@@ -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 0xc0010140arch/x86/kvm/svm.c +9
-0@@ -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 ; }Quoted 8 lines
arch/x86/kvm/svm.c static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data) · line 3150
Quoted 7 lines
arch/x86/kvm/svm.c +7
-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?
Quoted 7 lines
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().
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
arch/x86/kvm/svm.c +10
-0+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 ; + } + }
Quoted 7 lines
Quoted 4 lines
That's not enough. First of all KVM does not currently checks SVMDIS before allowing setting of
EFER_SVME. Secondsvm_set_vm_cr()erroneously ignores writes intoVM_CRSVMDIS/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.
Quoted 2 lines
Any ways, I will include them in the next patch series.
Thanks.
Quoted 7 lines
Quoted 2 lines
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_keyto 0 and reset the VM_CR.LOCK. Both of these can be part ofinit_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.