| Message ID | 20200820133339.372823-5-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: ondemand nested state allocation + smm fixes | expand |
On Thu, Aug 20, 2020 at 6:34 AM Maxim Levitsky <mlevitsk@redhat.com> wrote: > > This will be used later to return an error when setting this msr fails. > > For VMX, it already has an error condition when EFER is > not in the shared MSR list, so return an error in this case. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1471,7 +1471,8 @@ 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); > + if (kvm_x86_ops.set_efer(vcpu, efer)) > + return 1; This seems like a userspace ABI change to me. Previously, it looks like userspace could always use KVM_SET_MSRS to set MSR_EFER to 0 or EFER_SCE, and it would always succeed. Now, it looks like it will fail on CPUs that don't support EFER in hardware. (Perhaps it should fail, but it didn't before, AFAICT.)
On Thu, Aug 20, 2020 at 02:43:56PM -0700, Jim Mattson wrote: > On Thu, Aug 20, 2020 at 6:34 AM Maxim Levitsky <mlevitsk@redhat.com> wrote: > > > > This will be used later to return an error when setting this msr fails. > > > > For VMX, it already has an error condition when EFER is > > not in the shared MSR list, so return an error in this case. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -1471,7 +1471,8 @@ 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); > > + if (kvm_x86_ops.set_efer(vcpu, efer)) > > + return 1; > > This seems like a userspace ABI change to me. Previously, it looks > like userspace could always use KVM_SET_MSRS to set MSR_EFER to 0 or > EFER_SCE, and it would always succeed. Now, it looks like it will fail > on CPUs that don't support EFER in hardware. (Perhaps it should fail, > but it didn't before, AFAICT.) KVM emulates SYSCALL, presumably that also works when EFER doesn't exist in hardware. The above also adds weirdness to nested VMX as vmx_set_efer() simply can't fail.
On Thu, 2020-08-20 at 17:43 -0700, Sean Christopherson wrote: > On Thu, Aug 20, 2020 at 02:43:56PM -0700, Jim Mattson wrote: > > On Thu, Aug 20, 2020 at 6:34 AM Maxim Levitsky <mlevitsk@redhat.com> wrote: > > > This will be used later to return an error when setting this msr fails. > > > > > > For VMX, it already has an error condition when EFER is > > > not in the shared MSR list, so return an error in this case. > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > --- > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -1471,7 +1471,8 @@ 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); > > > + if (kvm_x86_ops.set_efer(vcpu, efer)) > > > + return 1; > > > > This seems like a userspace ABI change to me. Previously, it looks > > like userspace could always use KVM_SET_MSRS to set MSR_EFER to 0 or > > EFER_SCE, and it would always succeed. Now, it looks like it will fail > > on CPUs that don't support EFER in hardware. (Perhaps it should fail, > > but it didn't before, AFAICT.) > > KVM emulates SYSCALL, presumably that also works when EFER doesn't exist in > hardware. This is a fair point. How about checking the return value only when '!msr_info->host_initiated' in set_efer? This way userspace initiated EFER write will work as it did before, but guest initiated write will fail (and set_efer already checks and fails for many cases) I also digged a bit around the failure check in VMX, the 'find_msr_entry(vmx, MSR_EFER);' This one if I am not mistaken will only fail when host doesn't support EFER. I don't mind ignoring this error as well as it was before. > > The above also adds weirdness to nested VMX as vmx_set_efer() simply can't > fail. It will now fail on non 64 bit Intel CPUs that support VMX. I do think that we had these for a while. As I said I'll return 0 when find_msr_entry fails, thus return this behavior as it was on Intel. Best regards, Maxim Levitsky
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 5ab3af7275d8..bd0519e26053 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 7bb094bf6494..f4569899361f 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 ab913468f9cb..468c58a91534 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -349,7 +349,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 46ba2e03a892..e90b9e68c7ea 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2862,13 +2862,13 @@ 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; + return 1; vcpu->arch.efer = efer; if (efer & EFER_LMA) { @@ -2880,6 +2880,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 2db369a64f29..cad5d9778a21 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1471,7 +1471,8 @@ 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); + if (kvm_x86_ops.set_efer(vcpu, efer)) + return 1; /* Update reserved bits */ if ((efer ^ old_efer) & EFER_NX)
This will be used later to return an error when setting this msr fails. For VMX, it already has an error condition when EFER is not in the shared MSR list, so return an error in this case. 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 | 5 +++-- arch/x86/kvm/x86.c | 3 ++- 5 files changed, 9 insertions(+), 6 deletions(-)