diff mbox

[v7,05/13] perf/x86/intel/pt: Introduce a new function to get capability of Intel PT

Message ID 1525349323-9938-6-git-send-email-luwei.kang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Luwei Kang May 3, 2018, 12:08 p.m. UTC
New function __pt_cap_get() will be invoked in KVM to check
if a specific capability is availiable in KVM guest.
Another function pt_cap_get() can only check the hardware
capabilities but this may different with KVM guest because
some features may not be exposed to guest.

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 arch/x86/events/intel/pt.c      | 10 ++++++++--
 arch/x86/include/asm/intel_pt.h |  2 ++
 2 files changed, 10 insertions(+), 2 deletions(-)

Comments

Alexander Shishkin May 3, 2018, 10:50 a.m. UTC | #1
On Thu, May 03, 2018 at 08:08:35PM +0800, Luwei Kang wrote:
> New function __pt_cap_get() will be invoked in KVM to check
> if a specific capability is availiable in KVM guest.
> Another function pt_cap_get() can only check the hardware
> capabilities but this may different with KVM guest because
> some features may not be exposed to guest.

Do we really need both in KVM?

> diff --git a/arch/x86/include/asm/intel_pt.h b/arch/x86/include/asm/intel_pt.h
> index 2de4db0..3a4f524 100644
> --- a/arch/x86/include/asm/intel_pt.h
> +++ b/arch/x86/include/asm/intel_pt.h
> @@ -27,9 +27,11 @@ enum pt_capabilities {
>  #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_INTEL)
>  void cpu_emergency_stop_pt(void);
>  extern u32 pt_cap_get(enum pt_capabilities cap);
> +extern u32 __pt_cap_get(u32 *caps, enum pt_capabilities cap);

I'd call it something like pt_cap_decode().
Luwei Kang May 3, 2018, 11:04 a.m. UTC | #2
> > New function __pt_cap_get() will be invoked in KVM to check if a
> > specific capability is availiable in KVM guest.
> > Another function pt_cap_get() can only check the hardware capabilities
> > but this may different with KVM guest because some features may not be
> > exposed to guest.
> 
> Do we really need both in KVM?

Yes, KVM need get host capability to estimate if can expose this feature to guest and get guest capability to confirm if some bits of MSRs are accessible.

Thanks,
Luwei Kang

> 
> > diff --git a/arch/x86/include/asm/intel_pt.h
> > b/arch/x86/include/asm/intel_pt.h index 2de4db0..3a4f524 100644
> > --- a/arch/x86/include/asm/intel_pt.h
> > +++ b/arch/x86/include/asm/intel_pt.h
> > @@ -27,9 +27,11 @@ enum pt_capabilities {  #if
> > defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_INTEL)  void
> > cpu_emergency_stop_pt(void);  extern u32 pt_cap_get(enum
> > pt_capabilities cap);
> > +extern u32 __pt_cap_get(u32 *caps, enum pt_capabilities cap);
> 
> I'd call it something like pt_cap_decode().
Alexander Shishkin May 3, 2018, 12:13 p.m. UTC | #3
On Thu, May 03, 2018 at 11:04:52AM +0000, Kang, Luwei wrote:
> > > New function __pt_cap_get() will be invoked in KVM to check if a
> > > specific capability is availiable in KVM guest.
> > > Another function pt_cap_get() can only check the hardware capabilities
> > > but this may different with KVM guest because some features may not be
> > > exposed to guest.
> > 
> > Do we really need both in KVM?
> 
> Yes, KVM need get host capability to estimate if can expose this feature
> to guest

Can you elaborate on this, what information do we need besides
MSR_IA32_VMX_MISC[14]?

Thanks,
--
Alex
Paolo Bonzini May 3, 2018, 12:30 p.m. UTC | #4
On 03/05/2018 14:13, Alexander Shishkin wrote:
>>>> New function __pt_cap_get() will be invoked in KVM to check if a
>>>> specific capability is availiable in KVM guest.
>>>> Another function pt_cap_get() can only check the hardware capabilities
>>>> but this may different with KVM guest because some features may not be
>>>> exposed to guest.
>>> Do we really need both in KVM?
>> Yes, KVM need get host capability to estimate if can expose this feature
>> to guest
> Can you elaborate on this, what information do we need besides
> MSR_IA32_VMX_MISC[14]?

It needs all the CPUID data, and it's nice to query it in a more
manageable form than with bit shifts.

Paolo
Luwei Kang May 3, 2018, 12:30 p.m. UTC | #5
> > > > New function __pt_cap_get() will be invoked in KVM to check if a
> > > > specific capability is availiable in KVM guest.
> > > > Another function pt_cap_get() can only check the hardware
> > > > capabilities but this may different with KVM guest because some
> > > > features may not be exposed to guest.
> > >
> > > Do we really need both in KVM?
> >
> > Yes, KVM need get host capability to estimate if can expose this
> > feature to guest
> 
> Can you elaborate on this, what information do we need besides MSR_IA32_VMX_MISC[14]?

Enable Intel PT in guest depend on SECONDARY_EXEC_PT_USE_GPA, EPT, MSR_IA32_VMX_MISC[14] and the capability of " PT_CAP_topa_output" on hardware. PT driver will return " -ENODEV" if hardware not support "PT_CAP_topa_output".

Thanks,
Luwei Kang

> 
> Thanks,
> --
> Alex
Paolo Bonzini May 3, 2018, 12:32 p.m. UTC | #6
On 03/05/2018 14:30, Kang, Luwei wrote:
>> Can you elaborate on this, what information do we need besides
>> MSR_IA32_VMX_MISC[14]?
>
> Enable Intel PT in guest depend on SECONDARY_EXEC_PT_USE_GPA, EPT,
> MSR_IA32_VMX_MISC[14] and the capability of " PT_CAP_topa_output" on
> hardware. PT driver will return " -ENODEV" if hardware not support
> "PT_CAP_topa_output".

I actually don't understand why PT_CAP_topa_output matters for the
purpose of enabling PT in the guest.  However you still need
__pt_cap_get() in the CPUID checks.

Paolo
Luwei Kang May 3, 2018, 12:50 p.m. UTC | #7
> >> Can you elaborate on this, what information do we need besides

> >> MSR_IA32_VMX_MISC[14]?

> >

> > Enable Intel PT in guest depend on SECONDARY_EXEC_PT_USE_GPA, EPT,

> > MSR_IA32_VMX_MISC[14] and the capability of " PT_CAP_topa_output" on

> > hardware. PT driver will return " -ENODEV" if hardware not support

> > "PT_CAP_topa_output".

> 

> I actually don't understand why PT_CAP_topa_output matters for the purpose of enabling PT in the guest.  However you still need

> __pt_cap_get() in the CPUID checks.


Hi Paolo,
I think we should expose Intel Processor Trace to guest that can be detected and initialized. But without "PT_CAP_topa_output" Intel PT can't work in Linux.  So I add this feature as precondition. 
About why need this check in driver I think Alexander may know the reason.

Thanks,
Luwei Kang

> 

> Paolo
Alexander Shishkin May 3, 2018, 12:59 p.m. UTC | #8
On Thu, May 03, 2018 at 12:50:48PM +0000, Kang, Luwei wrote:
> > >> Can you elaborate on this, what information do we need besides
> > >> MSR_IA32_VMX_MISC[14]?
> > >
> > > Enable Intel PT in guest depend on SECONDARY_EXEC_PT_USE_GPA, EPT,
> > > MSR_IA32_VMX_MISC[14] and the capability of " PT_CAP_topa_output" on
> > > hardware. PT driver will return " -ENODEV" if hardware not support
> > > "PT_CAP_topa_output".
> > 
> > I actually don't understand why PT_CAP_topa_output matters for the purpose of enabling PT in the guest.  However you still need
> > __pt_cap_get() in the CPUID checks.
> 
> Hi Paolo,
> I think we should expose Intel Processor Trace to guest that can be detected and initialized. But without "PT_CAP_topa_output" Intel PT can't work in Linux.  So I add this feature as precondition. 
> About why need this check in driver I think Alexander may know the reason.

The driver only operates on ToPA at the moment, so if PT doesn't support
it, there's nothing for the driver to do. But other things, such as
simple_pt, might still be functional without CAP_topa_output, so it still
makes sense to virtualize it.

Regards,
--
Alex
diff mbox

Patch

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index d5819a2..a1fe6ff 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -76,14 +76,20 @@ 
 	PT_CAP(psb_periods,		1, CPUID_EBX, 0xffff0000),
 };
 
