| Message ID | 20160607065520.GD13643@dhcppc6 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
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
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; }