On Mon, 2021-05-17 at 15:50 +0200, Vitaly Kuznetsov wrote:
> Unlike regular set_current_vmptr(), nested_vmx_handle_enlightened_vmptrld()
> can not be called directly from vmx_set_nested_state() as KVM may not have
> all the information yet (e.g. HV_X64_MSR_VP_ASSIST_PAGE MSR may not be
> restored yet). Enlightened VMCS is mapped later while getting nested state
> pages. In the meantime, vmx->nested.hv_evmcs remains NULL and using it
> for various checks is incorrect. In particular, if KVM_GET_NESTED_STATE is
> called right after KVM_SET_NESTED_STATE, KVM_STATE_NESTED_EVMCS flag in the
> resulting state will be unset (and such state will later fail to load).
>
> Introduce nested_evmcs_is_used() and use 'is_guest_mode(vcpu) &&
> vmx->nested.current_vmptr == -1ull' check to detect not-yet-mapped eVMCS
> after restore.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
> arch/x86/kvm/vmx/nested.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 6058a65a6ede..3080e00c8f90 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -141,6 +141,27 @@ static void init_vmcs_shadow_fields(void)
> max_shadow_read_write_fields = j;
> }
>
> +static inline bool nested_evmcs_is_used(struct vcpu_vmx *vmx)
> +{
> + struct kvm_vcpu *vcpu = &vmx->vcpu;
> +
> + if (vmx->nested.hv_evmcs)
> + return true;
> +
> + /*
> + * After KVM_SET_NESTED_STATE, enlightened VMCS is mapped during
> + * KVM_REQ_GET_NESTED_STATE_PAGES handling and until the request is
> + * processed vmx->nested.hv_evmcs is NULL. It is, however, possible to
> + * detect such state by checking 'nested.current_vmptr == -1ull' when
> + * vCPU is in guest mode, it is only possible with eVMCS.
> + */
> + if (unlikely(vmx->nested.enlightened_vmcs_enabled && is_guest_mode(vcpu) &&
> + (vmx->nested.current_vmptr == -1ull)))
> + return true;
> +
> + return false;
> +}
I think that this is a valid way to solve the issue,
but it feels like there might be a better way.
I don't mind though to accept this patch as is.
So here are my 2 cents about this:
First of all after studying how evmcs works I take my words back
about needing to migrate its contents.
It is indeed enough to migrate its physical address,
or maybe even just a flag that evmcs is loaded
(and to my surprise we already do this - KVM_STATE_NESTED_EVMCS)
So how about just having a boolean flag that indicates that evmcs is in use,
but doesn't imply that we know its address or that it is mapped
to host address space, something like 'vmx->nested.enlightened_vmcs_loaded'
On migration that flag saved and restored as the KVM_STATE_NESTED_EVMCS,
otherwise it set when we load an evmcs and cleared when it is released.
Then as far as I can see we can use this flag in nested_evmcs_is_used
since all its callers don't touch evmcs, thus don't need it to be
mapped.
What do you think?
Best regards,
Maxim Levitsky
> /*
> * The following 3 functions, nested_vmx_succeed()/failValid()/failInvalid(),
> * set the success or error code of an emulated VMX instruction (as specified
> @@ -187,7 +208,7 @@ static int nested_vmx_fail(struct kvm_vcpu *vcpu, u32 vm_instruction_error)
> * failValid writes the error number to the current VMCS, which
> * can't be done if there isn't a current VMCS.
> */
> - if (vmx->nested.current_vmptr == -1ull && !vmx->nested.hv_evmcs)
> + if (vmx->nested.current_vmptr == -1ull && !nested_evmcs_is_used(vmx))
> return nested_vmx_failInvalid(vcpu);
>
> return nested_vmx_failValid(vcpu, vm_instruction_error);
> @@ -2208,7 +2229,7 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
> u32 exec_control;
> u64 guest_efer = nested_vmx_calc_efer(vmx, vmcs12);
>
> - if (vmx->nested.dirty_vmcs12 || vmx->nested.hv_evmcs)
> + if (vmx->nested.dirty_vmcs12 || nested_evmcs_is_used(vmx))
> prepare_vmcs02_early_rare(vmx, vmcs12);
>
> /*
> @@ -3437,7 +3458,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
>
> load_vmcs12_host_state(vcpu, vmcs12);
> vmcs12->vm_exit_reason = exit_reason.full;
> - if (enable_shadow_vmcs || vmx->nested.hv_evmcs)
> + if (enable_shadow_vmcs || nested_evmcs_is_used(vmx))
> vmx->nested.need_vmcs12_to_shadow_sync = true;
> return NVMX_VMENTRY_VMEXIT;
> }
> @@ -4032,7 +4053,7 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> - if (vmx->nested.hv_evmcs)
> + if (nested_evmcs_is_used(vmx))
> sync_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
>
> vmx->nested.need_sync_vmcs02_to_vmcs12_rare = !vmx->nested.hv_evmcs;
> @@ -6056,7 +6077,7 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
> if (vmx_has_valid_vmcs12(vcpu)) {
> kvm_state.size += sizeof(user_vmx_nested_state->vmcs12);
>
> - if (vmx->nested.hv_evmcs)
> + if (nested_evmcs_is_used(vmx))
> kvm_state.flags |= KVM_STATE_NESTED_EVMCS;
>
> if (is_guest_mode(vcpu) &&
