[RFC] Revert "kvm: nVMX: Restrict VMX capability MSR changes"

[RFC] Revert "kvm: nVMX: Restrict VMX capability MSR changes"

[Vitaly Kuznetsov]

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(-)

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);
-- 
2.24.1

Re: [RFC] Revert "kvm: nVMX: Restrict VMX capability MSR changes"

[Paolo Bonzini]

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

Re: [RFC] Revert "kvm: nVMX: Restrict VMX capability MSR changes"

[Vitaly Kuznetsov]

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.

-- 
Vitaly

Re: [RFC] Revert "kvm: nVMX: Restrict VMX capability MSR changes"

[Paolo Bonzini]

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

Re: [RFC] Revert "kvm: nVMX: Restrict VMX capability MSR changes"

[Liran Alon]

> 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