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 <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
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;
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;
}
--
2.25.1
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 <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> 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 <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> 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 <[email protected]>
> >> Cc: Mark Rutland <[email protected]>
> >> Cc: Catalin Marinas <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Signed-off-by: Anshuman Khandual <[email protected]>
> >> ---
> >> 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 <[email protected]>
>>>> Cc: Mark Rutland <[email protected]>
>>>> Cc: Catalin Marinas <[email protected]>
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>>> ---
>>>> 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.