Page MenuHomeFreeBSD

Introduce support for HW watchpoints and single stepping for ARMv6/v7
ClosedPublic

Authored by zbb on Oct 30 2015, 4:17 PM.

Details

Summary

Allows for using hardware watchpoints for 1, 2, 4, 8 byte long addresses.
The default configuration of watchpoint is RW but code allows to select
RO or WO and X.
Since debugging registers are per-CPU (CP14) the watchpoint is set on
the CPU that was lucky (or not) to enter DDB.

HW breakpoints are used to perform single step in KDB.
When HW breakpoint is enabled all watchpoints are temporary disabled
to avoid recursive abort on both watchpoint and breakpoint.
In case of branch, the breakpoint is set to both - next instruction
and possible branch address. This requires at least 2 breakpoints
supported in the CPU however this is a must for ARMv6/v7 CPUs.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

zbb retitled this revision from to Introduce support for HW watchpoints and single stepping for ARMv6/v7.
zbb updated this object.
zbb edited the test plan for this revision. (Show Details)
zbb added reviewers: ian, imp, andrew, br.
zbb set the repository for this revision to rS FreeBSD src repository - subversion.
imp edited edge metadata.

This looks good to me, but I don't know the arm debug registers enough to know if they are right, so others should look at this too.

This revision is now accepted and ready to land.Oct 30 2015, 4:28 PM
andrew requested changes to this revision.Oct 30 2015, 4:30 PM
andrew edited edge metadata.

I can see some style issue, and would like to check a few things.

This revision now requires changes to proceed.Oct 30 2015, 4:30 PM
In D4037#84452, @andrew wrote:

I can see some style issue, and would like to check a few things.

Thanks. Understood.
I'm waiting for details.

sys/arm/arm/debug_monitor.c
305 ↗(On Diff #9825)

Why 0x3?

461 ↗(On Diff #9825)

Why not just make this, and all calls to dprintf, a db_printf?

596 ↗(On Diff #9825)

Does this not need an isb?

872 ↗(On Diff #9825)

Can we rely on the isb below for this?

sys/arm/include/db_machdep.h
53 ↗(On Diff #9825)
#include <machine/acle-compat.h>
...
#if __ARM_ARCH >= 6
sys/arm/include/kdb.h
39 ↗(On Diff #9825)
#include <machine/acle-compat.h>
...
#if __ARM_ARCH >= 6
sys/conf/files.arm
30 ↗(On Diff #9825)

Does this build on arm < v6? It seems to use registers not present there.

sys/arm/arm/debug_monitor.c
872 ↗(On Diff #9825)

We need to be sure that OS lock is removed before accessing register below.

sys/conf/files.arm
30 ↗(On Diff #9825)

Yes, it should be armv6 dependent

zbb edited edge metadata.
zbb removed rS FreeBSD src repository - subversion as the repository for this revision.
sys/arm/arm/db_trace.c
135 ↗(On Diff #9890)

It doesn't seem this is defined on arm < v6.

zbb marked 10 inline comments as done.Nov 3 2015, 5:13 PM

Thanks. Any more comments?

So... do you accept this revision?

Are you sure that CP14 is accessible on all v6+ cores (including qcom, marvell ones) ? Under all circumstances (e.g. in secure / not-secure world) ? If yes, i'm fine with this patch.

sys/arm/arm/debug_monitor.c
120–135 ↗(On Diff #9904)

These and below should be #define<tab>DBG_FOO.

491 ↗(On Diff #9904)

Why ~0U? This returns a signed integer.

496 ↗(On Diff #9904)

i is a u_int, but this functions return an int.

537–540 ↗(On Diff #9904)

return ((cp14_dbgdscrint_get() & DBGSCR_MDBG_EN) != 0);

zbb marked 4 inline comments as done.Nov 30 2015, 1:27 PM

You might find the r250851 interesting, it fixed hw watchpoints for the SMP amd64 ddb.

In D4037#90965, @kib wrote:

You might find the r250851 interesting, it fixed hw watchpoints for the SMP amd64 ddb.

Thanks. I will add it as a separate commit. Please check out this one: https://reviews.freebsd.org/D4338

This revision was automatically updated to reflect the committed changes.