linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM: VMX: Disable Intel PT before VM-entry
@ 2020-03-18  3:48 Luwei Kang
  2020-03-18 15:48 ` Sean Christopherson
  0 siblings, 1 reply; 9+ messages in thread
From: Luwei Kang @ 2020-03-18  3:48 UTC (permalink / raw)
  To: kvm, linux-kernel
  Cc: pbonzini, sean.j.christopherson, vkuznets, wanpengli, jmattson,
	joro, Luwei Kang

If the logical processor is operating with Intel PT enabled (
IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the “load
IA32_RTIT_CTL” VM-entry control must be 0(SDM 26.2.1.1).

The first disabled the host Intel PT(Clear TraceEn) will make all the
buffered packets are flushed out of the processor and it may cause
an Intel PT PMI. The host Intel PT will be re-enabled in the host Intel
PT PMI handler.

handle_pmi_common()
    -> intel_pt_interrupt()
            -> pt_config_start()

This patch will disable the Intel PT twice to make sure the Intel PT
is disabled before VM-Entry.

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 26f8f31..d936a91 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1095,6 +1095,8 @@ static inline void pt_save_msr(struct pt_ctx *ctx, u32 addr_range)
 
 static void pt_guest_enter(struct vcpu_vmx *vmx)
 {
+	u64 rtit_ctl;
+
 	if (pt_mode == PT_MODE_SYSTEM)
 		return;
 
@@ -1103,8 +1105,14 @@ static void pt_guest_enter(struct vcpu_vmx *vmx)
 	 * Save host state before VM entry.
 	 */
 	rdmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
-	if (vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) {
+	if (vmx->pt_desc.host.ctl & RTIT_CTL_TRACEEN) {
 		wrmsrl(MSR_IA32_RTIT_CTL, 0);
+		rdmsrl(MSR_IA32_RTIT_CTL, rtit_ctl);
+		if (rtit_ctl)
+			wrmsrl(MSR_IA32_RTIT_CTL, 0);
+	}
+
+	if (vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) {
 		pt_save_msr(&vmx->pt_desc.host, vmx->pt_desc.addr_range);
 		pt_load_msr(&vmx->pt_desc.guest, vmx->pt_desc.addr_range);
 	}
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Disable Intel PT before VM-entry
  2020-03-18  3:48 [PATCH] KVM: VMX: Disable Intel PT before VM-entry Luwei Kang
@ 2020-03-18 15:48 ` Sean Christopherson
  2020-03-19  6:13   ` Kang, Luwei
  2020-03-23  9:20   ` Kang, Luwei
  0 siblings, 2 replies; 9+ messages in thread
From: Sean Christopherson @ 2020-03-18 15:48 UTC (permalink / raw)
  To: Luwei Kang
  Cc: kvm, linux-kernel, pbonzini, vkuznets, wanpengli, jmattson, joro

On Wed, Mar 18, 2020 at 11:48:18AM +0800, Luwei Kang wrote:
> If the logical processor is operating with Intel PT enabled (
> IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the “load
> IA32_RTIT_CTL” VM-entry control must be 0(SDM 26.2.1.1).
> 
> The first disabled the host Intel PT(Clear TraceEn) will make all the
> buffered packets are flushed out of the processor and it may cause
> an Intel PT PMI. The host Intel PT will be re-enabled in the host Intel
> PT PMI handler.
> 
> handle_pmi_common()
>     -> intel_pt_interrupt()
>             -> pt_config_start()

IIUC, this is only possible when PT "plays nice" with VMX, correct?
Otherwise pt->vmx_on will be true and pt_config_start() would skip the
WRMSR.

And IPT PMI must be delivered via NMI (though maybe they're always
delivered via NMI?).

In any case, redoing WRMSR doesn't seem safe, and it certainly isn't
performant, e.g. what prevents the second WRMSR from triggering a second
IPT PMI?

pt_guest_enter() is called after the switch to the vCPU has already been
recorded, can't this be handled in the IPT code, e.g. something like this?

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 1db7a51d9792..e38ddae9f0d1 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -405,7 +405,7 @@ static void pt_config_start(struct perf_event *event)
        ctl |= RTIT_CTL_TRACEEN;
        if (READ_ONCE(pt->vmx_on))
                perf_aux_output_flag(&pt->handle, PERF_AUX_FLAG_PARTIAL);
-       else
+       else (!(current->flags & PF_VCPU))
                wrmsrl(MSR_IA32_RTIT_CTL, ctl);

        WRITE_ONCE(event->hw.config, ctl);

> This patch will disable the Intel PT twice to make sure the Intel PT
> is disabled before VM-Entry.
> 
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 26f8f31..d936a91 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1095,6 +1095,8 @@ static inline void pt_save_msr(struct pt_ctx *ctx, u32 addr_range)
>  
>  static void pt_guest_enter(struct vcpu_vmx *vmx)
>  {
> +	u64 rtit_ctl;
> +
>  	if (pt_mode == PT_MODE_SYSTEM)
>  		return;
>  
> @@ -1103,8 +1105,14 @@ static void pt_guest_enter(struct vcpu_vmx *vmx)
>  	 * Save host state before VM entry.
>  	 */
>  	rdmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
> -	if (vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) {
> +	if (vmx->pt_desc.host.ctl & RTIT_CTL_TRACEEN) {
>  		wrmsrl(MSR_IA32_RTIT_CTL, 0);
> +		rdmsrl(MSR_IA32_RTIT_CTL, rtit_ctl);
> +		if (rtit_ctl)
> +			wrmsrl(MSR_IA32_RTIT_CTL, 0);
> +	}
> +
> +	if (vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) {
>  		pt_save_msr(&vmx->pt_desc.host, vmx->pt_desc.addr_range);
>  		pt_load_msr(&vmx->pt_desc.guest, vmx->pt_desc.addr_range);
>  	}
> -- 
> 1.8.3.1
> 

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [PATCH] KVM: VMX: Disable Intel PT before VM-entry
  2020-03-18 15:48 ` Sean Christopherson
@ 2020-03-19  6:13   ` Kang, Luwei
  2020-03-23  9:20   ` Kang, Luwei
  1 sibling, 0 replies; 9+ messages in thread
From: Kang, Luwei @ 2020-03-19  6:13 UTC (permalink / raw)
  To: Christopherson, Sean J
  Cc: kvm, linux-kernel, pbonzini, vkuznets, wanpengli, jmattson, joro

> > If the logical processor is operating with Intel PT enabled (
> > IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the “load
> > IA32_RTIT_CTL” VM-entry control must be 0(SDM 26.2.1.1).
> >
> > The first disabled the host Intel PT(Clear TraceEn) will make all the
> > buffered packets are flushed out of the processor and it may cause an
> > Intel PT PMI. The host Intel PT will be re-enabled in the host Intel
> > PT PMI handler.
> >
> > handle_pmi_common()
> >     -> intel_pt_interrupt()
> >             -> pt_config_start()
> 
> IIUC, this is only possible when PT "plays nice" with VMX, correct?
> Otherwise pt->vmx_on will be true and pt_config_start() would skip the
> WRMSR.

Yes. The pt_config_start() would skip the WRMSR if the HW doesn't support enable PT across the VMX.

> 
> And IPT PMI must be delivered via NMI (though maybe they're always
> delivered via NMI?).

Yes, IPT PMI is an NMI in the host.

> 
> In any case, redoing WRMSR doesn't seem safe, and it certainly isn't
> performant, e.g. what prevents the second WRMSR from triggering a second
> IPT PMI?

Twice is enough. There is probably a lot of PT trace not flush out and a PMI will happen after the first WRMSR. There are not so much data between the end of the PMI handler and the second WRMSR.

> 
> pt_guest_enter() is called after the switch to the vCPU has already been
> recorded, can't this be handled in the IPT code, e.g. something like this?
> 
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c index
> 1db7a51d9792..e38ddae9f0d1 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -405,7 +405,7 @@ static void pt_config_start(struct perf_event *event)
>         ctl |= RTIT_CTL_TRACEEN;
>         if (READ_ONCE(pt->vmx_on))
>                 perf_aux_output_flag(&pt->handle, PERF_AUX_FLAG_PARTIAL);
> -       else
> +       else (!(current->flags & PF_VCPU))
>                 wrmsrl(MSR_IA32_RTIT_CTL, ctl);
> 
>         WRITE_ONCE(event->hw.config, ctl);

Thanks. But I am afraid the host perf don’t like any virtualization specific changes. I will try this.

Luwei Kang

> 
> > This patch will disable the Intel PT twice to make sure the Intel PT
> > is disabled before VM-Entry.
> >
> > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index
> > 26f8f31..d936a91 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -1095,6 +1095,8 @@ static inline void pt_save_msr(struct pt_ctx
> > *ctx, u32 addr_range)
> >
> >  static void pt_guest_enter(struct vcpu_vmx *vmx)  {
> > +	u64 rtit_ctl;
> > +
> >  	if (pt_mode == PT_MODE_SYSTEM)
> >  		return;
> >
> > @@ -1103,8 +1105,14 @@ static void pt_guest_enter(struct vcpu_vmx *vmx)
> >  	 * Save host state before VM entry.
> >  	 */
> >  	rdmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
> > -	if (vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) {
> > +	if (vmx->pt_desc.host.ctl & RTIT_CTL_TRACEEN) {
> >  		wrmsrl(MSR_IA32_RTIT_CTL, 0);
> > +		rdmsrl(MSR_IA32_RTIT_CTL, rtit_ctl);
> > +		if (rtit_ctl)
> > +			wrmsrl(MSR_IA32_RTIT_CTL, 0);
> > +	}
> > +
> > +	if (vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) {
> >  		pt_save_msr(&vmx->pt_desc.host, vmx-
> >pt_desc.addr_range);
> >  		pt_load_msr(&vmx->pt_desc.guest, vmx-
> >pt_desc.addr_range);
> >  	}
> > --
> > 1.8.3.1
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] KVM: VMX: Disable Intel PT before VM-entry
  2020-03-18 15:48 ` Sean Christopherson
  2020-03-19  6:13   ` Kang, Luwei
@ 2020-03-23  9:20   ` Kang, Luwei
  2020-03-30 17:21     ` Sean Christopherson
  1 sibling, 1 reply; 9+ messages in thread
From: Kang, Luwei @ 2020-03-23  9:20 UTC (permalink / raw)
  To: Christopherson, Sean J
  Cc: kvm, linux-kernel, pbonzini, vkuznets, wanpengli, jmattson, joro

> On Wed, Mar 18, 2020 at 11:48:18AM +0800, Luwei Kang wrote:
> > If the logical processor is operating with Intel PT enabled (
> > IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the “load
> > IA32_RTIT_CTL” VM-entry control must be 0(SDM 26.2.1.1).
> >
> > The first disabled the host Intel PT(Clear TraceEn) will make all the
> > buffered packets are flushed out of the processor and it may cause an
> > Intel PT PMI. The host Intel PT will be re-enabled in the host Intel
> > PT PMI handler.
> >
> > handle_pmi_common()
> >     -> intel_pt_interrupt()
> >             -> pt_config_start()
> 
> IIUC, this is only possible when PT "plays nice" with VMX, correct?
> Otherwise pt->vmx_on will be true and pt_config_start() would skip the
> WRMSR.
> 
> And IPT PMI must be delivered via NMI (though maybe they're always
> delivered via NMI?).
> 
> In any case, redoing WRMSR doesn't seem safe, and it certainly isn't
> performant, e.g. what prevents the second WRMSR from triggering a second
> IPT PMI?
> 
> pt_guest_enter() is called after the switch to the vCPU has already been
> recorded, can't this be handled in the IPT code, e.g. something like this?
> 
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c index
> 1db7a51d9792..e38ddae9f0d1 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -405,7 +405,7 @@ static void pt_config_start(struct perf_event *event)
>         ctl |= RTIT_CTL_TRACEEN;
>         if (READ_ONCE(pt->vmx_on))
>                 perf_aux_output_flag(&pt->handle, PERF_AUX_FLAG_PARTIAL);
> -       else
> +       else (!(current->flags & PF_VCPU))
>                 wrmsrl(MSR_IA32_RTIT_CTL, ctl);

Intel PT can work in SYSTEM and HOST_GUEST mode by setting the kvm-intel.ko parameter "pt_mode".
In SYSTEM mode, the host and guest PT trace will be saved in the host buffer. The KVM do nothing during VM-entry/exit in SYSTEM mode and Intel PT PMI may happened on any place. The PT trace may be disabled when running in KVM(PT only needs to be disabled before VM-entry in HOST_GUEST mode).

Thanks,
Luwei Kang

> 
>         WRITE_ONCE(event->hw.config, ctl);
> 
> > This patch will disable the Intel PT twice to make sure the Intel PT
> > is disabled before VM-Entry.
> >
> > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index
> > 26f8f31..d936a91 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -1095,6 +1095,8 @@ static inline void pt_save_msr(struct pt_ctx
> > *ctx, u32 addr_range)
> >
> >  static void pt_guest_enter(struct vcpu_vmx *vmx)  {
> > +	u64 rtit_ctl;
> > +
> >  	if (pt_mode == PT_MODE_SYSTEM)
> >  		return;
> >
> > @@ -1103,8 +1105,14 @@ static void pt_guest_enter(struct vcpu_vmx *vmx)
> >  	 * Save host state before VM entry.
> >  	 */
> >  	rdmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
> > -	if (vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) {
> > +	if (vmx->pt_desc.host.ctl & RTIT_CTL_TRACEEN) {
> >  		wrmsrl(MSR_IA32_RTIT_CTL, 0);
> > +		rdmsrl(MSR_IA32_RTIT_CTL, rtit_ctl);
> > +		if (rtit_ctl)
> > +			wrmsrl(MSR_IA32_RTIT_CTL, 0);
> > +	}
> > +
> > +	if (vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) {
> >  		pt_save_msr(&vmx->pt_desc.host, vmx-
> >pt_desc.addr_range);
> >  		pt_load_msr(&vmx->pt_desc.guest, vmx-
> >pt_desc.addr_range);
> >  	}
> > --
> > 1.8.3.1
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Disable Intel PT before VM-entry
  2020-03-23  9:20   ` Kang, Luwei
@ 2020-03-30 17:21     ` Sean Christopherson
  2020-03-31  3:29       ` Kang, Luwei
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Christopherson @ 2020-03-30 17:21 UTC (permalink / raw)
  To: Kang, Luwei
  Cc: kvm, linux-kernel, pbonzini, vkuznets, wanpengli, jmattson, joro

On Mon, Mar 23, 2020 at 02:20:01AM -0700, Kang, Luwei wrote:
> > On Wed, Mar 18, 2020 at 11:48:18AM +0800, Luwei Kang wrote:
> > > If the logical processor is operating with Intel PT enabled (
> > > IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the “load
> > > IA32_RTIT_CTL” VM-entry control must be 0(SDM 26.2.1.1).
> > >
> > > The first disabled the host Intel PT(Clear TraceEn) will make all the
> > > buffered packets are flushed out of the processor and it may cause an
> > > Intel PT PMI. The host Intel PT will be re-enabled in the host Intel
> > > PT PMI handler.
> > >
> > > handle_pmi_common()
> > >     -> intel_pt_interrupt()
> > >             -> pt_config_start()
> > 
> > IIUC, this is only possible when PT "plays nice" with VMX, correct?
> > Otherwise pt->vmx_on will be true and pt_config_start() would skip the
> > WRMSR.
> > 
> > And IPT PMI must be delivered via NMI (though maybe they're always
> > delivered via NMI?).
> > 
> > In any case, redoing WRMSR doesn't seem safe, and it certainly isn't
> > performant, e.g. what prevents the second WRMSR from triggering a second
> > IPT PMI?
> > 
> > pt_guest_enter() is called after the switch to the vCPU has already been
> > recorded, can't this be handled in the IPT code, e.g. something like this?
> > 
> > diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c index
> > 1db7a51d9792..e38ddae9f0d1 100644
> > --- a/arch/x86/events/intel/pt.c
> > +++ b/arch/x86/events/intel/pt.c
> > @@ -405,7 +405,7 @@ static void pt_config_start(struct perf_event *event)
> >         ctl |= RTIT_CTL_TRACEEN;
> >         if (READ_ONCE(pt->vmx_on))
> >                 perf_aux_output_flag(&pt->handle, PERF_AUX_FLAG_PARTIAL);
> > -       else
> > +       else (!(current->flags & PF_VCPU))
> >                 wrmsrl(MSR_IA32_RTIT_CTL, ctl);
> 
> Intel PT can work in SYSTEM and HOST_GUEST mode by setting the kvm-intel.ko
> parameter "pt_mode".  In SYSTEM mode, the host and guest PT trace will be
> saved in the host buffer. The KVM do nothing during VM-entry/exit in SYSTEM
> mode and Intel PT PMI may happened on any place. The PT trace may be disabled
> when running in KVM(PT only needs to be disabled before VM-entry in
> HOST_GUEST mode).

Ah, right.  What about enhancing intel_pt_handle_vmx() and 'struct pt' to
replace vmx_on with a field that incorporates the KVM mode?  From an
outsider's perspective, that'd be an improvment irrespective of this bug
fix as 'vmx_on' is misleading, e.g. it can be %false when the CPU is post-
VMXON, and really means "post-VMXON and Intel PT can't trace it".

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] KVM: VMX: Disable Intel PT before VM-entry
  2020-03-30 17:21     ` Sean Christopherson
@ 2020-03-31  3:29       ` Kang, Luwei
  2020-04-09 18:34         ` Sean Christopherson
  2020-04-16 14:14         ` Paolo Bonzini
  0 siblings, 2 replies; 9+ messages in thread
From: Kang, Luwei @ 2020-03-31  3:29 UTC (permalink / raw)
  To: Christopherson, Sean J
  Cc: kvm, linux-kernel, pbonzini, vkuznets, wanpengli, jmattson, joro

> > > On Wed, Mar 18, 2020 at 11:48:18AM +0800, Luwei Kang wrote:
> > > > If the logical processor is operating with Intel PT enabled (
> > > > IA32_RTIT_CTL.TraceEn = 1) at the time of VM entry, the “load
> > > > IA32_RTIT_CTL” VM-entry control must be 0(SDM 26.2.1.1).
> > > >
> > > > The first disabled the host Intel PT(Clear TraceEn) will make all
> > > > the buffered packets are flushed out of the processor and it may
> > > > cause an Intel PT PMI. The host Intel PT will be re-enabled in the
> > > > host Intel PT PMI handler.
> > > >
> > > > handle_pmi_common()
> > > >     -> intel_pt_interrupt()
> > > >             -> pt_config_start()
> > >
> > > IIUC, this is only possible when PT "plays nice" with VMX, correct?
> > > Otherwise pt->vmx_on will be true and pt_config_start() would skip
> > > the WRMSR.
> > >
> > > And IPT PMI must be delivered via NMI (though maybe they're always
> > > delivered via NMI?).
> > >
> > > In any case, redoing WRMSR doesn't seem safe, and it certainly isn't
> > > performant, e.g. what prevents the second WRMSR from triggering a
> > > second IPT PMI?
> > >
> > > pt_guest_enter() is called after the switch to the vCPU has already
> > > been recorded, can't this be handled in the IPT code, e.g. something like
> this?
> > >
> > > diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> > > index
> > > 1db7a51d9792..e38ddae9f0d1 100644
> > > --- a/arch/x86/events/intel/pt.c
> > > +++ b/arch/x86/events/intel/pt.c
> > > @@ -405,7 +405,7 @@ static void pt_config_start(struct perf_event *event)
> > >         ctl |= RTIT_CTL_TRACEEN;
> > >         if (READ_ONCE(pt->vmx_on))
> > >                 perf_aux_output_flag(&pt->handle, PERF_AUX_FLAG_PARTIAL);
> > > -       else
> > > +       else (!(current->flags & PF_VCPU))
> > >                 wrmsrl(MSR_IA32_RTIT_CTL, ctl);
> >
> > Intel PT can work in SYSTEM and HOST_GUEST mode by setting the
> > kvm-intel.ko parameter "pt_mode".  In SYSTEM mode, the host and guest
> > PT trace will be saved in the host buffer. The KVM do nothing during
> > VM-entry/exit in SYSTEM mode and Intel PT PMI may happened on any
> > place. The PT trace may be disabled when running in KVM(PT only needs
> > to be disabled before VM-entry in HOST_GUEST mode).
> 
> Ah, right.  What about enhancing intel_pt_handle_vmx() and 'struct pt' to
> replace vmx_on with a field that incorporates the KVM mode?

Some history is the host perf didn't fully agree with introducing HOST_GUEST mode for PT in KVM. Because the KVM will disable the host trace before VM-entry in HOST_GUEST mode and KVM guest will win in this case. e.g. Intel PT has been enabled in KVM guest and the host wants to start system-wide trace(collect all the trace on this system) at this time, the trace produced by the Guest OS will be saved in guest PT buffer and host buffer can't get this. So I prefer don't introduce the KVM PT mode to host perf framework. The similar problem happens on PEBS virtualization via DS as well.

Thanks,
Luwei Kang

>  From an outsider's perspective, that'd be an improvment irrespective of this bug fix as
> 'vmx_on' is misleading, e.g. it can be %false when the CPU is post- VMXON,
> and really means "post-VMXON and Intel PT can't trace it".

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Disable Intel PT before VM-entry
  2020-03-31  3:29       ` Kang, Luwei
@ 2020-04-09 18:34         ` Sean Christopherson
  2020-04-16 14:14         ` Paolo Bonzini
  1 sibling, 0 replies; 9+ messages in thread
From: Sean Christopherson @ 2020-04-09 18:34 UTC (permalink / raw)
  To: Kang, Luwei
  Cc: kvm, linux-kernel, pbonzini, vkuznets, wanpengli, jmattson, joro

On Mon, Mar 30, 2020 at 08:29:26PM -0700, Kang, Luwei wrote:
> > > > On Wed, Mar 18, 2020 at 11:48:18AM +0800, Luwei Kang wrote:
> > Ah, right.  What about enhancing intel_pt_handle_vmx() and 'struct pt' to
> > replace vmx_on with a field that incorporates the KVM mode?
> 
> Some history is the host perf didn't fully agree with introducing HOST_GUEST
> mode for PT in KVM. Because the KVM will disable the host trace before
> VM-entry in HOST_GUEST mode and KVM guest will win in this case. e.g. Intel
> PT has been enabled in KVM guest and the host wants to start system-wide
> trace(collect all the trace on this system) at this time, the trace produced
> by the Guest OS will be saved in guest PT buffer and host buffer can't get
> this. So I prefer don't introduce the KVM PT mode to host perf framework. The
> similar problem happens on PEBS virtualization via DS as well.

A maintainer's distaste for a feature isn't a good reason to put a hack
into KVM.  Perf burying its head in the sand won't change the fact that
"pt->vmx_on" is poorly named and misleading.  Disagreement over features
is fine, but things will go sideways quick if perf and KVM are outright
hostile towards each other.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Disable Intel PT before VM-entry
  2020-03-31  3:29       ` Kang, Luwei
  2020-04-09 18:34         ` Sean Christopherson
@ 2020-04-16 14:14         ` Paolo Bonzini
  2020-04-17  1:49           ` Kang, Luwei
  1 sibling, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2020-04-16 14:14 UTC (permalink / raw)
  To: Kang, Luwei, Christopherson, Sean J
  Cc: kvm, linux-kernel, vkuznets, wanpengli, jmattson, joro

On 31/03/20 05:29, Kang, Luwei wrote:
>> Ah, right.  What about enhancing intel_pt_handle_vmx() and 'struct
>> pt' to replace vmx_on with a field that incorporates the KVM mode?
>
> Some history is the host perf didn't fully agree with introducing
> HOST_GUEST mode for PT in KVM.

I don't think this is accurate.  IIRC the maintainers wanted packets in 
the host-side trace to signal where the trace was interrupted.  In the 
end we solved the issue by 1) dropping host-only mode since it can be 
achieved in userspace 2) making host-guest an opt in feature.

I think it would make sense to rename vmx_on into vmx_state and make it an

enum pt_vmx_state {
	PT_VMX_OFF,
	PT_VMX_ON_DISABLED,
	PT_VMX_ON_SYSTEM,
	PT_VMX_ON_HOST_GUEST
};

KVM would pass the enum to intel_pt_handle_vmx (one of PT_VMX_OFF, 
PT_VMX_ON_SYSTEM, PT_VMX_ON_HOST_GUEST).  Inside intel_pt_handle_vmx you 
can do

	if (pt_pmu.vmx) {
		WRITE_ONCE(pt->vmx_state, state);
		return;
	}

	local_irq_save(flags);
	WRITE_ONCE(pt->vmx_state,
		   state == PT_VMX_OFF ? PT_VMX_OFF : PT_VMX_ON_DISABLED);
	...

and in pt_config_start:

	...
	vmx = READ_ONCE(pt->vmx_start);
	if (vmx == PT_VMX_ON_DISABLED)
                perf_aux_output_flag(&pt->handle, PERF_AUX_FLAG_PARTIAL);
        else if (vmx == PT_VMX_ON_SYSTEM ||
		 !(current->flags & PF_VCPU))
                wrmsrl(MSR_IA32_RTIT_CTL, ctl);
	...

Thanks,

Paolo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] KVM: VMX: Disable Intel PT before VM-entry
  2020-04-16 14:14         ` Paolo Bonzini
@ 2020-04-17  1:49           ` Kang, Luwei
  0 siblings, 0 replies; 9+ messages in thread
From: Kang, Luwei @ 2020-04-17  1:49 UTC (permalink / raw)
  To: Paolo Bonzini, Christopherson, Sean J
  Cc: kvm, linux-kernel, vkuznets, wanpengli, jmattson, joro

> >> Ah, right.  What about enhancing intel_pt_handle_vmx() and 'struct
> >> pt' to replace vmx_on with a field that incorporates the KVM mode?
> >
> > Some history is the host perf didn't fully agree with introducing
> > HOST_GUEST mode for PT in KVM.
> 
> I don't think this is accurate.  IIRC the maintainers wanted packets in the host-
> side trace to signal where the trace was interrupted.  In the end we solved the
> issue by 1) dropping host-only mode since it can be achieved in userspace 2)
> making host-guest an opt in feature.
> 
> I think it would make sense to rename vmx_on into vmx_state and make it an
> 
> enum pt_vmx_state {
> 	PT_VMX_OFF,
> 	PT_VMX_ON_DISABLED,
> 	PT_VMX_ON_SYSTEM,
> 	PT_VMX_ON_HOST_GUEST
> };
> 
> KVM would pass the enum to intel_pt_handle_vmx (one of PT_VMX_OFF,
> PT_VMX_ON_SYSTEM, PT_VMX_ON_HOST_GUEST).  Inside
> intel_pt_handle_vmx you can do
> 
> 	if (pt_pmu.vmx) {
> 		WRITE_ONCE(pt->vmx_state, state);
> 		return;
> 	}
> 
> 	local_irq_save(flags);
> 	WRITE_ONCE(pt->vmx_state,
> 		   state == PT_VMX_OFF ? PT_VMX_OFF :
> PT_VMX_ON_DISABLED);
> 	...
> 
> and in pt_config_start:
> 
> 	...
> 	vmx = READ_ONCE(pt->vmx_start);
> 	if (vmx == PT_VMX_ON_DISABLED)
>                 perf_aux_output_flag(&pt->handle, PERF_AUX_FLAG_PARTIAL);
>         else if (vmx == PT_VMX_ON_SYSTEM ||
> 		 !(current->flags & PF_VCPU))
>                 wrmsrl(MSR_IA32_RTIT_CTL, ctl);
> 	...

I will try this. Thanks.

Luwei Kang

> 
> Thanks,
> 
> Paolo


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-04-17  1:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-18  3:48 [PATCH] KVM: VMX: Disable Intel PT before VM-entry Luwei Kang
2020-03-18 15:48 ` Sean Christopherson
2020-03-19  6:13   ` Kang, Luwei
2020-03-23  9:20   ` Kang, Luwei
2020-03-30 17:21     ` Sean Christopherson
2020-03-31  3:29       ` Kang, Luwei
2020-04-09 18:34         ` Sean Christopherson
2020-04-16 14:14         ` Paolo Bonzini
2020-04-17  1:49           ` Kang, Luwei

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).