Skip to content

Make the data of the *context registers 32 bits wide.#981

Merged
rtwfroody merged 2 commits intorc2from
context
Mar 19, 2024
Merged

Make the data of the *context registers 32 bits wide.#981
rtwfroody merged 2 commits intorc2from
context

Conversation

@rtwfroody
Copy link
Collaborator

Still todo, is figure out what to call XLEN there. In the introduction we mention what XLEN means for Debug and M-Mode, but some of these registers are accessible in other modes also.

Comment on lines +16 to +19
In this section XLEN refers to the effective XLEN in the current execution
mode. This is DXLEN when in Debug Mode.
On systems where those values of XLEN can differ, this is handled as
follows.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that "those" makes sense anymore. If we're going to remove the MXLEN reference, shouldn't we also remove DXLEN since elsewhere it says that XLEN is DXLEN in debug mode?

Maybe: "In this section XLEN refers to the effective XLEN in the current execution mode. On systems where XLEN values can differ between modes, this is handled as follows."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept DXLEN in there to just make it extra clear than when the hart is halted DXLEN applies. But you're right it's not necessary, and I guess it is odd to only mention that one and not any other modes. I'll simplify as you suggest.


<field name="hcontext" bits="XLEN-1:0" access="WARL" reset="0">
<field name="0" bits="XLEN-1:14" access="R" reset="0" />
<field name="hcontext" bits="13:0" access="WARL" reset="0">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really 13:0 or 31:0? It's "recommended" to implement no more than 14 bits but that isn't a requirement. The required max seems like it used to be XLEN and is now 32.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to change it to 14 bits because there is no value in implementing additional bits. None of our triggers look at bits higher than bit 13 in this register. Or did I miss something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I forgot that the width of mhvalue basically makes it useless to have anything wider than 14 bits. I think that we should change the final sentence of the mcontext register's hcontext field to be:

If the H extension is implemented, it’s recommended to implement no more than 7 bits on RV32.

Because it would now be impossible to implement more than 14 on RV64.

Comment on lines +287 to +288
0. It's recommended to implement no more than 16 bits on RV32, and
34 on RV64.
32 on RV64.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I notice that this recommendation is confusing because it's not possible to implement more than 32 bits. We can just drop everything after the comma.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had read this as:

  1. It's recommended to implement no more than 16 bits on RV32.
  2. It's recommended to implement 32 bits on RV64.

But I agree it's ambiguous.

Maybe we should scrap the "no more" altogether, and just recommend 16 and 32? It's already clear that fewer are allowed.

E.g.

It's recommended to implement 16 bits on RV32, and 32 on RV64.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with your proposal but I'd eliminate the comma and then maybe say "bits" to be more symmetric: "It's recommended to implement 16 bits on RV32 and 32 bits on RV64."

Comment on lines +287 to +288
0. It's recommended to implement 16 bits on RV32, and 32 bits on
RV64.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
0. It's recommended to implement 16 bits on RV32, and 32 bits on
RV64.
0. It's recommended to implement 16 bits on RV32 and 32 bits on
RV64.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Clarify bit recommendations on scontext and mcontext.
Not just M-Mode and D-Mode.
Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it!

@rtwfroody rtwfroody merged commit c26ba92 into rc2 Mar 19, 2024
@rtwfroody rtwfroody deleted the context branch March 19, 2024 16:41
YenHaoChen added a commit to YenHaoChen/riscv-isa-sim that referenced this pull request Mar 25, 2024
The commit provdes the change between debug spec 1.0.0-rc1 and 1.0.0-rc2

Reference: riscv/riscv-debug-spec#981
christian-herber-nxp pushed a commit to NXP/riscv-isa-sim that referenced this pull request May 30, 2024
The commit provdes the change between debug spec 1.0.0-rc1 and 1.0.0-rc2

Reference: riscv/riscv-debug-spec#981
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants