From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752966AbdBAQtO (ORCPT ); Wed, 1 Feb 2017 11:49:14 -0500 Received: from mga06.intel.com ([134.134.136.31]:3152 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752816AbdBAQtN (ORCPT ); Wed, 1 Feb 2017 11:49:13 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,320,1477983600"; d="scan'208";a="220161001" From: Alexander Shishkin To: Ingo Molnar Cc: Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, vince@deater.net, eranian@google.com, Arnaldo Carvalho de Melo , Borislav Petkov , Thomas Gleixner Subject: Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing In-Reply-To: <20170201101704.GA450@gmail.com> References: <20170127151644.8585-1-alexander.shishkin@linux.intel.com> <20170127151644.8585-3-alexander.shishkin@linux.intel.com> <20170201101704.GA450@gmail.com> User-Agent: Notmuch/0.23.3 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Wed, 01 Feb 2017 18:49:09 +0200 Message-ID: <87lgtpambe.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Ingo Molnar writes: > * Alexander Shishkin wrote: > >> Now that Intel PT supports more types of trace content than just branch >> tracing, it may be useful to allow the user to disable branch tracing >> when it is not needed. >> >> The special case is BDW, where not setting BranchEn is not supported. >> >> This patch adds 'no_branch' event format string to PT events, which >> will disable setting BranchEn bit in the hardware trace configuration. > >> + /* trying to unset BRANCH_EN where it is not supported */ > > Please capitalize comments consistently and use the typical tense. This one should > be something like: > > /* Try to unset BRANCH_EN where it is not supported: */ Will do. >> + if (!(event->attr.config & RTIT_CTL_BRANCH_EN)) >> + reg |= RTIT_CTL_BRANCH_EN; >> + else >> + event->attr.config &= ~RTIT_CTL_BRANCH_EN; > > > So I really hate this ABI hack - it's these unclean approaches how ABIs degrade > over time, by death of a thousand cuts... Agreed. > Any reason why we couldn't add a separate pt_feature_branch_disable and > pt_feature_trace_disable bits and interpret them in a straightforward way, or > something like that? > > ( This means two more bits, but that's our punishment for not anticipating > extensions to the hardware feature. ) Most features (all but BranchEn) are at the moment set-to-enable. It is only this unfortunate default of BranchEn=1 that is a pain. Another relatively painless thing we can do is add a 'passthrough' bit, which will allow the user to (not) set BranchEn bit when passthrough=1 and behave like it did before, when passthrough=0. Like below: >>From d28174b582aba285baa1ab9fea73b554e8937f89 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Thu, 19 Jan 2017 10:00:52 +0200 Subject: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing Now that Intel PT supports more types of trace content than just branch tracing, it may be useful to allow the user to disable branch tracing when it is not needed. The special case is BDW, where not setting BranchEn is not supported. This is slightly trickier than necessary, because up to this moment the driver has been setting BranchEn automatically and the userspace assumes as much. Instead of reversing the semantics of BranchEn, we introduce a 'passthrough' bit, which will forego the default and allow the user to set BranchEn to their heart's content. A new synthetic capability is also introduced to advertise the availability of this 'passthrough' bit. Signed-off-by: Alexander Shishkin --- arch/x86/events/intel/pt.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-- arch/x86/events/intel/pt.h | 2 ++ 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c index ece5fb06db..a31b570577 100644 --- a/arch/x86/events/intel/pt.c +++ b/arch/x86/events/intel/pt.c @@ -28,6 +28,7 @@ #include #include #include +#include #include "../perf_event.h" #include "pt.h" @@ -50,6 +51,10 @@ static struct pt_pmu pt_pmu; #define PT_CAP(_n, _l, _r, _m) \ [PT_CAP_ ## _n] = { .name = __stringify(_n), .leaf = _l, \ .reg = _r, .mask = _m } +/* + * Software-defined/driver capabilities + */ +#define PT_SW_CAP 0xffffffff static struct pt_cap_desc { const char *name; @@ -72,12 +77,14 @@ static struct pt_cap_desc { PT_CAP(mtc_periods, 1, CPUID_EAX, 0xffff0000), PT_CAP(cycle_thresholds, 1, CPUID_EBX, 0xffff), PT_CAP(psb_periods, 1, CPUID_EBX, 0xffff0000), + PT_CAP(passthrough, PT_SW_CAP, 0, 1), }; static u32 pt_cap_get(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 = cd->leaf == PT_SW_CAP ? 1 : + pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg]; unsigned int shift = __ffs(cd->mask); return (c & cd->mask) >> shift; @@ -98,6 +105,7 @@ static struct attribute_group pt_cap_group = { .name = "caps", }; +PMU_FORMAT_ATTR(passthrough, "config:0" ); PMU_FORMAT_ATTR(cyc, "config:1" ); PMU_FORMAT_ATTR(pwr_evt, "config:4" ); PMU_FORMAT_ATTR(fup_on_ptw, "config:5" ); @@ -105,11 +113,13 @@ PMU_FORMAT_ATTR(mtc, "config:9" ); PMU_FORMAT_ATTR(tsc, "config:10" ); PMU_FORMAT_ATTR(noretcomp, "config:11" ); PMU_FORMAT_ATTR(ptw, "config:12" ); +PMU_FORMAT_ATTR(branch, "config:13" ); PMU_FORMAT_ATTR(mtc_period, "config:14-17" ); PMU_FORMAT_ATTR(cyc_thresh, "config:19-22" ); PMU_FORMAT_ATTR(psb_period, "config:24-27" ); static struct attribute *pt_formats_attr[] = { + &format_attr_passthrough.attr, &format_attr_cyc.attr, &format_attr_pwr_evt.attr, &format_attr_fup_on_ptw.attr, @@ -117,6 +127,7 @@ static struct attribute *pt_formats_attr[] = { &format_attr_tsc.attr, &format_attr_noretcomp.attr, &format_attr_ptw.attr, + &format_attr_branch.attr, &format_attr_mtc_period.attr, &format_attr_cyc_thresh.attr, &format_attr_psb_period.attr, @@ -197,6 +208,19 @@ static int __init pt_pmu_hw_init(void) pt_pmu.tsc_art_den = eax; } + /* model-specific quirks */ + switch (boot_cpu_data.x86_model) { + case INTEL_FAM6_BROADWELL_CORE: + case INTEL_FAM6_BROADWELL_XEON_D: + case INTEL_FAM6_BROADWELL_GT3E: + case INTEL_FAM6_BROADWELL_X: + /* not setting BRANCH_EN will #GP, erratum BDM106 */ + pt_pmu.branch_en_always_on = true; + break; + default: + break; + } + if (boot_cpu_has(X86_FEATURE_VMX)) { /* * Intel SDM, 36.5 "Tracing post-VMXON" says that @@ -263,8 +287,22 @@ static int __init pt_pmu_hw_init(void) #define RTIT_CTL_PTW (RTIT_CTL_PTW_EN | \ RTIT_CTL_FUP_ON_PTW) -#define PT_CONFIG_MASK (RTIT_CTL_TSC_EN | \ +/* + * Bit 0 (TraceEn) in the attr.config is meaningless as the + * corresponding bit in the RTIT_CTL can only be controlled + * by the driver; therefore, repurpose it to mean: pass + * through the bit that was previously assumed to be always + * on for PT, thereby allowing the user to *not* set it if + * they so wish. See also pt_event_valid() and pt_config(). + * The synthetic "passthough" capability tells the userspace + * that this feature is available. + */ +#define RTIT_CTL_PASSTHROUGH RTIT_CTL_TRACEEN + +#define PT_CONFIG_MASK (RTIT_CTL_TRACEEN | \ + RTIT_CTL_TSC_EN | \ RTIT_CTL_DISRETC | \ + RTIT_CTL_BRANCH_EN | \ RTIT_CTL_CYC_PSB | \ RTIT_CTL_MTC | \ RTIT_CTL_PWR_EVT_EN | \ @@ -332,6 +370,33 @@ static bool pt_event_valid(struct perf_event *event) return false; } + /* + * Setting bit 0 (TraceEn in RTIT_CTL MSR) in the attr.config + * clears the assomption that BranchEn must always be enabled, + * as was the case with the first implementation of PT. + * If this bit is not set, the legacy behavior is preserved + * for compatibility with the older userspace. + * + * Re-using bit 0 for this purpose is fine because it is never + * directly set by the user; previous attempts at setting it in + * the attr.config resulted in -EINVAL. + */ + if (config & RTIT_CTL_PASSTHROUGH) { + /* + * Disallow not setting BRANCH_EN where BRANCH_EN is + * always required. + */ + if (pt_pmu.branch_en_always_on && + !(config & RTIT_CTL_BRANCH_EN)) + return false; + } else { + /* + * Disallow BRANCH_EN without the PASSTHROUGH. + */ + if (config & RTIT_CTL_BRANCH_EN) + return false; + } + return true; } @@ -419,7 +484,20 @@ static void pt_config(struct perf_event *event) } reg = pt_config_filters(event); - reg |= RTIT_CTL_TOPA | RTIT_CTL_BRANCH_EN | RTIT_CTL_TRACEEN; + reg |= RTIT_CTL_TOPA | RTIT_CTL_TRACEEN; + + /* + * Previously, we had BRANCH_EN on by default, but now that PT has + * grown features outside of branch tracing, it is useful to allow + * the user to disable it. Setting bit 0 in the event's attr.config + * allows BRANCH_EN to pass through instead of being always on. See + * also the comment in pt_event_valid(). + */ + if (event->attr.config & BIT(0)) { + reg |= event->attr.config & RTIT_CTL_BRANCH_EN; + } else { + reg |= RTIT_CTL_BRANCH_EN; + } if (!event->attr.exclude_kernel) reg |= RTIT_CTL_OS; diff --git a/arch/x86/events/intel/pt.h b/arch/x86/events/intel/pt.h index 53473c21b5..0f17367b7b 100644 --- a/arch/x86/events/intel/pt.h +++ b/arch/x86/events/intel/pt.h @@ -104,12 +104,14 @@ enum pt_capabilities { PT_CAP_mtc_periods, PT_CAP_cycle_thresholds, PT_CAP_psb_periods, + PT_CAP_passthrough, }; struct pt_pmu { struct pmu pmu; u32 caps[PT_CPUID_REGS_NUM * PT_CPUID_LEAVES]; bool vmx; + bool branch_en_always_on; unsigned long max_nonturbo_ratio; unsigned int tsc_art_num; unsigned int tsc_art_den; -- 2.11.0