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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> 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