From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DB70DC55ABD for ; Thu, 12 Nov 2020 10:55:13 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4F6FE22201 for ; Thu, 12 Nov 2020 10:55:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="NECM9CHG" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4F6FE22201 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=n/f1AJf84M7GFWRqnSzYYF0KscxjdaRGMPp3oOesLYg=; b=NECM9CHGFSCDHX7eAUePW8dRx S9WNU1nwPhzKhXSnN5+2YDIaOR5NiEhqOp5D4b2aPXQKScuKiwWgHsWnT/vzx7zhswZbI7ddm2Z8v pogjeHTn8If/uLdV+QPjg6UlIgoUzciwcXBv6K9FRuT4BNemaMbdSN/N8QLRQPJwOuX93LsYx34vi PrdQLTSIA3h85YMxpNkNeofjkfdjISN/WFEiDtf/TdarTnZGy7Vi8DDAiMGGy69tKsH5eu4YJ/0Fd 0W5pGXdZaqqtr+xBfhhVlzdztAuZ1EEbOv14zz+lVdgMr8dxxKEvfao6fhggGr2COzQBOG49kdaDP R6JlZqKTQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdAFS-0000LE-5A; Thu, 12 Nov 2020 10:54:38 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdAFP-0000Ku-4q for linux-arm-kernel@lists.infradead.org; Thu, 12 Nov 2020 10:54:36 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 70804139F; Thu, 12 Nov 2020 02:54:31 -0800 (PST) Received: from [10.57.23.123] (unknown [10.57.23.123]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3AD5F3F73C; Thu, 12 Nov 2020 02:54:30 -0800 (PST) Subject: Re: [PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid To: Leo Yan References: <20201110183313.1823760-1-suzuki.poulose@arm.com> <20201110183313.1823760-3-suzuki.poulose@arm.com> <20201112100010.GB17274@leoy-ThinkPad-X240s> From: Suzuki K Poulose Message-ID: Date: Thu, 12 Nov 2020 10:54:23 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.1 MIME-Version: 1.0 In-Reply-To: <20201112100010.GB17274@leoy-ThinkPad-X240s> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201112_055435_382257_7A6C5714 X-CRM114-Status: GOOD ( 34.09 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: al.grant@arm.com, mathieu.poirier@linaro.org, anshuman.khandual@arm.com, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Leo, Thanks for looking at the patch. On 11/12/20 10:00 AM, Leo Yan wrote: > On Tue, Nov 10, 2020 at 06:33:12PM +0000, Suzuki Kuruppassery Poulose wrote: >> If the kernel is running at EL2, the pid of the task is exposed >> via VMID instead of the CONTEXTID. Add support for this in the >> perf tool. >> >> By default the perf tool requests contextid and timestamp for >> task bound events. Instead of hard coding contextid, switch >> to "pid" config exposed by the kernel. >> >> Cc: Leo Yan >> Cc: Mike Leach >> Cc: Mathieu Poirier >> Cc: Al Grant >> Signed-off-by: Suzuki K Poulose >> --- >> tools/include/linux/coresight-pmu.h | 11 +++-- >> tools/perf/arch/arm/util/cs-etm.c | 65 +++++++++++++++++++++-------- >> 2 files changed, 54 insertions(+), 22 deletions(-) >> >> diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h >> index b0e35eec6499..927c6285ce5d 100644 >> --- a/tools/include/linux/coresight-pmu.h >> +++ b/tools/include/linux/coresight-pmu.h >> @@ -11,16 +11,19 @@ >> #define CORESIGHT_ETM_PMU_SEED 0x10 >> >> /* ETMv3.5/PTM's ETMCR config bit */ >> -#define ETM_OPT_CYCACC 12 >> -#define ETM_OPT_CTXTID 14 >> -#define ETM_OPT_TS 28 >> -#define ETM_OPT_RETSTK 29 >> +#define ETM_OPT_CYCACC 12 >> +#define ETM_OPT_CTXTID 14 >> +#define ETM_OPT_CTXTID_IN_VMID 15 >> +#define ETM_OPT_TS 28 >> +#define ETM_OPT_RETSTK 29 >> >> /* ETMv4 CONFIGR programming bits for the ETM OPTs */ >> #define ETM4_CFG_BIT_CYCACC 4 >> #define ETM4_CFG_BIT_CTXTID 6 >> +#define ETM4_CFG_BIT_VMID 7 >> #define ETM4_CFG_BIT_TS 11 >> #define ETM4_CFG_BIT_RETSTK 12 >> +#define ETM4_CFG_BIT_VMID_OPT 15 >> >> static inline int coresight_get_trace_id(int cpu) >> { >> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c >> index cad7bf783413..e6207ce7cc85 100644 >> --- a/tools/perf/arch/arm/util/cs-etm.c >> +++ b/tools/perf/arch/arm/util/cs-etm.c >> @@ -59,14 +59,15 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = { >> >> static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); >> >> -static int cs_etm_set_context_id(struct auxtrace_record *itr, >> - struct evsel *evsel, int cpu) >> +static int cs_etm_set_pid(struct auxtrace_record *itr, >> + struct evsel *evsel, int cpu) >> { >> struct cs_etm_recording *ptr; >> struct perf_pmu *cs_etm_pmu; >> char path[PATH_MAX]; >> int err = -EINVAL; >> u32 val; >> + u64 pid_fmt; >> >> ptr = container_of(itr, struct cs_etm_recording, itr); >> cs_etm_pmu = ptr->cs_etm_pmu; >> @@ -86,21 +87,43 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, >> goto out; >> } >> >> - /* >> - * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing >> - * is supported: >> - * 0b00000 Context ID tracing is not supported. >> - * 0b00100 Maximum of 32-bit Context ID size. >> - * All other values are reserved. >> - */ >> - val = BMVAL(val, 5, 9); >> - if (!val || val != 0x4) { >> + pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid"); >> + if (!pid_fmt) >> + pid_fmt = 1ULL << ETM_OPT_CTXTID; > > If doesn't find "pid" format bits, should it mean the kernel is > running an old version so doesn't support "pid" entry? It's good to > add comment for this. Yes, you're right. I will add it. > >> + >> + switch (pid_fmt) { >> + case (1ULL << ETM_OPT_CTXTID): >> + /* >> + * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing >> + * is supported: >> + * 0b00000 Context ID tracing is not supported. >> + * 0b00100 Maximum of 32-bit Context ID size. >> + * All other values are reserved. >> + */ >> + val = BMVAL(val, 5, 9); >> + if (!val || val != 0x4) { >> + err = -EINVAL; >> + goto out; >> + } >> + break; >> + case (1ULL << ETM_OPT_CTXTID_IN_VMID): >> + /* >> + * TRCIDR2.VMIDOPT[30:29] != 0 and >> + * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual context id size) >> + */ >> + if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) { > > The comment is not alignment with the code. Based on the comment, the > code should be: > > if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) != 4) { > The reasoning is any value > 4, would imply a size > 32. This is really making it future proof. I could restrict this to 4 now if you insist. >> +#define ETM_SET_OPT_PID (1 << 0) >> +#define ETM_SET_OPT_TS (1 << 1) >> +#define ETM_SET_OPT_MASK (ETM_SET_OPT_PID | ETM_SET_OPT_TS) >> + >> static int cs_etm_set_option(struct auxtrace_record *itr, >> struct evsel *evsel, u32 option) >> { >> @@ -169,17 +196,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr, >> !cpu_map__has(online_cpus, i)) >> continue; >> >> - if (option & ETM_OPT_CTXTID) { >> - err = cs_etm_set_context_id(itr, evsel, i); >> + if (option & ETM_SET_OPT_PID) { >> + err = cs_etm_set_pid(itr, evsel, i); > > I don't understand what's the reason for introducing the new macros > "ETM_SET_OPT_XXX", seems to me the old macros still can be used at > here. Could you help explian for this? Sure, with the change now, we either set ETM_OPT_CTXTID or ETM_OPT_CTXTID_IN_VMID in the config. So, using the flag ETM_OPT_CTXTID to indicate the same is going to be confusing. This makes it explicit and uses a separate flag. Or the in otherways it is really making clear that we want to set PID tracing and de-coupling it from "CONTEXTID" as it is not the correct config anymore and has to be determined at runtime. I could make it explicit in a comment. Cheers Suzuki > > Thanks, > Leo > >> if (err) >> goto out; >> } >> - if (option & ETM_OPT_TS) { >> + if (option & ETM_SET_OPT_TS) { >> err = cs_etm_set_timestamp(itr, evsel, i); >> if (err) >> goto out; >> } >> - if (option & ~(ETM_OPT_CTXTID | ETM_OPT_TS)) >> + if (option & ~(ETM_SET_OPT_MASK)) >> /* Nothing else is currently supported */ >> goto out; >> } >> @@ -406,7 +433,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, >> evsel__set_sample_bit(cs_etm_evsel, CPU); >> >> err = cs_etm_set_option(itr, cs_etm_evsel, >> - ETM_OPT_CTXTID | ETM_OPT_TS); >> + ETM_SET_OPT_PID | ETM_SET_OPT_TS); >> if (err) >> goto out; >> } >> @@ -485,7 +512,9 @@ static u64 cs_etmv4_get_config(struct auxtrace_record *itr) >> config |= BIT(ETM4_CFG_BIT_TS); >> if (config_opts & BIT(ETM_OPT_RETSTK)) >> config |= BIT(ETM4_CFG_BIT_RETSTK); >> - >> + if (config_opts & BIT(ETM_OPT_CTXTID_IN_VMID)) >> + config |= BIT(ETM4_CFG_BIT_VMID) | >> + BIT(ETM4_CFG_BIT_VMID_OPT); >> return config; >> } >> >> -- >> 2.24.1 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel