Liran Alon <liran.alon@xxxxxxxxxx> writes:
>> 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?
Good idea. We, however, need their Hyper-V host team and not Linux
folks.
>
> 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?
Yes. My current understanding (correct me if I'm wrong!) is that both
KVM-on-Hyper-V and Hyper-V-on-KVM work well with Enlightened
VMCS. Surprisingly, KVM-on-KVM doesn't - and this is because of the
issue you're trying to address by the patch.
--
Vitaly
