Hi Sean,

On Thu, 2021-05-20 at 15:32 +0000, Sean Christopherson wrote:
> On Wed, May 19, 2021, Stefano De Venuto wrote:
> > 
> > This patch moves the tracepoints closer to the events, for both
> > Intel
> > and AMD, so that a combined host-guest trace will offer a more
> > realistic representation of what is really happening, as shown
> > here:
> > 
> >            trace.dat:  CPU 0/KVM-2553  [000]  2.190290: write_msr:
> > 48, value 0
> 
> I'm not sure this is a good thing, as it's not clear to me that
> invoking tracing
> with the guest's SPEC_CTRL loaded is desirable.  Maybe it's a non-
> issue, but it
> should be explicitly called out and discussed.
> 
Oh, this is actually a very good point. Considering the rest of the
review comments, it looks like we're not putting the tracepoint past
the SPEC_CTRL msr-write, not for now at least. :-) But I agree that it
must be explicitly considered and thought about, if/when trying to do
it again.

> And to some extent, the current behavior is _more_ accurate because
> it shows that
> KVM started its VM-Enter sequence and then the WRMSR occured as part
> of that
> sequence.  It is writing the guest's value after all.  Ditto for
> XCR0, XSS, PKRU,
> Intel PT, etc...
> 
Yaah, I guess it comes to what we want/assume the meaning of the
VMEnter/Exit tracepoints to be. I.e., is it the beginning of the
sequence of operations necessary to enter a guest, or the exact point
in time where we switch to it (and vice-versa, for exit)?

In my view and in my experience of trying to trace host and guest at
the same time, I find the latter more useful, but I appreciate that the
former is valid too especially considering that, as you say, some
operations are altering the guest's state already.

> A more concrete example would be perf; on VMX, if a perf NMI happens
> after KVM
> invokes atomic_switch_perf_msrs() then I absolutely want to see that
> reflected
> in the trace, e.g. to help debug the PEBS mess[*].  If the VM-Enter
> tracepoint
> is moved closer to VM-Enter, that may or may not hold true depending
> on where the
> NMI lands.
> 
> On VMX, I think the tracepoint can be moved below the VMWRITEs
> without much
> contention (though doing so is likely a nop), but moving it below
> kvm_load_guest_xsave_state() requires a bit more discussion.
> 
Ok, well, IMO closer is better, even if no past XSAVE state handling.
:-) Let us look into this a little bit.

> I 100% agree that the current behavior can be a bit confusing, but I
> wonder if
> we'd be better off "solving" that problem through documentation.
> 
Indeed. So, do you happen to have in mind what could be the best place
and the best way for documenting this?

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)