riscv / sail-riscv

Sail RISC-V model
https://lists.riscv.org/g/tech-golden-model
Other
456 stars 166 forks source link

Support PMAs #320

Open Timmmm opened 1 year ago

Timmmm commented 1 year ago

Currently the thing close to PMAs in the model is this:

val phys_mem_segments : unit -> list((xlenbits, xlenbits))
function phys_mem_segments() =
  (plat_rom_base (), plat_rom_size ()) ::
  (plat_ram_base (), plat_ram_size ()) ::
  [||]

In other words you can have exactly 2 regions that exist. But the only attribute you can set is "exists". Additionally this implementation has a slight flaw in that you can't make all memory exist because the base and size are both uint64.

We need to change it so that you can have an unlimited (or at least not very limited) number of PMAs with these attributes (this is from the privileged spec):

This already affects some things in the model (e.g. FIOM) which currently assume that all memory is "normal" cacheable, idempotent, etc.

allenjbaum commented 1 year ago

Here is and old proposal for a standard PMA description:

Property type enum Dynamic* comment Address Range(s) int lower, upper Boolean PA is unsigned, lower<=upper memory Type enum memtype Memory, IO, empty Boolean <-- could this be implied by idempotency/cacheable/permissons? Cacheable boolean cacheable Boolean <-- this needs more work and only applies for type memory? Coherent boolean coherent Boolean Idempotent boolean Rd, Wt Boolean <--must be true for type Memory Size & Alignment restriction uint8 sz_mask, misalign_mask Boolean

On Tue, Oct 17, 2023 at 4:57 AM Tim Hutt @.***> wrote:

Currently the thing close to PMAs in the model is this:

val phys_mem_segments : unit -> list((xlenbits, xlenbits)) function phys_mem_segments() = (plat_rom_base (), plat_rom_size ()) :: (plat_ram_base (), plat_ram_size ()) :: [||]

In other words you can have exactly 2 regions that exist. But the only attribute you can set is "exists". Additionally this implementation has a slight flaw in that you can't make all memory exist because the base and size are both uint64.

We need to change it so that you can have an unlimited (or at least not very limited) number of PMAs with these attributes (this is from the privileged spec):

  • Atomicity: AMONone, AMOSwap, AMOLogical, AMOArithmetic
  • Reservability: RsrvNone, RsrvNonEventual, RsrvEventual
  • Alignment: Which access widths/types/alignments are allowed (spec is very open)
  • Memory Ordering?
  • Coherence
  • Cacheability
  • Idempotency

This already affects some things in the model (e.g. FIOM) which currently assume that all memory is "normal" cacheable, idempotent, etc.

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/issues/320, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJURMLLHUNTX5MLYFGTX7ZXCFAVCNFSM6AAAAAA6DW3OYKVHI2DSMVQWIX3LMV43ASLTON2WKOZRHE2DOMRUGI4DKOI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Timmmm commented 1 year ago

Can you paste that again in a code block?

allenjbaum commented 1 year ago

I'm not quite sure how to do that from an excel spreadsheet to gmail How about I attach the spreadsheet

On Tue, Oct 17, 2023 at 7:59 AM Tim Hutt @.***> wrote:

Can you paste that again in a code block?

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/issues/320#issuecomment-1766597094, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJTH45BJLUY76DLQILLX72MLTAVCNFSM6AAAAAA6DW3OYKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRWGU4TOMBZGQ . You are receiving this because you commented.Message ID: @.***>

Timmmm commented 1 year ago

Yes please :-)

allenjbaum commented 1 year ago

That's weird; I thought I had PMA properties.xlsx

Timmmm commented 1 year ago

For those without Excel:

Property type enum Dynamic† comment
Address Range(s) int lower, upper Boolean PA is unsigned, lower<=upper
memory Type enum memtype Memory, IO, empty Boolean <-- could this be implied by idempotency/cacheable/permissons?
Cacheable boolean cacheable Boolean <-- this needs more work and only applies for type memory?
Coherent boolean coherent Boolean
Idempotent boolean Rd, Wt Boolean <--must be true for type Memory
Size & Alignment restriction uint8 sz_mask, misalign_mask Boolean ‡ <- bitN means access of 2^N bytes allowed/misaligned accesses allowed
Max Boundary uint8 boundary Boolean ‡ <- means misalign trap if addrN changes from LSB to MSB, 0 means never trap
Atomic support enum atom_supp AMONone, AMOSwap, AMPLogical, AMOArithmetic Boolean <- do we also need to define per/region misalign atomic support?
Reservability support enum rsrv_supp RsrvNone, RsrvNonEventual, RsrvEventual Boolean
Permissions - type boolean R, W, X, PTE_Rd, PTE_Wt Boolean
Permissions - mode boolean M, S, VS,U, VU Boolean <-- (not in spec) we do want crossproduct of type/mode, so 5*6 bits
Ordering Channel uint8 channel Boolean
IO Strong or Weak boolean IO Boolean <-- predefined if channel 0 or 1
Memory Ordering enum memorder RVWMO, RVTSO, Custom? Boolean

