PTEs are invalid if reserved bits are non-zero #677

Timmmm · 2025-01-10T12:13:31Z

This has only been the case since version 1.12 of the spec, so I added support for setting the spec version. It isn't exposed on the command line yet since hopefully we'll be using the new config system soon.

github-actions · 2025-01-10T12:27:18Z

Test Results

392 tests ±0 392 ✅ ±0 1m 19s ⏱️ ±0s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit fc31269. ± Comparison against base commit 2dfc4ff.

♻️ This comment has been updated with latest results.

arichardson · 2025-01-10T18:39:18Z

model/riscv_types.sail

+// Machine and Supervisor Architectures v1.12
+function clause extensionEnabled(Ext_Sx1p12) = extensionEnabled(Ext_Sx1p11) | true
+// Machine and Supervisor Architectures v1.13
+function clause extensionEnabled(Ext_Sx1p13) = extensionEnabled(Ext_Sx1p12) | true


Should this be an & instead of an | ? I.e. 1.13 requires 1.12?

Not that it really matters since it's all hardcoded to 1.

Yeah I think you're right. I'll change it.

If we are going to go with this approach and not a proper extension version function, then I think the following would make more sense. The current use of | is definitely not right (enabling 1.11 would result in 1.13 being enabled), but I think using & is also cumbersome because enabling 1.13 would require also manually enabling 1.11 and 1.12.

// Machine and Supervisor Architectures v1.11 function clause extensionEnabled(Ext_Sx1p11) = extensionEnabled(Ext_Sx1p12) | true // Machine and Supervisor Architectures v1.12 function clause extensionEnabled(Ext_Sx1p12) = extensionEnabled(Ext_Sx1p13) | true // Machine and Supervisor Architectures v1.13 function clause extensionEnabled(Ext_Sx1p13) = true

That way the user just needs to enable the latest version that they support and other versions would implicitly be enabled as well. This probably covers most cases (since there are very few backwards incompatible changes) and the few backwards incompatible scenarios can use something like if extensionEnabled(Ext_Sx1p12) & not(extensionEnabled(Ext_Sx1p13))

Yeah I think Jessica is right - I'm going to rework this to use a proper versioning function.

Sounds good. That is definitely the more robust approach.

Great. Happy to review that when you have it.

model/riscv_extensions.sail

jrtc27 · 2025-01-10T18:48:41Z

model/riscv_types.sail

+// Machine and Supervisor Architectures v1.11
+function clause extensionEnabled(Ext_Sx1p11) = true
+// Machine and Supervisor Architectures v1.12
+function clause extensionEnabled(Ext_Sx1p12) = extensionEnabled(Ext_Sx1p11) | true


This doesn't make sense to me. You can't have multiple versions of an extension implemented. Ss1p12 is implemented if and only if priv 1.12 is the version your implementation chose. You have the Ss and Sm extensions and they have a version corresponding to whatever's implemented.

What you're after is some extensionVersion function that, given Ss or Sm (NB: no suffix on the extension name in the enum), tells you what version is implemented, so you can do >= 1.12 etc where needed.

Hmm yeah that is definitely the more correct way to do it, but it seemed a lot simpler to pretend they are separate extensions and then ensure that version N implies versions <N.

Open to other options though.

Simpler, sure, but definitely wrong, and for the official model that's a real problem.

Is it actually wrong? If a chip supports Ss1p12 and some instruction requires Ss1p11 then isn't it fair to say the chip supports Ss1p12 and Ss1p11... Unless the versions aren't backwards compatible I guess. Which tbf is the sometimes the case.

Exactly, they're not backwards compatible. PTE reserved bit handling being such a case. Each SsXpY is its own independent thing, where a later version is generally a superset of the previous version, but not always entirely.

Ah ok I understand. I'll try and come up with something better.

Timmmm · 2025-02-25T16:21:31Z

Ok @jrtc27 I have given this another stab. It's kind of simpler - just priv and unpriv spec versions. You can compare them with ==, >, etc.

One problem is I'm having trouble actually coming up with a list of versions. I couldn't find a good one on the RVI website, and the git history/tags doesn't seem to be very helpful either. Does anyone know where there is an archive of released specs and their version numbers?

Timmmm · 2025-02-25T16:25:55Z

RISC-V Technical Specifications has the latest versions, Unpriv 20240411 and Priv 20240411

RISC-V Technical Specifications Archive has Unpriv 20191213 and Priv 20211203

Are there really only two versions of the specs?

It doesn't help that they seem to have half switched from major.minor to dates.

Timmmm · 2025-02-25T16:27:11Z

model/riscv_vmem_pte.sail

