Review comments/questions on Smmtt v0.1

[SiFiveHolland]

• Why are two separate encodings used for 4M_PAGES and 2M_PAGES? These can never be used at the same time.
• It's not possible to atomically update 32 consecutive MTTL2 entries representing a 1 GiB range. Given the possibility of speculative lookups or lookups from another hart or I/O MTT, the walk behavior and fence behavior should be well-defined even when the consecutive MTTL2 entries do not match.
• The text claims that Figure 8 is applicable to Smmtt46, but this is false, due to pn[0] being 3 bits for Smmtt32, but 4 bits for Smmtt46.
• The "ZERO" fields in the MTTL3 and MTTL2 entries should be renamed "Reserved", so they can be used for a future specification.
• Why is a different encoding used for MTTL1 and 4M_PAGES/2M_PAGES? Why not use the low bits (without gaps) in both cases? Any future change that uses the other two bits in MTTL1 is likely to apply to MTTL2 as well.
• In section 4.3, the exception "but excluding MTT structure accesses" should be clarified to only exclude implicit MTT structure accesses (i.e. MTT walks), not just any memory access to the MTT. (Granted, S-mode shouldn't be able to explicitly access the MTT, because the MTT should be protected by itself or PMP.)

[ram2532]

I'd like to expand on the last bullet item - regarding section 4.3's MTT structure access. This section also includes this line:

MTT structure accesses are to be treated as implicit M-mode accesses and are subject to PMP/Smepmp and IOPMP checks.

Should the phrase "MTT structure accesses" say "implicit MTT structure accesses"? To be explicit that these accesses are part of a MTT walk?

[rsahita]

Why are two separate encodings used for 4M_PAGES and 2M_PAGES? These can never be used at the same time.

yeah can use the same.

[rsahita]

It's not possible to atomically update 32 consecutive MTTL2 entries representing a 1 GiB range. Given the possibility of speculative lookups or lookups from another hart or I/O MTT, the walk behavior and fence behavior should be well-defined even when the consecutive MTTL2 entries do not match.

With the recent updates to the MFENCE instructions and the walk behavior - is there any additional explanation needed?

[rsahita]

The text claims that Figure 8 is applicable to Smmtt46, but this is false, due to pn[0] being 3 bits for Smmtt32, but 4 bits for Smmtt46.

yes will fix the reference.

[rsahita]

The "ZERO" fields in the MTTL3 and MTTL2 entries should be renamed "Reserved", so they can be used for a future specification.

ack.

[rsahita]

Why is a different encoding used for MTTL1 and 4M_PAGES/2M_PAGES? Why not use the low bits (without gaps) in both cases? Any future change that uses the other two bits in MTTL1 is likely to apply to MTTL2 as well.

wanted to keep current PERMS and future reserved bits corresponding to a PA together. can group the other bit field the same way too.

[rsahita]

In section 4.3, the exception "but excluding MTT structure accesses" should be clarified to only exclude implicit MTT structure accesses (i.e. MTT walks), not just any memory access to the MTT. (Granted, S-mode shouldn't be able to explicitly access the MTT, because the MTT should be protected by itself or PMP.)

makes sense - will update.

[SiFiveHolland]

It's not possible to atomically update 32 consecutive MTTL2 entries representing a 1 GiB range. Given the possibility of speculative lookups or lookups from another hart or I/O MTT, the walk behavior and fence behavior should be well-defined even when the consecutive MTTL2 entries do not match.

With the recent updates to the MFENCE instructions and the walk behavior - is there any additional explanation needed?

I think so. Currently Table 3 has the text:

When configuring 1 GiB ranges, RDSM must ensure that 32 MTTL2 entries, each corresponding to 32 MiB of address space, have identical TYPE field values.

This implies that the behavior is undefined if the TYPE values are not identical. But the TYPE values will necessarily be non-identical if the permissions for a 1 GiB region ever change. So I would recommend constraining the behavior to be predicable while allowing caching flexibility by replacing that sentence with something like the following:

If this entry type is used, and the 32 consecutive MTTL2 entries corresponding to this 1 GiB address range do not have identical TYPE field values, it is unspecified which of these entries is used to determine the permissions for this address range.

Or, we could remove that sentence from the table cells and add a separate paragraph outside the table:

If any MTTL2 entry corresponding to a portion of a naturally-aligned 1 GiB address range uses one of the 1G_* entry types, then it is unspecified which of the 32 consecutive MTTL2 entries corresponding to that address range determines the permissions for the entire range.

This is similar to the behavior for I/O MTT rules with mismatching IOSDIDs added in #82.

---

Why is a different encoding used for MTTL1 and 4M_PAGES/2M_PAGES? Why not use the low bits (without gaps) in both cases? Any future change that uses the other two bits in MTTL1 is likely to apply to MTTL2 as well.

wanted to keep current PERMS and future reserved bits corresponding to a PA together. can group the other bit field the same way too.

OK, but there aren't enough bits to rearrange the MTTL2 encoding that way without making the INFO field wider. It would need to be 24/48 bits wide to allow even one reserved bit for each 2M/4M page.

[rsahita]

Adding this clarification in an incoming PR for the 1 GiB cases: "If this entry type is used, and the 32 consecutive MTTL2 entries corresponding to this 1 GiB address range do not have identical TYPE field values, it is unspecified which of these entries is used to determine the permissions for this address range."

[rsahita]

Why is a different encoding used for MTTL1 and 4M_PAGES/2M_PAGES? Why not use the low bits (without gaps) in both cases? Any future change that uses the other two bits in MTTL1 is likely to apply to MTTL2 as well.

wanted to keep current PERMS and future reserved bits corresponding to a PA together. can group the other bit field the same way too.

OK, but there aren't enough bits to rearrange the MTTL2 encoding that way without making the INFO field wider. It would need to be 24/48 bits wide to allow even one reserved bit for each 2M/4M page.

right, there is no additional space in the INFO fields - I will modify per your original suggestion to use the low bits for MTTL1 as well.

[rsahita]

merged PR #102