-u32 pt_cap_get(enum pt_capabilities cap)
+u32 __pt_cap_get(u32 *caps, enum pt_capabilities cap)
 {
 	struct pt_cap_desc *cd = &pt_caps[cap];
-	u32 c = pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
+	u32 c = caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
 	unsigned int shift = __ffs(cd->mask);
 
 	return (c & cd->mask) >> shift;
 }
+EXPORT_SYMBOL_GPL(__pt_cap_get);
+
+u32 pt_cap_get(enum pt_capabilities cap)
+{
+	return __pt_cap_get(pt_pmu.caps, cap);
+}
 EXPORT_SYMBOL_GPL(pt_cap_get);
 
 static ssize_t pt_cap_show(struct device *cdev,
diff --git a/arch/x86/include/asm/intel_pt.h b/arch/x86/include/asm/intel_pt.h
index 2de4db0..3a4f524 100644
--- a/arch/x86/include/asm/intel_pt.h
+++ b/arch/x86/include/asm/intel_pt.h
@@ -27,9 +27,11 @@  enum pt_capabilities {
 #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_INTEL)
 void cpu_emergency_stop_pt(void);
 extern u32 pt_cap_get(enum pt_capabilities cap);
+extern u32 __pt_cap_get(u32 *caps, enum pt_capabilities cap);
 #else
 static inline void cpu_emergency_stop_pt(void) {}
 static inline u32 pt_cap_get(enum pt_capabilities cap) { return 0; }
+static u32 __pt_cap_get(u32 *caps, enum pt_capabilities cap) { return 0; }
 #endif
 
 #endif /* _ASM_X86_INTEL_PT_H */