> On 31 Oct 2018, at 19:41, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>
> 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
Hmm… Vitaly, maybe it is a good idea that you will Cc some Microsoft guys you know on this thread?
It seems to me from what you describe that Hyper-V has a bug of using revision_id specified in IA32_VMX_BASIC_MSR
instead of using one of the eVMCS supported versions reported in CPUID.4000000A.
I’m also not sure I understand what you suggest as “change the other side of KVM to make it’s behaviour match Hyper-V”.
Do you mean that we will change KVM usage of eVMCS such that alloc_vmcs_cpu() will always set vmcs->hdr.revision_id to vmcs_config.revision_id?
-Liran
