| Message ID | 20200921131923.120833-4-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: ondemand nested state allocation | expand |
On Mon, 2020-09-21 at 08:41 -0700, Sean Christopherson wrote: > On Mon, Sep 21, 2020 at 04:19:22PM +0300, Maxim Levitsky wrote: > > This will be used later to return an error when setting this msr fails. > > > > Note that we ignore this return value for qemu initiated writes to > > avoid breaking backward compatibility. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -2835,13 +2835,15 @@ static void enter_rmode(struct kvm_vcpu *vcpu) > > kvm_mmu_reset_context(vcpu); > > } > > > > -void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer) > > +int vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer) > > { > > struct vcpu_vmx *vmx = to_vmx(vcpu); > > struct shared_msr_entry *msr = find_msr_entry(vmx, MSR_EFER); > > > > - if (!msr) > > - return; > > + if (!msr) { > > + /* Host doen't support EFER, nothing to do */ > > + return 0; > > + } > > Kernel style is to omit braces, even with a line comment. Though I would > do something like so to avoid the question. I didn't knew this, but next time I'll will take this in account! > > /* Nothing to do if hardware doesn't support EFER. */ > if (!msr) > return 0 I'll do this. > > > > vcpu->arch.efer = efer; > > if (efer & EFER_LMA) { > > @@ -2853,6 +2855,7 @@ void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer) > > msr->data = efer & ~EFER_LME; > > } > > setup_msrs(vmx); > > + return 0; > > } > > > > #ifdef CONFIG_X86_64 > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index b6c67ab7c4f34..cab189a71cbb7 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -1456,6 +1456,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > { > > u64 old_efer = vcpu->arch.efer; > > u64 efer = msr_info->data; > > + int r; > > > > if (efer & efer_reserved_bits) > > return 1; > > @@ -1472,7 +1473,12 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > efer &= ~EFER_LMA; > > efer |= vcpu->arch.efer & EFER_LMA; > > > > - kvm_x86_ops.set_efer(vcpu, efer); > > + r = kvm_x86_ops.set_efer(vcpu, efer); > > + > > + if (r && !msr_info->host_initiated) { > > I get the desire to not break backwards compatibility, but this feels all > kinds of wrong, and potentially dangerous as it will KVM in a mixed state. > E.g. vcpu->arch.efer will show that nSVM is enabled, but SVM will not have > the necessary tracking state allocated. That could lead to a userspace > triggerable #GP/panic. Actually I take care to restore the vcpu->arch.efer to its old value if an error happens, so in case of failure everything would indicate that nothing happened, and the offending EFER write can even be retried, however since we agreed that .set_efer will only fail with negative errors like -ENOMEM, I agree that there is no reason to treat userspace writes differently. This code is actually a leftover from previous version, which I should have removed. I'll send a new version soon. Thanks for the review, Best regards, Maxim Levitsky > > Is ignoring OOM scenario really considered backwards compability? The VM > is probably hosted if KVM returns -ENOMEM, e.g. a sophisticated userspace > stack could trigger OOM killer to free memory and resume the VM. On the > other hand, the VM is most definitely hosed if KVM ignores the error and > puts itself into an invalid state. > > > + WARN_ON(r > 0); > > + return r; > > + } > > > > /* Update reserved bits */ > > if ((efer ^ old_efer) & EFER_NX) > > -- > > 2.26.2 > >
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 5303dbc5c9bce..b273c199b9a55 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1069,7 +1069,7 @@ struct kvm_x86_ops { void (*get_cs_db_l_bits)(struct kvm_vcpu *vcpu, int *db, int *l); void (*set_cr0)(struct kvm_vcpu *vcpu, unsigned long cr0); int (*set_cr4)(struct kvm_vcpu *vcpu, unsigned long cr4); - void (*set_efer)(struct kvm_vcpu *vcpu, u64 efer); + int (*set_efer)(struct kvm_vcpu *vcpu, u64 efer); void (*get_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 3da5b2f1b4a19..18f8af55e970a 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -263,7 +263,7 @@ static int get_max_npt_level(void) #endif } -void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer) +int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer) { struct vcpu_svm *svm = to_svm(vcpu); vcpu->arch.efer = efer; @@ -283,6 +283,7 @@ void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer) svm->vmcb->save.efer = efer | EFER_SVME; vmcb_mark_dirty(svm->vmcb, VMCB_CR); + return 0; } static int is_external_interrupt(u32 info) diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 45496775f0db2..1e1842de0efe7 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -338,7 +338,7 @@ static inline bool gif_set(struct vcpu_svm *svm) #define MSR_INVALID 0xffffffffU u32 svm_msrpm_offset(u32 msr); -void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer); +int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer); void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0); int svm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4); void svm_flush_tlb(struct kvm_vcpu *vcpu); diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 6f9a0c6d5dc59..8aef1926e26be 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2835,13 +2835,15 @@ static void enter_rmode(struct kvm_vcpu *vcpu) kvm_mmu_reset_context(vcpu); } -void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer) +int vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer) { struct vcpu_vmx *vmx = to_vmx(vcpu); struct shared_msr_entry *msr = find_msr_entry(vmx, MSR_EFER); - if (!msr) - return; + if (!msr) { + /* Host doen't support EFER, nothing to do */ + return 0; + } vcpu->arch.efer = efer; if (efer & EFER_LMA) { @@ -2853,6 +2855,7 @@ void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer) msr->data = efer & ~EFER_LME; } setup_msrs(vmx); + return 0; } #ifdef CONFIG_X86_64 diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b6c67ab7c4f34..cab189a71cbb7 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1456,6 +1456,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info) { u64 old_efer = vcpu->arch.efer; u64 efer = msr_info->data; + int r; if (efer & efer_reserved_bits) return 1; @@ -1472,7 +1473,12 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info) efer &= ~EFER_LMA; efer |= vcpu->arch.efer & EFER_LMA; - kvm_x86_ops.set_efer(vcpu, efer); + r = kvm_x86_ops.set_efer(vcpu, efer); + + if (r && !msr_info->host_initiated) { + WARN_ON(r > 0); + return r; + } /* Update reserved bits */ if ((efer ^ old_efer) & EFER_NX)
This will be used later to return an error when setting this msr fails. Note that we ignore this return value for qemu initiated writes to avoid breaking backward compatibility. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/include/asm/kvm_host.h | 2 +- arch/x86/kvm/svm/svm.c | 3 ++- arch/x86/kvm/svm/svm.h | 2 +- arch/x86/kvm/vmx/vmx.c | 9 ++++++--- arch/x86/kvm/x86.c | 8 +++++++- 5 files changed, 17 insertions(+), 7 deletions(-)