diff mbox series

[v12,03/12] perf/x86/intel/pt: Introduce a new function to get capability of Intel PT

Message ID 1532598182-10711-4-git-send-email-luwei.kang@intel.com (mailing list archive)
State New, archived
Headers show
Series Intel Processor Trace virtualization enabling | expand

Commit Message

Luwei Kang July 26, 2018, 9:42 a.m. UTC 
New function pt_cap_decode() will be invoked in KVM to check
if a specific capability is available 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.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 arch/x86/events/intel/pt.c      | 12 +++++++++---
 arch/x86/include/asm/intel_pt.h |  2 ++
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Thomas Gleixner Oct. 18, 2018, 9:47 a.m. UTC | #1 
On Thu, 26 Jul 2018, Luwei Kang wrote:

   perf/x86/intel/pt: Introduce a new function to get capability of Intel PT

Subjects should be short and consice.

   perf/x86/intel/pt: Introduce intel_pt_cap_decode()

Also the decsription in $subject is not what the description below
says. Please make sure that your subjects and changelogs are consistent.

> New function pt_cap_decode() will be invoked in KVM to check
> if a specific capability is available 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.

Again it took me a while to figure out what you are trying to
say. Suggestion:

  intel_pt_validate_cap() validates whether a given PT capability is
  supported by the hardware. It checks the PT capability array which
  reflects the capabilites of the hardware on which the code is executed.

  For setting up PT for KVM guests this is not correct as the capability
  array for the guest can be different from the host array.

  Provide a new function to check against a given capability array.

> -u32 pt_cap_get(enum pt_capabilities cap)
> +u32 pt_cap_decode(u32 *caps, enum pt_capabilities capability)

>  {
> -	struct pt_cap_desc *cd = &pt_caps[cap];
> -	u32 c = pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
> +	struct pt_cap_desc *cd = &pt_caps[capability];
> +	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_decode);
> +
> +u32 pt_cap_get(enum pt_capabilities cap)
> +{
> +	return pt_cap_decode(pt_pmu.caps, cap);
> +}
>  EXPORT_SYMBOL_GPL(pt_cap_get);

pt_cap_decode() is yet another misnomer.

So what you really want is:

   s/pt_cap_get()/intel_pt_validate_hw_cap()/

and the new function wants to be:

   intel_pt_validate_cap()

So it's entirely clear from the naming that the former pt_cap_get() is
related to the hardware capabilities of the machine on which the code is
executing.

Thanks,

	tglx
Luwei Kang Oct. 19, 2018, 12:58 a.m. UTC | #2 
>    perf/x86/intel/pt: Introduce a new function to get capability of Intel PT
> 
> Subjects should be short and consice.
> 
>    perf/x86/intel/pt: Introduce intel_pt_cap_decode()
> 
> Also the decsription in $subject is not what the description below says. Please make sure that your subjects and changelogs are consistent.
> 
> > New function pt_cap_decode() will be invoked in KVM to check if a
> > specific capability is available 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.
> 
> Again it took me a while to figure out what you are trying to say. Suggestion:
> 
>   intel_pt_validate_cap() validates whether a given PT capability is
>   supported by the hardware. It checks the PT capability array which
>   reflects the capabilites of the hardware on which the code is executed.
> 
>   For setting up PT for KVM guests this is not correct as the capability
>   array for the guest can be different from the host array.
> 
>   Provide a new function to check against a given capability array.
> 

Thanks for your suggestion. It looks good to me.

> > -u32 pt_cap_get(enum pt_capabilities cap)
> > +u32 pt_cap_decode(u32 *caps, enum pt_capabilities capability)
> 
> >  {
> > -	struct pt_cap_desc *cd = &pt_caps[cap];
> > -	u32 c = pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
> > +	struct pt_cap_desc *cd = &pt_caps[capability];
> > +	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_decode);
> > +
> > +u32 pt_cap_get(enum pt_capabilities cap) {
> > +	return pt_cap_decode(pt_pmu.caps, cap); }
> >  EXPORT_SYMBOL_GPL(pt_cap_get);
> 
> pt_cap_decode() is yet another misnomer.
> 
> So what you really want is:
> 
>    s/pt_cap_get()/intel_pt_validate_hw_cap()/
> 
> and the new function wants to be:
> 
>    intel_pt_validate_cap()
> 
> So it's entirely clear from the naming that the former pt_cap_get() is related to the hardware capabilities of the machine on which the code
> is executing.
> 

Will be fixed it next version.

Thanks,
Luwei Kang
diff mbox series

Patch

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 9f54d8e..bac781b 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -75,14 +75,20 @@ 
 	PT_CAP(psb_periods,		1, CPUID_EBX, 0xffff0000),
 };
 
-u32 pt_cap_get(enum pt_capabilities cap)
+u32 pt_cap_decode(u32 *caps, enum pt_capabilities capability)
 {
-	struct pt_cap_desc *cd = &pt_caps[cap];
-	u32 c = pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
+	struct pt_cap_desc *cd = &pt_caps[capability];
+	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_decode);
+
+u32 pt_cap_get(enum pt_capabilities cap)
+{
+	return pt_cap_decode(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 4270421..e2f80b6 100644
--- a/arch/x86/include/asm/intel_pt.h
+++ b/arch/x86/include/asm/intel_pt.h
@@ -26,9 +26,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_decode(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 inline u32 pt_cap_decode(u32 *caps, enum pt_capabilities capability) { return 0; }
 #endif
 
 #endif /* _ASM_X86_INTEL_PT_H */