Re: [PATCH RFC] i386/kvm: fix enlightened VMCS with fine-grained VMX feature enablement

[Vitaly Kuznetsov]

Paolo Bonzini <address@hidden> writes:

> On 07/01/20 13:08, Vitaly Kuznetsov wrote:
>> Honestly I forgot the story why we filtered out these features upon
>> eVMCS enablement in KVM. As there are no corresponding eVMCS fields,
>> there's no way a guest can actually use them.
>
> Well, mostly because we mimicked what Hyper-V was doing I guess.
>
>> I'm going to check that nothing breaks if we remove the filter. I'll go
>> and test Hyper-V 2016 and 2019.
>
> KVM would break, right?  But we can mark that patch as stable material.
>

While we are trying to understand how APIC virtualization works without
apic_access_addr field (maybe it doesn't?), should we fix the immediate
issue with QEMU-4.2 with a hack like:

diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index 72359709cdc1..038297e63396 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -357,15 +357,15 @@ int nested_enable_evmcs(struct kvm_vcpu *vcpu,
        if (vmcs_version)
                *vmcs_version = nested_get_evmcs_version(vcpu);
 
-       /* We don't support disabling the feature for simplicity. */
-       if (evmcs_already_enabled)
-               return 0;
-
-       vmx->nested.msrs.pinbased_ctls_high &= ~EVMCS1_UNSUPPORTED_PINCTRL;
-       vmx->nested.msrs.entry_ctls_high &= ~EVMCS1_UNSUPPORTED_VMENTRY_CTRL;
-       vmx->nested.msrs.exit_ctls_high &= ~EVMCS1_UNSUPPORTED_VMEXIT_CTRL;
-       vmx->nested.msrs.secondary_ctls_high &= ~EVMCS1_UNSUPPORTED_2NDEXEC;
-       vmx->nested.msrs.vmfunc_controls &= ~EVMCS1_UNSUPPORTED_VMFUNC;
-
        return 0;
 }
+
+void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata) {
+       /*
+        * Enlightened VMCS doesn't have apic_access_addr field but Hyper-V
+        * still tries to enable SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES when
+        * it is available, filter it out
+        */
+       if (msr_index == MSR_IA32_VMX_PROCBASED_CTLS2)
+               *pdata &= ~((u64)SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES << 32);
+}
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 07ebf6882a45..b88d9807a796 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -201,5 +201,6 @@ bool nested_enlightened_vmentry(struct kvm_vcpu *vcpu, u64 
*evmcs_gpa);
 uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu);
 int nested_enable_evmcs(struct kvm_vcpu *vcpu,
                        uint16_t *vmcs_version);
+void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata);
 
 #endif /* __KVM_X86_VMX_EVMCS_H */
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e3394c839dea..8eb74618b8d8 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1849,8 +1849,14 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
        case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
                if (!nested_vmx_allowed(vcpu))
                        return 1;
-               return vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index,
-                                      &msr_info->data);
+               if (vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index,
+                                   &msr_info->data))
+                       return 1;
+               if (!msr_info->host_initiated &&
+                   vmx->nested.enlightened_vmcs_enabled)
+                       nested_evmcs_filter_control_msr(msr_info->index,
+                                                       &msr_info->data);
+               break;
        case MSR_IA32_RTIT_CTL:
                if (pt_mode != PT_MODE_HOST_GUEST)
                        return 1;

This should probably be complemented with a patch to not enable
unsupported controls when KVM is acting as a guest on eVMCS + a check
that none of the unsupported controls are enabled.

What do you think?

-- 
Vitaly