Added sbytemask field to textra32 and textra64 by bruceable · Pull Request #588 · riscv/riscv-debug-spec

Bruce Ableidinger (bruceable) · 2020-11-05T22:06:01Z

sbytemask field in textra32 and textra64 provides byte-granular comparison of the svalue trigger value to the scontext CSR

Co-authored-by: Ernie Edgar <43148441+ernie-sifive@users.noreply.github.com>

Byte masking of svalue comparison only applies when sselect==1.

Ernie Edgar (ernie-sifive)

Looks good to me.

Tim Newsome (timsifive)

The feature looks good. I have a bunch of comments about formatting etc, however.

Tim Newsome (timsifive) · 2020-11-07T00:14:39Z

        0 (ignore).

+        Byte-granular comparison of the scontext CSR to the trigger's svalue in
+        textra32 and textra64 registers allows scontext to be defined to include


Throughout this change, can you refer to textra32 as \RcsrTextraThirtytwo, textra64 as \RcsrTextraSixtyfour, and scontext as \RcsrScontext? Then they'll get formatted slightly different and link to the register in the doc.

Tim Newsome (timsifive) · 2020-11-07T00:19:50Z

+        such as process ID and thread ID.  The user can then program byte compares of
+        the trigger to include one or more of the contexts in the compare. For textra32
+        there are two byte-compare mask bits; for textra64 there are 5 byte-compare mask bits.
+        Byte masking only applies to scontext comparison; i.e when sselect==1.


when \FcsrTextraThirtytwoSselect is 1 instead of when sselect==1.

Tim Newsome (timsifive) · 2020-11-07T00:31:07Z

+        Byte-granular comparison of the scontext CSR to the trigger's svalue in
+        textra32 and textra64 registers allows scontext to be defined to include


This text is part of the description of textra32. It should probably be repeated in the textra64 section, in which case you don't need to mention the register name at all. E.g. "Byte-granular comparison of \RcsrScontext to \FcsrTextraThirtytwoSvalue allows \RcsrScontext to be defined to include ..."

Tim Newsome (timsifive) · 2020-11-09T21:10:04Z

+            0=compare, 1=don't compare byte, i.e. byte is masked out
+            bit 18 = lower byte compare of svalue (bits [7:0])
+            bit 19 = upper byte compare of svalue (bits [15:8])