-  | pte_ext[reserved] != zeros()
+  // Since version 1.12 of the spec, reserved bits must be zero.
+  | privVersion() >= Version_1_12 & (
+    // All bits are currently reserved for non-leaf PTEs.


This is a fix btw; the current code is missing it.

jordancarlin · 2025-02-25T17:11:21Z

For Priv there are defined versions. 1.13, 1.12, and 1.11 seem sufficient for Sail, though technically 1.10 and 1.9 also exist. I'll see if I can find the documentation where I found this.

Timmmm · 2025-02-25T17:29:17Z

I think they switched from 1.XX to date based versions though. Except... not completely, so now there are both, e.g. 1.12 is 20211203. It seems to be a mess. Maybe we should ask Andrew Waterman.

jordancarlin · 2025-02-25T17:32:16Z

My understanding is there there are date based releases of the manuals, but 1.XX releases of versions of the spec. So a particular date based release will have priv 1.12 and a different date based release will have priv 1.13. The intermittent date based releases might add other extensions to the priv manual.

I think we should base it off of extension versions, so 1.12, 1.13, etc. instead of manual releases since those seem somewhat arbitrary.

Timmmm · 2025-02-25T18:40:28Z

Hmm maybe. I'm going to ask Mr Waterman. This seems like something that could be a lot clearer!

This has only been the case since version 1.12 of the spec, so I added support for setting the spec version. It isn't exposed on the command line yet since hopefully we'll be using the new config system soon.

Timmmm · 2025-03-03T16:26:32Z

Well the discussion on riscv-isa-manual wasn't very illuminating and after discussing it in the meeting today we came to the conclusion that just using the date was the best option. I thought it would also allow us to easily then support non-released versions (since there have apparently only been 2 releases ever), but because Git commit/author dates are a little woolly it's not quite as nice as I'd hoped. I've pushed a version that does that anyway.

The full giving-up-on-their-version-numbers option would be to use riscv-isa-manual commit hashes. I'm actually thinking that might make sense. It has the added benefit that people can easily find where the behaviour changed in the spec (since it doesn't have the greatest changelog). We don't need to add every commit; just released ones and ones where something changed. Something like the previous version:

enum IsaVersion = {
    Version_31bc179b214fc0374075ea9284aa939b8341b843,
    ... more versions *in order* ...
}

// TODO: Replace with 'config ...priv_version' or similar when the config system is available.
function isaVersion() -> IsaVersion = ...

...
overload operator < = {isa_version_lt}
...

I think this is the most robust option. The downside is obviously that nobody really likes reading commit hashes. But I think given that there isn't an "official" versioning scheme that really makes any sense maybe this is the best option?

allenjbaum · 2025-03-03T16:31:06Z

We can't use hashes if we ever need to compare them, if version>x.

…

On Mon, Mar 3, 2025 at 8:26 AM Tim Hutt ***@***.***> wrote: Well the discussion on riscv-isa-manual wasn't very illuminating and after discussing it in the meeting today we came to the conclusion that just using the date was the best option. I thought it would also allow us to easily then support non-released versions (since there have apparently only been 2 releases ever), but because Git commit/author dates are a little woolly it's not quite as nice as I'd hoped. I've pushed a version that does that anyway. The full giving-up-on-their-version-numbers option would be to use riscv-isa-manual commit hashes. I'm actually thinking that might make sense. It has the added benefit that people can easily find where the behaviour changed in the spec (since it doesn't have the greatest changelog). We don't need to add *every* commit; just released ones and ones where something changed. Something like the previous version: enum IsaVersion = { Version_31bc179b214fc0374075ea9284aa939b8341b843, ... more versions *in order* ... } // TODO: Replace with 'config ...priv_version' or similar when the config system is available. function isaVersion() -> IsaVersion = ... ... overload operator < = {isa_version_lt} ... I think this is the most robust option. The downside is obviously that nobody really likes reading commit hashes. But I think given that there isn't an "official" versioning scheme that really makes any sense maybe this is the best option? — Reply to this email directly, view it on GitHub <#677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJQ3MNWBXZV3VQ7FUFD2SR7FHAVCNFSM6AAAAABU6H375SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJUHEZDSNJUGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***> [image: Timmmm]*Timmmm* left a comment (riscv/sail-riscv#677) <#677 (comment)> Well the discussion on riscv-isa-manual wasn't very illuminating and after discussing it in the meeting today we came to the conclusion that just using the date was the best option. I thought it would also allow us to easily then support non-released versions (since there have apparently only been 2 releases ever), but because Git commit/author dates are a little woolly it's not quite as nice as I'd hoped. I've pushed a version that does that anyway. The full giving-up-on-their-version-numbers option would be to use riscv-isa-manual commit hashes. I'm actually thinking that might make sense. It has the added benefit that people can easily find where the behaviour changed in the spec (since it doesn't have the greatest changelog). We don't need to add *every* commit; just released ones and ones where something changed. Something like the previous version: enum IsaVersion = { Version_31bc179b214fc0374075ea9284aa939b8341b843, ... more versions *in order* ... } // TODO: Replace with 'config ...priv_version' or similar when the config system is available. function isaVersion() -> IsaVersion = ... ... overload operator < = {isa_version_lt} ... I think this is the most robust option. The downside is obviously that nobody really likes reading commit hashes. But I think given that there isn't an "official" versioning scheme that really makes any sense maybe this is the best option? — Reply to this email directly, view it on GitHub <#677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJQ3MNWBXZV3VQ7FUFD2SR7FHAVCNFSM6AAAAABU6H375SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJUHEZDSNJUGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

Alasdair · 2025-03-03T16:42:23Z

Do we need to support non-released versions? Could we just have some sort of Version_next which is greater than all released versions to capture ongoing changes?

I didn't bring it up in the meeting, but the ISA string text in the manuals does specify a versioning scheme as something like ExtXpY for version X.Y of an extension - is that related or is priv+unpriv just versioned differently?

Timmmm · 2025-03-03T17:17:31Z

We can't use hashes if we ever need to compare them, if version>x.

This is solved (awkwardly) by listing the ones we use in order in the enum.

Do we need to support non-released versions?

I think we do, given that there have apparently only been two released versions of the ISA manual. Did people really wait for 4 years after 20191213 without implementing any of the new specs? I'd be really surprised if there aren't chips out there targeting versions between 20191213 and 20240411. The chip I'm working on targets 2023-04-27.

but the ISA string text in the manuals does specify a versioning scheme as something like ExtXpY for version X.Y of an extension - is that related or is priv+unpriv just versioned differently?

The RISC-V profiles mention:

Ss1p12 Privileged Architecture version 1.12.

But then it goes on to contradict itself:

Sm1p11, Machine Architecture v1.11
Sm1p12, Machine Architecture v1.12
Ss1p11, Supervisor Architecture v1.11
Ss1p12, Supervisor Architecture v1.12

Also the actual ISA manual doesn't mention the Sm or Ss extensions anywhere I can see (difficult to search for though so I may have missed it).

Timmmm · 2025-03-03T17:18:28Z

If we can come up with exact commits for v1.11, 1.12, etc. then we could maybe use those, but that seems to have been lost to history.

arichardson · 2025-03-03T20:53:32Z

If we can come up with exact commits for v1.11, 1.12, etc. then we could maybe use those, but that seems to have been lost to history.

1.12 seems to be 20211203: https://github.com/riscv/riscv-isa-manual/releases/tag/Priv-v1.12

arichardson · 2025-03-03T20:54:37Z

And 1.11 looks like it's https://github.com/riscv/riscv-isa-manual/releases/tag/Ratified-IMFDQC-and-Priv-v1.11, i.e. 20190608

arichardson · 2025-03-03T20:55:41Z

For 1.10 I can find https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-priv-1.10 which suggests 20170509

Timmmm · 2025-03-04T16:33:16Z

Ah good detective work! What about unpriv? If we can find sensible commits for all of them then I would say we can use Priv_1_12 etc and just add a comment saying exactly which commit we mean. I guess the Sail model can become the authoritative source on that :-D

jordancarlin · 2025-03-04T23:11:45Z

At this point, it sort of seems like we would be better off basing things off of extension versions instead of trying to capture a manual release version. Everything(?) in the spec is contained within an extension, and there are not technically any rules preventing someone from implementing versions of extensions from different manual releases, so this seems the most consistent approach.

For most unprivileged extensions there is just the one version (1.0), so almost nothing needs to change. Some of the base extensions are on version 2.0/2.1/2.2, but I doubt we need to worry about older versions of those. The only significant one we might want to support is the older version of I that included Zicsr and Zifencei.

For privileged extensions, most have their own version numbers (all 1.0 from a quick glance) except machine mode and supervisor mode themselves, which we'd want to support 1.10, 1.11, 1.12, and 1.13 for.

Seems like for an initial attempt at this, we can just support various versions of Machine and Supervisor modes and address older versions of other extensions as they come up if users need them.

Timmmm · 2025-03-05T11:02:01Z

For most unprivileged extensions there is just the one version (1.0), so almost nothing needs to change.

Yeah the problem is that RVI hasn't been very disciplined about versioning, so it's unclear exactly which document version N of spec Y is, and there have been occasional "clarifications" without changing version numbers. That's the big advantage of using Git commits - you can actually know with certainty what specification you are talking about.

I think it's fine if we use version numbers and then also say in a comment which commit hash we mean though:

enum PrivVersion = {
  // 98964261c931d51884f733664b22efe43de93033
  Version_1_12,
  etc.
}

arichardson · 2025-03-05T17:14:59Z

model/riscv_extensions.sail

+type SpecVersion = range(20000000, 21000000)
+
+// TODO: Replace with 'config ...priv_version' or similar when the config system is available.
+let configPrivVersion : SpecVersion = 20240411


Maybe the easiest approach here to allow using relational operators would be to add variables such as Priv_1_12 = 20211203 and use those in the comparisons?

And a comment above with a commit hash and a link to the release on github?

Timmmm added the tgmm-agenda Tagged for the next Golden Model meeting agenda. label Jan 10, 2025

arichardson reviewed Jan 10, 2025

View reviewed changes

jrtc27 reviewed Jan 10, 2025

View reviewed changes

model/riscv_extensions.sail Outdated Show resolved Hide resolved

jrtc27 reviewed Jan 10, 2025

View reviewed changes

Timmmm removed the tgmm-agenda Tagged for the next Golden Model meeting agenda. label Jan 22, 2025

Timmmm mentioned this pull request Feb 3, 2025

Add initial Sail snippets riscv/riscv-isa-manual#1838

Open

Timmmm force-pushed the user/timh/priv_unpriv_spec_versions branch from c913739 to 172a86a Compare February 25, 2025 16:18

Timmmm commented Feb 25, 2025

View reviewed changes

Timmmm added 2 commits March 3, 2025 16:01

PTEs are invalid if reserved bits are non-zero

a416d0e

This has only been the case since version 1.12 of the spec, so I added support for setting the spec version. It isn't exposed on the command line yet since hopefully we'll be using the new config system soon.

Switch to date based version

fc31269

Timmmm force-pushed the user/timh/priv_unpriv_spec_versions branch from 172a86a to fc31269 Compare March 3, 2025 16:18

arichardson reviewed Mar 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PTEs are invalid if reserved bits are non-zero #677

PTEs are invalid if reserved bits are non-zero #677

Timmmm commented Jan 10, 2025

github-actions bot commented Jan 10, 2025 •

edited

Loading

arichardson Jan 10, 2025

Timmmm Jan 10, 2025

jordancarlin Jan 13, 2025

Timmmm Jan 13, 2025

jordancarlin Jan 13, 2025

jrtc27 Jan 13, 2025

jrtc27 Jan 10, 2025 •

edited

Loading

Timmmm Jan 10, 2025

jrtc27 Jan 10, 2025

Timmmm Jan 10, 2025

jrtc27 Jan 10, 2025

Timmmm Jan 10, 2025

Timmmm commented Feb 25, 2025

Timmmm commented Feb 25, 2025 •

edited

Loading

Timmmm Feb 25, 2025

jordancarlin commented Feb 25, 2025

Timmmm commented Feb 25, 2025

jordancarlin commented Feb 25, 2025

Timmmm commented Feb 25, 2025

Timmmm commented Mar 3, 2025

allenjbaum commented Mar 3, 2025 via email

Alasdair commented Mar 3, 2025

Timmmm commented Mar 3, 2025

Timmmm commented Mar 3, 2025

arichardson commented Mar 3, 2025

arichardson commented Mar 3, 2025

arichardson commented Mar 3, 2025

Timmmm commented Mar 4, 2025

jordancarlin commented Mar 4, 2025

Timmmm commented Mar 5, 2025

arichardson Mar 5, 2025

arichardson Mar 5, 2025

PTEs are invalid if reserved bits are non-zero #677

Are you sure you want to change the base?

PTEs are invalid if reserved bits are non-zero #677

Conversation

Timmmm commented Jan 10, 2025

github-actions bot commented Jan 10, 2025 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timmmm commented Feb 25, 2025

Timmmm commented Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

jordancarlin commented Feb 25, 2025

Timmmm commented Feb 25, 2025

jordancarlin commented Feb 25, 2025

Timmmm commented Feb 25, 2025

Timmmm commented Mar 3, 2025

allenjbaum commented Mar 3, 2025 via email

Alasdair commented Mar 3, 2025

Timmmm commented Mar 3, 2025

Timmmm commented Mar 3, 2025

arichardson commented Mar 3, 2025

arichardson commented Mar 3, 2025

arichardson commented Mar 3, 2025

Timmmm commented Mar 4, 2025

jordancarlin commented Mar 4, 2025

Timmmm commented Mar 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2025 •

edited

Loading

jrtc27 Jan 10, 2025 •

edited

Loading

Timmmm commented Feb 25, 2025 •

edited

Loading