Re: [PATCH] KVM: nVMX: Skip IBPB when switching between vmcs01 and vmcs02

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

On Thu, Apr 30, 2020 at 11:22:20PM +0200, Alexander Graf wrote:
> 
> On 30.04.20 22:41, Sean Christopherson wrote:
> >
> >diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> >index 3ab6ca6062ce..818dd8ba5e9f 100644
> >--- a/arch/x86/kvm/vmx/vmx.c
> >+++ b/arch/x86/kvm/vmx/vmx.c
> >@@ -1311,10 +1311,12 @@ static void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
> >                 pi_set_on(pi_desc);
> >  }
> >
> >-void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu)
> >+void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu,
> >+                       struct loaded_vmcs *buddy)
> >  {
> >         struct vcpu_vmx *vmx = to_vmx(vcpu);
> >         bool already_loaded = vmx->loaded_vmcs->cpu == cpu;
> >+       struct vmcs *prev;
> >
> >         if (!already_loaded) {
> >                 loaded_vmcs_clear(vmx->loaded_vmcs);
> >@@ -1333,10 +1335,12 @@ void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu)
> >                 local_irq_enable();
> >         }
> >
> >-       if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
> >+       prev = per_cpu(current_vmcs, cpu);
> >+       if (prev != vmx->loaded_vmcs->vmcs) {
> >                 per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
> >                 vmcs_load(vmx->loaded_vmcs->vmcs);
> >-               indirect_branch_prediction_barrier();
> >+               if (!buddy || buddy->vmcs != prev)
> >+                       indirect_branch_prediction_barrier();
> 
> I fail to understand the logic here though. What exactly are you trying to
> catch? We only do the barrier when the current_vmcs as loaded by
> vmx_vcpu_load_vmcs is different from the vmcs of the context that was
> issuing the vmcs load.
> 
> Isn't this a really complicated way to say "Don't flush for nested"? Why not
> just make it explicit and pass in a bool that says "nested = true" from
> vmx_switch_vmcs()? Is there any case I'm missing where that would be unsafe?o

I don't think so, the 'buddy' check was added out of paranoia, and partly
because I was rushing.  I originally had a 'bool nested_switch' as well.

I think I like the boolean approach better, the above check should never
fail in the nested switch case.  I'll add a WARN in vmx_switch_vmcs() with
a note in the patch to let Paolo know it's likely paranoia and can be
ripped out at his discretion.