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 38549C001DF for ; Mon, 31 Jul 2023 08:08:39 +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:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=VsOiescPDFd7UZ82/jEktM9V//mNf194D+P2tYYHwSI=; b=VU522XEpOD5z9R hVFn/YC9juIiQ+pFb7zgarABx7gE1fv2/qHUS/pRqNu/Nv1oS2KS4gYdQfYCeYAh7gFxJKpRP/cWX 6UvqLnE0GuEpMXkTDlm/2rs5TfdWon9o7vdaxqF7wi/qC9XmviaocKlpJSdL5UeWS1MQsV/bA6VkG WcGJR+cEIlt+PULqIGX1k52DQuT1iGF37Ii6gunCRSvQxDjDBKI9BC8qWZFGMMfrrcjscSDRH/Teq Sq67rtk0oRD4rfQ32oQTBzCLhaAx0dNXbeXjxbDppJwEO+4DxkDIP9XpdqcTmNSPFWavZ3WRIfTzc v/kiJjDL7rs6JAM1A5xA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qQNwp-00ETxy-0r; Mon, 31 Jul 2023 08:08:11 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qQNwl-00ETxC-2J for linux-arm-kernel@lists.infradead.org; Mon, 31 Jul 2023 08:08:09 +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 C6126D75; Mon, 31 Jul 2023 01:08:45 -0700 (PDT) Received: from [192.168.1.3] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A96153F59C; Mon, 31 Jul 2023 01:08:00 -0700 (PDT) Message-ID: Date: Mon, 31 Jul 2023 09:07:53 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH V13 - RESEND 02/10] arm64/perf: Add BRBE registers and fields To: Anshuman Khandual , Will Deacon Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, mark.rutland@arm.com, Mark Brown , Rob Herring , Marc Zyngier , Suzuki Poulose , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , linux-perf-users@vger.kernel.org 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> Content-Language: en-US From: James Clark 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_010807_870426_FF184F5F X-CRM114-Status: GOOD ( 25.63 ) 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 31/07/2023 03:33, 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. During testing, all > those captured branch records looked alright. But that is no excuse, for not > doing the right thing to begin with i.e adding explicit brackets. I will fix > these in next version. Are you sure? If you see the return value here, it's 0 until register 16, then it becomes 1: https://godbolt.org/z/c7zhbno3n If you add the bracket it does actually change the return value. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel