Re: [Xen-devel] [PATCH] VMX: sync CPU state upon vCPU destruction

[Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>]

On Thu, 2017-11-09 at 07:49 -0700, Jan Beulich wrote:
> See the code comment being added for why we need this.
> 
> Reported-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -479,7 +479,13 @@ static void vmx_vcpu_destroy(struct vcpu
>       * we should disable PML manually here. Note that vmx_vcpu_destroy is 
> called
>       * prior to vmx_domain_destroy so we need to disable PML for each vcpu
>       * separately here.
> +     *
> +     * Before doing that though, flush all state for the vCPU previously 
> having
> +     * run on the current CPU, so that this flushing of state won't happen 
> from
> +     * the TLB flush IPI handler behind the back of a vmx_vmcs_enter() /
> +     * vmx_vmcs_exit() section.
>       */
> +    sync_local_execstate();
>      vmx_vcpu_disable_pml(v);
>      vmx_destroy_vmcs(v);
>      passive_domain_destroy(v);

This patch fixes only one particular issue and not the general problem.
What if vmcs is cleared, possibly in some future code, at another place?

The original intent of vmx_vmcs_reload() is correct: it lazily loads
the vmcs when it's needed. It's just the logic which checks for
v->is_running inside vmx_ctxt_switch_from() is flawed: v might be
"running" on another pCPU.

IMHO there are 2 possible solutions:

    1. Add additional pCPU check into vmx_ctxt_switch_from()
    2. Drop v->is_running check inside vmx_ctxt_switch_from() making
       vmx_vmcs_reload() unconditional.

Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel