| Message ID | 20200120151141.227254-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] Revert "kvm: nVMX: Restrict VMX capability MSR changes" | expand |
On 20/01/20 16:11, Vitaly Kuznetsov wrote: > > RFC. I think the check for vmx->nested.vmxon is legitimate for everything > but restore so removing it (what I do with the revert) is likely a no-go. > I'd like to gather opinions on the proper fix: should we somehow check > that the vCPU is in 'restore' start (has never being run) and make > KVM_SET_MSRS pass or should we actually mandate that KVM_SET_NESTED_STATE > is run after KVM_SET_MSRS by userspace? > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> I think this should be fixed in QEMU, by doing KVM_SET_MSRS for feature MSRs way earlier. I'll do it since I'm currently working on a patch to add a KVM_SET_MSR for the microcode revision. Thanks, Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 20/01/20 16:11, Vitaly Kuznetsov wrote: >> >> RFC. I think the check for vmx->nested.vmxon is legitimate for everything >> but restore so removing it (what I do with the revert) is likely a no-go. >> I'd like to gather opinions on the proper fix: should we somehow check >> that the vCPU is in 'restore' start (has never being run) and make >> KVM_SET_MSRS pass or should we actually mandate that KVM_SET_NESTED_STATE >> is run after KVM_SET_MSRS by userspace? >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > I think this should be fixed in QEMU, by doing KVM_SET_MSRS for feature > MSRs way earlier. I'll do it since I'm currently working on a patch to > add a KVM_SET_MSR for the microcode revision. Works for me, thanks) The bigger issue is that the vCPU setup sequence (like QEMU's kvm_arch_put_registers()) effectively becomes an API convention and as it gets more complex it would be great to document it for KVM.
On 20/01/20 17:33, Vitaly Kuznetsov wrote: >> I think this should be fixed in QEMU, by doing KVM_SET_MSRS for feature >> MSRs way earlier. I'll do it since I'm currently working on a patch to >> add a KVM_SET_MSR for the microcode revision. > Works for me, thanks) > > The bigger issue is that the vCPU setup sequence (like QEMU's > kvm_arch_put_registers()) effectively becomes an API convention and as > it gets more complex it would be great to document it for KVM. Indeed, it's tricky to get right. Though this is smaller compared to the issue of the ordering between VCPU events and everything else. Paolo
> On 20 Jan 2020, at 17:41, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 20/01/20 16:11, Vitaly Kuznetsov wrote: >> >> RFC. I think the check for vmx->nested.vmxon is legitimate for everything >> but restore so removing it (what I do with the revert) is likely a no-go. >> I'd like to gather opinions on the proper fix: should we somehow check >> that the vCPU is in 'restore' start (has never being run) and make >> KVM_SET_MSRS pass or should we actually mandate that KVM_SET_NESTED_STATE >> is run after KVM_SET_MSRS by userspace? >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > I think this should be fixed in QEMU, by doing KVM_SET_MSRS for feature > MSRs way earlier. I agree. > I'll do it since I'm currently working on a patch to > add a KVM_SET_MSR for the microcode revision. Please Cc me. Thanks, -Liran
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 4aea7d304beb..bb8afe0c5e7f 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -1321,13 +1321,6 @@ int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data) { struct vcpu_vmx *vmx = to_vmx(vcpu); - /* - * Don't allow changes to the VMX capability MSRs while the vCPU - * is in VMX operation. - */ - if (vmx->nested.vmxon) - return -EBUSY; - switch (msr_index) { case MSR_IA32_VMX_BASIC: return vmx_restore_vmx_basic(vmx, data);
This reverts commit a943ac50d10aac96dca63d0460365a699d41fdd0. Fine-grained VMX feature enablement in QEMU broke live migration with nested guest: (qemu) qemu-kvm: error: failed to set MSR 0x48e to 0xfff9fffe04006172 The problem is that QEMU does KVM_SET_NESTED_STATE before KVM_SET_MSRS, although it can probably be changed. RFC. I think the check for vmx->nested.vmxon is legitimate for everything but restore so removing it (what I do with the revert) is likely a no-go. I'd like to gather opinions on the proper fix: should we somehow check that the vCPU is in 'restore' start (has never being run) and make KVM_SET_MSRS pass or should we actually mandate that KVM_SET_NESTED_STATE is run after KVM_SET_MSRS by userspace? Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx/nested.c | 7 ------- 1 file changed, 7 deletions(-)