Received-SPF: pass (google.com: domain of linux-kernel+bounces-80423-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <1901fadb-1d71-4374-be8c-00935bb27854@arm.com>
Date: Mon, 26 Feb 2024 08:19:39 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] arm64/hw_breakpoint: Determine lengths from generic perf
 breakpoint macros
Content-Language: en-US
To: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org, broonie@kernel.org,
 Mark Rutland <mark.rutland@arm.com>,
 Catalin Marinas <catalin.marinas@arm.com>, linux-kernel@vger.kernel.org
References: <20240223113102.4027779-1-anshuman.khandual@arm.com>
 <20240223125224.GC10641@willie-the-truck>
From: Anshuman Khandual <anshuman.khandual@arm.com>
In-Reply-To: <20240223125224.GC10641@willie-the-truck>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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.