This was developed by Semihalf under the sponsorship of the FreeBSD Foundation.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
@zbb, let me know if you'd like to do the commit to HEAD, but I can take care of it otherwise - I'd just like to get this in as early as possible.
Comment Actions
First pass though the full patch (and not as a collection of smaller patches).
The cache has been enabled and we have pmap_extract.
In general a call to WRITE_SPECIALREG should be followed by isb() if the code after it relies on the write.
| sys/arm64/arm64/db_interface.c | ||
|---|---|---|
| 110 | pmap_extract is implemented. | |
| 164 | We have the D-cache enabled now. | |
| sys/arm64/arm64/debug_monitor.c | ||
| 207 | If the order of these writes is important there should be calls to isb() after them to ensure no later instructions are executed before the earlier instructions. | |
| 339 | In what cases can this be called? i.e. is there any way we could be scheduled off this cpu (I assume not given it's part of ddb). If it could be called from a thread that could be scheduled out we would need to add a critical section. | |
| 475 | braekpoints? | |
| sys/arm64/arm64/db_trace.c | ||
|---|---|---|
| 114 | Good question - I actually prefer %08x for addresses that happen to be in userland, but it should probably be %0x16 so that everything lines up. This is what we get today: ... do_el0_sync() at handle_el0_sync+0x54 pc = 0xffffff80003d02c4 lr = 0xffffff80003c51b0 sp = 0xffffff8056fc1ab0 fp = 0x7ffffff610 handle_el0_sync() at 0x40466068 pc = 0xffffff80003c51b0 lr = 0x40466068 sp = 0x7ffffff620 fp = 0x7ffffff660 $d.6() at 0x403198 pc = 0x40466068 lr = 0x00403198 sp = 0x7ffffff670 fp = 0x7ffffffb00 ... | |
Comment Actions
Arg, I generated the new diff without context and that confused Phabricator on files.amd64. There is no change in the 2nd upload there.
(I'd expect Phabricator's diff parser to use the line number in the diff to set the line numbers there, though.)
Comment Actions
As discussed this morning @zbb will take care of committing to HEAD.
The updated version of this change (copyright dates, typos etc.) are in the GitHub FreeBSDFoundation/arm64-head-ddb branch now - this commit:
https://github.com/FreeBSDFoundation/freebsd/commit/53fb4e609e9f4f48504da9a2bd782ba22835f549
Comment Actions
Looks good to me - two comments from @andrew that still need a response, but I would be happy to have this committed now.
If the order of these writes is important there should be calls to isb() after them to ensure no later instructions are executed before the earlier instructions.
In what cases can this be called? i.e. is there any way we could be scheduled off this cpu (I assume not given it's part of ddb). If it could be called from a thread that could be scheduled out we would need to add a critical section.
| sys/arm64/arm64/db_interface.c | ||
|---|---|---|
| 116 | This should be p != NULL || p->p_vmspace != NULL || | |
| 134 | u_int? It's too small. | |
| 153 | u_int | |
| sys/arm64/arm64/db_trace.c | ||
| 82 | What are these magic numbers? | |
| sys/arm64/arm64/debug_monitor.c | ||
| 87 | Could this be READ_SPECIALREG? | |
| 187 | Should there be an isb to ensure the above registers have been written (as per the ARMv8A ARM)? | |
| 206 | Could we change this to read mdscr_el1 to a variable, modify it as needed, then write it back? | |
| 446 | Do we need an instruction barrier between these? i.e. could the disable be reordered before the write, and if so would it be bad? | |