Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed changes to mttp CSR #15

Merged
merged 4 commits into from
Dec 11, 2023
Merged

Proposed changes to mttp CSR #15

merged 4 commits into from
Dec 11, 2023

Conversation

rsahita
Copy link
Collaborator

@rsahita rsahita commented Nov 28, 2023

proposed changes to mttp CSR to reserve upper bits for expanded PA (future), and reduce SDID to 6 bits and reserve remaining bits

proposed changes to mttp CSR to reserve upper bits for expanded PA (future), and reduce SDID to 6 bits and reserve remaining bits 

Signed-off-by: Ravi Sahita <[email protected]>
@gagachang
Copy link

Hello Ravi, may I know the rationale of reducing SDID to 6 bits?

@rsahita
Copy link
Collaborator Author

rsahita commented Nov 28, 2023

Hi @gagachang - we had not formally discussed the width of this field - given what we expect for usages, such as Confidential Computing, Keystone, various TEEs, VSM, pKVM etc, it seemed that a handful of supervisor domains would be reasonable, so 6 bits seemed apropos. (We can keep some bits reserved for future in the register as specified).

chapter3.adoc Outdated
{bits: 4, name: 'Mode'},
{bits: 44, name: 'MTTPPN'},
{bits: 8, name: 'Zero'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zero -> WPRI
MTTPPN -> PPN (since the CSR already says its MTT).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kasanovic any feedback on this change - please ack if none, then I can merge it.

Copy link
Collaborator

@ved-rivos ved-rivos Dec 1, 2023

Choose a reason for hiding this comment

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

Should we organize the fields as: [MODE | SDID | WPRI | PPN]. Instead of splitting the reserved field into two group of bits make it one contiguous run of reserved bits between the MODE, SDID, and PPN fields.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so the first WPRI bits are for other control bits if needed (like lock) and the second WPRI bits are for the PPN expansion (future)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other controls could be filled in the WPRI run of bits for future - growing towards right. The PPN would want to expand and grow towards left.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack

chapter3.adoc Outdated Show resolved Hide resolved
chapter3.adoc Outdated Show resolved Hide resolved
chapter3.adoc Outdated Show resolved Hide resolved
chapter3.adoc Outdated Show resolved Hide resolved
chapter3.adoc Outdated Show resolved Hide resolved
@kasanovic
Copy link
Collaborator

The format of the remaining fields should be dependent on mode setting, to leave the most flexibility for future enhancements. So the breakdown into SDID/PPN/WPRI should be mode-specific, and so we're only specifying this split for the modes we're proposing in this version of spec, not all possible values of mode field.

@kasanovic
Copy link
Collaborator

I see now there's already a sentence in spec saying what's in my last comment, so can ignore.

@kasanovic
Copy link
Collaborator

The PPN should be aligned in the LSBs just as with satp.

chapter3.adoc Outdated Show resolved Hide resolved
chapter3.adoc Outdated Show resolved Hide resolved
@rsahita
Copy link
Collaborator Author

rsahita commented Dec 11, 2023

updated per feedback

@rsahita rsahita merged commit 4dabcb4 into riscv:main Dec 11, 2023
1 check passed
@ved-rivos
Copy link
Collaborator

The update defined it as [PPN, WPRI, SDID, MODE] instead of [MODE, SDID, WPRI, PPN].

@ved-rivos
Copy link
Collaborator

@rsahita The change needed is as follows (Changing lanes to 2 also improves the readability in the PDF)
For RV64:

{reg: [
  {bits:  44, name: 'PPN (WARL)'},
  {bits:  10, name: 'WPRI'},
  {bits:   6, name: 'SDID (WARL)'},
  {bits:   4, name: 'Mode (WARL)'},
], config:{lanes: 2, hspace:1024}}

For RV32:

{reg: [
  {bits:  22, name: 'PPN (WARL)'},
  {bits:   2, name: 'WPRI'},
  {bits:   6, name: 'SDID (WARL)'},
  {bits:   2, name: 'Mode (WARL)'},
], config:{lanes: 2, hspace:1024}}

@rsahita
Copy link
Collaborator Author

rsahita commented Dec 11, 2023

yes i noticed the error and had fixed it :-) - will update to improve readability per your comment (thanks)

@ved-rivos
Copy link
Collaborator

@rsahita another thing to fix is perhaps to remove "All sub-fields are WARL." since some subfields are not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants