Liran Alon <liran.alon@xxxxxxxxxx> writes:
> vmcs12 represents the per-CPU cache of L1 active vmcs12.
>
> This cache can be loaded by one of the following:
> 1) Guest making a vmcs12 active by exeucting VMPTRLD
> 2) Guest specifying eVMCS in VP assist page and executing
> VMLAUNCH/VMRESUME.
>
> Either way, vmcs12 should have revision_id of VMCS12_REVISION.
> Which is not equal to eVMCS revision_id which specifies used
> VersionNumber of eVMCS struct (e.g. KVM_EVMCS_VERSION).
>
> Specifically, this causes an issue in restoring a nested VM state
> because vmx_set_nested_state() verifies that vmcs12->revision_id
> is equal to VMCS12_REVISION which was not true in case vmcs12
> was populated from an eVMCS by vmx_get_nested_state() which calls
> copy_enlightened_to_vmcs12().
>
> Reviewed-by: Darren Kenny <darren.kenny@xxxxxxxxxx>
> Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
> ---
> arch/x86/kvm/vmx.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4555077d69ce..6e92624fe8d6 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8664,8 +8664,6 @@ static int copy_enlightened_to_vmcs12(struct vcpu_vmx *vmx)
> struct vmcs12 *vmcs12 = vmx->nested.cached_vmcs12;
> struct hv_enlightened_vmcs *evmcs = vmx->nested.hv_evmcs;
>
> - vmcs12->hdr.revision_id = evmcs->revision_id;
> -
> /* HV_VMX_ENLIGHTENED_CLEAN_FIELD_NONE */
> vmcs12->tpr_threshold = evmcs->tpr_threshold;
> vmcs12->guest_rip = evmcs->guest_rip;
> @@ -9390,9 +9388,11 @@ static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu,
> * present in struct hv_enlightened_vmcs, ...). Make sure there
> * are no leftovers.
> */
> - if (from_launch)
> - memset(vmx->nested.cached_vmcs12, 0,
> - sizeof(*vmx->nested.cached_vmcs12));
> + if (from_launch) {
> + struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> + memset(vmcs12, 0, sizeof(*vmcs12));
> + vmcs12->hdr.revision_id = VMCS12_REVISION;
> + }
>
> }
> return 1;
I was wondering how evmcs selftest was passing before this but then I
remembered the Hyper-V bug (which puts VMCS revision from
IA32_VMX_BASIC_MSR - where we have VMCS12_REVISION and not
KVM_EVMCS_VERSION) so 'evmcs->revision_id' in this context is actually
VMCS12_REVISION and there's no change post-patch.
We, however, will need to fix the selftest after we start allowing
KVM_EVMCS_VERSION as it currently mocks erroneous Hyper-V behavior :-)
Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
Thanks!
--
Vitaly
