Re: [PATCH v1 7/7] x86/vmx: switch IPT MSRs on vmentry/vmexit

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 17, 2020 at 08:56:57PM +0200, Michał Leszczyński wrote:
> ----- 17 cze 2020 o 17:14, Andrew Cooper andrew.cooper3@xxxxxxxxxx napisał(a):
> 
> > On 17/06/2020 13:51, Roger Pau Monné wrote:
> >> On Wed, Jun 17, 2020 at 01:54:45PM +0200, Michał Leszczyński wrote:
> >>> ----- 17 cze 2020 o 11:09, Roger Pau Monné roger.pau@xxxxxxxxxx 
> >>> napisał(a):
> >>>
> >>>> 24 Virtual Machine Control Structures -> 24.8 VM-entry Control Fields -> 
> >>>> 24.8.1
> >>>> VM-Entry Controls
> >>>> Software should consult the VMX capability MSRs IA32_VMX_ENTRY_CTLS to 
> >>>> determine
> >>>> how it should set the reserved bits.
> >>> Please look at bit position 18 "Load IA32_RTIT_CTL".
> >> I think this is something different from what I was referring to.
> >> Those options you refer to (load/clear IA32_RTIT_CTL) deal with
> >> loading/storing a specific field on the vmcs that maps to the guest
> >> IA32_RTIT_CTL.
> >>
> >> OTOH MSR load lists can be used to load and store any arbitrary MSR on
> >> vmentry/vmexit, see section 26.4 LOADING MSRS on the SDM. There's
> >> already infrastructure on Xen to do so, see vmx_{add/del/find}_msr.
> > 
> > If I remember the historic roadmaps correctly, there are 3 cases.
> > 
> > The first hardware to support PT (Broadwell?) prohibited its use
> > completely in VMX operations.  In this case, we can use it to trace PV
> > guests iff we don't enable VMX in hardware to begin with.
> > 
> > This was relaxed in later hardware (Skylake?) to permit use within VMX
> > operations, but without any help in the VMCS.  (i.e. manual context
> > switching per this patch, or MSR load lists as noted in the SDM.)
> > 
> > Subsequent support for "virtualised PT" was added (IceLake?) which adds
> > the load/save controls, and the ability to translate the output buffer
> > under EPT.
> > 
> > 
> > All of this is from memory so I'm quite possibly wrong with details, but
> > I believe this is why the current complexity exists.
> > 
> > ~Andrew
> 
> 
> I've managed to toggle MSR_IA32_RTIT_CTL values using MSR load lists, as in:
> 
> > 35.5.2.2 Guest-Only Tracing
> > "For this usage, VM-entry is programmed to enable trace packet generation, 
> > while VM-exit is programmed to clear MSR_IA32_RTIT_CTL.TraceEn so as to 
> > disable trace-packet generation in the host."
> 
> it actually helped a bit. With patch v1 there were parts of hypervisor 
> recorded in the trace (i.e. the moment between TRACE_EN being set and actual 
> vmenter, and the moment between vmexit and TRACE_EN being unset). Using MSR 
> load list this was eliminated. This change will be reflected in patch v2.
> 
> 
> I can't however implement any working scenario in which all these MSRs are 
> managed using MSR load lists. As in "35.3.3 Flushing Trace Output": packets 
> are buffered internally and are flushed only when TRACE_EN bit in 
> MSR_IA32_RTIT_CTL is set to 0. The values of remaining registers will be 
> stable after everything is serialized. I think this is too complex for the 
> load lists alone. I belive that currently SDM instructs to use load lists 
> only for toggling this single bit on-or-off.

I think that's exactly what we want: handling TraceEn at
vmentry/vmexit, so that no hypervisor packets are recorded. The rest
of the MSRs can be handled in VMM mode without issues. Switching those
on every vmentry/vmexit would also add more overhead that needed,
since I assume they don't need to be modified on every entry/exit?

> 
> Thus, for now I propose to stay with MSR_IA32_RTIT_CTL being managed by MSR 
> load lists and the rest of related MSRs being managed manually.

Yes, that' seems like a good approach.

Roger.