Re: [PATCH] kvm: nVMX: flush TLB when decoded insn != VM-exit reason

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

On Fri, Aug 14, 2020 at 09:56:33AM -0500, Oliver Upton wrote:
> On Thu, Aug 13, 2020 at 6:59 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Thu, Aug 13, 2020 at 03:44:08PM -0500, Oliver Upton wrote:
> > > On Thu, Aug 13, 2020 at 12:03 PM Sean Christopherson
> > > > > +      *
> > > > > +      * Rather than synthesizing a VM-exit into L1 for every possible
> > > > > +      * instruction just flush the TLB, resume L2, and let hardware generate
> > > > > +      * the appropriate VM-exit.
> > > > > +      */
> > > > > +     vmx_flush_tlb_gva(vcpu, kvm_rip_read(vcpu));
> > > >
> > > > This is wrong, it should flush kvm_get_linear_rip(vcpu).
> > > >
> > >
> > > I do not believe that the aim of this patch will work anymore, since:
> > >
> > > 1dbf5d68af6f ("KVM: VMX: Add guest physical address check in EPT
> > > violation and misconfig")
> > >
> > > Since it is possible to get into the emulator on any instruction that
> > > induces an EPT violation, we'd wind up looping when we believe the
> > > instruction needs to exit to L1 (TLB flush, resume guest, hit the same
> > > EPT violation. Rinse, wash, repeat).
> >
> > kvm_get_linear_rip() doesn't walk any page tables, it simply accounts for a
> > non-zero CS.base when !64-bit mode.  If we really wanted to, this could use
> > the emulation context's cached _eip, but I don't see any value in that since
> > both GUEST_CS_* and GUEST_RIP will already be cached by KVM.
> >
> > unsigned long kvm_get_linear_rip(struct kvm_vcpu *vcpu)
> > {
> >         if (is_64_bit_mode(vcpu))
> >                 return kvm_rip_read(vcpu);
> >         return (u32)(get_segment_base(vcpu, VCPU_SREG_CS) +
> >                      kvm_rip_read(vcpu));
> > }
> 
> Sorry, I was a tad imprecise. I haven't any issues with your
> suggestion. Rather, I believe that my overall patch is ineffective.
> 
> Suppose we had an EPT violation for a GPA that exceeded the guest's
> MAXPHYADDR. Let's also say that the EPT violation occurred on the
> memory operand of an LMSW instruction. Per the aforementioned patch,
> we will dive into the emulator. Since we check intercepts before
> reading the operand out of memory, we will fall through to the default
> case, set intercepted = true, flush TLB and resume.

Hrm.  The new invocation of kvm_emulate_instruction() feels incomplete from
the perspective that it doesn't have a flag that states "this should always
cause a #PF, freak out if it doesn't".  Such a flag would allow keeping this
approach as this interception logic could bail early if it is set, knowing
that the emulator will inject a #PF or bail to userspace (or something like
that).