† is this needed? If it can be changed, there must be a CSR for it, and then we can declare it RW in its YAML description instead of being a boolean, then perhaps indicating the CSR numberand field instead?

‡ this tries to define when a misaligned access is not permitted, and which sized access are permitted. examples:

Note that even if permitted, traps may occur if 2nd TLB entry isn't present or cacheline isn't present

Timmmm commented 1 year ago

IMO the answers to the questions are:

do they need to be dynamic?

It seems like they technically can, but there's no need for a standard CSR for it so I don't think the model needs to consider this. Probably over-complication for the model at this point. The spec says:

PMAs are inherent properties of the underlying hardware and rarely change during system operation. [...] Some devices might be configurable at run time to support different uses that imply different PMAs—for example, an on-chip scratchpad RAM might be cached privately by one core in one end-application, or accessed as a shared non-cached memory in another end-application.

Where the attributes are run-time configurable, platform-specific memory-mapped control registers can be provided to specify these attributes at a granularity appropriate to each region on the platform


could this be implied by idempotency/cacheable/permissons?

Yes. It seems "main memory" implies attributes but it isn't an attribute itself:

The most important characterization of a given memory address range is whether it holds regular main memory, or I/O devices, or is vacant. Regular main memory is required to have a number of properties, specified below, whereas I/O devices can have a much broader range of attributes. Memory regions that do not fit into regular main memory, for example, device scratchpad RAMs, are categorized as I/O regions. Vacant regions are also classified as I/O regions but with attributes specifying that no accesses are supported.


I think it's probably worth starting with the attributes that are very simple and clearly defined, that is:

Property Values
Cachable true, false
Coherent true, false
Idempotent Reads true, false
Idempotent Writes true, false
Reservability RsrvNone, RsrvNonEventual, RsrvEventual
Atomicity AMONone, AMOSwap, AMOLogical, AMOArithmetic

Other attributes can be added later.

Alasdair commented 1 year ago

Having memory ordering being a memory attribute is interesting. How are programs that interleave TSO and non-TSO operations between these regions defined in such a world? I think for the Apple chips that support TSO in addition to the regular Arm memory model it's a global flag like a CSR that affects all memory reads.

martinberger commented 1 year ago

The PMA spec leaves open the question whether PMA ranges can overlap or not. If overlapping is allowed, a host of issues arise, including:

All those problems go away if PMAs do not overlap. Is the Priv spec deliberately vague about this?

@allenjbaum @billmcspadden-riscv @Timmmm

Timmmm commented 1 year ago

I don't think the PMA spec needs to concern itself with ranges at all. It's simply that each address is associate with a single set of PMAs. If you want to achieve that mapping with a range based system then it's up to you how to resolve overlaps. As long as you end up with a function that maps from address to PMAs you can do anything you want.

The real question is what happens when an access spans addresses with different PMAs. It should probably specify that.

allenjbaum commented 1 year ago

That is loosely defined, because I believe that can only occur for unaligned accesses, but is not different than what happens for PMP or PT exceptions. (assume PMA regions can't be smaller than XLEN, and ignore vector load/store for the moment).

In that case there are several allowed results: For a load: if it passes PMA, PMP and PT permissions, it could still trap with a misaligned access fault Otherwise, it will trap with the reported exception (address & cause) depending on a hierarchy of cause priorities, some of which are implementation specific (e.g it could be both misaligned and have an access violation; which is reported is implementation choice. If both halves have access violations, and misaligned is lower priority than access violation, you report the highest priority access violation ( I think) If both violations have the same priority, it is an implementation choice as to which gets reported (I think)

Store is more interestinig; it has all the behaviors above, plus it may perform the store for either half which doesn't report a violation. Or not. Implementation choice.

On Fri, Oct 20, 2023 at 3:02 AM Tim Hutt @.***> wrote:

I don't think the PMA spec needs to concern itself with ranges at all. It's simply that each address is associate with a single set of PMAs. If you want to achieve that mapping with a range based system then it's up to you how to resolve overlaps. As long as you end up with a function that maps from address to PMAs you can do anything you want.

The real question is what happens when an access spans addresses with different PMAs. It should probably specify that.

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/issues/320#issuecomment-1772440474, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJRFCX7NYMDEPR4HZWDYAJD33AVCNFSM6AAAAAA6DW3OYKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZSGQ2DANBXGQ . You are receiving this because you were mentioned.Message ID: @.***>