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 02360C433F5 for ; Tue, 15 Feb 2022 19:47:25 +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=2g1rLx11yR1Lxg3agshDysgewFNraOQaFj/XNLqPMEY=; b=Qjcl10SOy32ify HN51m+I5Gv1grXrlq2oEEq96Hqw3WCdpcC5K9B9bp9fKYtJHXvdzJj7mWp/Nh56r3eVtjr1u4rNIN dx8M6vImqNPDa5/U382dTs4hISFo5oSLD5o0tcE93wpaI1QU+bsvyumNsfK1ECMsmgpKDruwDo2MR k+W2Ke/PpjL5pjML4DGax3vZZF6qXNDlv9OT51WkPwpAmooz8kDGAmTw7/5emINwFv1J13q/oPrTZ r3fczXLkuT3khEY9qVAsVhQAAZ9S3CoLqEXOXM/ejj0rgqELY8GYbO/v2RaNoxjauA5lJMRMA9rVE 4Yn8E5UFOYAbMZSJsHtg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nK3ls-004O8S-JY; Tue, 15 Feb 2022 19:45:56 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nK3lm-004O5d-Bd for linux-arm-kernel@lists.infradead.org; Tue, 15 Feb 2022 19:45:54 +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 9D2251480; Tue, 15 Feb 2022 11:45:46 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.89.173]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B178B3F70D; Tue, 15 Feb 2022 11:45:41 -0800 (PST) Date: Tue, 15 Feb 2022 19:45:21 +0000 From: Mark Rutland To: Rob Herring Cc: Anshuman Khandual , linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Will Deacon Subject: Re: [PATCH 2/2] perf: Expand perf_branch_entry.type Message-ID: References: <1643348653-24367-1-git-send-email-anshuman.khandual@arm.com> <1643348653-24367-3-git-send-email-anshuman.khandual@arm.com> <6168f881-92a4-54f8-929a-c2f40a36c112@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220215_114550_547797_2AC50FE8 X-CRM114-Status: GOOD ( 42.56 ) 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 Tue, Feb 15, 2022 at 01:01:06PM -0600, Rob Herring wrote: > On Fri, Feb 04, 2022 at 04:02:04PM +0000, Mark Rutland wrote: > > On Fri, Feb 04, 2022 at 10:25:24AM +0530, Anshuman Khandual wrote: > > > On 2/2/22 5:27 PM, Mark Rutland wrote: > > > > On Fri, Jan 28, 2022 at 11:14:13AM +0530, Anshuman Khandual wrote: > > > >> @@ -1370,8 +1376,8 @@ struct perf_branch_entry { > > > >> in_tx:1, /* in transaction */ > > > >> abort:1, /* transaction abort */ > > > >> cycles:16, /* cycle count to last branch */ > > > >> - type:4, /* branch type */ > > > >> - reserved:40; > > > >> + type:6, /* branch type */ > > > > > > > > As above, is this a safe-change ABI-wise? > > > > > > If the bit fields here cannot be expanded without breaking ABI, then > > > there is a fundamental problem. Only remaining option will be to add > > > new fields (with new width value) which could accommodate these new > > > required branch types. > > > > Unfortunately, I think expanding this does break ABI, and is a fundamental > > problem, as: > > > > (a) Any new values in the expanded field will be truncated when read by old > > userspace, and so those may be mis-reported. Maybe we're not too worried > > about this case. > > 'type' or specfically branch stack is not currently supported on arm64. > Do we expect an old userspace which this didn't work on to start working > with a new kernel? I agree for arm64 specifically this probably doesn't matter; I just wanted to have a clear explanation of why this *could* be a problem, since this could affect other architectures. > Given at least some of the new types are arch specific, perhaps > the existing type field should get a new 'PERF_BR_ARCH_SPECIFIC' or > 'PERF_BR_EXTENDED' value (or use PERF_BR_UNKNOWN?) which means read a > new 'arch_type' field. Yup; something of that shape sounds good to me -- that was roughly what I had suggested elsewhere. > Another option is maybe some of these additional types just shouldn't be > exposed to userspace? For example, are branches to FIQ useful or leaking > any info about secure world? Debug mode branches also seem minimally > useful to me (though I'm no expert in how this is used). I agree; this wasn't clear to me, and regardless I think many of the types added in the prior patch should not be generic since they're very specific to the Arm architecture. > > (b) Depending on how the field is placed, existing values might get stored > > differently. This could break any mismatched combination of > > {old,new}-kernel and {old,new}-userspace. > > > > In practice, I think this means that this is broken for BE, and happens to > > work for LE, but I don't know how bitfields are defined for each > > architecture, so there could be other brokenness. > > > > Consider the test case below: > > [...] > > > ... where the low bits of the field have moved, and so this is broken even for > > existing values! > > So that is a separate issue to be fixed and not directly related to the > size of 'type'. I agree if you moved the entire field that's broken everywhere, but in this case it *is* directly related to the size changing. In my example the meaning of specific bits changed *because* the size of the field changed and in BE that meant the low bits of the field moved, even though the field started at the same position. > Looks like it needs similar '#if > defined(__LITTLE_ENDIAN_BITFIELD)' treatment as some of the other struct > bitfields. Though somehow BE PPC hasn't had issues? IIRC there were recent problems in this area, and I think historically we've broken ABI and people only noticed much later. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel