Message ID | 20240223113102.4027779-1-anshuman.khandual@arm.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-78221-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:a81b:b0:108:e6aa:91d0 with SMTP id bq27csp522332dyb; Fri, 23 Feb 2024 03:35:03 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCXoTQMDMj7ILWa7PBjHR+1xuQcno8DKreHyCcHYig3JCswtTmBjNXUvb9SfKbk0blk+xqRPh/3A/cgNeXnPeSUsWypyAQ== X-Google-Smtp-Source: AGHT+IFZiUqtr8AS/J8R2BpSnE5xC11NivBl21VhXeeHtJcMvz5IXaITUjCxwxYwyuehizF8ZYvN X-Received: by 2002:a17:90b:90f:b0:29a:3eae:c4b6 with SMTP id bo15-20020a17090b090f00b0029a3eaec4b6mr1206982pjb.28.1708688103074; Fri, 23 Feb 2024 03:35:03 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708688103; cv=pass; d=google.com; s=arc-20160816; b=l9jdris0d3ibNqhh1u2wcUbWhcjs7/7BWOkU9N5Fcl5wTEwz0XU9nvw7t+ubRJt1SF B4t3l+w1YMUPJCxcs+IPkyWWiBbP+WFg8/XuI1cnIzKGd8wqw7TtiWh9ZJmRaSNBRQK+ W4PS/iOMkzTsc2Wx55JTrfPAGaxgYaN3wDO4fLWOoF9F5LEYTCfRCnwPZhAQWrl9Q0Vt unOUpbbSq4qR6KF+Cv//Pzao8SWyxH1XiV3grY1PGIZRPLezw0HXpnQMf0FUOoqmEC/m Ssh9nC3/AZUZW0bQWZYCf75qNOBnvHUAeVEMck50QPs0A6tZNzBiqb4ffuYdTsxl5Ie5 Luhw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from; bh=aG0Q3tmaMFT7fXkXeN/yYv7bS6ZLDfO7VWog9ZNRtB4=; fh=MXolhX1m9kctuPXyDYxFwIPst9rjHcYkNAPnI2NP2KE=; b=OEcddtpaCwBzDatDkWm+UkYd8aEHRs3cHbPYiRlc27VlrO2kRKIG8UWIaoXirf7tDP VSQUgltgkZlwYgzYZy8oG2j4LAlIial3uwPuPe4xcal4yM99ddZ6oFDIKexU0k6/u0xH NCLr0siLHzXBbDWfapcNwG64DBBuNTa5GMSN7/gsQ3+4VdQyiKvbiuV7Dyaj2i4v0GDn UZ4NS8NNPX+KlAzn8m52cMPatapteZIzG8hhMUoAA8qKUqO1gESaCj6PnAL2T9y3DBID zpIBHdmmsyI5gSAKu6SEbfMDTFLf2cY7iNWG2X5KIQJJzRvnRZcxp0IsJ8AD+YuiAzvC 6eYQ==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; arc=pass (i=1 spf=pass spfdomain=arm.com dmarc=pass fromdomain=arm.com); spf=pass (google.com: domain of linux-kernel+bounces-78221-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-78221-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [147.75.48.161]) by mx.google.com with ESMTPS id su16-20020a17090b535000b0029a7246d150si1022852pjb.84.2024.02.23.03.35.02 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 23 Feb 2024 03:35:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-78221-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161; Authentication-Results: mx.google.com; arc=pass (i=1 spf=pass spfdomain=arm.com dmarc=pass fromdomain=arm.com); spf=pass (google.com: domain of linux-kernel+bounces-78221-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-78221-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id F297EB23A89 for <ouuuleilei@gmail.com>; Fri, 23 Feb 2024 11:31:38 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 7B62579DB5; Fri, 23 Feb 2024 11:31:19 +0000 (UTC) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id B91F478667 for <linux-kernel@vger.kernel.org>; Fri, 23 Feb 2024 11:31:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708687878; cv=none; b=n1tb2lpfGlLTJkmBV66f7OlVDjXBOwtWJHTacJ1KRFraF7baWN2c3TiKo7lUH4IrNS2dsN9E6b40xdzx6nVhy+mIFvGMZSgy9q/OizWQ87ZJ+cn0wR9r+6Yme4CubdNI30KYSwErZ8uyC0Dl2ezvVEk7gkENQfUdVIieptGWm6M= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708687878; c=relaxed/simple; bh=mKlrDlToVS/Maui1mRuEIRqDVpR4qfopYdpXi7rgAME=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version; b=VokkERuW7mhLNEpc3KBMWEriaYarDrf+B91GOCPAjWwh1EdyAxt4VS5Te3YuuuIbqBvQBXq0W0GSeBtMKO45gGoUWhZddYTIxYlId3BDJfzTtyUVyNgER0pQc7zFZ6uNXHCz1+cxYrD5bBfCYNqdXsRhDAv+Le4f3mKqeA9fWiw= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com 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 83A6811FB; Fri, 23 Feb 2024 03:31:53 -0800 (PST) Received: from a077893.arm.com (unknown [10.163.47.143]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 324713F762; Fri, 23 Feb 2024 03:31:11 -0800 (PST) From: Anshuman Khandual <anshuman.khandual@arm.com> To: linux-arm-kernel@lists.infradead.org Cc: broonie@kernel.org, Anshuman Khandual <anshuman.khandual@arm.com>, Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Catalin Marinas <catalin.marinas@arm.com>, linux-kernel@vger.kernel.org Subject: [PATCH] arm64/hw_breakpoint: Determine lengths from generic perf breakpoint macros Date: Fri, 23 Feb 2024 17:01:02 +0530 Message-Id: <20240223113102.4027779-1-anshuman.khandual@arm.com> X-Mailer: git-send-email 2.25.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791689336432734518 X-GMAIL-MSGID: 1791689336432734518 |
Series |
arm64/hw_breakpoint: Determine lengths from generic perf breakpoint macros
|
|
Commit Message
Anshuman Khandual
Feb. 23, 2024, 11:31 a.m. UTC
Both platform i.e ARM_BREAKPOINT_LEN_X and generic i.e HW_BREAKPOINT_LEN_X
macros are used interchangeably to convert event->attr.bp_len and platform
breakpoint control arch_hw_breakpoint_ctrl->len. Let's be consistent while
deriving one from the other. This does not cause any functional changes.
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This applies on v6.8-rc5
arch/arm64/kernel/hw_breakpoint.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Comments
On Fri, Feb 23, 2024 at 05:01:02PM +0530, Anshuman Khandual wrote: > Both platform i.e ARM_BREAKPOINT_LEN_X and generic i.e HW_BREAKPOINT_LEN_X > macros are used interchangeably to convert event->attr.bp_len and platform > breakpoint control arch_hw_breakpoint_ctrl->len. Let's be consistent while > deriving one from the other. This does not cause any functional changes. > > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > This applies on v6.8-rc5 > > arch/arm64/kernel/hw_breakpoint.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c > index 35225632d70a..1ab9fc865ddd 100644 > --- a/arch/arm64/kernel/hw_breakpoint.c > +++ b/arch/arm64/kernel/hw_breakpoint.c > @@ -301,28 +301,28 @@ static int get_hbp_len(u8 hbp_len) > > switch (hbp_len) { > case ARM_BREAKPOINT_LEN_1: > - len_in_bytes = 1; > + len_in_bytes = HW_BREAKPOINT_LEN_1; I don't think we should do this. The HW_BREAKPOINT_LEN_* definitions are part of the user ABI and, although they correspond to the length in bytes, that's not necessarily something we should rely on. Will
On 2/23/24 18:22, Will Deacon wrote: > On Fri, Feb 23, 2024 at 05:01:02PM +0530, Anshuman Khandual wrote: >> Both platform i.e ARM_BREAKPOINT_LEN_X and generic i.e HW_BREAKPOINT_LEN_X >> macros are used interchangeably to convert event->attr.bp_len and platform >> breakpoint control arch_hw_breakpoint_ctrl->len. Let's be consistent while >> deriving one from the other. This does not cause any functional changes. >> >> Cc: Will Deacon <will@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> This applies on v6.8-rc5 >> >> arch/arm64/kernel/hw_breakpoint.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c >> index 35225632d70a..1ab9fc865ddd 100644 >> --- a/arch/arm64/kernel/hw_breakpoint.c >> +++ b/arch/arm64/kernel/hw_breakpoint.c >> @@ -301,28 +301,28 @@ static int get_hbp_len(u8 hbp_len) >> >> switch (hbp_len) { >> case ARM_BREAKPOINT_LEN_1: >> - len_in_bytes = 1; >> + len_in_bytes = HW_BREAKPOINT_LEN_1; > > I don't think we should do this. The HW_BREAKPOINT_LEN_* definitions are > part of the user ABI and, although they correspond to the length in bytes, > that's not necessarily something we should rely on. Why should not we rely on the user ABI macros if these byte lengths were initially derived from them. But also there are similar conversions in arch_bp_generic_fields(). These hard coded raw byte length numbers seems cryptic, where as in reality these are just inter converted from generic HW breakpoints lengths.
On Mon, Feb 26, 2024 at 08:19:39AM +0530, Anshuman Khandual wrote: > On 2/23/24 18:22, Will Deacon wrote: > > On Fri, Feb 23, 2024 at 05:01:02PM +0530, Anshuman Khandual wrote: > >> Both platform i.e ARM_BREAKPOINT_LEN_X and generic i.e HW_BREAKPOINT_LEN_X > >> macros are used interchangeably to convert event->attr.bp_len and platform > >> breakpoint control arch_hw_breakpoint_ctrl->len. Let's be consistent while > >> deriving one from the other. This does not cause any functional changes. > >> > >> Cc: Will Deacon <will@kernel.org> > >> Cc: Mark Rutland <mark.rutland@arm.com> > >> Cc: Catalin Marinas <catalin.marinas@arm.com> > >> Cc: linux-arm-kernel@lists.infradead.org > >> Cc: linux-kernel@vger.kernel.org > >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > >> --- > >> This applies on v6.8-rc5 > >> > >> arch/arm64/kernel/hw_breakpoint.c | 16 ++++++++-------- > >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c > >> index 35225632d70a..1ab9fc865ddd 100644 > >> --- a/arch/arm64/kernel/hw_breakpoint.c > >> +++ b/arch/arm64/kernel/hw_breakpoint.c > >> @@ -301,28 +301,28 @@ static int get_hbp_len(u8 hbp_len) > >> > >> switch (hbp_len) { > >> case ARM_BREAKPOINT_LEN_1: > >> - len_in_bytes = 1; > >> + len_in_bytes = HW_BREAKPOINT_LEN_1; > > > > I don't think we should do this. The HW_BREAKPOINT_LEN_* definitions are > > part of the user ABI and, although they correspond to the length in bytes, > > that's not necessarily something we should rely on. > > Why should not we rely on the user ABI macros if these byte lengths were > initially derived from them. Why should we change the clear: len_in_bytes = 1; .. to the longer, and less clear: len_in_bytes = HW_BREAKPOINT_LEN_1; .. ? > But also there are similar conversions in arch_bp_generic_fields(). Those are specifically for converting from the rch_hw_breakpoint_ctrl encodings to the perf_event_attr encodings. There we don't care about the specific value of the byte, just that we're using the correct encoding. > These hard coded raw byte length numbers seems cryptic, where as in reality > these are just inter converted from generic HW breakpoints lengths. There are three distinct concepts here: 1. The length in bytes, as returned above by get_hbp_len() 2. The length as encoded in the ARM_BREAKPOINT_LEN_* encoding 3. The length as encoded in the HW_BREAKPOINT_LEN_* encoding. I think you're arguing that since 1 and 3 happen to have the values we should treat them as the same thing. I think that Will and I believe that they should be kept distinct because they are distinct concepts. I don't think this needs to change, and can be left as-is. Mark.
On 2/26/24 16:34, Mark Rutland wrote: > On Mon, Feb 26, 2024 at 08:19:39AM +0530, Anshuman Khandual wrote: >> On 2/23/24 18:22, Will Deacon wrote: >>> On Fri, Feb 23, 2024 at 05:01:02PM +0530, Anshuman Khandual wrote: >>>> Both platform i.e ARM_BREAKPOINT_LEN_X and generic i.e HW_BREAKPOINT_LEN_X >>>> macros are used interchangeably to convert event->attr.bp_len and platform >>>> breakpoint control arch_hw_breakpoint_ctrl->len. Let's be consistent while >>>> deriving one from the other. This does not cause any functional changes. >>>> >>>> Cc: Will Deacon <will@kernel.org> >>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>> Cc: linux-arm-kernel@lists.infradead.org >>>> Cc: linux-kernel@vger.kernel.org >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>>> --- >>>> This applies on v6.8-rc5 >>>> >>>> arch/arm64/kernel/hw_breakpoint.c | 16 ++++++++-------- >>>> 1 file changed, 8 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c >>>> index 35225632d70a..1ab9fc865ddd 100644 >>>> --- a/arch/arm64/kernel/hw_breakpoint.c >>>> +++ b/arch/arm64/kernel/hw_breakpoint.c >>>> @@ -301,28 +301,28 @@ static int get_hbp_len(u8 hbp_len) >>>> >>>> switch (hbp_len) { >>>> case ARM_BREAKPOINT_LEN_1: >>>> - len_in_bytes = 1; >>>> + len_in_bytes = HW_BREAKPOINT_LEN_1; >>> >>> I don't think we should do this. The HW_BREAKPOINT_LEN_* definitions are >>> part of the user ABI and, although they correspond to the length in bytes, >>> that's not necessarily something we should rely on. >> >> Why should not we rely on the user ABI macros if these byte lengths were >> initially derived from them. > > Why should we change the clear: > > len_in_bytes = 1; > > ... to the longer, and less clear: > > len_in_bytes = HW_BREAKPOINT_LEN_1; > > ... ? > >> But also there are similar conversions in arch_bp_generic_fields(). > > Those are specifically for converting from the rch_hw_breakpoint_ctrl encodings > to the perf_event_attr encodings. There we don't care about the specific value > of the byte, just that we're using the correct encoding. > >> These hard coded raw byte length numbers seems cryptic, where as in reality >> these are just inter converted from generic HW breakpoints lengths. > > There are three distinct concepts here: > > 1. The length in bytes, as returned above by get_hbp_len() > > 2. The length as encoded in the ARM_BREAKPOINT_LEN_* encoding > > 3. The length as encoded in the HW_BREAKPOINT_LEN_* encoding. > > I think you're arguing that since 1 and 3 happen to have the values we should > treat them as the same thing. I think that Will and I believe that they should > be kept distinct because they are distinct concepts. > > I don't think this needs to change, and can be left as-is. Fair enough, but just wondering how about deriving len_in_bytes from hweight_long(ARM_BREAKPOINT_LEN_*) instead ? This also drops the hard coding using the platform macros itself, without going to user ABI.
On Tue, Feb 27, 2024 at 11:01:54AM +0530, Anshuman Khandual wrote: > On 2/26/24 16:34, Mark Rutland wrote: > > I don't think this needs to change, and can be left as-is. > > Fair enough, but just wondering how about deriving len_in_bytes from > hweight_long(ARM_BREAKPOINT_LEN_*) instead ? This also drops the hard > coding using the platform macros itself, without going to user ABI. Please leave this code alone. It's fine like it is and there are plenty of other things that would actually benefit from your attention. The BRBE series, for example. Will
On 2/27/24 14:35, Will Deacon wrote: > On Tue, Feb 27, 2024 at 11:01:54AM +0530, Anshuman Khandual wrote: >> On 2/26/24 16:34, Mark Rutland wrote: >>> I don't think this needs to change, and can be left as-is. >> >> Fair enough, but just wondering how about deriving len_in_bytes from >> hweight_long(ARM_BREAKPOINT_LEN_*) instead ? This also drops the hard >> coding using the platform macros itself, without going to user ABI. > > Please leave this code alone. It's fine like it is and there are plenty of > other things that would actually benefit from your attention. The BRBE > series, for example. Sure, no problem, will drop this patch.
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index 35225632d70a..1ab9fc865ddd 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -301,28 +301,28 @@ static int get_hbp_len(u8 hbp_len) switch (hbp_len) { case ARM_BREAKPOINT_LEN_1: - len_in_bytes = 1; + len_in_bytes = HW_BREAKPOINT_LEN_1; break; case ARM_BREAKPOINT_LEN_2: - len_in_bytes = 2; + len_in_bytes = HW_BREAKPOINT_LEN_2; break; case ARM_BREAKPOINT_LEN_3: - len_in_bytes = 3; + len_in_bytes = HW_BREAKPOINT_LEN_3; break; case ARM_BREAKPOINT_LEN_4: - len_in_bytes = 4; + len_in_bytes = HW_BREAKPOINT_LEN_4; break; case ARM_BREAKPOINT_LEN_5: - len_in_bytes = 5; + len_in_bytes = HW_BREAKPOINT_LEN_5; break; case ARM_BREAKPOINT_LEN_6: - len_in_bytes = 6; + len_in_bytes = HW_BREAKPOINT_LEN_6; break; case ARM_BREAKPOINT_LEN_7: - len_in_bytes = 7; + len_in_bytes = HW_BREAKPOINT_LEN_7; break; case ARM_BREAKPOINT_LEN_8: - len_in_bytes = 8; + len_in_bytes = HW_BREAKPOINT_LEN_8; break; }