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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 129BFC001E0 for ; Mon, 31 Jul 2023 09:06:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=saiNAyFKtrvs01f1K7cpM7YsL6CxEmAce0QwckWPVXU=; b=H4p7XBM6k8WCTU iN7MhqEa53jEaiH+vZo6pMca2p1JiqsyWbZ57w0VQbBWY/UXPgm+Gr5pGWYchWHrkOc/RxoVOMKsm XieAZPDVsxcB/NXTCWRJSY7Pz+/ft+xU7UObQp2x1AqCi0HWoKJmkba8xwN49LRsYq5IB+hLEE/Lk tLkCzgspLkdYi+sapM6hOhAUe1qJFzuINkfmSa0nSGehP5WiTxRNuKDBPY7pwTaWoduDnn46Tvojm O/jxbn0aoGBhcQa5gkJxCbRtiAE6xgYM35Yf9cnEHwhi6DXd+ZSQqwQD2Bbljt2QORKNu51+7pjJB cdDo/1jBkA8QC0dbQ9LQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qQOrA-00EiNC-3C; Mon, 31 Jul 2023 09:06:24 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qQOr8-00EiLH-00 for linux-arm-kernel@lists.infradead.org; Mon, 31 Jul 2023 09:06:23 +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 63E33D75; Mon, 31 Jul 2023 02:07:00 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.89.192]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 929E73F59C; Mon, 31 Jul 2023 02:06:14 -0700 (PDT) Date: Mon, 31 Jul 2023 10:06:08 +0100 From: Mark Rutland To: Anshuman Khandual Cc: James Clark , Will Deacon , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, Mark Brown , Rob Herring , Marc Zyngier , Suzuki Poulose , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , linux-perf-users@vger.kernel.org Subject: Re: [PATCH V13 - RESEND 02/10] arm64/perf: Add BRBE registers and fields Message-ID: References: <20230711082455.215983-1-anshuman.khandual@arm.com> <20230711082455.215983-3-anshuman.khandual@arm.com> <20230728162011.GA22050@willie-the-truck> <89ce4bc4-00c5-a763-3179-e1d3e9f198b7@arm.com> <937468a1-b325-7d05-8daf-765f911c9240@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <937468a1-b325-7d05-8daf-765f911c9240@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230731_020622_168141_3FBF912C X-CRM114-Status: GOOD ( 32.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Jul 31, 2023 at 08:03:21AM +0530, Anshuman Khandual wrote: > > > On 7/28/23 22:22, James Clark wrote: > > > > > > On 28/07/2023 17:20, Will Deacon wrote: > >> On Tue, Jul 11, 2023 at 01:54:47PM +0530, Anshuman Khandual wrote: > >>> This adds BRBE related register definitions and various other related field > >>> macros there in. These will be used subsequently in a BRBE driver which is > >>> being added later on. > >>> > >>> Cc: Catalin Marinas > >>> Cc: Will Deacon > >>> Cc: Marc Zyngier > >>> Cc: Mark Rutland > >>> Cc: linux-arm-kernel@lists.infradead.org > >>> Cc: linux-kernel@vger.kernel.org > >>> Tested-by: James Clark > >>> Reviewed-by: Mark Brown > >>> Signed-off-by: Anshuman Khandual > >>> --- > >>> arch/arm64/include/asm/sysreg.h | 103 +++++++++++++++++++++ > >>> arch/arm64/tools/sysreg | 158 ++++++++++++++++++++++++++++++++ > >>> 2 files changed, 261 insertions(+) > >>> > >>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > >>> index b481935e9314..f95e30c13c8b 100644 > >>> --- a/arch/arm64/include/asm/sysreg.h > >>> +++ b/arch/arm64/include/asm/sysreg.h > >>> @@ -163,6 +163,109 @@ > >>> #define SYS_DBGDTRTX_EL0 sys_reg(2, 3, 0, 5, 0) > >>> #define SYS_DBGVCR32_EL2 sys_reg(2, 4, 0, 7, 0) > >>> > >>> +#define __SYS_BRBINFO(n) sys_reg(2, 1, 8, ((n) & 0xf), ((((n) & 0x10)) >> 2 + 0)) > >>> +#define __SYS_BRBSRC(n) sys_reg(2, 1, 8, ((n) & 0xf), ((((n) & 0x10)) >> 2 + 1)) > >>> +#define __SYS_BRBTGT(n) sys_reg(2, 1, 8, ((n) & 0xf), ((((n) & 0x10)) >> 2 + 2)) > >> > >> It's that time on a Friday but... aren't these macros busted? I think you > >> need brackets before adding the offset, otherwise wouldn't, for example, > >> target registers 0-15 all access info register 0 and __SYS_BRBTGT(16) would > >> then start accessing source register 0? > >> > >> I'm surprised that the compiler doesn't warn about this, but even more > >> surprised that you managed to test this. > >> > >> Please tell me I'm wrong! > >> > >> Will > > > > No I think you are right, it is wrong. Luckily there is already an > > extraneous bracket so you you can fix it by moving one a place down: > > > > sys_reg(2, 1, 8, ((n) & 0xf), ((((n) & 0x10) >> 2) + 2)) > > > > It's interesting because the test [1] is doing quite a bit and looking > > at the branch info, and that src and targets match up to function names. > > I also manually looked at the branch buffers and didn't see anything > > obviously wrong like things that looked like branch infos in the source > > or target fields. Will have to take another look to see if it would be > > possible for the test to catch this. > > > > James > > > > [1]: > > https://gitlab.arm.com/linux-arm/linux-jc/-/commit/3a7ddce70c2daadb63fcc511de0a89055ca48b32 > > ((((n) & 0x10)) >> 2 + 2) ---> ((((n) & 0x10) >> 2) + 2) > > The additional brackets are useful in explicitly telling the compiler but > what it the compiler is just doing the right thing implicitly i.e computing > the shifting operation before doing the offset addition. No; that is not correct. In c, '+' has higher precedence than '>>'. For 'a >> b + c' the compiler *must* treat that as 'a >> (b + c)', and not as '(a >> b) + c' That's trivial to test: | [mark@gravadlaks:~]% cat shiftadd.c | #include | | unsigned long logshiftadd(unsigned long a, | unsigned long b, | unsigned long c) | { | printf("%ld >> %ld + %ld is %ld\n", | a, b, c, a >> b + c); | } | | int main(int argc, char *argv) | { | logshiftadd(0, 0, 0); | logshiftadd(0, 0, 1); | logshiftadd(0, 0, 2); | printf("\n"); | logshiftadd(1024, 0, 0); | logshiftadd(1024, 0, 1); | logshiftadd(1024, 0, 2); | printf("\n"); | logshiftadd(1024, 2, 0); | logshiftadd(1024, 2, 1); | logshiftadd(1024, 2, 2); | | return 0; | } | [mark@gravadlaks:~]% gcc shiftadd.c -o shiftadd | [mark@gravadlaks:~]% ./shiftadd | 0 >> 0 + 0 is 0 | 0 >> 0 + 1 is 0 | 0 >> 0 + 2 is 0 | | 1024 >> 0 + 0 is 1024 | 1024 >> 0 + 1 is 512 | 1024 >> 0 + 2 is 256 | | 1024 >> 2 + 0 is 256 | 1024 >> 2 + 1 is 128 | 1024 >> 2 + 2 is 64 > During testing, all > those captured branch records looked alright. I think we clearly need better testing here. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel