Re: [RFC PATCH v2 1/1] kvm: Add documentation and ABI/API header for VM introspection

["Mihai Donțu" <mdontu@bitdefender.com>]

On Tue, 2017-07-11 at 18:51 +0200, Paolo Bonzini wrote:
> On 11/07/2017 18:48, Adalbert Lazar wrote:
> > On Mon, 10 Jul 2017 19:03:06 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > I'm not sure what you think of removing KVMI_EVENT_ACTION_SET_REGS and
> > > more or less standardizing on actions SKIP/RETRY/ALLOW/CRASH.
> > > 
> > > The main remaining issue seems to be map/unmap.
> > 
> > Definitely, SKIP/RETRY/ALLOW/CRASH looks better, but SET_REGS helps
> > performance wise. Maybe we could have it as an optional flag for ALLOW?
> 
> Actually it makes more sense for SKIP, I think, where the introspector
> is actually doing emulation?

I'm afraid I don't undestand your question, however we were looking at
using KVM's x86 emulator rather than putting together our own, as such
software might be fun to write but they take a very long time to get
right. I'd argue that KVM's emulator has already seen a lot of
coverage.

In the future we are looking at maybe moving away from it on Intel-s,
by way of VMFUNC and #VE.

> But why is KVMI_SET_REGS slower than a set regs command followed by an
> action?

To be honest, we just looked at the Xen implementation which gates
writing back the registers to VMCS on them actually having been
changed. I just figured the less VMWRITE-s, the better. I'm also
looking over some older stats that show the registers being written
back quite often. Allow me 1-2 days to gather new ones and followup on
this thread.

> > Or at least for the hot paths?
> > 
> > Summarily, on events, besides CRASH (vs SHUTDOWN cmd) and any other
> > additional flag:
> > 
> >   * CR, MSR - ALLOW with untouched new_value will let the guest continue,
> >               but with "new_value = old_value" is a "deny"
> > 
> >   * xsetbv - ALLOW is implied
> > 
> >   * breakpoint - SKIP means the BP is processed by the introspector,
> >                  ALLOW means let the guest handle it
> > 
> >   * hypercall - ALLOW is implied
> > 
> >   * page_fault - ALLOW means emulate, RETRY means let guest re-trigger the PF,
> >                  ALLOW with adjusted PC is a "skip" (done by the tool
> >                  for the moment).
> 
> This is more complicated than necessary.  I would just make it simple:
> 
> - SKIP, adjust RIP to point to the next instruction and enter the guest
> 
> - RETRY, enter the guest
> 
> - ALLOW, emulate the instruction with information coming from the
> response packet
> 
> - CRASH, self-explanatory

I see no major problem with your version. The reason we removed SKIP
from the #PF description is that the RIP adjustment is done by the
introspection tool after it decodes the instruction and computes its
opcode length. So when the event response reaches KVM, it all looks
like ALLOW was requested as the host-side code would be oblivous to the
RIP change.

-- 
Mihai Donțu