Support for unaligned watchpoints in arm/arm64

Support for unaligned watchpoints in arm/arm64

[Pavel Labath]

Hello all,

I've been wondering if there are any plans about adding support for
unaligned watchpoints to the kernel. It seems quite a shame that
applications are not able not use them, even though the hardware
should support that feature.

I've filed a feature request at
<https://bugzilla.kernel.org/show_bug.cgi?id=117621> with a bit more
details about the issue.

cheers,
pl

Support for unaligned watchpoints in arm/arm64

[Will Deacon]

On Thu, May 26, 2016 at 05:04:46PM +0100, Pavel Labath wrote:
> Hello all,

Hi Pavel,

> I've been wondering if there are any plans about adding support for
> unaligned watchpoints to the kernel. It seems quite a shame that
> applications are not able not use them, even though the hardware
> should support that feature.

I'm actually coming round to the idea of ditching the perf hw_breakpoint
mechanism entirely and simply writing a ptrace back-end that can expose
the hardware features directly to userspace. The two issues with this
are:

  (1) It's a fair amount of work
  (2) We might already have users of the perf interface (including compat)

I'm really not happy with the way hw_breakpoint worked out :(

Will

Support for unaligned watchpoints in arm/arm64

[Pratyush Anand]

On 31/05/2016:01:38:27 PM, Will Deacon wrote:
> On Thu, May 26, 2016 at 05:04:46PM +0100, Pavel Labath wrote:
> > Hello all,
> 
> Hi Pavel,
> 
> > I've been wondering if there are any plans about adding support for
> > unaligned watchpoints to the kernel. It seems quite a shame that
> > applications are not able not use them, even though the hardware
> > should support that feature.
> 
> I'm actually coming round to the idea of ditching the perf hw_breakpoint
> mechanism entirely and simply writing a ptrace back-end that can expose
> the hardware features directly to userspace. The two issues with this
> are:
> 
>   (1) It's a fair amount of work

So, by the time this new interface would come, probably we can consider a fixup
like following to resolve this issue at hand. Probably, things should work by
just allowing hw_breakpoint.c to pass checks for unaligned offset when it is a
WATCHPOINT, no?

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 26a6bf77d272..c803347c1413 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -384,7 +384,12 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
                *gen_len = HW_BREAKPOINT_LEN_8;
@@ -384,7 +384,12 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
                *gen_len = HW_BREAKPOINT_LEN_8;
                break;
        default:
-               return -EINVAL;
+               if (ctrl.type == ARM_BREAKPOINT_EXECUTE
+                               || ctrl.len & (ctrl.len + 1)
+                               || ctrl.len < ARM_BREAKPOINT_LEN_1
+                               || ctrl.len > ARM_BREAKPOINT_LEN_8)
+                       return -EINVAL;
+               *gen_len = ffs(ctrl.len + 1) - 1;
        }

        return 0;
@@ -430,7 +435,11 @@ static int arch_build_bp_info(struct perf_event *bp)
                info->ctrl.len = ARM_BREAKPOINT_LEN_8;
                break;
        default:
-               return -EINVAL;
+               if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE
+                               || bp->attr.bp_len < HW_BREAKPOINT_LEN_1
+                               || bp->attr.bp_len > HW_BREAKPOINT_LEN_8)
+                       return -EINVAL;
+               info->ctrl.len = (1 << bp->attr.bp_len) - 1;
        }

        /*

@Pavel, does above patch helps to resolve the issue.

~Pratyush

>   (2) We might already have users of the perf interface (including compat)
> 
> I'm really not happy with the way hw_breakpoint worked out :(
> 
> Will
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Support for unaligned watchpoints in arm/arm64

[Pratyush Anand]

On 07/06/2016:12:25:20 PM, Pratyush Anand wrote:
> On 31/05/2016:01:38:27 PM, Will Deacon wrote:
> > On Thu, May 26, 2016 at 05:04:46PM +0100, Pavel Labath wrote:
> > > Hello all,
> > 
> > Hi Pavel,
> > 
> > > I've been wondering if there are any plans about adding support for
> > > unaligned watchpoints to the kernel. It seems quite a shame that
> > > applications are not able not use them, even though the hardware
> > > should support that feature.
> > 
> > I'm actually coming round to the idea of ditching the perf hw_breakpoint
> > mechanism entirely and simply writing a ptrace back-end that can expose
> > the hardware features directly to userspace. The two issues with this
> > are:
> > 
> >   (1) It's a fair amount of work
> 
> So, by the time this new interface would come, probably we can consider a fixup
> like following to resolve this issue at hand. Probably, things should work by
> just allowing hw_breakpoint.c to pass checks for unaligned offset when it is a
> WATCHPOINT, no?

Sorry, this is not going to work.
There could be some BAS values where few consecutive LSBs are 0s.

~Pratyush
> 
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index 26a6bf77d272..c803347c1413 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -384,7 +384,12 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>                 *gen_len = HW_BREAKPOINT_LEN_8;
> @@ -384,7 +384,12 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>                 *gen_len = HW_BREAKPOINT_LEN_8;
>                 break;
>         default:
> -               return -EINVAL;
> +               if (ctrl.type == ARM_BREAKPOINT_EXECUTE
> +                               || ctrl.len & (ctrl.len + 1)
> +                               || ctrl.len < ARM_BREAKPOINT_LEN_1
> +                               || ctrl.len > ARM_BREAKPOINT_LEN_8)
> +                       return -EINVAL;
> +               *gen_len = ffs(ctrl.len + 1) - 1;
>         }
> 
>         return 0;
> @@ -430,7 +435,11 @@ static int arch_build_bp_info(struct perf_event *bp)
>                 info->ctrl.len = ARM_BREAKPOINT_LEN_8;
>                 break;
>         default:
> -               return -EINVAL;
> +               if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE
> +                               || bp->attr.bp_len < HW_BREAKPOINT_LEN_1
> +                               || bp->attr.bp_len > HW_BREAKPOINT_LEN_8)
> +                       return -EINVAL;
> +               info->ctrl.len = (1 << bp->attr.bp_len) - 1;
>         }
> 
>         /*
> 
> @Pavel, does above patch helps to resolve the issue.
> 
> ~Pratyush
> 
> >   (2) We might already have users of the perf interface (including compat)
> > 
> > I'm really not happy with the way hw_breakpoint worked out :(
> > 
> > Will
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Support for unaligned watchpoints in arm/arm64

[Pavel Labath]

On 7 June 2016 at 08:17, Pratyush Anand <panand@redhat.com> wrote:
> On 07/06/2016:12:25:20 PM, Pratyush Anand wrote:
>> On 31/05/2016:01:38:27 PM, Will Deacon wrote:
>> > On Thu, May 26, 2016 at 05:04:46PM +0100, Pavel Labath wrote:
>> > > Hello all,
>> >
>> > Hi Pavel,
>> >
>> > > I've been wondering if there are any plans about adding support for
>> > > unaligned watchpoints to the kernel. It seems quite a shame that
>> > > applications are not able not use them, even though the hardware
>> > > should support that feature.
>> >
>> > I'm actually coming round to the idea of ditching the perf hw_breakpoint
>> > mechanism entirely and simply writing a ptrace back-end that can expose
>> > the hardware features directly to userspace. The two issues with this
>> > are:
>> >
>> >   (1) It's a fair amount of work
>>
>> So, by the time this new interface would come, probably we can consider a fixup
>> like following to resolve this issue at hand. Probably, things should work by
>> just allowing hw_breakpoint.c to pass checks for unaligned offset when it is a
>> WATCHPOINT, no?
>
> Sorry, this is not going to work.
> There could be some BAS values where few consecutive LSBs are 0s.
>

Thank you for the replies.

I would definitely welcome a more immediate fix to this issue,
although I can appreciate the reluctance to add ugly fixups to make
this work.

Pratyush, building on your proposed fix, I think we could extend it to
make it work for watching values which are not word-aligned by
tweaking  perf_event_attr.bp_addr field. E.g. if the BAS value is
00111100 then we can set bp_addr to "aligned_addr + 2" and bp_len to
4. From an interface point of view I think this would be a nice
solution. ptrace userspace user would set these breakpoints just like
it would do when programming a real register, and the contents of the
struct perf_event_attr will make perfect sense: "we have set a
watchpoint at bp_addr with length bp_len". The only problem is that
the code for converting between the perf_event_attr and hardware
register representations will be quite ugly. Also, I don't know if
this will cause a problem with any other users of perf_event_attr if
they expect bp_addr field to be word-aligned.

If you would be willing to consider such a patch, I can take a stab at
writing it. What do you think?

regards,
pl

Support for unaligned watchpoints in arm/arm64

[Pratyush Anand]

Hi Pavel,

On 07/06/2016:05:15:03 PM, Pavel Labath wrote:
> On 7 June 2016 at 08:17, Pratyush Anand <panand@redhat.com> wrote:
> > On 07/06/2016:12:25:20 PM, Pratyush Anand wrote:
> >> On 31/05/2016:01:38:27 PM, Will Deacon wrote:
> >> > On Thu, May 26, 2016 at 05:04:46PM +0100, Pavel Labath wrote:
> >> > > Hello all,
> >> >
> >> > Hi Pavel,
> >> >
> >> > > I've been wondering if there are any plans about adding support for
> >> > > unaligned watchpoints to the kernel. It seems quite a shame that
> >> > > applications are not able not use them, even though the hardware
> >> > > should support that feature.
> >> >
> >> > I'm actually coming round to the idea of ditching the perf hw_breakpoint
> >> > mechanism entirely and simply writing a ptrace back-end that can expose
> >> > the hardware features directly to userspace. The two issues with this
> >> > are:
> >> >
> >> >   (1) It's a fair amount of work
> >>
> >> So, by the time this new interface would come, probably we can consider a fixup
> >> like following to resolve this issue at hand. Probably, things should work by
> >> just allowing hw_breakpoint.c to pass checks for unaligned offset when it is a
> >> WATCHPOINT, no?
> >
> > Sorry, this is not going to work.
> > There could be some BAS values where few consecutive LSBs are 0s.
> >
> 
> Thank you for the replies.
> 
> I would definitely welcome a more immediate fix to this issue,
> although I can appreciate the reluctance to add ugly fixups to make
> this work.
> 
> Pratyush, building on your proposed fix, I think we could extend it to
> make it work for watching values which are not word-aligned by
> tweaking  perf_event_attr.bp_addr field. E.g. if the BAS value is
> 00111100 then we can set bp_addr to "aligned_addr + 2" and bp_len to

I think, DBGWVR will receive only aligned address.

> 4. From an interface point of view I think this would be a nice
> solution. ptrace userspace user would set these breakpoints just like
> it would do when programming a real register, and the contents of the
> struct perf_event_attr will make perfect sense: "we have set a
> watchpoint at bp_addr with length bp_len". The only problem is that
> the code for converting between the perf_event_attr and hardware
> register representations will be quite ugly. Also, I don't know if

Yes, will have to adjust programming of both DBGWVR and DBGWCR  if bp_addr is
unaligned.

> this will cause a problem with any other users of perf_event_attr if
> they expect bp_addr field to be word-aligned.
> 
> If you would be willing to consider such a patch, I can take a stab at
> writing it. What do you think?

I am not the right person to answer this. Will Deacon is.

Thanks for your willingness to help on it :-)

~Pratyush