Add support for batch read/write handling for targets without abstractauto #1314
Conversation
…tauto Handle batch reads/writes on targets lacking abstractauto by issuing multiple abstract commands per read/write.
|
@nadime15, thanks for the patch! I'd suggest trying to keep using the batched access. Without batching (one batch per element, like in the current implementation) the resulting speed will be much slower (this is related to USB communication delays). The idea is to add writes of the This will allow for a better performance and should not be too hard to implement. Are you using some simulator to test this or is this tested on HW? Please, consider adding an argument to control |
| if (dm_read(target, &abstractauto, DM_ABSTRACTAUTO) == ERROR_OK) { | ||
| uint32_t autoexecdata_mask = get_field(abstractauto, DM_ABSTRACTAUTO_AUTOEXECDATA); | ||
| // Check each bit individually | ||
| for(unsigned int i = 0; i < info->datacount; i++){ |
There was a problem hiding this comment.
This seems like quite a bit of work. I'd suggest using yes_no_maybe and evaluating the results lazily.
See examine_progbuf() for an example.
Oh, I'm sorry I didn't read the code carefully.
There was a problem hiding this comment.
Oh, I'm sorry I didn't read the code carefully. Please, disregard my first comment.
I've looked at the spec and from [3.14.8. Abstract Command Autoexec (abstractauto, at 0x18)]:
If this register is implemented then bits corresponding to implemented progbuf and data registers
must be writable. Other bits must be hard-wired to 0.
It seems like there are only two options -- either all the bits are supported or none of the bits are supported.
Perhaps we can drop the array then?
There was a problem hiding this comment.
I dont think this is what it means. I think it means that the abstractauto.autoexecdata (and autoexecprogbuf) bits are writable for the data* and progbuf* registers that are implemented, while the others are hardwired to 0.
For example, if an implementation has 4 data registers (data0 through data3), these would be writable (bits 3:0), but the rest (bits 11:4) would be hardwired to 0.
Furthermore, it says:
When a bit in this field is 1, read or write accesses to the
corresponding data word cause the DM to act as if the
current value in command was written there again after
the access to data completes.
The phrase “When a bit in this field is 1...” implies that each bit can be either 1 or 0. Besides that, I think it would be misleading to have, for example, all 12 bits for autoexecdata supported (writable to 1) when the debug module only has 4 data registers.
If this needs more clarification, I could open an issue on the debug specification repo and ask there as well.
|
Thanks @en-sc for the feedback. I will take a look at
I am working on debug module for the Sail RISC-V Model. I have not opened a PR yet but I tested it using my own implementation (which still needs quite a bit of work). So far, it works correctly on my setup, I have tested it under different scenarios, such as when the JTAG module is much faster than the hart (to trigger retries due to the debug module being busy) and vice versa. But I agree I also would feel more comfortable adding an option to Spike to test it. I open a PR shortly! |
Yes, please look at
This is very cool! Perhaps you can point me at some documentation/examples I can use to try it out? |
2 participants
This is currently just a draft, but this potential PR aims to remove the use of the optional abstractauto register, which enables efficient batch reads and writes, and attempts to address the issue described in #587.
I would appreciate any feedback on my approach, as well as suggestions for alternative or improved ways to handle this.
The part I am uncertain about is the handling of dirty registers. I am not entirely sure if my current approach requires modification. While testing, GDB occasionally reported that S0 and S1 were dirty.