target/riscv: cache requests to trigger configuration #928

AnastasiyaChernikova · 2023-09-28T15:59:36Z

Depending on configuration, the existing implementation of watchpoints is rather inefficient for certain scenarios. Consider HW that:

triggers 0-3 can be used as instruction breakpoints
triggers 4-7 can be used as data breakpoints (watchpoints)
NAPOT triggers are not supported.

Now, consider that we have a pending watchpoint. And we perform a "step" operation. According to the current implementation:

OpenOCD will disable watchpoints
Perform a single-step
Will try to restore the original watchpoints. It will need 12 attempts to find a suitable trigger: (8 attempts to try NAPOT, and another 4 to try GE+LE).

This patch introduces a dedicated cache for requests to triggers. It significantly speeds things up, since we cache failed attempts and no additional interactions with HW is necessary.

timsifive

I love this improvement. It's going to help a lot, as you explained.

Why did you choose to use a linked list for the data structure? It looks like you're always allocating the whole thing. I think using an array would simplify the code significantly, but perhaps I'm missing some downside.

src/target/riscv/riscv.c

AnastasiyaChernikova · 2023-10-04T16:37:23Z

Why did you choose to use a linked list for the data structure? It looks like you're always allocating the whole thing. I think using an array would simplify the code significantly, but perhaps I'm missing some downside.

When caching tdata2 data, each new call is written to the top of the list (or moved if such a call has already been made), thus making it easier to find the data. And for tdata1 a list to make code uniform.

timsifive · 2023-10-10T16:22:02Z

src/target/riscv/riscv.c

+
+	struct tdata1_cache *tdata1_cache = find_tdata1(&(r->wp_triggers_cache[idx]), tdata1);
+	if (tdata1_cache == NULL) {
+		tdata1_cache = create_new_tdata1_cache(&(r->wp_triggers_cache[idx]), tdata1);


Are you making a new cache per trigger index here? I don't understand that. There can only be one tdata1 value per trigger at a time.

We can use one trigger to set wp of different types (eq, napot, ge, lt). And in this implementation, all this information is cached. This is necessary if we installed wp on a certain trigger, then deleted it and want to reinstall it.

src/target/riscv/riscv.c

timsifive

OK, I think I got it now. Overall looks good. I have a lot of naming/comment suggestions that would have made this code more obvious to me, but nothing really about the structure of the code.

src/target/riscv/riscv.h

timsifive · 2023-10-17T17:37:14Z

src/target/riscv/riscv.c

@@ -700,6 +732,99 @@ static void log_trigger_request_info(struct trigger_request_info trig_info)
 			trig_info.tdata1, trig_info.tdata2, trig_info.tdata1_ignore_mask);
 };

+static struct tdata1_cache *create_new_tdata1_cache(struct list_head *tdata1_cache_head, riscv_reg_t tdata1)


Following the example of e.g. riscv_batch_alloc, let's call this:

Suggested change

static struct tdata1_cache *create_new_tdata1_cache(struct list_head *tdata1_cache_head, riscv_reg_t tdata1)

static struct tdata1_cache *tdata1_cache_alloc(struct list_head *tdata1_cache_head, riscv_reg_t tdata1)

(How does this square with e.g. riscv_free_registers()? It's a bit messy and inconsistent, but at least when there's a simple struct then using _alloc() seems more consistent.)

timsifive · 2023-10-17T17:37:29Z

src/target/riscv/riscv.c

+	return elem;
+}
+
+static void create_new_tdata2_cache(struct list_head *tdata2_cache_head, riscv_reg_t tdata2)


Suggested change

static void create_new_tdata2_cache(struct list_head *tdata2_cache_head, riscv_reg_t tdata2)

static void tdata2_cache_alloc(struct list_head *tdata2_cache_head, riscv_reg_t tdata2)

timsifive · 2023-10-17T17:45:58Z

src/target/riscv/riscv.c

+	list_add(&elem->elem_tdata2, tdata2_cache_head);
+}
+
+struct tdata2_cache *find_call_in_tdata2_cache(struct list_head *tdata2_cache_head, riscv_reg_t find_tdata2)


What is "call" in this context? Isn't this simply tdata2_cache_search?

timsifive · 2023-10-17T17:46:17Z

src/target/riscv/riscv.c

+	return NULL;
+}
+
+struct tdata1_cache *find_tdata1(struct list_head *tdata1_cache_head, riscv_reg_t find_tdata1)


Suggested change

struct tdata1_cache *find_tdata1(struct list_head *tdata1_cache_head, riscv_reg_t find_tdata1)

struct tdata1_cache *tdata1_cache_search(struct list_head *tdata1_cache_head, riscv_reg_t find_tdata1)

src/target/riscv/riscv.c

timsifive · 2023-10-17T17:59:17Z

src/target/riscv/riscv.c

+static bool is_use_wp_trigger_in_cache(struct target *target, unsigned int idx,
+	riscv_reg_t tdata1, riscv_reg_t tdata2)


Suggested change

static bool is_use_wp_trigger_in_cache(struct target *target, unsigned int idx,

riscv_reg_t tdata1, riscv_reg_t tdata2)

static bool wp_triggers_cache_search(struct target *target, unsigned int idx,

riscv_reg_t tdata1, riscv_reg_t tdata2)

Alternate suggestion for your consideration:

is_known_unsupported_trigger(...)

src/target/riscv/riscv.c

src/target/riscv/riscv.h

JanMatCodasip · 2023-10-18T08:29:06Z

src/target/riscv/riscv.c

+static bool is_use_wp_trigger_in_cache(struct target *target, unsigned int idx,
+	riscv_reg_t tdata1, riscv_reg_t tdata2)


Alternate suggestion for your consideration:

is_known_unsupported_trigger(...)

AnastasiyaChernikova · 2023-10-30T17:13:45Z

@timsifive All issues were addressed. Could you please take a look?

JanMatCodasip

There seem to be two unaddressed (overlooked?) items:

Other than that, the code looks all right.

AnastasiyaChernikova · 2023-11-03T10:17:21Z

@timsifive this commit has an unexpected (to me test failure) that I haven't observed locally. That being said - I used relatively old riscv-tests to check my changes (riscv-tests ed0a63ea4f , which is from around Jul 10, 2023). Looks like I'll need to re-test my changes with an updated riscv-tests.

aap-sc · 2023-11-03T10:42:54Z

@timsifive this commit has an unexpected (to me test failure) that I haven't observed locally. That being said - I used relatively old riscv-tests to check my changes (riscv-tests ed0a63ea4f , which is from around Jul 10, 2023). Looks like I'll need to re-test my changes with an updated riscv-tests.

@AnastasiyaChernikova , @timsifive, @JanMatCodasip actually, I've tried to update riscv-tests version to the current TOT and I have UnavailableHaltedTest test failure without this MR applied. Looks like the test is broken. Could it be a known issue with the test itself?

aap-sc · 2023-11-03T12:04:51Z

I have UnavailableHaltedTest test failure without this MR applied.

and just for the reference (I think this should be mentioned). This attempt to update was in repository that has few additional patches applied on top of riscv-openocd (minor fixes that were backported from https://sourceforge.net/p/openocd/ . And I used a relatively old version of spike (around 2 months old)

TommyMurphyTM1234 · 2023-11-03T16:31:50Z

@timsifive this commit has an unexpected (to me test failure) that I haven't observed locally. That being said - I used relatively old riscv-tests to check my changes (riscv-tests ed0a63ea4f , which is from around Jul 10, 2023). Looks like I'll need to re-test my changes with an updated riscv-tests.

@AnastasiyaChernikova , @timsifive, @JanMatCodasip actually, I've tried to update riscv-tests version to the current TOT and I have UnavailableHaltedTest test failure without this MR applied. Looks like the test is broken. Could it be a known issue with the test itself?

If I build everything (without the changes in this PR) from scratch from the latest sources (toolchain, Spike, RISC-V OpenOCD) then the UnavailableHaltedTest runs succeeed.

If I copy in the modified riscv.c and riscv.h from here and rebuild OpenOCD those test runs still succeed.

https://github.com/AnastasiyaChernikova/riscv-openocd/tree/triggers/src/target/riscv

timsifive · 2023-11-03T21:24:47Z

UnavailableHaltedTest seems to fail intermittently in the github action. I haven't seen it fail this way when I run it locally. :-( Hopefully I'll have some time to investigate more next week.

timsifive · 2023-11-06T17:14:26Z

I've restarted this test run several times, and it fails UnavailableHaltedTest consistently. Is it simply based on older code?
Can you rebase this off the latest riscv branch? That should also address the conflict that has appeared.

Depending on configuration, the existing implementation of watchpoints is rather inefficient for certain scenarios. Consider HW that: 1. triggers 0-3 can be used as instruction breakpoints 2. triggers 4-7 can be used as data breakpoints (watchpoints) 3. NAPOT triggers are not supported. Now, consider that we have a pending watchpoint. And we perform a "step" operation. According to the current implementation: * OpenOCD will disable watchpoints * Perform a single-step * Will try to restore the original watchpoints. It will need 12 attempts to find a suitable trigger: (8 attempts to try NAPOT, and another 4 to try GE+LE). This patch introduces a dedicated cache for requests to triggers. It significantly speeds things up, since we cache failed attempts and no additional interactions with HW is necessary. Change-Id: Ic272895eaa763a7ae84d14f7633790afd015ca9d Signed-off-by: Anastasiya Chernikova <[email protected]>

timsifive · 2023-11-07T21:44:29Z

For some reason, in this branch only, UnavailableHaltedTest consistently fails. I have no idea why. It doesn't make any sense with this change and what that test does. I cannot reproduce the failure on my machine either. :-(

I'm weary merging this change, though, because I don't want all testing to suddenly start failing. Is this blocking other work for you?

aap-sc · 2023-11-07T22:24:11Z

@timsifive

For some reason, in this branch only, UnavailableHaltedTest consistently fails. I have no idea why. It doesn't make any sense with this change and what that test does. I cannot reproduce the failure on my machine either. :-(

I can actually. However, the issue is reproducible even without this patch on my machine.

This may be related to python version or some weird timing issues. Sometimes OpenOCD log file contain non-printable characters (like ^H and stuff). It seems that this sometimes messes up matching algorithms used in testing infrastructure. There are 2 types of issues I observe:

testlib.TestLibError: Timed out waiting for b'riscv\\ dmi_write\\ 0x1f\\ 0x3\\\n' - this I cannot reproduce reliably (yet).
cannot use a string pattern on a bytes-like object these are caused that sometimes we have a mismatch between regexp and self.log_buf from expect method of testlib.py. Mismatch happens because sometimes these objects are bytes and sometimes these are strings

aap-sc · 2023-11-08T01:09:53Z

This may be related to python version or some weird timing issues.

I think I've found the root cause. It's a timing/parsing issue. I've filed riscv-software-src/riscv-tests#520 against riscv-tests.

AnastasiyaChernikova force-pushed the triggers branch from 180d934 to ddc5f78 Compare September 28, 2023 17:07

timsifive requested changes Sep 28, 2023

View reviewed changes

MarekVCodasip reviewed Oct 2, 2023

View reviewed changes

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

timsifive reviewed Oct 10, 2023

View reviewed changes

AnastasiyaChernikova force-pushed the triggers branch from ddc5f78 to 22567be Compare October 12, 2023 08:09

timsifive requested changes Oct 17, 2023

View reviewed changes

JanMatCodasip reviewed Oct 18, 2023

View reviewed changes

AnastasiyaChernikova force-pushed the triggers branch from 22567be to b5e3b3c Compare October 19, 2023 08:19

AnastasiyaChernikova force-pushed the triggers branch from b5e3b3c to 02e41a5 Compare October 26, 2023 08:50

JanMatCodasip requested changes Oct 31, 2023

View reviewed changes

AnastasiyaChernikova force-pushed the triggers branch from 02e41a5 to 6c43689 Compare November 2, 2023 15:12

timsifive previously approved these changes Nov 2, 2023

View reviewed changes

JanMatCodasip previously approved these changes Nov 3, 2023

View reviewed changes

AnastasiyaChernikova dismissed stale reviews from JanMatCodasip and timsifive via fea20e2 November 7, 2023 11:52

AnastasiyaChernikova force-pushed the triggers branch from 6c43689 to fea20e2 Compare November 7, 2023 11:52

aap-sc mentioned this pull request Nov 8, 2023

Some debug tests like are UnavailableHaltedTest broken riscv-software-src/riscv-tests#520

Closed

timsifive approved these changes Nov 10, 2023

View reviewed changes

timsifive merged commit 1ea0e9b into riscv-collab:riscv Nov 10, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

target/riscv: cache requests to trigger configuration #928

target/riscv: cache requests to trigger configuration #928

AnastasiyaChernikova commented Sep 28, 2023 •

edited

Loading

timsifive left a comment

AnastasiyaChernikova commented Oct 4, 2023 •

edited

Loading

timsifive Oct 10, 2023

AnastasiyaChernikova Oct 12, 2023

timsifive left a comment

timsifive Oct 17, 2023

AnastasiyaChernikova Oct 26, 2023

timsifive Oct 17, 2023

AnastasiyaChernikova Oct 26, 2023

timsifive Oct 17, 2023

AnastasiyaChernikova Oct 26, 2023

timsifive Oct 17, 2023

AnastasiyaChernikova Oct 26, 2023

timsifive Oct 17, 2023

JanMatCodasip Oct 18, 2023

AnastasiyaChernikova Oct 26, 2023

JanMatCodasip Oct 18, 2023

AnastasiyaChernikova commented Oct 30, 2023

JanMatCodasip left a comment

AnastasiyaChernikova commented Nov 3, 2023

aap-sc commented Nov 3, 2023

aap-sc commented Nov 3, 2023 •

edited

Loading

TommyMurphyTM1234 commented Nov 3, 2023 •

edited

Loading

timsifive commented Nov 3, 2023

timsifive commented Nov 6, 2023

timsifive commented Nov 7, 2023

aap-sc commented Nov 7, 2023

aap-sc commented Nov 8, 2023

	static struct tdata1_cache create_new_tdata1_cache(struct list_head tdata1_cache_head, riscv_reg_t tdata1)
	static struct tdata1_cache tdata1_cache_alloc(struct list_head tdata1_cache_head, riscv_reg_t tdata1)

	static void create_new_tdata2_cache(struct list_head *tdata2_cache_head, riscv_reg_t tdata2)
	static void tdata2_cache_alloc(struct list_head *tdata2_cache_head, riscv_reg_t tdata2)

	struct tdata1_cache find_tdata1(struct list_head tdata1_cache_head, riscv_reg_t find_tdata1)
	struct tdata1_cache tdata1_cache_search(struct list_head tdata1_cache_head, riscv_reg_t find_tdata1)

		static bool is_use_wp_trigger_in_cache(struct target *target, unsigned int idx,
		riscv_reg_t tdata1, riscv_reg_t tdata2)

target/riscv: cache requests to trigger configuration #928

target/riscv: cache requests to trigger configuration #928

Conversation

AnastasiyaChernikova commented Sep 28, 2023 • edited Loading

timsifive left a comment

Choose a reason for hiding this comment

AnastasiyaChernikova commented Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsifive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnastasiyaChernikova commented Oct 30, 2023

JanMatCodasip left a comment

Choose a reason for hiding this comment

AnastasiyaChernikova commented Nov 3, 2023

aap-sc commented Nov 3, 2023

aap-sc commented Nov 3, 2023 • edited Loading

TommyMurphyTM1234 commented Nov 3, 2023 • edited Loading

timsifive commented Nov 3, 2023

timsifive commented Nov 6, 2023

timsifive commented Nov 7, 2023

aap-sc commented Nov 7, 2023

aap-sc commented Nov 8, 2023

AnastasiyaChernikova commented Sep 28, 2023 •

edited

Loading

AnastasiyaChernikova commented Oct 4, 2023 •

edited

Loading

aap-sc commented Nov 3, 2023 •

edited

Loading

TommyMurphyTM1234 commented Nov 3, 2023 •

edited

Loading