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=-8.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 51427C433E0 for ; Thu, 18 Jun 2020 10:50:39 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 F193D207DD for ; Thu, 18 Jun 2020 10:50:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="CcLQIjY8" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F193D207DD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-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=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type: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=gc0g8ovorIJmkAQ2At7E/vFrJ5YYayimR7sp1GyuDHk=; b=CcLQIjY8XTe5Lw R2D7B9aBp7cV37nqibl1RECLCYrM8bvwcGkURfco0dgShV9vzb87sDx/XRCpJTP7bFa8UaUqk52lv vjxss2hh0xpJhFGdLoUyG4xqa90j7GVnIqmTbDv86xdK2lyqvacrSgqZEyQgTZBKnXNmL0TiPxkOC XatDkn4HUzmapxmATGy2y5913fc4KRVtv6J3Sgog4INu624J62Poh6Qy2/6pb36HkL6wFcxAQCGvZ MD+EEw1krWkK1/lm/2kbv3JIK2+2/ehVlZiA1OW+ptK4CzjLa86bZNOQO4G6h2mKdNuXqMpnxnaXN C4X+KHLXr08b2ZFoWYCw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jls7r-0005pn-VU; Thu, 18 Jun 2020 10:50:31 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jls7j-0005hH-KV for linux-arm-kernel@lists.infradead.org; Thu, 18 Jun 2020 10:50:25 +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 143BE31B; Thu, 18 Jun 2020 03:50:23 -0700 (PDT) Received: from [192.168.0.110] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 23EE53F71F; Thu, 18 Jun 2020 03:50:21 -0700 (PDT) Subject: Re: [PATCH v5 2/7] arm64: perf: Avoid PMXEV* indirection To: Stephen Boyd , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <20200617113851.607706-1-alexandru.elisei@arm.com> <20200617113851.607706-3-alexandru.elisei@arm.com> <159242468708.62212.1739215996563155762@swboyd.mtv.corp.google.com> From: Alexandru Elisei Message-ID: Date: Thu, 18 Jun 2020 11:51:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: <159242468708.62212.1739215996563155762@swboyd.mtv.corp.google.com> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200618_035023_818515_6CFA6A72 X-CRM114-Status: GOOD ( 20.92 ) 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: mark.rutland@arm.com, will@kernel.org, Julien Thierry , Peter Zijlstra , maz@kernel.org, Will Deacon , Arnaldo Carvalho de Melo , Alexander Shishkin , Ingo Molnar , catalin.marinas@arm.com, Namhyung Kim , Jiri Olsa , Julien Thierry Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hello, On 6/17/20 9:11 PM, Stephen Boyd wrote: > Quoting Alexandru Elisei (2020-06-17 04:38:46) >> From: Mark Rutland >> >> Currently we access the counter registers and their respective type >> registers indirectly. This requires us to write to PMSELR, issue an ISB, >> then access the relevant PMXEV* registers. >> >> This is unfortunate, because: >> >> * Under virtualization, accessing one registers requires two traps to > one register? Not plural presumably. That's another typo, will fix it. > >> the hypervisor, even though we could access the register directly with >> a single trap. >> >> * We have to issue an ISB which we could otherwise avoid the cost of. >> >> * When we use NMIs, the NMI handler will have to save/restore the select >> register in case the code it preempted was attempting to access a >> counter or its type register. >> >> We can avoid these issues by directly accessing the relevant registers. >> This patch adds helpers to do so. >> >> In armv8pmu_enable_event() we still need the ISB to prevent the PE from >> reordering the write to PMINTENSET_EL1 register. If the interrupt is >> enabled before we disable the counter and the new event is configured, >> we might get an interrupt triggered by the previously programmed event >> overflowing, but which we wrongly attribute to the event that we are >> enabling. >> >> In the process, remove the comment that refers to the ARMv7 PMU. >> >> Cc: Julien Thierry >> Cc: Will Deacon >> Cc: Peter Zijlstra >> Cc: Ingo Molnar >> Cc: Arnaldo Carvalho de Melo >> Cc: Alexander Shishkin >> Cc: Jiri Olsa >> Cc: Namhyung Kim >> Cc: Catalin Marinas >> Signed-off-by: Mark Rutland >> [Julien T.: Don't inline read/write functions to avoid big code-size >> increase, remove unused read_pmevtypern function, >> fix counter index issue.] >> Signed-off-by: Julien Thierry >> [Removed comment, removed trailing semicolons in macros, added ISB] >> Signed-off-by: Alexandru Elisei >> --- >> arch/arm64/kernel/perf_event.c | 95 +++++++++++++++++++++++++++++----- >> 1 file changed, 81 insertions(+), 14 deletions(-) >> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >> index ee180b2a5b39..e95b5ca70a53 100644 >> --- a/arch/arm64/kernel/perf_event.c >> +++ b/arch/arm64/kernel/perf_event.c >> @@ -323,6 +323,73 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event) >> #define ARMV8_IDX_TO_COUNTER(x) \ >> (((x) - ARMV8_IDX_COUNTER0) & ARMV8_PMU_COUNTER_MASK) >> >> +/* >> + * This code is really good >> + */ > Superb! Exactly! I thought so too, that's why I kept the comment. > >> + >> +#define PMEVN_CASE(n, case_macro) \ >> + case n: case_macro(n); break >> + >> +#define PMEVN_SWITCH(x, case_macro) \ >> + do { \ >> + switch (x) { \ >> + PMEVN_CASE(0, case_macro); \ >> + PMEVN_CASE(1, case_macro); \ >> + PMEVN_CASE(2, case_macro); \ >> + PMEVN_CASE(3, case_macro); \ >> + PMEVN_CASE(4, case_macro); \ >> + PMEVN_CASE(5, case_macro); \ >> + PMEVN_CASE(6, case_macro); \ >> + PMEVN_CASE(7, case_macro); \ >> + PMEVN_CASE(8, case_macro); \ >> + PMEVN_CASE(9, case_macro); \ >> + PMEVN_CASE(10, case_macro); \ >> + PMEVN_CASE(11, case_macro); \ >> + PMEVN_CASE(12, case_macro); \ >> + PMEVN_CASE(13, case_macro); \ >> + PMEVN_CASE(14, case_macro); \ >> + PMEVN_CASE(15, case_macro); \ >> + PMEVN_CASE(16, case_macro); \ >> + PMEVN_CASE(17, case_macro); \ >> + PMEVN_CASE(18, case_macro); \ >> + PMEVN_CASE(19, case_macro); \ >> + PMEVN_CASE(20, case_macro); \ >> + PMEVN_CASE(21, case_macro); \ >> + PMEVN_CASE(22, case_macro); \ >> + PMEVN_CASE(23, case_macro); \ >> + PMEVN_CASE(24, case_macro); \ >> + PMEVN_CASE(25, case_macro); \ >> + PMEVN_CASE(26, case_macro); \ >> + PMEVN_CASE(27, case_macro); \ >> + PMEVN_CASE(28, case_macro); \ >> + PMEVN_CASE(29, case_macro); \ >> + PMEVN_CASE(30, case_macro); \ >> + default: WARN(1, "Invalid PMEV* index"); \ > Missing newline on that WARN message? Indeed, will add it. > >> + } \ >> + } while (0) >> + >> +#define RETURN_READ_PMEVCNTRN(n) \ >> + return read_sysreg(pmevcntr##n##_el0) >> +static unsigned long read_pmevcntrn(int n) >> +{ >> + PMEVN_SWITCH(n, RETURN_READ_PMEVCNTRN); >> + return 0; >> +} >> + >> +#define WRITE_PMEVCNTRN(n) \ >> + write_sysreg(val, pmevcntr##n##_el0) >> +static void write_pmevcntrn(int n, unsigned long val) >> +{ >> + PMEVN_SWITCH(n, WRITE_PMEVCNTRN); >> +} >> + >> +#define WRITE_PMEVTYPERN(n) \ >> + write_sysreg(val, pmevtyper##n##_el0) >> +static void write_pmevtypern(int n, unsigned long val) >> +{ >> + PMEVN_SWITCH(n, WRITE_PMEVTYPERN); >> +} >> + >> static inline u32 armv8pmu_pmcr_read(void) >> { >> return read_sysreg(pmcr_el0); >> @@ -351,17 +418,11 @@ static inline int armv8pmu_counter_has_overflowed(u32 pmnc, int idx) >> return pmnc & BIT(ARMV8_IDX_TO_COUNTER(idx)); >> } >> >> -static inline void armv8pmu_select_counter(int idx) >> +static inline u32 armv8pmu_read_evcntr(int idx) >> { >> u32 counter = ARMV8_IDX_TO_COUNTER(idx); >> - write_sysreg(counter, pmselr_el0); >> - isb(); >> -} >> >> -static inline u64 armv8pmu_read_evcntr(int idx) >> -{ >> - armv8pmu_select_counter(idx); >> - return read_sysreg(pmxevcntr_el0); >> + return read_pmevcntrn(counter); >> } >> >> static inline u64 armv8pmu_read_hw_counter(struct perf_event *event) >> @@ -433,8 +494,9 @@ static u64 armv8pmu_read_counter(struct perf_event *event) >> >> static inline void armv8pmu_write_evcntr(int idx, u64 value) >> { >> - armv8pmu_select_counter(idx); >> - write_sysreg(value, pmxevcntr_el0); >> + u32 counter = ARMV8_IDX_TO_COUNTER(idx); > Might be a good idea to make ARMV8_IDX_TO_COUNTER a static inline > function that has a return type of u32. I had to go check the code to > make sure it wasn't something larger. Architecturally, there are at most 32 counter registers, which would fit in an s8, so I don't think type checking would really help us here. > >> + >> + write_pmevcntrn(counter, value); >> } >> >> static inline void armv8pmu_write_hw_counter(struct perf_event *event, >> @@ -469,9 +531,10 @@ static void armv8pmu_write_counter(struct perf_event *event, u64 value) >> >> static inline void armv8pmu_write_evtype(int idx, u32 val) >> { >> - armv8pmu_select_counter(idx); >> + u32 counter = ARMV8_IDX_TO_COUNTER(idx); >> + >> val &= ARMV8_PMU_EVTYPE_MASK; >> - write_sysreg(val, pmxevtyper_el0); >> + write_pmevtypern(counter, val); >> } >> >> static inline void armv8pmu_write_event_type(struct perf_event *event) >> @@ -491,7 +554,10 @@ static inline void armv8pmu_write_event_type(struct perf_event *event) >> armv8pmu_write_evtype(idx - 1, hwc->config_base); >> armv8pmu_write_evtype(idx, chain_evt); >> } else { >> - armv8pmu_write_evtype(idx, hwc->config_base); >> + if (idx == ARMV8_IDX_CYCLE_COUNTER) >> + write_sysreg(hwc->config_base, pmccfiltr_el0); >> + else >> + armv8pmu_write_evtype(idx, hwc->config_base); >> } >> } >> >> @@ -595,9 +661,10 @@ static void armv8pmu_enable_event(struct perf_event *event) >> * Disable counter >> */ >> armv8pmu_disable_event_counter(event); >> + isb(); > Same comment about uncommented isb(). Will add a comment explaining the ISB. Thanks, Alex _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel