Skip to content

Add Sspmp extension#2573

Open
Andrew Waterman (aswaterman) wants to merge 10 commits into
mainfrom
ybc-alkaid-Sspmp
Open

Add Sspmp extension#2573
Andrew Waterman (aswaterman) wants to merge 10 commits into
mainfrom
ybc-alkaid-Sspmp

Conversation

@aswaterman

Copy link
Copy Markdown
Member

Supersedes #2521

@bgrayson-mips

Copy link
Copy Markdown
Contributor

Andrew Waterman (@aswaterman) maybe I missed it, but I still don't see the CSR assignment numbers for mpmpdeleg, sspmpswitch, et al. See the issue here: riscv/riscv-spmp#74. Apparently the ARC assigned all the indirect CSRs a while ago, but not the direct CSRs (at least so far).

@aswaterman

Copy link
Copy Markdown
Member Author

Brian Grayson (MIPS) (@bgrayson-mips) I believe that oversight is still unresolved. I'll handle it.

@aswaterman

Copy link
Copy Markdown
Member Author

Brian Grayson (MIPS) (@bgrayson-mips) done, tentatively.

Comment thread src/sspmp.adoc Outdated
In RV64, a context switch for the SPMP mechanism involves updating as many as 64 address registers and configuration registers.
The `Sspmpmask` extension, introduced in this chapter, is an optional feature designed to enhance the performance of SPMP context switches.

* For RV64 architectures, it introduces a 64-bit WARL CSR, `spmpmask`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For CSRs, traditionally the first letter refers to the privilege mode. In this case, this is the "S-mode SPMP Mask" register, which means it should be named sspmpmask, not merely spmpmask, which would be "S-mode's PMP Mask" register which doesn't exist. I don't necessarily like the redundancy of sspmpmask, but it's what I think we ought to do (and why we changed spmpswitch to sspmpswitch a few months ago).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The ARC continues to debate the naming of this register, but one thing we are in agreement on is that "Supervisor Supervisor PMP" is an unhelpful redundancy.

Comment thread src/sspmp.adoc Outdated
+
. The `sstatus.MXR` (Make eXecutable Readable) bit must be *writable*.
+
. If the Hypervisor Extension is implemented in conjunction with `Sspmp`, the only mandatory translation mode in both hgatp and satp is Bare.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a backward-incompatible change. Software can currently assume that the presence of H implies the presence of Sv32/Sv39.

It was short-sighted to specify H with that requirement, but the whole point of extensions is to provide a behavioral contract.

The cleanest solution would be to define a new extension that is the subset of H, and use it with Sspmp.
If RVI is fine with possible breakage, then specifying that H doesn't depend on Sv32/Sv39 seems better than letting new extensions stealthily break ratified extensions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi Radim Krčmář (@radimkrcmar),
I think Jose makes a fair point on the hypervisor-sig mailing list:
https://lists.riscv.org/g/sig-hypervisors/message/812

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The ARC plans to discuss a generalization of this topic at one of our next couple meetings.

(Not speaking for the ARC, I find attractive the pragmatic approach of stating that H+Sspmp has fewer constraints than H. Purists will reasonably disagree.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bicheng Yang (@ybc-alkaid) I think Jose made slightly incorrect assumptions. I spent too much time dealing with subtle bugs due to incorrect assumptions, so I am strongly biased against breaking correct assumptions.

Comment thread src/sspmp.adoc

. A key component of this CSR is the `pmpnum` WARL field, which defines the starting index for delegation.
All PMP entries with an index equal to or greater than `pmpnum` are delegated as SPMP entries.
. If a write to `pmpnum` specifies a value exceeding the number of writable PMP entries, the field subsequently reads back the total count of writable entries. In such a case, no SPMP entries are delegated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No. The field is only 7 bits wide (= maximum of 127). A fairly sensible implementation is 128 PMP entries, which would always leave a single entry delegated to S-mode.

This is not a secure default. To keep parity with mstateen, the field would ideally be wide enough to capture all writable PMP entries.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi Radim Krčmář (@radimkrcmar),
The 7-bit field is for backward compatibility.
Because the privileged spec says:

Up to 64 PMP entries are supported.

and the only way to obtain an SPMP entry is to delegate from PMP.

So I think this is still a secure default.
And you are right that once the PMP entry limit increases, the pmpnum field width should be adjusted accordingly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, thanks for pointing out that we are still limited by 64 in Sspmp, I agree it's a safe default.

I just think it would be a bit nicer to define the bigger field now when we already plan to extend it, since changing the field width opens room for errors... We can still keep the PMP entries limited at 64 until a future extension.

Software should implement a bitmask when manipulating the CSR, and preserve the reserved bits, which creates potential issues when simply extending the field.

  • If there were 128 entries, only bit 8 would be set, so M-mode aware of SPMP would think it has 0 PMP entries and possibly fail to boot. (Failed boot is the safest case of incompatibility.)
  • For more than 128 entries, M-mode might contain an assumption that SPMP entry X refers to PMP entry X + mpmpdeleg[7:0], which would be off by at least 128.
  • Another issue could arise if software didn't preserve the currently reserved bits as it should, where it would result in delegating more entries to S-mode than M-mode context-switches.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My concern with this approach is that it will create some inconsistency.
Ideally, the software can determine hardware capabilities by probing CSRs, mpmpdeleg in this case. E.g., if mpmpdeleg.pmpnum == 64, it can safely assume 64 PMP entries. However, if mpmpdeleg is 8-bit, the probe result will be incorrect, and the software must hardcode a limit of 64 entries for M-mode PMPs.

Besides, changing the PMP entry limit in future requests modifications to both hardware and software that are compiled for current RISC-V ISA. So honestly, I don't see the benefit in extending the mpmpdeleg in advance.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, the inconsistency is my concern as well. Although I'm arguing to make the field larger to avoid it. :)
I was off by one in the previous post, 128 is the 8th bit, called bit 7, sorry.

Reset values of mpmpdeleg[6:0] == 64, mpmpdeleg[7:0] == 64, or mpmpdeleg[63:0] == 64 all mean that there are 64 entries. The amount of zero topmost bits should be of very little consequence.
The inconsistency would arise if the field was later extended, and mpmpdeleg[6:0] == X (old software), mpmpdeleg[7:0] == 128 + X (new software).

Besides, changing the PMP entry limit in future requests modifications to both hardware and software that are compiled for current RISC-V ISA. So honestly, I don't see the benefit in extending the mpmpdeleg in advance.

mpmpdeleg is the first extension that makes it possible to address PMP entries above 64, so I think it's simpler to define a forward-compatible new interface now that to change the interface of mpmpdeleg in a backward-compatible way later. (For example, changing the mpmpdeleg.pmpnum field width is not backward-compatible.)

If we don't prepare software for the possibility of PMPs above 64 with mpmpdeleg, then it's probably better to later define a new extension, mutually exclusive with Smpmpdeleg, when we actually want to raise the limit.
I think it's a bit of a shame, since the work to make Smpmpdeleg able to handle increased PMP limit is almost complete.
All we need to say that the maximum number of PMP entries must be expressible in Smpmpdeleg.pmpnum.
(PMP entries above 64 won't break compatibility -- the entries won't have an effect on memory accesses, old M-mode software won't address them, and new S-mode software won't be able to address them when running on old M-mode software.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For the reasons Bicheng Yang (@ybc-alkaid) mentioned, I would just leave this as-is.

Previous drafts of the Sspmp spec said that adding the Sspmp extension
removes the requirement that the H extension imply Sv32/Sv39.  That
choice violates the RISC-V principle that extensions are purely additive.
Fix by defining a new hypervisor extension that lacks that requirement.
@ybc-alkaid

Copy link
Copy Markdown
Member

Hi Andrew Waterman (@aswaterman) , the SPMP spec is revised after public review, please check PR #3225
Many thanks!

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