On Thu, Nov 14, 2019 at 12:58:56PM +0100, Paolo Bonzini wrote:
> Ok, it's not _so_ ugly after all.
>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 39 +++++++++++++++++++++++++++++++++++++--
> > include/linux/kvm_host.h | 1 +
> > 2 files changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 5d21a4a..5c67061 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -5924,7 +5924,9 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
> > }
> > }
> >
> > - if (exit_reason < kvm_vmx_max_exit_handlers
> > + if (vcpu->fast_vmexit)
> > + return 1;
> > + else if (exit_reason < kvm_vmx_max_exit_handlers
>
> Instead of a separate vcpu->fast_vmexit, perhaps you can set exit_reason
> to vmx->exit_reason to -1 if the fast path succeeds.
Actually, rather than make this super special case, what about moving the
handling into vmx_handle_exit_irqoff()? Practically speaking that would
only add ~50 cycles (two VMREADs) relative to the code being run right
after kvm_put_guest_xcr0(). It has the advantage of restoring the host's
hardware breakpoints, preserving a semi-accurate last_guest_tsc, and
running with vcpu->mode set back to OUTSIDE_GUEST_MODE. Hopefully it'd
also be more intuitive for people unfamiliar with the code.
>
> > + if (ret == 0)
> > + ret = kvm_skip_emulated_instruction(vcpu);
>
> Please move the "kvm_skip_emulated_instruction(vcpu)" to
> vmx_handle_exit, so that this basically is
>
> #define EXIT_REASON_NEED_SKIP_EMULATED_INSN -1
>
> if (ret == 0)
> vcpu->exit_reason = EXIT_REASON_NEED_SKIP_EMULATED_INSN;
>
> and handle_ipi_fastpath can return void.
I'd rather we add a dedicated variable to say the exit has already been
handled. Overloading exit_reason is bound to cause confusion, and that's
probably a best case scenario.
> > + };
> > + };
> > +
> > + return ret;
> > +}
> > +
> > static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
> > {
> > struct vcpu_vmx *vmx = to_vmx(vcpu);
> > @@ -6615,6 +6645,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
> > | (1 << VCPU_EXREG_CR3));
> > vcpu->arch.regs_dirty = 0;
> >
> > + vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> > + vcpu->fast_vmexit = false;
> > + if (!is_guest_mode(vcpu) &&
> > + vmx->exit_reason == EXIT_REASON_MSR_WRITE)
> > + vcpu->fast_vmexit = handle_ipi_fastpath(vcpu);
>
> This should be done later, at least after kvm_put_guest_xcr0, because
> running with partially-loaded guest state is harder to audit. The best
> place to put it actually is right after the existing vmx->exit_reason
> assignment, where we already handle EXIT_REASON_MCE_DURING_VMENTRY.
>
> > pt_guest_exit(vmx);
> >
> > /*
> > @@ -6634,7 +6670,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
> > vmx->nested.nested_run_pending = 0;
> > vmx->idt_vectoring_info = 0;
> >
> > - vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> > if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
> > kvm_machine_check();
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 719fc3e..7a7358b 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -319,6 +319,7 @@ struct kvm_vcpu {
> > #endif
> > bool preempted;
> > bool ready;
> > + bool fast_vmexit;
> > struct kvm_vcpu_arch arch;
> > struct dentry *debugfs_dentry;
> > };
> >
>
