Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> writes:
> Liran Alon <liran.alon@xxxxxxxxxx> writes:
>
>> According to TLFS section 16.11.2 Enlightened VMCS, the first u32
>> field of eVMCS should specify eVMCS VersionNumber.
>>
>> This version should be in the range of supported eVMCS versions exposed
>> to guest via CPUID.0x4000000A.EAX[0:15].
>> The range which KVM expose to guest in this CPUID field should be the
>> same as the value returned in vmcs_version by nested_enable_evmcs().
>>
>> According to the above, eVMCS VMPTRLD should verify that version specified
>> in given eVMCS is in the supported range. However, current code
>> mistakenly verfies this field against VMCS12_REVISION.
>>
>> One can also see that when KVM use eVMCS, it makes sure that
>> alloc_vmcs_cpu() sets allocated eVMCS revision_id to KVM_EVMCS_VERSION.
>>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@xxxxxxxxxx>
>> Reviewed-by: Mark Kanda <mark.kanda@xxxxxxxxxx>
>> Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
>> ---
>> arch/x86/kvm/vmx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 4555077d69ce..36b7b6c64547 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -9369,7 +9369,7 @@ static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu,
>>
>> vmx->nested.hv_evmcs = kmap(vmx->nested.hv_evmcs_page);
>>
>> - if (vmx->nested.hv_evmcs->revision_id != VMCS12_REVISION) {
>> + if (vmx->nested.hv_evmcs->revision_id != KVM_EVMCS_VERSION) {
>> nested_release_evmcs(vcpu);
>> return 0;
>> }
>
> The patch made me wonder how it all worked before, I decided to give it
> a try with Hyper-V on KVM and it fails to boot - and I think that was
> the reason why VMCS12_REVISION is here. evmcs selftest also seems to
> fail post-patch (no surprises).
>
> I, however, see the reason behind your patch. Let me go back and refresh
> my memory on what's going on.
It seems genuine Hyper-V always put VMCS revision from
IA32_VMX_BASIC_MSR(0x480) to eVMCS regardless of what's in
CPUID.4000000A. This all works just because when we run on genuine
Hyper-V we see revision '1' in both. I don't think we would want to
adjust IA32_VMX_BASIC_MSR in KVM to be '1' - as using Enlightened VMCS
is just an option, we have no idea if the guest is going to use it.
So I think the patch needs to change the other side of KVM to make its
behavior match Hyper-V's. Sigh.
--
Vitaly
