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.
