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

From: "Michał Leszczyński" <michal.leszczynski@cert.pl>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	luwei kang <luwei.kang@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v1 7/7] x86/vmx: switch IPT MSRs on vmentry/vmexit
Date: Thu, 18 Jun 2020 13:07:33 +0200 (CEST)	[thread overview]
Message-ID: <633218159.9539851.1592478453009.JavaMail.zimbra@cert.pl> (raw)
In-Reply-To: <20200618085208.GG735@Air-de-Roger>

----- 18 cze 2020 o 10:52, Roger Pau Monné roger.pau@citrix.com napisał(a):

> On Wed, Jun 17, 2020 at 08:56:57PM +0200, Michał Leszczyński wrote:
>> ----- 17 cze 2020 o 17:14, Andrew Cooper andrew.cooper3@citrix.com 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@citrix.com 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?

Assuming that there is a single DomU per pcpu and they are never migrated between pcpus then you never need to modify the remaining MSRs.

In case DomUs are floating or there are multiple DomUs per pcpu, we need to read out a few MSRs on vm-exit and restore them on vm-entry. Right now I'm always using this approach as I'm pretty not sure how to optimize it without introducing additional bugs. I will show the implementation in patch v2.

> 
>> 
>> 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.