target/riscv: support multiple DMs #875

zqb-all · 2023-06-27T08:39:28Z

Support assign DMI address of the debug module by pass -dbgbase to the target create command

Change-Id: I774c3746567f6e6d77c43a62dea5e9e67bb25770

zqb-all · 2023-06-27T09:09:34Z

There is an issue about multiple DMs: #377

This patch need to add some checks mentioned in that issue:

There should be validity checks for the address.
That it is unique.
That such debug module really exists.

I sent this patch out because I had some questions and want to see if anyone could give me some advice:

I noticed that select_dtmcontrol in dtmcontrol_scan https://github.com/riscv/riscv-openocd/blob/a45589d60aa6864475fddcded885c8ff47d50be1/src/target/riscv/riscv-013.c#L434 is init with value DTMCONTROL https://github.com/riscv/riscv-openocd/blob/a45589d60aa6864475fddcded885c8ff47d50be1/src/target/riscv/riscv.c#L40-L44 for multiple DMs, can I just add dbgbase to select_dtmcontrol.out_value before do jtag_add_ir_scan ?
there is a TODO comment about multiple DMs. https://github.com/riscv/riscv-openocd/blob/a45589d60aa6864475fddcded885c8ff47d50be1/src/target/riscv/riscv-013.c#L1715 Any advice about how to fix it?

timsifive

This looks like it's heading in the right direction.

src/target/riscv/riscv-013.c

timsifive · 2023-06-27T17:36:22Z

I don't think you can update the global variable, because the same one is currently used for every target. You'll have to make a new one for every scan because you never know which DM you'll be asked to talk to. (If you really want you could store a computed value in dm013_info_t, but I doubt the performance difference would be noticeable.)
info->index stores the debug index in the DM that it is in. The first hart in each DM has debug index 0. target->coreid is globally unique in OpenOCD. So if there are 2 DMs on a single OpenOCD, then those numbers can't be the same. I think you'll have to track for each DM the highest index seen so far, and assume that the next created target is the next one.

timsifive · 2023-06-27T17:36:31Z

How are you testing these changes?

zqb-all · 2023-06-28T01:44:24Z

info->index stores the debug index in the DM that it is in. The first hart in each DM has debug index 0. target->coreid is globally unique in OpenOCD. So if there are 2 DMs on a single OpenOCD, then those numbers can't be the same. I think you'll have to track for each DM the highest index seen so far, and assume that the next created target is the next one.

I see, my hardware happens to have globally unique coreid, so it doesn't bother me here, but it would be better to add some protection

zqb-all · 2023-06-28T01:48:20Z

How are you testing these changes?

I test it on an fpga. With this patch, I can debug the cores on 2 different DMs

JanMatCodasip

Hi @zqb-all,
thank you for taking the time to prepare this change. I believe there are some items to fix - please, take a look at my comments.

src/target/riscv/riscv-013.c

zqb-all · 2023-06-28T15:44:27Z

Hi @zqb-all, thank you for taking the time to prepare this change. I believe there are some items to fix - please, take a look at my comments.

Thank you for your detailed review suggestions, which are very helpful. I will add them to this patch tomorrow

timsifive · 2023-06-28T16:44:32Z

I see, my hardware happens to have globally unique coreid, so it doesn't bother me here, but it would be better to add some protection

What do you mean by coreid? In the hardware there are 2 numbers that are relevant AFAIK:

The RISC-V hartid, readable in the mhartid CSR. These can be anything, but must be "unique within the execution environment."
The index of each hart on the Debug Module. For each Debug Module these must start at 0 and be contiguous.

zqb-all · 2023-06-28T17:09:53Z

What do you mean by coreid? In the hardware there are 2 numbers that are relevant AFAIK:

The RISC-V hartid, readable in the mhartid CSR. These can be anything, but must be "unique within the execution environment."

The index of each hart on the Debug Module. For each Debug Module these must start at 0 and be contiguous.

It seems that I misunderstood something, I thought you meant that the hart on different DMs might have same hartid.

Let me clarify my usage.

I mean the number which config as -coreid in target create is unique.
When I debug 2 DMs and each with 2 harts , my config file is like

# cluster 0
target create $_TARGETNAME_0 riscv -chain-position $_CHIPNAME.cpu -rtos hwthread
target create $_TARGETNAME_1 riscv -chain-position $_CHIPNAME.cpu -coreid 1

# cluster 1
target create $_TARGETNAME_4 riscv -chain-position $_CHIPNAME.cpu -coreid 4 -dbgbase 0x400
target create $_TARGETNAME_5 riscv -chain-position $_CHIPNAME.cpu -coreid 5 -dbgbase 0x400

target smp $_TARGETNAME_0 $_TARGETNAME_1 $_TARGETNAME_4 $_TARGETNAME_5

this config file works with the code
https://github.com/riscv/riscv-openocd/blob/a45589d60aa6864475fddcded885c8ff47d50be1/src/target/riscv/riscv-013.c#L1715-L1716

timsifive · 2023-06-28T17:48:12Z

So that then means that to select the first hart on cluster 1 (DM 1), you write 4 to dmcontrol.hartsel. If that works, then your implementation is not compliant with the debug spec. The first hart in a DM must be selected by dmcontrol.hartsel=0, the second by dmcontrol.hartsel=1, and so on. (This is so that it's possible for a debugger to automatically detect every hart on the DM. The fact that OpenOCD still requires you to manually list them is a problem with OpenOCD, but other debuggers exist.)

zqb-all · 2023-06-29T02:37:49Z

So that then means that to select the first hart on cluster 1 (DM 1), you write 4 to dmcontrol.hartsel.

Yes

If that works, then your implementation is not compliant with the debug spec. The first hart in a DM must be selected by dmcontrol.hartsel=0, the second by dmcontrol.hartsel=1, and so on.

This DM implementation only handle dmcontrol.hartsel=0/1/2/3 now，so when I write 4/5 to dmcontrol.hartsel, the high bits are ignored, just like write 0/1 to dmcontrol.hartsel

So it's a coincidence that it just works, I get the point now and will fix it.

when config with target create --coreid xxx, xxx means hartid and is unique. Then openocd should iterate over all harts and read out mhartid , if mhartid match xxx，set info->index. Is this the correct behavior we expect ?

zqb-all · 2023-06-29T14:35:15Z

when config with target create --coreid xxx, xxx means hartid and is unique. Then openocd should iterate over all harts and read out mhartid , if mhartid match xxx，set info->index. Is this the correct behavior we expect ?

Iterate over all harts (contain those not examine yet) and read the mhartid CSR, that doesn't sounds like a good idea,
Is there a better way for us to map hartid and info->index(The index of hart on the Debug Module) ?

timsifive · 2023-06-29T16:50:55Z

This DM implementation only handle dmcontrol.hartsel=0/1/2/3 now，so when I write 4/5 to dmcontrol.hartsel, the high bits are ignored, just like write 0/1 to dmcontrol.hartsel

That's a clever trick to make this work for your implementation. :)

when config with target create --coreid xxx, xxx means hartid and is unique. Then openocd should iterate over all harts and read out mhartid , if mhartid match xxx，set info->index. Is this the correct behavior we expect ?

I don't think so. It's conceivable that OpenOCD is debugging 2 completely separate RISC-V chips (on different scan chains, or even merely different DMs). Since they're completely separate, they could have conflicting hart ids. So we shouldn't force OpenOCD coreid to be the same as RISC-V hartid. At the same time, if it is possible then we do want them to be the same. It just makes everything much easier for users. For the purposes of this pull request, I don't think you need to do anything special regarding hartids. It is a separate issue, that can be solved separately some day.

Iterate over all harts (contain those not examine yet) and read the mhartid CSR, that doesn't sounds like a good idea,
Is there a better way for us to map hartid and info->index(The index of hart on the Debug Module) ?

I don't think it's as bad as you think. examine() already halts each hart and reads misa. Reading mhartid in addition is not a big change. Some day there will be a unified discovery mechanism, but that group is only just starting up again.

But I don't think we need to worry about hartids right now. We do need to worry about debug index and OpenOCD coreid no longer being equal.

zqb-all · 2023-06-29T17:24:34Z

But I don't think we need to worry about hartids right now. We do need to worry about debug index and OpenOCD coreid no longer being equal.

Make sense, ignore hartid for now would make things easier.
How about let info->index = target->coreid - target->dbgbase ?

config target create -coreid 0x0 --> target->coreid=0, target->dbgbase=0, info->index = 0
config target create -coreid 0x1 --> target->coreid=1, target->dbgbase=0, info->index = 1
config target create -coreid 0x400 -dbgbase 0x400 --> target->core_d=0x400, target->dbgbase=0x400, info->index = 0
config target create -coreid 0x401 -dbgbase 0x400 --> target->coreid=0x401, target->dbgbase=0x400, info->index = 1

src/target/riscv/riscv-013.c

JanMatCodasip · 2023-06-30T12:14:12Z

I will be able to re-review this change in the week of July 10.

src/target/riscv/riscv-013.c

src/target/riscv/riscv.c

src/target/riscv/riscv-013.c

JanMatCodasip

This looks fine! I have only few last suggestions regarding the documentation, comments and prints.

@zqb-all Thanks for bearing with all the review comments.

doc/openocd.texi

src/target/riscv/batch.h

JanMatCodasip · 2023-07-25T07:29:10Z

src/target/riscv/riscv-013.c

+		LOG_TARGET_DEBUG(target, "dm @ 0x%x --> nextdm=0x%x", current_dm, next_dm);
+		/* Check if it's last one in the chain. */
+		if (next_dm == 0) {
+			LOG_TARGET_ERROR(target, "Reach end of DM chain");


I believe you can change this error print to debug. There is already a good and easy-to-understand error message in the function that calls check_dbgbase_exists().

If someone needs to dig deeper and troubleshoot the situation, they will most likely enable the verbose log (-d) and will be able to easily find out what happened.

LOG_TARGET_DEBUG(target, "Reach end of DM chain");

I disagree. Both of these should be LOG_TARGET_ERROR. These errors will be very rare, so there's no risk of clutter. When the error in the caller does get printed, the user will always want to know immediately which of the two cases were hit. If you only want a single error message (which is not important to me in this case), then the correct way is to remove the error message in the calling function and add more details to these ones here.

I don't mind both of those being LOG_TARGET_ERROR. It is a detail.

(Still, the message about reaching the end of the DM chain feels redundant: If the search failed, it implicitly means we went through the list and reached the end without finding the item. On the other hand, if it is the very rare case hitting of maximum number of DMs on the DMI bus, then that second error print is justified.)

Let's just use LOG_TARGET_ERROR.
And to make it slightly more useful, I add count of DMs

https://github.com/riscv/riscv-openocd/compare/5cc1a4307e7e7981371530dcaf9c8aecef0192aa..b98698ac7c40125a72de14f5c67329a19c4b92d1

Sounds good :)

src/target/riscv/riscv.c

JanMatCodasip

Two last details, otherwise I consider the review complete from my side.

Thanks!

src/target/riscv/riscv.c

zqb-all · 2023-07-25T13:26:32Z

Two last details, otherwise I consider the review complete from my side.

Thanks!

Thank you for your detailed and helpful review

prepare for support multiple DMs Change-Id: Ia313006376e4fa762449343e5522b59d3bfd068a Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>

Support assign DMI address of the debug module by pass -dbgbase to the target create command Change-Id: I774c3746567f6e6d77c43a62dea5e9e67bb25770 Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>

src/target/riscv/riscv-013.c

Change-Id: I0d65bf9b33fb6d10c33f4f038045832594579e58 Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>

zqb-all force-pushed the support_multiple_DMs branch from 8650c73 to b2948d3 Compare June 27, 2023 15:30

timsifive reviewed Jun 27, 2023

View reviewed changes

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

zqb-all force-pushed the support_multiple_DMs branch 4 times, most recently from 7073a36 to 36c47a6 Compare June 28, 2023 09:17

JanMatCodasip requested changes Jun 28, 2023

View reviewed changes

zqb-all force-pushed the support_multiple_DMs branch 2 times, most recently from 0d008ae to 3211a2c Compare June 29, 2023 13:52

zqb-all force-pushed the support_multiple_DMs branch 2 times, most recently from 4be233d to 7191871 Compare June 30, 2023 02:39

zqb-all commented Jun 30, 2023

View reviewed changes

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

zqb-all force-pushed the support_multiple_DMs branch from 7191871 to a36acdb Compare June 30, 2023 02:55

zqb-all requested a review from JanMatCodasip June 30, 2023 03:24

MarekVCodasip reviewed Jun 30, 2023

View reviewed changes

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

zqb-all force-pushed the support_multiple_DMs branch from a36acdb to deb5b35 Compare July 8, 2023 14:32

zqb-all force-pushed the support_multiple_DMs branch 3 times, most recently from 6e970e9 to 1f9dd23 Compare July 24, 2023 15:15

timsifive requested changes Jul 24, 2023

View reviewed changes

src/target/riscv/riscv.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

zqb-all force-pushed the support_multiple_DMs branch 3 times, most recently from 27b2297 to 20ada6c Compare July 24, 2023 17:53

timsifive previously approved these changes Jul 24, 2023

View reviewed changes

JanMatCodasip requested changes Jul 25, 2023

View reviewed changes

zqb-all dismissed timsifive’s stale review via 752b288 July 25, 2023 09:54

zqb-all force-pushed the support_multiple_DMs branch 2 times, most recently from 752b288 to 198e0ee Compare July 25, 2023 09:56

JanMatCodasip previously approved these changes Jul 25, 2023

View reviewed changes

src/target/riscv/riscv.c Outdated Show resolved Hide resolved

src/target/riscv/riscv.c Outdated Show resolved Hide resolved

zqb-all dismissed JanMatCodasip’s stale review via ed393e6 July 25, 2023 13:20

zqb-all force-pushed the support_multiple_DMs branch from 198e0ee to ed393e6 Compare July 25, 2023 13:20

JanMatCodasip previously approved these changes Jul 25, 2023

View reviewed changes

zqb-all added 2 commits July 26, 2023 01:06

target/riscv: add dm layer

895185c

prepare for support multiple DMs Change-Id: Ia313006376e4fa762449343e5522b59d3bfd068a Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>

target/riscv: support multiple DMs

80a8aa9

Support assign DMI address of the debug module by pass -dbgbase to the target create command Change-Id: I774c3746567f6e6d77c43a62dea5e9e67bb25770 Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>

zqb-all dismissed JanMatCodasip’s stale review via 5cc1a43 July 25, 2023 17:07

zqb-all force-pushed the support_multiple_DMs branch 2 times, most recently from 5cc1a43 to b98698a Compare July 26, 2023 06:03

JanMatCodasip reviewed Jul 26, 2023

View reviewed changes

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

target/riscv: support check dbgbase exist

a9f28da

Change-Id: I0d65bf9b33fb6d10c33f4f038045832594579e58 Signed-off-by: Mark Zhuang <mark.zhuang@spacemit.com>

zqb-all force-pushed the support_multiple_DMs branch from b98698a to a9f28da Compare July 26, 2023 06:31

JanMatCodasip approved these changes Jul 26, 2023

View reviewed changes

timsifive approved these changes Jul 26, 2023

View reviewed changes

timsifive merged commit 1997e68 into riscv-collab:riscv Jul 27, 2023
5 checks passed

JanMatCodasip mentioned this pull request Aug 29, 2023

Support for multiple DMs #377

Closed

TommyMurphyTM1234 mentioned this pull request Oct 22, 2024

"riscv set_dm_baseaddr" command is not shown in User's Guide #1153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

target/riscv: support multiple DMs #875

target/riscv: support multiple DMs #875

zqb-all commented Jun 27, 2023

zqb-all commented Jun 27, 2023

timsifive left a comment

timsifive commented Jun 27, 2023

timsifive commented Jun 27, 2023

zqb-all commented Jun 28, 2023

zqb-all commented Jun 28, 2023

JanMatCodasip left a comment

zqb-all commented Jun 28, 2023

timsifive commented Jun 28, 2023

zqb-all commented Jun 28, 2023

timsifive commented Jun 28, 2023

zqb-all commented Jun 29, 2023 •

edited

Loading

zqb-all commented Jun 29, 2023

timsifive commented Jun 29, 2023

zqb-all commented Jun 29, 2023

JanMatCodasip commented Jun 30, 2023

JanMatCodasip left a comment •

edited

Loading

JanMatCodasip Jul 25, 2023 •

edited

Loading

timsifive Jul 25, 2023

JanMatCodasip Jul 26, 2023 •

edited

Loading

zqb-all Jul 26, 2023

JanMatCodasip Jul 26, 2023

JanMatCodasip left a comment

zqb-all commented Jul 25, 2023

target/riscv: support multiple DMs #875

target/riscv: support multiple DMs #875

Conversation

zqb-all commented Jun 27, 2023

zqb-all commented Jun 27, 2023

timsifive left a comment

Choose a reason for hiding this comment

timsifive commented Jun 27, 2023

timsifive commented Jun 27, 2023

zqb-all commented Jun 28, 2023

zqb-all commented Jun 28, 2023

JanMatCodasip left a comment

Choose a reason for hiding this comment

zqb-all commented Jun 28, 2023

timsifive commented Jun 28, 2023

zqb-all commented Jun 28, 2023

timsifive commented Jun 28, 2023

zqb-all commented Jun 29, 2023 • edited Loading

zqb-all commented Jun 29, 2023

timsifive commented Jun 29, 2023

zqb-all commented Jun 29, 2023

JanMatCodasip commented Jun 30, 2023

JanMatCodasip left a comment • edited Loading

Choose a reason for hiding this comment

JanMatCodasip Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

timsifive Jul 25, 2023

Choose a reason for hiding this comment

JanMatCodasip Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

zqb-all Jul 26, 2023

Choose a reason for hiding this comment

JanMatCodasip Jul 26, 2023

Choose a reason for hiding this comment

JanMatCodasip left a comment

Choose a reason for hiding this comment

zqb-all commented Jul 25, 2023

zqb-all commented Jun 29, 2023 •

edited

Loading

JanMatCodasip left a comment •

edited

Loading

JanMatCodasip Jul 25, 2023 •

edited

Loading

JanMatCodasip Jul 26, 2023 •

edited

Loading