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@xxxxxxxxxx> wrote:
> > On Sun, Apr 21, 2013 at 02:12:21PM +0530, prasadjoshi.linux@xxxxxxxxx wrote:
> >> From: Prasad Joshi <prasadjoshi.linux@xxxxxxxxx>
> >>
> >> 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@xxxxxxxxx>
> >> ---
> >> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
