Improve PMP support #350
Merged
Improve PMP support #350
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Ooh, nice diagrams! These should be added to the spec. The reason for
defining read-only-zero entries is because the spec says options are 0,15,
and 64, so if you want to implement 8 entries, then you need to pad the
last 8 with zeroes (which must be readable and not trap - at least in priv
1.12)
Note that it also perfectly legal for bits in the cfg registers to be
rdonly0/1 (it is WARL….) which I would expect to be used to restrict the
mode types (e.g. only allow OFF and NAPOT or disallow NA4)
You say you’re adding support for rdonly0- how are you passing which
registers are rdonly0 to Sail?
…On Friday, November 17, 2023, Tim Hutt ***@***.***> wrote:
This implements a lot of missing functionality for PMPs.
- Support 64 PMPs as well as 0 and 16.
- Support read-only zero PMPs
- Support setting PMP grain
- Return correct address bits on read (some read as 0 or 1 depending
on the grain and match type)
- Unlock PMPs on reset
- Implement pmpcfg WARL legalisation
------------------------------
Notes:
This is a moderately large PR and I am open to breaking it down if
necessary. Also there are some caveats that I would like feedback on:
- The pmp_writable is a 64-bit bitmask that is a little tricky to
integrate into the OCaml CLI because it doesn't support 64-bit integers
natively. If someone could help me with that that would be great. We could
potentially just remove it since we only added it because a Codasip chip
has a slightly unusual PMP configuration. I suspect there aren't many chips
that implement PMPs but make some of them read-only zero.
- I haven't updated the lem code yet. I am unsure how to test that.
- There are no tests added (pending Bill's work on testing
infrastructure). Additionally the PMP spec is one of the less clearly
written parts of the spec. However we have been using this for some time
now and haven't found any issues for a while.
- The WARL legalisation is just one possible legalisation option. It
is likely a reasonable one though. It should be made configurable at a
future point (pending riscv-config support probably).
Here's a diagram I made to help me understand pmpaddr which may be
helpful:
[image: image]
<https://user-images.githubusercontent.com/376842/283762504-f6b47425-11f6-456f-bbe9-c1d8f6788bd5.png>
------------------------------
You can view, comment on, or merge this pull request online at:
#350
Commit Summary
- ccbeed5
<ccbeed5>
Improve PMP support
File Changes
(19 files <https://github.com/riscv/sail-riscv/pull/350/files>)
- *M* c_emulator/riscv_platform.c
<https://github.com/riscv/sail-riscv/pull/350/files#diff-99f5aea243e6075806ba23237734946f5cc1f5df28df7ad460c613ea8e19fd99>
(20)
- *M* c_emulator/riscv_platform.h
<https://github.com/riscv/sail-riscv/pull/350/files#diff-867f024d1abc02161c425618f491948da4c938a99b7213b2b95460abb802ecb5>
(5)
- *M* c_emulator/riscv_platform_impl.c
<https://github.com/riscv/sail-riscv/pull/350/files#diff-4a737f162bb33b9a645875323182a167db71ee1603a7e0b2b69ad45e006cc853>
(5)
- *M* c_emulator/riscv_platform_impl.h
<https://github.com/riscv/sail-riscv/pull/350/files#diff-b8ca9c413288aca4760beeb4153266a2832e6fbcb714463c1954b9cc5b09d8a5>
(5)
- *M* c_emulator/riscv_sim.c
<https://github.com/riscv/sail-riscv/pull/350/files#diff-6c055f3c60e99346ccba11c821c5a97e86542a33459036341860ff71d36d11f0>
(27)
- *M* handwritten_support/0.11/riscv_extras.lem
<https://github.com/riscv/sail-riscv/pull/350/files#diff-6c008e6140bd8ccf2eda179d2f10bc0ade72a5ebb308889f18bf7fc18da13d3a>
(4)
- *M* handwritten_support/0.11/riscv_extras_sequential.lem
<https://github.com/riscv/sail-riscv/pull/350/files#diff-401327fc62b13443164ddd8721ea8ec078a57c91ee71326796eb3d2669d8a997>
(4)
- *M* handwritten_support/riscv_extras.lem
<https://github.com/riscv/sail-riscv/pull/350/files#diff-c6f8ec74cfb6c62c093f624dbf9e11a8f01d574dba187456c4f080c1a07c64fa>
(4)
- *M* handwritten_support/riscv_extras_sequential.lem
<https://github.com/riscv/sail-riscv/pull/350/files#diff-1659f42ebae38a39c8dea3497c57292907b796947ca7890c1cd31480e15c95a1>
(4)
- *M* model/riscv_csr_map.sail
<https://github.com/riscv/sail-riscv/pull/350/files#diff-8ef8c4f1ec7b813dbe3e53af2077629d062818e37defa2eec6f17996057208a3>
(60)
- *M* model/riscv_insts_zicsr.sail
<https://github.com/riscv/sail-riscv/pull/350/files#diff-c10b5137a896b995de0b1486ff9a26a27cca56d063e8ce5c2e3d6572c7960997>
(59)
- *M* model/riscv_mem.sail
<https://github.com/riscv/sail-riscv/pull/350/files#diff-5ca5dad678e3d7da2175c4ee1931c3608f6e2b131893f618522dd93c2723d413>
(4)
- *M* model/riscv_platform.sail
<https://github.com/riscv/sail-riscv/pull/350/files#diff-99ea418980d5c649ed764c60511d1e827ab318746db5c4ef9473e62e8fe895b0>
(6)
- *M* model/riscv_pmp_control.sail
<https://github.com/riscv/sail-riscv/pull/350/files#diff-0e4c71d06b6c23f8db3460610a898daf54557d0b112a9461dc379152bd957204>
(139)
- *M* model/riscv_pmp_regs.sail
<https://github.com/riscv/sail-riscv/pull/350/files#diff-3d251956b0e0495882279272a39852422ce72366f9ca0d4b6f337c8489691444>
(193)
- *M* model/riscv_sys_control.sail
<https://github.com/riscv/sail-riscv/pull/350/files#diff-e89888d2ba8a243ac03cfe4ecce7acdb820a3135cafc486394d369a2054712e9>
(32)
- *M* model/riscv_sys_regs.sail
<https://github.com/riscv/sail-riscv/pull/350/files#diff-e1a3fe7050c3af4c308a5e39bfb7f0f31094022b9892cee9a611bcb57ccb6726>
(9)
- *M* ocaml_emulator/platform.ml
<https://github.com/riscv/sail-riscv/pull/350/files#diff-372041ac1dc4ebeafe114dce35e7bdc72abd74af4debe84f54be0e91683cffeb>
(13)
- *M* ocaml_emulator/riscv_ocaml_sim.ml
<https://github.com/riscv/sail-riscv/pull/350/files#diff-9146c826ecf64091ecdf16e5b350b7f5895c1d27d5c0eee8ea2b86e7cb5f2710>
(12)
Patch Links:
- https://github.com/riscv/sail-riscv/pull/350.patch
- https://github.com/riscv/sail-riscv/pull/350.diff
—
Reply to this email directly, view it on GitHub
<#350>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJTX7FM3HER3DXDMOSTYE432ZAVCNFSM6AAAAAA7PSFP5SVHI2DSMVQWIX3LMV43ASLTON2WKOZRHE4TQNZYGAZDIMY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
The
But why not just say "you must implement 64 PMP entries but any can be read-only zero" (which is what it says for HPM counters) or "you can implement 0 to 64 PMP entries". Why have two different mechanisms for implementing less than 64 entries, and why restrict the count to 0, 16, or 64? Quite a strange bit of the spec IMO. |
|
I didn’t know there was a bitmask for easily. Technically the readonly
value could be anything, not just zero. That sounds like and odd thing to
do, but might be useful in security situations.
The reason for specifying 0, 16, or 64 is so firmware can depend on
accessing them without trapping (since there is no other way to determine
which ones are implemented)
…On Friday, November 17, 2023, Tim Hutt ***@***.***> wrote:
You say you’re adding support for rdonly0- how are you passing which
registers are rdonly0 to Sail?
The sys_pmp_writable() is a 64-bit value that's used as a bitmask. If bit
N of that is 0 then pmpaddrN is read-only zero.
if you want to implement 8 entries, then you need to pad the
last 8 with zeroes
But why not just say "you must implement 64 PMP entries but any can be
read-only zero" (which is what it says for HPM counters) or "you can
implement 0 to 64 PMP entries". Why have two different mechanisms for
implementing less than 64 entries, and why restrict the count to 0, 16, or
64? Quite a strange bit of the spec IMO.
—
Reply to this email directly, view it on GitHub
<#350 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJVL6CEEC5NQINNST3DYE6KM3AVCNFSM6AAAAAA7PSFP5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJWG44DINJYGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
That's true if all 64 should always be implemented too though. I guess it's probably for historical reasons.
Yeah I think ultimately this will be solved by |
|
I don't know much about PMPs but the code is replacing some rather ugly copy+paste logic with generic logic that handles more PMP registers in a generic way, which I think is an improvement 👍 |
allenjbaum
reviewed
Dec 5, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
reviewed
Dec 6, 2023
allenjbaum
requested changes
Dec 6, 2023
There was a problem hiding this comment.
This is a first step, but this should be modified to fully support 0,16 AND 64 sets of CSRs, not just 0 and 16 (there is some code that will handle 64, but others do not).
This implementation assumes architectural parameter values that need to be supplied by the vendor, and this will need to get updated - someday soon, but not right now. WARL fields will be difficult, but there should be hooks such that the implementation supplied configurations can be easily supplied in the future.
How many of these things are features that are present today in the Sail model? Anything that is not present today should not block this refactoring, those are additional feature requests that can come later. This is not a "do everything you could possibly want for PMPs" PR. |
|
The point here is that this code
- is refactored to make the number of supported entries a variable (with
only 3 specific values)
- defines the data structures to allow 64 registers,
- but only defines the loops that test, read, and write them to use an
index up to 15.
What it takes to handle 64 is the change of a constant in a few places, and
it would be stupid to only go part way and leave defined structures unused.
Tests are being written that will test how many are "implemented". (which
means they won't trap if read in Mmode)
…On Tue, Dec 5, 2023 at 9:41 PM Jessica Clarke ***@***.***> wrote:
This is a first step, but this should be modified to and 0,16 AND 64 sets
of CSRs, not just 0 and 16 (there is some code that will handle 64, but
others do not. This implementation assumes architectural parameter values
that need to be supplied by the vendor, and this will need to get updated.
WARL fields will be difficult, but there should be hooks such that the
implemetation supplied configurations can be easily supplied in the future.
How many of these things are features that are present today in the Sail
model? Anything that is not present today should not block this
refactoring, those are additional feature requests that can come later.
This is not a "do everything you could possibly want for PMPs" PR.
—
Reply to this email directly, view it on GitHub
<#350 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJT6TZUP2TUTCE3Z7OTYIAAQRAVCNFSM6AAAAAA7PSFP5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBSGEZDKMJXGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Oh, and the WARL fields and per register default values: that can wait - or
not, if someone feels ambitious. I think it will need to done in
conjunction with the general WARL field handling,
but I'm pretty sure there are chips that will fail tests as this is defined
now.
On Tue, Dec 5, 2023 at 9:53 PM Allen Baum ***@***.***>
wrote:
… The point here is that this code
- is refactored to make the number of supported entries a variable (with
only 3 specific values)
- defines the data structures to allow 64 registers,
- but only defines the loops that test, read, and write them to use an
index up to 15.
What it takes to handle 64 is the change of a constant in a few places,
and it would be stupid to only go part way and leave defined structures
unused.
Tests are being written that will test how many are "implemented". (which
means they won't trap if read in Mmode)
On Tue, Dec 5, 2023 at 9:41 PM Jessica Clarke ***@***.***>
wrote:
> This is a first step, but this should be modified to and 0,16 AND 64 sets
> of CSRs, not just 0 and 16 (there is some code that will handle 64, but
> others do not. This implementation assumes architectural parameter values
> that need to be supplied by the vendor, and this will need to get updated.
> WARL fields will be difficult, but there should be hooks such that the
> implemetation supplied configurations can be easily supplied in the future.
>
> How many of these things are features that are present today in the Sail
> model? Anything that is not present today should not block this
> refactoring, those are additional feature requests that can come later.
> This is not a "do everything you could possibly want for PMPs" PR.
>
> —
> Reply to this email directly, view it on GitHub
> <#350 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHPXVJT6TZUP2TUTCE3Z7OTYIAAQRAVCNFSM6AAAAAA7PSFP5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBSGEZDKMJXGI>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
|
|
Ok I removed the |
|
I realize that bits(6) doesn't support addition; but I thought there were a
bits-to-int and int-to bits functions that could be used to effectively do
that.
I also understand that it would wrap; in this case, you'd either have to
filter that out (which is easy), else you'd get two matches.
…On Mon, Dec 11, 2023 at 1:41 AM Tim Hutt ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In model/riscv_insts_zicsr.sail
<#350 (comment)>:
> - (0x3B2, _) => pmpaddr2,
- (0x3B3, _) => pmpaddr3,
- (0x3B4, _) => pmpaddr4,
- (0x3B5, _) => pmpaddr5,
- (0x3B6, _) => pmpaddr6,
- (0x3B7, _) => pmpaddr7,
- (0x3B8, _) => pmpaddr8,
- (0x3B9, _) => pmpaddr9,
- (0x3BA, _) => pmpaddr10,
- (0x3BB, _) => pmpaddr11,
- (0x3BC, _) => pmpaddr12,
- (0x3BD, _) => pmpaddr13,
- (0x3BE, _) => pmpaddr14,
- (0x3BF, _) => pmpaddr15,
+ // pmpcfgN
+ (0x3A @ idx : bits(4), _) if idx[0] == bitzero | sizeof(xlen) == 32 => pmpReadCfgReg(unsigned(idx)),
Ah I see what you are proposing now. Unfortunately it won't work because:
- Sail doesn't support refinement types for bits(), so bits(6) + 0x10
isn't a valid type.
- Addition on bits() wraps anyway. It doesn't overflow past the @
which is just for concatenation.
Given that Sail doesn't support that, this is the best available option I
know of. I don't think it's too bad really.
—
Reply to this email directly, view it on GitHub
<#350 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJVRVN65JZJJRSRCM7LYI3ILLAVCNFSM6AAAAAA7PSFP5SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZUGY4DSOBVGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
There is |
arichardson
reviewed
Dec 18, 2023
Timmmm
force-pushed
the
user/timh/pma
branch
from
a8ea2b6
to
8133fc1
Compare
|
Thanks. I've sent an invitation to you which will give you write access
(should you accept). Let me know if it works for you.
Bill Mc.
…On Mon, Jan 15, 2024 at 5:09 PM Alexander Richardson < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In model/riscv_pmp_control.sail
<#350 (comment)>:
> - pmp9cfg = update_A(pmp9cfg, pmpAddrMatchType_to_bits(OFF));
- pmp10cfg = update_A(pmp10cfg, pmpAddrMatchType_to_bits(OFF));
- pmp11cfg = update_A(pmp11cfg, pmpAddrMatchType_to_bits(OFF));
- pmp12cfg = update_A(pmp12cfg, pmpAddrMatchType_to_bits(OFF));
- pmp13cfg = update_A(pmp13cfg, pmpAddrMatchType_to_bits(OFF));
- pmp14cfg = update_A(pmp14cfg, pmpAddrMatchType_to_bits(OFF));
- pmp15cfg = update_A(pmp15cfg, pmpAddrMatchType_to_bits(OFF))
+ assert(
+ sys_pmp_count() == 0 | sys_pmp_count() == 16 | sys_pmp_count() == 64,
+ "sys_pmp_count() must be 0, 16, or 64"
+ );
+
+ foreach (i from 0 to 63) {
+ // On reset the PMP register's A and L bits are set to 0 unless the plaform
+ // mandates a different value.
+ pmpcfg_n[i] = update_A(pmpcfg_n[i], pmpAddrMatchType_to_bits(OFF));
It appears you need repository write access or be the creator of the PR to
resolve comments, so I can't do it, but as far as I can tell this has been
resolved.
—
Reply to this email directly, view it on GitHub
<#350 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXROLOAQDFDVD5UZUJO2O5LYOXHMVAVCNFSM6AAAAAA7PSFP5SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMRSGM4TANZUG4>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
--
Bill McSpadden
Formal Verification Engineer
RISC-V International
mobile: 503-807-9309
|
|
Thanks, I can now mark comments as resolved. |
arichardson
reviewed
Jan 31, 2024
| case OPT_PMP_COUNT: | ||
| pmp_count = atol(optarg); | ||
| fprintf(stderr, "PMP count: %lld\n", pmp_count); | ||
| rv_pmp_count = pmp_count; |
There was a problem hiding this comment.
Do we need to handle invalid optarg here?
There was a problem hiding this comment.
Ah yes the Sail assumes it is <=64 so we need to check that here. I'll add that.
There was a problem hiding this comment.
Done. I added a check for pmp_grain too.
arichardson
reviewed
Jan 31, 2024
arichardson
approved these changes
Jan 31, 2024
|
@billmcspadden-riscv can this be merged? Both @arichardson and @allenjbaum have reviewed it. |
This implements a lot of missing functionality for PMPs. * Support 64 PMPs as well as 0 and 16. * Support setting PMP grain * Return correct address bits on read (some read as 0 or 1 depending on the grain and match type) * Unlock PMPs on reset * Implement pmpcfg WARL legalisation Co-authored-by: Ben Fletcher <[email protected]>
|
Hooray, thanks! 🎉 |
This was referenced Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements a lot of missing functionality for PMPs.
Notes:
This is a moderately large PR and I am open to breaking it down if necessary. Also there are some caveats that I would like feedback on:
Here's a diagram I made to help me understand
pmpaddrwhich may be helpful: