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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx]
> Sent: Thursday, July 19, 2018 9:09 PM
> To: Kang, Luwei <luwei.kang@xxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx; thomas.lendacky@xxxxxxx; bp@xxxxxxx;
> konrad.wilk@xxxxxxxxxx; mattst88@xxxxxxxxx; Janakarajan.Natarajan@xxxxxxx; dwmw@xxxxxxxxxxxx;
> alexander.shishkin@xxxxxxxxxxxxxxx; songliubraving@xxxxxx; kstewart@xxxxxxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> peterz@xxxxxxxxxxxxx; pbonzini@xxxxxxxxxx; rkrcmar@xxxxxxxxxx; david@xxxxxxxxxx; bsd@xxxxxxxxxx; marcorr@xxxxxxxxxx;
> joro@xxxxxxxxxx
> Subject: Re: [PATCH v11 03/12] perf/x86/intel/pt: Introduce a new function to get capability of Intel PT
> 
> On Mon, 9 Jul 2018, Luwei Kang wrote:
> >
> > -u32 pt_cap_get(enum pt_capabilities cap)
> > +u32 pt_cap_decode(u32 *caps, enum pt_capabilities cap)
> 
> Please make the arguments visually distinguishable. caps vs. cap is really not good to parse. The second one is a selector or index, so
> please spell it out.

Thanks for the code review. Do you mean change the "cap" to "capability" ? That is 
From
	u32 pt_cap_decode(u32 *caps, enum pt_capabilities cap)
TO
	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];
> > +	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..a741d33 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 u32 pt_cap_decode(u32 *caps, enum pt_capabilities cap) {
> > +return 0; }
> 
> This will throw warnings when included into code which does not use this stub function and rightfully so. Stubs are 'static inline' for
> a reason.

Get it. Will fix it.
Have any comments on other x86 patches?

Thanks,
Luwei Kang



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux