| Message ID | 1530612254-16642-6-git-send-email-luwei.kang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On Jul 3, 2018, at 3:04 AM, Luwei Kang <luwei.kang@intel.com> wrote: > > 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. > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Song Liu <songliubraving@fb.com> > Cc: Kate Stewart <kstewart@linuxfoundation.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > arch/x86/events/intel/pt.c | 10 ++++++++-- > arch/x86/include/asm/intel_pt.h | 2 ++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c > index 00b079e..edbc930 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_decode(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]; We are accessing offset "cd->leaf * PT_CPUID_REGS_NUM + cd->reg" of array caps. But the array may not be big enough. Is it sufficient to use "struct pt_pmu *pt_pmu" and "pt_pmu->caps" instead? Thanks, Song > 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 2de4db0..9c71453 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_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; } > #endif > > #endif /* _ASM_X86_INTEL_PT_H */ > -- > 1.8.3.1 >
> > --- 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_decode(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]; > > We are accessing offset "cd->leaf * PT_CPUID_REGS_NUM + cd->reg" of array caps. But the array may not be big enough. Is it > sufficient to use "struct pt_pmu *pt_pmu" and "pt_pmu->caps" instead? > Thanks for your review. Function pt_cap_get() can get the capability of native because "pt_pmu.caps[] " include native Intel PT CPUID info. In virtualization, the guest CPUID info is configurable. So I introduce this function pt_cap_decode() to check if guest CPUID support specific capability. I introduce a structure "struct pt_desc" which include a member "u32 caps[PT_CPUID_REGS_NUM * PT_CPUID_LEAVES]" like native "struct pt_pmu" in patch 8. So, I can't use "struct pt_pmu *pt_pmu" or "pt_pmu->caps" here because they are native parameter not guest. Thanks, Luwei Kang > > > 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 2de4db0..9c71453 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_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; } > > #endif > > > > #endif /* _ASM_X86_INTEL_PT_H */ > > -- > > 1.8.3.1 > >
Luwei Kang <luwei.kang@intel.com> writes: > 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. I would word it more like this: "This patch adds a function to query PT capabilities from a cached PT CPUID leaf. This is useful for KVM that maintains its own modified copy of this leaf, that only contains capabilities that it wants to expose to the guest." Also, is there any reason why this and 02/12 are separate patches? Regards, -- Alex
> On Jul 3, 2018, at 6:56 PM, Kang, Luwei <luwei.kang@intel.com> wrote: > >>> --- 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_decode(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]; >> >> We are accessing offset "cd->leaf * PT_CPUID_REGS_NUM + cd->reg" of array caps. But the array may not be big enough. Is it >> sufficient to use "struct pt_pmu *pt_pmu" and "pt_pmu->caps" instead? >> > > Thanks for your review. > Function pt_cap_get() can get the capability of native because "pt_pmu.caps[] " include native Intel PT CPUID info. > In virtualization, the guest CPUID info is configurable. So I introduce this function pt_cap_decode() to check if guest CPUID support specific capability. I introduce a structure "struct pt_desc" which include a member "u32 caps[PT_CPUID_REGS_NUM * PT_CPUID_LEAVES]" like native "struct pt_pmu" in patch 8. > So, I can't use "struct pt_pmu *pt_pmu" or "pt_pmu->caps" here because they are native parameter not guest. > > Thanks, > Luwei Kang Thanks for the explanation. I think this is OK. Acked-by: Song Liu <songliubraving@fb.com> >> >>> 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 2de4db0..9c71453 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_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; } >>> #endif >>> >>> #endif /* _ASM_X86_INTEL_PT_H */ >>> -- >>> 1.8.3.1 >>> >
> > > --- 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_decode(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]; > > > > We are accessing offset "cd->leaf * PT_CPUID_REGS_NUM + cd->reg" of > > array caps. But the array may not be big enough. Is it sufficient to use "struct pt_pmu *pt_pmu" and "pt_pmu->caps" instead? > > > > Thanks for your review. > Function pt_cap_get() can get the capability of native because "pt_pmu.caps[] " include native Intel PT CPUID info. > In virtualization, the guest CPUID info is configurable. So I introduce this function pt_cap_decode() to check if guest CPUID support > specific capability. I introduce a structure "struct pt_desc" which include a member "u32 caps[PT_CPUID_REGS_NUM * > PT_CPUID_LEAVES]" like native "struct pt_pmu" in patch 8. > So, I can't use "struct pt_pmu *pt_pmu" or "pt_pmu->caps" here because they are native parameter not guest. > What about move pt_cap_decode() to kvm and remove the static of "pt_caps[]" so that kvm can access this variable. To avoid the array may not be big enough, change the function parameter like this: u32 pt_cap_decode(struct pt_desc *pt_desc, enum pt_capabilities cap) { struct pt_cap_desc *cd = &pt_caps[cap]; u32 c = pt_desc->caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg]; ... ... } Thanks, Luwei Kang > > > > > > 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 2de4db0..9c71453 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_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; } > > > #endif > > > > > > #endif /* _ASM_X86_INTEL_PT_H */ > > > -- > > > 1.8.3.1 > > >
> >>> --- 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_decode(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]; > >> > >> We are accessing offset "cd->leaf * PT_CPUID_REGS_NUM + cd->reg" of > >> array caps. But the array may not be big enough. Is it sufficient to use "struct pt_pmu *pt_pmu" and "pt_pmu->caps" instead? > >> > > > > Thanks for your review. > > Function pt_cap_get() can get the capability of native because "pt_pmu.caps[] " include native Intel PT CPUID info. > > In virtualization, the guest CPUID info is configurable. So I introduce this function pt_cap_decode() to check if guest CPUID support > specific capability. I introduce a structure "struct pt_desc" which include a member "u32 caps[PT_CPUID_REGS_NUM * > PT_CPUID_LEAVES]" like native "struct pt_pmu" in patch 8. > > So, I can't use "struct pt_pmu *pt_pmu" or "pt_pmu->caps" here because they are native parameter not guest. > > > > Thanks, > > Luwei Kang > > Thanks for the explanation. I think this is OK. > > Acked-by: Song Liu <songliubraving@fb.com> Thanks for your review. I sent another suggestion before read this mail. Please ignore if no necessary. Thanks, Luwei Kang > > > > >> > >>> 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 2de4db0..9c71453 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_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; } > >>> #endif > >>> > >>> #endif /* _ASM_X86_INTEL_PT_H */ > >>> -- > >>> 1.8.3.1 > >>> > >
> -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Alexander Shishkin > Sent: Thursday, July 5, 2018 8:12 PM > To: Kang, Luwei <luwei.kang@intel.com>; kvm@vger.kernel.org > Cc: x86@kernel.org; Kang, Luwei <luwei.kang@intel.com>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar > <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>; Peter Zijlstra (Intel) <peterz@infradead.org>; Song Liu > <songliubraving@fb.com>; Kate Stewart <kstewart@linuxfoundation.org>; Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Subject: Re: [PATCH v10 05/12] perf/x86/intel/pt: Introduce a new function to get capability of Intel PT > > Luwei Kang <luwei.kang@intel.com> writes: > > > 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. > > I would word it more like this: > > "This patch adds a function to query PT capabilities from a cached PT CPUID leaf. This is useful for KVM that maintains its own > modified copy of this leaf, that only contains capabilities that it wants to expose to the guest." > > Also, is there any reason why this and 02/12 are separate patches? Sorry for delay reply. This patch with 02/12 is one patch first, but be separated to two patches later because they are two different things. One is just moving and this patch is adding new capability. Thanks, Luwei Kang
diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c index 00b079e..edbc930 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_decode(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_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 2de4db0..9c71453 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_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; } #endif #endif /* _ASM_X86_INTEL_PT_H */
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. Signed-off-by: Luwei Kang <luwei.kang@intel.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Song Liu <songliubraving@fb.com> Cc: Kate Stewart <kstewart@linuxfoundation.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- arch/x86/events/intel/pt.c | 10 ++++++++-- arch/x86/include/asm/intel_pt.h | 2 ++ 2 files changed, 10 insertions(+), 2 deletions(-)