We use capitalization and punctuation in field descriptions. When mentioning the behavior of values we use : instead of = (see e.g. https://github.com/riscv/riscv-debug-spec/blob/master/xml/dm_registers.xml#L12).

I don't think we have any examples yet of where each bit inside a field has a different meaning. The closest is hawindow, where we say "I.e. bit 0 refers to hart $\RdmHawindowsel * 32$, while bit 31 refers to hart $\RdmHawindowsel * 32 + 31$." (https://github.com/riscv/riscv-debug-spec/blob/master/xml/dm_registers.xml#L347).

Putting that all together, I think we want language like:
When the least-significant bit of this field is 1, it causes bits 7:0 in the comparison to be ignored when \FcsrTextraThirtytwoSselect=1. When the next most significant bit of this field is 1, causes bits 15:8 to be ignored in the comparison when \FcsrTextraThirtytwoSselect=1, and so on.

Tim Newsome (timsifive) · 2020-11-09T21:10:17Z

+        <field name="0" bits="47:41" access="R" reset="0" />

+        <field name="sbytemask" bits="40:36" access="WARL" reset="0">
+            0=compare, 1=don't compare, i.e byte compare is masked out


Same as above.

Tim Newsome (timsifive) · 2020-11-09T21:10:50Z

+            Byte masking of svalue only applies to scontext comparison; i.e.
+            when sselect==1.


I think I mentioned that adequately in my suggestion enough. If not, use \FcsrTextraThirtytwoSselect=1 instead of sselect==1.

We used to not do this because those fields would often have the same name as some other field, and there was a name collision. Now however we have long, fully qualified names. This change is needed for #588 which references \FcsrTextraSixtyfourSselect.

Tim Newsome (timsifive) · 2020-11-10T18:32:28Z

        tie any number of upper bits to 0. The $|select|$ bits may only support
        0 (ignore).

+        Byte-granular comparison of the \RcsrScontext to \FcsrTextraThirtytwoSvalue


Just "\RcsrScontext" instead of "the \RcsrScontext"

This paragraph should refer to \FcsrTextraThirtytwoSbytemask since that is what implements the comparison.

Tim Newsome (timsifive) · 2020-11-10T18:43:47Z

+        For example, software instrumentation can program the \RcsrScontext value to be
+        the concatenation of different ID contexts such as process ID and thread ID.
+        The user can then program byte compares to include one or more of the contexts
+        in the compare. For \RcsrTextraThirtytwo there are two byte-compare mask bits.


No need to talk explicitly about textra32 because this paragraph is in the section that defines textra32, and the size is already clear from the figure that is placed directly below this block of text.

Tim Newsome (timsifive) · 2020-11-10T18:47:57Z

+            When the least significant bit of this field is 1, it causes bits 7:0
+            in the comparison to be ignored, when \FcsrTextraThirtytwoSselect=1.
+            When the next most significant bit of this field is 1, it causes bits 15:8
+            to be ignored in the comparison, when \FcsrTextraThirtytwoSselect=1


Needs a period at the end of the sentence.

Tim Newsome (timsifive) · 2020-11-10T18:48:22Z

        5, or 6 and XLEN=64. The fields are defined
        above, in \RcsrTextraThirtytwo.

+        Byte-granular comparison of the \RcsrScontext to \FcsrTextraSixtyfourSvalue in


Same feedback as textra32 version.

Tim Newsome (timsifive) · 2020-11-10T18:49:00Z

+            When the least significant bit of this field is 1, it causes bits 7:0
+            in the comparison to be ignored, when \FcsrTextraSixtyfourSselect=1.
+            When the next most significant bit of this field is 1, it causes bits 15:8
+            to be ignored in the comparison, when \FcsrTextraSixtyfourSselect=1


Need a period at the end of these sentences.

Tim Newsome (timsifive) · 2020-11-10T18:49:32Z

+            When the next most significant bit of this field is 1, it causes bits 31:24
+            to be ignored in the comparison, when \FcsrTextraSixtyfourSselect=1
+            When the next most significant bit of this field is 1, it causes bits 33:32
+            to be ignored in the comparison, when \FcsrTextraSixtyfourSselect=1


At some point we'll have to find a nicer way to write this, but I don't have any good ideas right now. Probably can merge like this and fix it later.

We used to not do this because those fields would often have the same name as some other field, and there was a name collision. Now however we have long, fully qualified names. This change is needed for #588 which references \FcsrTextraSixtyfourSselect.

Tim Newsome (timsifive)

Looks good, except for the typo.

I think it still won't pass the checks even with this fix because it depends on #591. You can fix this in your PR by merging latest master into your bytemask branch, but I'm OK skipping that step and just merging this once the typo is fixed.

Tim Newsome (timsifive) · 2020-11-11T18:47:17Z

+        allows \RcsrScontext to be defined to include more than one element of comparison.
+        For example, software instrumentation can program the \RcsrScontext value to be
+        the concatenation of different ID contexts such as process ID and thread ID.
+        The user can then program byte compares based on \FcsrTextraThritytwoSbytemask


Typo: FcsrTextraThritytwoSbytemask -> FcsrTextraThirtytwoSbytemask

Tim Newsome (timsifive)

I'll merge this tomorrow, unless there is more discussion.

Added sbytemask field to textra32 and textra64

552f2fa

sbytemask field in textra32 and textra64 provides byte-granular comparison of the svalue trigger value to the scontext CSR

Ernie Edgar (ernie-sifive) reviewed Nov 5, 2020

View reviewed changes

Comment thread xml/hwbp_registers.xml Outdated

Comment thread xml/hwbp_registers.xml Outdated

Comment thread xml/hwbp_registers.xml Outdated

Bruce Ableidinger and others added 2 commits November 6, 2020 10:43

Update xml/hwbp_registers.xml

28b79ed

Co-authored-by: Ernie Edgar <43148441+ernie-sifive@users.noreply.github.com>

Added comments about sbytemask

08a7206

Byte masking of svalue comparison only applies when sselect==1.

Ernie Edgar (ernie-sifive) approved these changes Nov 6, 2020

View reviewed changes

Ernie Edgar (ernie-sifive) requested a review from Tim Newsome (timsifive) November 6, 2020 23:44

Tim Newsome (timsifive) suggested changes Nov 9, 2020

View reviewed changes

Update hwbp_registers.xml

7bc435a

Tim Newsome (timsifive) mentioned this pull request Nov 10, 2020

Create LaTeX macros for fields without descriptions. #591

Merged

Tim Newsome (timsifive) suggested changes Nov 10, 2020

View reviewed changes

Update hwbp_registers.xml

573c54c

Tim Newsome (timsifive) suggested changes Nov 11, 2020

View reviewed changes

Update hwbp_registers.xml

f7ad41c

Tim Newsome (timsifive) approved these changes Nov 11, 2020

View reviewed changes

Tim Newsome (timsifive) merged commit 53191a4 into riscv:master Nov 12, 2020

		Byte-granular comparison of the scontext CSR to the trigger's svalue in
		textra32 and textra64 registers allows scontext to be defined to include

		Byte masking of svalue only applies to scontext comparison; i.e.
		when sselect==1.

Conversation

Bruce Ableidinger (bruceable) commented Nov 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ernie Edgar (ernie-sifive) left a comment

Choose a reason for hiding this comment

Uh oh!

Tim Newsome (timsifive) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim Newsome (timsifive) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim Newsome (timsifive) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants