Re: [PATCH RFC 2/3] x86/kvm/hyper-v: move VMX controls sanitization out of nested_enable_evmcs()

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> writes:

> Liran Alon <liran.alon@xxxxxxxxxx> writes:
>
>>> On 15 Jan 2020, at 19:10, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>>> 
>>> With fine grained VMX feature enablement QEMU>=4.2 tries to do KVM_SET_MSRS
>>> with default (matching CPU model) values and in case eVMCS is also enabled,
>>> fails.
>>> 
>>> It would be possible to drop VMX feature filtering completely and make
>>> this a guest's responsibility: if it decides to use eVMCS it should know
>>> which fields are available and which are not. Hyper-V mostly complies to
>>> this, however, there is at least one problematic control:
>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES
>>> which Hyper-V enables. As there is no 'apic_addr_field' in eVMCS, we
>>> fail to handle this properly in KVM. It is unclear how this is supposed
>>> to work, genuine Hyper-V doesn't expose the control so it is possible that
>>> this is just a bug (in Hyper-V).
>>
>> Have you tried contacted someone at Hyper-V team about this?
>>
>
> Yes, I have.

I heard back from MS, they admited the bug and suggested us ... to hide
the control from L1 when eVMCS is enabled. No surprises here. They also
said the bug is unlikely to get fixed in the existing Hyper-V versions
(2016 and 2019) so it seems we're stuck with the hack for awhile :-(

...

>>> 
>>> #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;
>>
>> Nit: It seems more elegant to me to put the call to nested_evmcs_filter_control_msr() inside vmx_get_vmx_msr().
>>
>
> Sure, will move it there (in case we actually decide to merge this)
>

Thinking more about it, we can't check host_initiated in
vmx_get_vmx_msr() as it's not passed there, this can probably get
changed but I don't see a big difference so I'll probably keep the hack
where it is now.

-- 
Vitaly