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 D8D66CD6907 for ; Tue, 10 Oct 2023 08:29:01 +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=QKSVPXLObdg9rbU4uNk/D/ipIU1jx2Q9tfXrYpnuxY4=; b=b082nh6SWfYqpS NMFyvtOlsVEeK0t59xECsmmRVMLj1amlSf0iKyiSMbanP9jkdzDTTge9XvWOuhf3ijjlik7iiPS/P TCvbyrE+O8KTMr5aatfauZsfyRnI9WcA7d76yd0lqJ0iTI2OkyXOyKz+phP+j5l5XhB6VY/LXpaE9 tSbbV9/29+6AfxToAudePCzgWegkjYL9yshjaB+4EjkCt8dWWgHDPZQqJH6Qx0u5qt4CrGZGtJ9OL jc1IDBoYib0bgpcX99QX01a/+IHiFDAZ/jkzk8G+APvA28S/NjokYkAeXTJOgkasmw1PBa6E2g5T6 Gz1ve5P7Nl9roPwRexcA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qq86W-00Cq23-2f; Tue, 10 Oct 2023 08:28:36 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qq86T-00Cq1S-0U for linux-arm-kernel@lists.infradead.org; Tue, 10 Oct 2023 08:28:35 +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 AC73F1FB; Tue, 10 Oct 2023 01:29:11 -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 BC76A3F7A6; Tue, 10 Oct 2023 01:28:29 -0700 (PDT) Message-ID: <948c6cdd-5a46-6b4b-37a9-9f971a4853a8@arm.com> Date: Tue, 10 Oct 2023 09:28:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [PATCH] drivers: perf: arm_pmuv3: Update 'pmc_width' based on actual HW event width Content-Language: en-US To: Anshuman Khandual , linux-arm-kernel@lists.infradead.org, Rob Herring , raphael.gault@arm.com Cc: Will Deacon , Mark Rutland , linux-kernel@vger.kernel.org References: <20231009043724.175100-1-anshuman.khandual@arm.com> <66125bc2-ff2a-a209-a753-c2e3d8310a45@arm.com> <8a5d2411-3fab-4f21-adb5-f99a2d4b3bee@arm.com> From: James Clark In-Reply-To: <8a5d2411-3fab-4f21-adb5-f99a2d4b3bee@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231010_012833_284125_86AC6E16 X-CRM114-Status: GOOD ( 32.58 ) 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 10/10/2023 04:03, Anshuman Khandual wrote: > Hi James, > > On 10/9/23 15:13, James Clark wrote: >> >> >> On 09/10/2023 05:37, Anshuman Khandual wrote: >>> This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's >>> width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT. >>> >> >> Might be worth adding why this is needed or what the actual effect is. > > To have correct 'pmc_width' visible to the user space ? Well yeah, but for example I didn't know what that was. And it's not clear why it needs updating at this point in time without a link to any other commit or relevant section from the Arm ARM. So I had a kind of a "why now" question. "To have correct 'pmc_width' visible to the user space" is definitely more of a what than a why. > >> >>> Cc: Will Deacon >>> Cc: Mark Rutland >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Anshuman Khandual >>> --- >>> This applies on v6.6-rc5. >>> >>> drivers/perf/arm_pmuv3.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c >>> index fe4db1831662..94723d00548e 100644 >>> --- a/drivers/perf/arm_pmuv3.c >>> +++ b/drivers/perf/arm_pmuv3.c >>> @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event, >>> if (userpg->cap_user_rdpmc) { >>> if (event->hw.flags & ARMPMU_EVT_64BIT) >>> userpg->pmc_width = 64; >>> + else if (event->hw.flags & ARMPMU_EVT_63BIT) >>> + userpg->pmc_width = 63; >>> + else if (event->hw.flags & ARMPMU_EVT_47BIT) >>> + userpg->pmc_width = 47; >> >> Although it doesn't explicitly say it, the bit of the docs about >> pmc_width in Documentation/arch/arm64/perf.rst loosely implies that this >> is always either 64 or 32. Now that this isn't the case it could mislead >> someone in userspace that they don't have to handle the now arbitrary >> bit widths rather than just whole bytes/ints. > > Are you suggesting that the user space would not handle pmc_width correctly > , once it deviates from a whole bytes/ints format ? In that case user space > handling might need some fixing. > Not really, I'm just suggesting that anyone writing a new tool and only reading the docs could make that assumption. Seeing as only 32 and 64 bit options are mentioned. So it's more to avoid misleading someone in the future than about fixing any existing code, as updating the docs wouldn't have that effect. >> >> I think the fix is as simple as adding something like "the width may not >> match the requested value or necessarily be a multiple of 8". Unless we >> think this is already widely known and I suppose we could leave it as >> is. (The existing bit in perf that uses it already handles it correctly). > > This is from perf_event_mmap_page definition where it does not assert the > width to be multiple of bytes or ints. Hence the assumption should not be > made into the user space tools. > Yeah I know its already ok for Perf which is why I mentioned it. But there are more tools out there than Perf, and ones that don't even exist yet, which people would normally read the documentation before writing. > /* > * If cap_user_rdpmc this field provides the bit-width of the value > * read using the rdpmc() or equivalent instruction. This can be used > * to sign extend the result like: > * > * pmc <<= 64 - width; > * pmc >>= 64 - width; // signed shift right > * count += pmc; > */ > __u16 pmc_width; > > Moreover, on x86 too 'userpg->pmc_width' gets assigned to different values > although multiple of 8. > > userpg->pmc_width = x86_pmu.cntval_bits > arch/x86/events/amd/core.c: .cntval_bits = 48 > arch/x86/events/intel/knc.c: .cntval_bits = 40 > arch/x86/events/intel/p6.c: .cntval_bits = 32 > >> >>> else >>> userpg->pmc_width = 32; >>> } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel