target/riscv: dump_field() shouldn't always decode #951

en-sc · 2023-11-01T14:15:20Z

Sometimes, the value from of some DMI scans has no meaning (e.g. when op is read). Such values should not be decoded. To make the dumps more consistent, <no decoding available> is printed when there is no decoding for a register.

Change-Id: I415f06a5a80f2fc8fb8ab3f79132bdf0602c8ad6

en-sc · 2023-11-01T14:32:41Z

Prior to the change:

Debug: 393 104 riscv-013.c:388 scan(): 40b r 00000000 @11 -> + 00000000 @16; 0i
Debug: 394 104 riscv-013.c:398 scan(): dmstatus=0 { version=none, anyrunning=0, allrunning=0, anyunavail=0, allunavail=0, anynonexistent=0, allnonexistent=0, anyresumeack=0, allresumeack=0, anyhavereset=0, allhavereset=0, impebreak=0, stickyunavail=current, ndmresetpending=false, confstrptrvalid=invalid, hasresethaltreq=0, authbusy=ready, authenticated=false, anyhalted=0, allhalted=0, } -> abstractcs=0 { datacount=0, relaxedpriv=full checks, busy=ready, progbufsize=0, cmderr=none, }
Debug: 395 104 riscv-013.c:388 scan(): 40b - 00000000 @11 -> + 00430382 @11; 0i
Debug: 396 104 riscv-013.c:398 scan(): dmstatus=0 { version=none, anyrunning=0, allrunning=0, anyunavail=0, allunavail=0, anynonexistent=0, allnonexistent=0, anyresumeack=0, allresumeack=0, anyhavereset=0, allhavereset=0, impebreak=0, stickyunavail=current, ndmresetpending=false, confstrptrvalid=invalid, hasresethaltreq=0, authbusy=ready, authenticated=false, anyhalted=0, allhalted=0, } -> dmstatus=0x430382 { version=0.13, anyrunning=0, allrunning=0, anyunavail=0, allunavail=0, anynonexistent=0, allnonexistent=0, anyresumeack=1, allresumeack=1, anyhavereset=0, allhavereset=0, impebreak=1, stickyunavail=current, ndmresetpending=false, confstrptrvalid=invalid, hasresethaltreq=0, authbusy=ready, authenticated=true, anyhalted=1, allhalted=1, }
...
Debug: 5379 820 riscv-013.c:388 scan(): 40b w 0032100c @17 -> + 00000000 @04; 1i
Debug: 5380 820 riscv-013.c:398 scan(): command=0x32100c { control=0x32100c, cmdtype=0, } ->
Debug: 5381 820 riscv-013.c:388 scan(): 40b r 00000000 @16 -> + 0032100c @17; 0i
Debug: 5382 820 riscv-013.c:398 scan(): abstractcs=0 { datacount=0, relaxedpriv=full checks, busy=ready, progbufsize=0, cmderr=none, } -> command=0x32100c { control=0x32100c, cmdtype=0, }
Debug: 5383 821 riscv-013.c:388 scan(): 40b - 00000000 @16 -> + 06000002 @16; 0i
Debug: 5384 821 riscv-013.c:398 scan(): abstractcs=0 { datacount=0, relaxedpriv=full checks, busy=ready, progbufsize=0, cmderr=none, } -> abstractcs=0x6000002 { datacount=2, relaxedpriv=full checks, busy=ready, progbufsize=6, cmderr=none, }
Debug: 5385 821 riscv-013.c:388 scan(): 40b r 00000000 @05 -> + 00000000 @16; 0i
Debug: 5386 821 riscv-013.c:398 scan():  -> abstractcs=0 { datacount=0, relaxedpriv=full checks, busy=ready, progbufsize=0, cmderr=none, }
Debug: 5387 821 riscv-013.c:388 scan(): 40b - 00000000 @05 -> + 00000000 @05; 0i
Debug: 5388 821 riscv-013.c:398 scan():  ->
Debug: 5389 821 riscv-013.c:388 scan(): 40b r 00000000 @04 -> + 00000000 @05; 0i
Debug: 5390 821 riscv-013.c:398 scan():  ->
Debug: 5391 821 riscv-013.c:388 scan(): 40b - 00000000 @04 -> + 00800000 @04; 0i
Debug: 5392 821 riscv-013.c:398 scan():  ->

After the change:

Debug: 4688 1567 riscv-013.c:415 dump_field(): 40b r 00000000 @11 -> + 00000000 @04; 103i
Debug: 4689 1567 riscv-013.c:415 dump_field(): 40b - 00000000 @11 -> + 00430382 @11; 103i
Debug: 4690 1567 riscv-013.c:436 dump_field():  -> dmstatus=0x430382 { version=0.13, anyrunning=0, allrunning=0, anyunavail=0, allunavail=0, anynonexistent=0, allnonexistent=0, anyresumeack=1, allresumeack=1, anyhavereset=0, allhavereset=0, impebreak=1, stickyunavail=current, ndmresetpending=false, confstrptrvalid=invalid, hasresethaltreq=0, authbusy=ready, authenticated=true, anyhalted=1, allhalted=1, }
...
Debug: 4947 1614 riscv-013.c:415 dump_field(): 40b w 0032100c @17 -> + 00000000 @04; 103i
Debug: 4948 1614 riscv-013.c:436 dump_field(): command=0x32100c { control=0x32100c, cmdtype=0, } ->
Debug: 4949 1615 riscv-013.c:415 dump_field(): 40b r 00000000 @16 -> + 0032100c @17; 103i
Debug: 4950 1615 riscv-013.c:415 dump_field(): 40b - 00000000 @16 -> + 06000002 @16; 103i
Debug: 4951 1615 riscv-013.c:436 dump_field():  -> abstractcs=0x6000002 { datacount=2, relaxedpriv=full checks, busy=ready, progbufsize=6, cmderr=none, }
Debug: 4952 1616 riscv-013.c:415 dump_field(): 40b r 00000000 @05 -> + 00000000 @16; 103i
Debug: 4953 1617 riscv-013.c:415 dump_field(): 40b - 00000000 @05 -> + 00000000 @05; 103i
Debug: 4954 1617 riscv-013.c:436 dump_field():  -> <no decoding available>
Debug: 4955 1617 riscv-013.c:415 dump_field(): 40b r 00000000 @04 -> + 00000000 @05; 103i
Debug: 4956 1618 riscv-013.c:415 dump_field(): 40b - 00000000 @04 -> + 00800000 @04; 103i
Debug: 4957 1618 riscv-013.c:436 dump_field():  -> data0=0x800000 { data=0x800000, }

src/target/riscv/riscv-013.c

timsifive

I like this idea to reduce debug clutter.

src/target/riscv/riscv-013.c

timsifive · 2023-11-07T19:56:18Z

src/target/riscv/riscv-013.c

+	if (decode_out)
+		out_text = decode_dmi(target, out_decoded, out_address, out_data)
+			? out_decoded
+			: "<no decoding available>";


"no decoding available" sounds a bit odd. We could decode it if we wanted to. We (probably) have the relevant field information available to decode. How about ""?

src/target/riscv/riscv-013.c

JanMatCodasip

This looks good to me.

As discussed, a more elaborate algorithm to detect validity of the incoming data could be implemented. Anyway, this change is still nice improvement over the current state.

timsifive

Minor feedback.

src/target/riscv/riscv-013.c

timsifive · 2023-11-10T23:35:00Z

This __PRETTY_FUNCTION__ change makes the output terrible:

Debug: 4020 1652 batch.c:224 void dump_field(int, const struct scan_field *)(): 40b w 5555aaaa @3c -> ?; 1i

I don't care to see the prototype for dump_field() all the time. Please revert that. There is really no reason to mention the function name at all. It just needs to be something concise that we can easily ID as a RISC-V JTAG scan.

en-sc · 2023-11-11T11:49:40Z

Debug: 4020 1652 batch.c:224 void dump_field(int, const struct scan_field *)(): 40b w 5555aaaa @3c -> ?; 1i

@timsifive, thank you for the catch! IMHO, this is quite interesting. The very line you quoted was not affected by my commit (it's a similar function for batch operations).
Have you compiled using Clang?
The thing is __PRETTY_FUNCTION__ is a GNU extension and it is defined to be the same as __func__ for C sources (in this usecase) but differ for C++ sources (full signature instead of just a name) by GCC documentation.
Currently Clang does support __PRETTY_FUNCTION__, though it uses GCC's C++ variant across C and C++.

I would like to leave log's format as consistent as possible, so I have just replaced __PRETTY_FUNCTION__ with __func__ in a separate commit (__func__ is already used in LOG_DEBUG and so on).

timsifive · 2023-11-13T17:51:25Z

Oh, I am building with clang. I switched because I was checking if something worked. I didn't realize that would cause this difference.

timsifive · 2023-11-13T17:55:17Z

Can you rebase this on top of the current riscv? It makes it more clear what changes are actually new.

Sometimes, the value from of some DMI scans has no meaning (e.g. when `op` is read). Such values should not be decoded. To make the dumps more consistent, `<no decoding available>` is printed when there is no decoding for a register. Change-Id: I415f06a5a80f2fc8fb8ab3f79132bdf0602c8ad6 Signed-off-by: Evgeniy Naydanov <evgeniy.naydanov@syntacore.com>

The reasoning for the change: * `__func__` is part of C99, `__PRETTY_FUNCTION__` is GNU extension. * `__PRETTY_FUNCTION__` is defined to be the same as `__func__` for C sources by GCC documentation but differ for C++ sources (full signature instead of just a name). * Currently Clang does support `__PRETTY_FUNCTION__`, though it uses GCC's C++ variant across C and C++. Therefore using `__PRETTY_FUNCTION__` creates confusion and does not provide any valueble information in the logs. Change-Id: Ie0db6d73f602784b6752a30911dcef3dd7ee4594

en-sc · 2023-11-15T11:10:10Z

Can you rebase this on top of the current riscv? It makes it more clear what changes are actually new.

Done.

en-sc mentioned this pull request Nov 1, 2023

Make register dump output more concise. riscv/riscv-debug-spec#909

Merged

timsifive reviewed Nov 2, 2023

View reviewed changes

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

en-sc force-pushed the en-sc/scan-dump-reg branch 4 times, most recently from ab35ffb to 47af939 Compare November 7, 2023 08:58

en-sc requested a review from timsifive November 7, 2023 10:27

timsifive requested changes Nov 7, 2023

View reviewed changes

JanMatCodasip requested changes Nov 8, 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

en-sc force-pushed the en-sc/scan-dump-reg branch 2 times, most recently from 2adf33b to 2b6b087 Compare November 8, 2023 14:45

JanMatCodasip previously approved these changes Nov 9, 2023

View reviewed changes

en-sc requested a review from timsifive November 9, 2023 08:18

timsifive requested changes Nov 9, 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

en-sc dismissed JanMatCodasip’s stale review via b6f3911 November 10, 2023 12:14

en-sc force-pushed the en-sc/scan-dump-reg branch from 2b6b087 to b6f3911 Compare November 10, 2023 12:14

timsifive previously approved these changes Nov 10, 2023

View reviewed changes

en-sc dismissed timsifive’s stale review via 1369ccb November 11, 2023 11:33

en-sc force-pushed the en-sc/scan-dump-reg branch from 1369ccb to edbd031 Compare November 11, 2023 11:35

en-sc requested a review from timsifive November 11, 2023 11:52

en-sc added 2 commits November 15, 2023 14:06

en-sc force-pushed the en-sc/scan-dump-reg branch from edbd031 to 00320fd Compare November 15, 2023 11:07

timsifive approved these changes Nov 15, 2023

View reviewed changes

JanMatCodasip approved these changes Nov 16, 2023

View reviewed changes

timsifive merged commit 46e8f7a into riscv-collab:riscv Nov 16, 2023
4 checks passed

en-sc deleted the en-sc/scan-dump-reg branch August 14, 2024 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

target/riscv: dump_field() shouldn't always decode #951

target/riscv: dump_field() shouldn't always decode #951

en-sc commented Nov 1, 2023 •

edited

Loading

en-sc commented Nov 1, 2023

timsifive left a comment

timsifive Nov 7, 2023

en-sc Nov 8, 2023

JanMatCodasip left a comment •

edited

Loading

timsifive left a comment

timsifive commented Nov 10, 2023 •

edited

Loading

en-sc commented Nov 11, 2023 •

edited

Loading

timsifive commented Nov 13, 2023

timsifive commented Nov 13, 2023

en-sc commented Nov 15, 2023

target/riscv: dump_field() shouldn't always decode #951

target/riscv: dump_field() shouldn't always decode #951

Conversation

en-sc commented Nov 1, 2023 • edited Loading

en-sc commented Nov 1, 2023

timsifive left a comment

Choose a reason for hiding this comment

timsifive Nov 7, 2023

Choose a reason for hiding this comment

en-sc Nov 8, 2023

Choose a reason for hiding this comment

JanMatCodasip left a comment • edited Loading

Choose a reason for hiding this comment

timsifive left a comment

Choose a reason for hiding this comment

timsifive commented Nov 10, 2023 • edited Loading

en-sc commented Nov 11, 2023 • edited Loading

timsifive commented Nov 13, 2023

timsifive commented Nov 13, 2023

en-sc commented Nov 15, 2023

en-sc commented Nov 1, 2023 •

edited

Loading

JanMatCodasip left a comment •

edited

Loading

timsifive commented Nov 10, 2023 •

edited

Loading

en-sc commented Nov 11, 2023 •

edited

Loading