Re: [PATCH 06/15] KVM: nVMX: Don't "put" vCPU or host state when switching VMCS

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Thu, Jun 06, 2019 at 06:24:43PM +0200, Paolo Bonzini wrote:
> On 07/05/19 18:06, Sean Christopherson wrote:
> > When switching between vmcs01 and vmcs02, KVM isn't actually switching
> > between guest and host.  If guest state is already loaded (the likely,
> > if not guaranteed, case), keep the guest state loaded and manually swap
> > the loaded_cpu_state pointer after propagating saved host state to the
> > new vmcs0{1,2}.
> > 
> > Avoiding the switch between guest and host reduces the latency of
> > switching between vmcs01 and vmcs02 by several hundred cycles, and
> > reduces the roundtrip time of a nested VM by upwards of 1000 cycles.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 18 +++++++++++++-
> >  arch/x86/kvm/vmx/vmx.c    | 52 ++++++++++++++++++++++-----------------
> >  arch/x86/kvm/vmx/vmx.h    |  3 ++-
> >  3 files changed, 48 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index a30d53823b2e..4651d3462df4 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -241,15 +241,31 @@ static void free_nested(struct kvm_vcpu *vcpu)
> >  static void vmx_switch_vmcs(struct kvm_vcpu *vcpu, struct loaded_vmcs *vmcs)
> >  {
> >  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> > +	struct vmcs_host_state *src;
> > +	struct loaded_vmcs *prev;
> >  	int cpu;
> >  
> >  	if (vmx->loaded_vmcs == vmcs)
> >  		return;
> >  
> >  	cpu = get_cpu();
> > -	vmx_vcpu_put(vcpu);
> > +	prev = vmx->loaded_cpu_state;
> >  	vmx->loaded_vmcs = vmcs;
> >  	vmx_vcpu_load(vcpu, cpu);
> > +
> > +	if (likely(prev)) {
> > +		src = &prev->host_state;
> > +
> > +		vmx_set_host_fs_gs(&vmcs->host_state, src->fs_sel, src->gs_sel,
> > +				   src->fs_base, src->gs_base);
> > +
> > +		vmcs->host_state.ldt_sel = src->ldt_sel;
> > +#ifdef CONFIG_X86_64
> > +		vmcs->host_state.ds_sel = src->ds_sel;
> > +		vmcs->host_state.es_sel = src->es_sel;
> > +#endif
> > +		vmx->loaded_cpu_state = vmcs;
> > +	}
> >  	put_cpu();
> 
> I'd like to extract this into a separate function:
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 438fae1fef2a..83e436f201bf 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -248,34 +248,40 @@ static void free_nested(struct kvm_vcpu *vcpu)
>  	free_loaded_vmcs(&vmx->nested.vmcs02);
>  }
>  
> +static void vmx_sync_vmcs_host_state(struct vcpu_vmx *vmx)

What about taking the vmcs pointers, and using old/new instead of
prev/cur?  Calling it prev is wonky since it's pulled from the current
value of loaded_cpu_state, especially since cur is the same type.
That oddity is also why I grabbed prev before setting loaded_vmcs,
it just felt wrong even though they really are two separate things.

static void vmx_sync_vmcs_host_state(struct vcpu_vmx *vmx,
				     struct loaded_vmcs *old,
				     struct loaded_vmcs *new)
{
	...
}


{
	vmx_sync_vmcs_host_state(vmx, vmx->loaded_cpu_state, vmcs);
}

> +{
> +	struct loaded_vmcs *prev = vmx->loaded_cpu_state;
> +	struct loaded_vmcs *cur;
> +	struct vmcs_host_state *dest, *src;
> +
> +	if (unlikely(!prev))
> +		return;
> +
> +	cur = &vmx->loaded_vmcs;
> +	src = &prev->host_state;
> +	dest = &cur->host_state;
> +
> +	vmx_set_host_fs_gs(dest, src->fs_sel, src->gs_sel, src->fs_base, src->gs_base);
> +	dest->ldt_sel = src->ldt_sel;
> +#ifdef CONFIG_X86_64
> +	dest->ds_sel = src->ds_sel;
> +	dest->es_sel = src->es_sel;
> +#endif
> +	vmx->loaded_cpu_state = cur;
> +}
> +
>  static void vmx_switch_vmcs(struct kvm_vcpu *vcpu, struct loaded_vmcs *vmcs)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> -	struct vmcs_host_state *src;
> -	struct loaded_vmcs *prev;
>  	int cpu;
>  
>  	if (vmx->loaded_vmcs == vmcs)
>  		return;
>  
>  	cpu = get_cpu();
> -	prev = vmx->loaded_cpu_state;
>  	vmx->loaded_vmcs = vmcs;
>  	vmx_vcpu_load(vcpu, cpu);
> -
> -	if (likely(prev)) {
> -		src = &prev->host_state;
> -
> -		vmx_set_host_fs_gs(&vmcs->host_state, src->fs_sel, src->gs_sel,
> -				   src->fs_base, src->gs_base);
> -
> -		vmcs->host_state.ldt_sel = src->ldt_sel;
> -#ifdef CONFIG_X86_64
> -		vmcs->host_state.ds_sel = src->ds_sel;
> -		vmcs->host_state.es_sel = src->es_sel;
> -#endif
> -		vmx->loaded_cpu_state = vmcs;
> -	}
> +	vmx_sync_vmcs_host_state(vmx);
>  	put_cpu();
>  
>  	vm_entry_controls_reset_shadow(vmx);
> 
> Paolo