do not assume DTM version unless dtmcontrol is read successfully #929

aap-sc · 2023-09-28T17:20:15Z

Change-Id: I02906dbbe04778623a51774bc21fa45b55d12881

aap-sc · 2023-09-28T17:24:44Z

@timsifive I know that changes to riscv-0.11 are discouraged, however in this case I believe it's better to add this fix. There were a couple of serious bugs:

dtmcontrol_scan could return an error code as a scanned value.
in case when dtmcontrol was read as "0" we had an implicit fallback to v0.11 that caused register cache to be populated. This could result in a weird situations like:

XLEN=64, misa=0x14112f

timsifive

How did you test the riscv-011 changes?

src/target/riscv/riscv-011.c

src/target/riscv/riscv.c

timsifive · 2023-09-29T19:33:32Z

src/target/riscv/riscv.c

 	LOG_TARGET_DEBUG(target, "dtmcontrol=0x%x", dtmcontrol);
+	// FIXME: if examine fails we may need to restore the original dtm_version


That makes sense. Can you add that change? Shouldn't be much more complicated.

will try... though I'm not sure that we need to fix it in this patch. examine has it's share of issues:

for example, it halts the target, but if something goes wrong - the target is not resumed. The fix for such situations may be complicated. But having said that - I'll try to address at least dtm_version stuff

addressed..

Apparently my whining about examine implementation is not relevant to riscv.c

aap-sc · 2023-10-02T16:13:02Z

How did you test the riscv-011 changes?

Regarding this...

I haven't tested on v0.11 since I don't have such HW, unfortunately. As far as v0.11 goes I've only tried to make sure that the change is reasonable by visually inspecting the code.
In case of v0.13 there was extensive testing: the whole riscv_tests and additional tests on fpga

TommyMurphyTM1234 · 2023-10-02T16:38:07Z

How did you test the riscv-011 changes?

Regarding this...

I haven't tested on v0.11 since I don't have such HW, unfortunately. As far as v0.11 goes I've only tried to make sure that the change is reasonable by visually inspecting the code.

If it helps, I can run the tests against a HiFive1 (Rev A I think) in the next couple of days?

aap-sc · 2023-10-02T17:01:40Z

If it helps, I can run the tests against a HiFive1 (Rev A I think) in the next couple of days?

Oh, it would be great! If you have the bandwidth - please, do. Though I'll need to address few comments by @timsifive

TommyMurphyTM1234 · 2023-10-04T16:37:18Z

If it helps, I can run the tests against a HiFive1 (Rev A I think) in the next couple of days?

Oh, it would be great! If you have the bandwidth - please, do. Though I'll need to address few comments by @timsifive

I take it that this PR still needs some changes before it makes sense for me to test against the HiFive1?
(Just to confirm - my HiFive1 is indeed Rev A01 but I presume that that's OK?).

aap-sc · 2023-10-04T16:40:04Z

I take it that this PR still needs some changes before it makes sense for me to test against the HiFive1?

I've already addressed all concerns raised by @timsifive . Maybe we should wait for Tim to confirm that this is good enough.

JanMatCodasip

I like this fix/improvement and the increased level of robustness it brings.

I have only few minor comments, otherwise it looks fine.

src/target/riscv/riscv-013.c

src/target/riscv/riscv.c

Change-Id: I5f2003b7ac5ce87af6ca9a4fcb46140682a8cfdf Signed-off-by: Parshintsev Anatoly <[email protected]>

JanMatCodasip

The changes look good to me.

When testing on HiFive01 Rev A01 with known fails skipped:

On HiFive1.py target, the testing passed without any issues.
On HiFive1-flash.py, there were 8 exceptions, occurring both with and without this change. I haven't investigated these further.

:::::::::::::::::::::::::::[ ran 72 tests in 465s ]:::::::::::::::::::::::::::
28 tests returned not_applicable
36 tests returned pass
8 tests returned exception
   DebugBreakpoint > logs/20231009-082756-HiFive1Flash-DebugBreakpoint.log
   DebugExit > logs/20231009-082844-HiFive1Flash-DebugExit.log
   DebugFunctionCall > logs/20231009-082926-HiFive1Flash-DebugFunctionCall.log
   Hwbp1 > logs/20231009-083025-HiFive1Flash-Hwbp1.log
   Hwbp2 > logs/20231009-083108-HiFive1Flash-Hwbp2.log
   Registers > logs/20231009-083240-HiFive1Flash-Registers.log
   TooManyHwbp > logs/20231009-083342-HiFive1Flash-TooManyHwbp.log
   UserInterrupt > logs/20231009-083442-HiFive1Flash-UserInterrupt.log

TommyMurphyTM1234 · 2023-10-09T08:36:03Z

The changes look good to me.

When testing on HiFive01 Rev A01 with known fails skipped:

On HiFive1.py target, the testing passed without any issues.

On HiFive1-flash.py, there were 8 exceptions, occurring both with and without this change. I haven't investigated these further.

If it's of any use, I plan to look at this imminently and could also look at those additional (?) exceptions?

debug: Fix tests so they pass/not_applicable on HiFive1 riscv-software-src/riscv-tests#488

JanMatCodasip · 2023-10-10T10:14:08Z

If it's of any use, I plan to look at this imminently and could also look at those additional (?) exceptions?

Any help with the failing tests is appreciated :)

aap-sc · 2023-10-13T17:59:40Z

Hi @timsifive , just wanted to double-check if there are any additional changes/fixes required for this PR?

timsifive requested changes Sep 29, 2023

View reviewed changes

aap-sc force-pushed the riscv branch from d76ac4b to 2cd8863 Compare October 3, 2023 12:34

JanMatCodasip reviewed Oct 5, 2023

View reviewed changes

do not assume DTM version unless dtmcontrol is read successfully

2c4118e

Change-Id: I5f2003b7ac5ce87af6ca9a4fcb46140682a8cfdf Signed-off-by: Parshintsev Anatoly <[email protected]>

aap-sc force-pushed the riscv branch from 2cd8863 to 2c4118e Compare October 6, 2023 15:52

JanMatCodasip approved these changes Oct 9, 2023

View reviewed changes

timsifive approved these changes Oct 10, 2023

View reviewed changes

timsifive merged commit e1fa78d into riscv-collab:riscv Oct 16, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do not assume DTM version unless dtmcontrol is read successfully #929

do not assume DTM version unless dtmcontrol is read successfully #929

aap-sc commented Sep 28, 2023

aap-sc commented Sep 28, 2023

timsifive left a comment

timsifive Sep 29, 2023

aap-sc Oct 2, 2023

aap-sc Oct 3, 2023

aap-sc commented Oct 2, 2023 •

edited

Loading

TommyMurphyTM1234 commented Oct 2, 2023

aap-sc commented Oct 2, 2023

TommyMurphyTM1234 commented Oct 4, 2023

aap-sc commented Oct 4, 2023

JanMatCodasip left a comment

JanMatCodasip left a comment •

edited

Loading

TommyMurphyTM1234 commented Oct 9, 2023

JanMatCodasip commented Oct 10, 2023

aap-sc commented Oct 13, 2023

		LOG_TARGET_DEBUG(target, "dtmcontrol=0x%x", dtmcontrol);
		// FIXME: if examine fails we may need to restore the original dtm_version

do not assume DTM version unless dtmcontrol is read successfully #929

do not assume DTM version unless dtmcontrol is read successfully #929

Conversation

aap-sc commented Sep 28, 2023

aap-sc commented Sep 28, 2023

timsifive left a comment

Choose a reason for hiding this comment

timsifive Sep 29, 2023

Choose a reason for hiding this comment

aap-sc Oct 2, 2023

Choose a reason for hiding this comment

aap-sc Oct 3, 2023

Choose a reason for hiding this comment

aap-sc commented Oct 2, 2023 • edited Loading

TommyMurphyTM1234 commented Oct 2, 2023

aap-sc commented Oct 2, 2023

TommyMurphyTM1234 commented Oct 4, 2023

aap-sc commented Oct 4, 2023

JanMatCodasip left a comment

Choose a reason for hiding this comment

JanMatCodasip left a comment • edited Loading

Choose a reason for hiding this comment

TommyMurphyTM1234 commented Oct 9, 2023

JanMatCodasip commented Oct 10, 2023

aap-sc commented Oct 13, 2023

aap-sc commented Oct 2, 2023 •

edited

Loading

JanMatCodasip left a comment •

edited

Loading