Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

target/riscv: Add support for external triggers #1179

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from
Open

target/riscv: Add support for external triggers #1179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/openocd.texi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11476,6 +11476,11 @@ The second argument configures how OpenOCD should use the selected trigger featu
With no parameters, prints current trigger features configuration.
@end deffn

@deffn {Command} {riscv set_external_trigger} value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RISC-V debug spec allows external triggers to be associated with halt and resume groups. I assume you are trying to add support for halt groups only. Looks like this command needs to be renamed, since resume groups are out of scope, or you should extend the interface to support resume groups too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed - my initial focus was only halt groups - this should be renamed if we go ahead with this very limited solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial idea was to create external triggers separately in the cfg file and list them in the target smp command, something like this:

# when no dmexttrigger
target create hart0 riscv -dbgbase 0
target create hart1 riscv -dbgbase 0x400
target smp hart0 hart1

# when support dmexttrigger, type0 halt, type1 resume
target create dm0extrigger0 riscv-dmexttrigger -index 0 -type 0  -dbgbase 0
target create dm0extrigger1 riscv-dmexttrigger  -index 1 -type 0  -dbgbase 0
target create hart0 riscv -dbgbase 0
target create dm1extrigger0 riscv-dmexttrigger -index 0 -type 0  -dbgbase 0x400
target create dm1extrigger1 riscv-dmexttrigger -index 1 -type 0  -dbgbase 0x400
target create hart1 riscv -dbgbase 0x400
target smp hart0 hart1 dm0extrigger0  dm0extrigger1  dm1extrigger0 dm1extrigger1

But there are a lot of changes required, so currently associating external trigger with a target seems like an easy enough way to start supporting it.

Suppose we support it this way, then my suggestion is let this cmd support multiple external triggers instead of just one.
For example, if the external trigger is not bidirectional, we need at least two triggers, one input-only and one output-only, to support smp with cores on other DM.
There are more complex situations that may require more triggers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design of this syntax looks really nice - i'm very happy to wait on this or if you prefer take a go at implementing that style myself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't started looking at the code yet, just a basic idea that needs further thought.
I think we can move forward with your current implementation and make the external trigger available to those who need it first.

Copy link
Collaborator

@JanMatCodasip JanMatCodasip Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zqb-all Thank you for your idea above.

Still, I'd recommend to not abuse target create in order to assign external triggers to SMP groups. "Target" has a well-defined meaning in OpenOCD: it is a debuggable entity with its state, registers, target API operations (target_type.h), ... External trigger is not a target. So I'd prefer to find a different syntax to assign external triggers to halt goups.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, I'd recommend to not abuse target create in order to assign external triggers to SMP groups.

Agreed. Some concepts in debug spec are not explicitly represented in OpenOCD cfg file now. For example, DM is only specified with dbgbase when target create, and halt group is only implicitly used for target smp, so it is not easy to create a suitable syntax to configure a dmextrigger to a specific group now.
When I have a more mature idea, I will create another PR to discuss it.
Thank you for your feedback on this idea.

Associate the supplied external trigger with the halt group for the harts. When
the external trigger fires the harts in the halt group will be halted.
@end deffn

@subsection RISC-V Authentication Commands

The following commands can be used to authenticate to a RISC-V system. Eg. a
Expand Down
29 changes: 29 additions & 0 deletions src/target/riscv/riscv-013.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,32 @@ static void deinit_target(struct target *target)
info->version_specific = NULL;
}

static int set_external_trigger(struct target *target, unsigned int group,
grouptype_t grouptype, unsigned int external_trigger)
{
uint32_t write_val = DM_DMCS2_HGWRITE | DM_DMCS2_HGSELECT;
assert(group <= 31);
assert(external_trigger < 16);
write_val = set_field(write_val, DM_DMCS2_GROUP, group);
write_val = set_field(write_val, DM_DMCS2_GROUPTYPE, (grouptype == HALT_GROUP) ? 0 : 1);
write_val = set_field(write_val, DM_DMCS2_DMEXTTRIGGER, external_trigger);
if (dm_write(target, DM_DMCS2, write_val) != ERROR_OK)
return ERROR_FAIL;
uint32_t read_val;
if (dm_read(target, &read_val, DM_DMCS2) != ERROR_OK)
return ERROR_FAIL;
if (get_field(read_val, DM_DMCS2_GROUP) == group &&
get_field(read_val, DM_DMCS2_DMEXTTRIGGER) == external_trigger &&
get_field(read_val, DM_DMCS2_HGSELECT) == 1) {
LOG_TARGET_INFO(target, "External trigger %d added to group %d", external_trigger,
group);
} else {
LOG_TARGET_ERROR(target, "External trigger %d not supported", external_trigger);
}

return ERROR_OK;
}

static int set_group(struct target *target, bool *supported, unsigned int group,
grouptype_t grouptype)
{
Expand Down Expand Up @@ -2051,6 +2077,9 @@ static int examine(struct target *target)
else
LOG_TARGET_INFO(target, "Core %d could not be made part of halt group %d.",
info->index, target->smp);
if (r->external_trigger)
Copy link
Collaborator

@aap-sc aap-sc Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. it's not quite clear why this code is under target->smp condition. We can control an external triggger without creating an SMP group.
  2. @en-sc I remember you wanted to implement an interface to debug module. Could you please assess if we can implement the functionality in question without this interface?
  3. if (r->external_trigger) condition is dubious, since even the debug spec implies that external trigger
    have numbers starting from "0":
By convention external triggers 0-7 are bidirectional,
triggers 8-11 are input-only, and triggers 12-15 are output-only but this is not required.
  1. what about trigger directions?
  2. do you have plans to add tests targeting this functionality?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aap-sc Thanks for taking the time to reply - some specific answers to your points

  1. I was using target->smp as that was a condition to have access to a halt group
  2. Well spotted - indeed trigger 0 is valid - this is wrong (I used external trigger 1 in my Spike test version for this change which is why I didn't spot this.) I will address this
  3. I'm not sure trigger direction is something OpenOCD can handle since it's just a convention in the numbering
  4. I hacked Spike to group some harts together separately and then "joined" them with an external trigger.

if (set_external_trigger(target, target->smp, HALT_GROUP, r->external_trigger) != ERROR_OK)
return ERROR_FAIL;
}

/* Some regression suites rely on seeing 'Examined RISC-V core' to know
Expand Down
27 changes: 27 additions & 0 deletions src/target/riscv/riscv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5262,6 +5262,26 @@ COMMAND_HANDLER(handle_riscv_virt2phys_mode)
return ERROR_OK;
}

COMMAND_HANDLER(riscv_set_external_trigger)
{
struct target *target = get_current_target(CMD_CTX);
RISCV_INFO(r);

if (CMD_ARGC != 1) {
LOG_ERROR("Command takes exactly 1 parameter.");
return ERROR_COMMAND_SYNTAX_ERROR;
}
int value = atoi(CMD_ARGV[0]);
if (value <= 0 || value > 16) {
LOG_ERROR("%s is not a valid integer argument for command.", CMD_ARGV[0]);
return ERROR_FAIL;
}

r->external_trigger = value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be safer to add some checks to prevent user from associating one trigger to multiple harts on same DM.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this is a good point - the external trigger is logically associated with a halt group which will span multiple targets (assuming the one target per hart configuration method.)


return ERROR_OK;
}

static const struct command_registration riscv_exec_command_handlers[] = {
{
.name = "dump_sample_buf",
Expand Down Expand Up @@ -5524,6 +5544,13 @@ static const struct command_registration riscv_exec_command_handlers[] = {
"When off, users need to take care of memory coherency themselves, for example by using "
"`riscv exec_progbuf` to execute fence or CMO instructions."
},
{
.name = "set_external_trigger",
.handler = riscv_set_external_trigger,
.mode = COMMAND_CONFIG,
.usage = "value",
.help = "Add the given external trigger to the halt group"
},
COMMAND_REGISTRATION_DONE
};

Expand Down
3 changes: 3 additions & 0 deletions src/target/riscv/riscv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ struct riscv_info {
/* The configured approach to translate virtual addresses to physical */
riscv_virt2phys_mode_t virt2phys_mode;

/* Halt group may be associated with an external trigger */
unsigned int external_trigger;
Comment on lines +194 to +195
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out in another comment, there can be multiple external triggers in a halt (or resume) group.

The code, on the other hand, seems to force 1:1 relationship.


bool triggers_enumerated;

/* Decremented every scan, and when it reaches 0 we clear the learned
Expand Down
Loading