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@xxxxxxxxx>
> > ---
> > 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
