Rewrite memory/IO/vacant description #1301
Conversation
Based on old PR - #665 Signed-off-by: Kersten Richter <kersten@riscv.org>
Signed-off-by: Kersten Richter <kersten@riscv.org>
|
@jscheid-ventana care to comment? |
|
@jscheid-ventana Got time to take a look? This is based on a PR that you submitted back in 2021. I put the change into asciidoc so it could be merged. |
| The most important characterization of a given memory address range is | ||
| whether it holds regular main memory, or I/O devices, or is vacant. | ||
| The most important characterization of a specific memory address range is | ||
| whether it holds regular main memory or I/O devices, or if it is vacant. |
There was a problem hiding this comment.
Are Oxford commas outside the consistent style for this document?
There was a problem hiding this comment.
it is really 2 ideas though - it can hold regular main memory or I/O devices. Or it can be vacant. The , or is just to separate the 2 ideas.
There was a problem hiding this comment.
I want to be sure of the formal model.
- {Main memory, I/O, Vacant} all addresses have one and only one of these three attributes.
- {Main memory, I/O} as one exclusive set attribute and Vacant as a separate boolean attribute. If so, are there any constraints between them? The prose talks about I/O + Vacant, but is Main memory + Vacant also allowed?
There was a problem hiding this comment.
The distinction may or may not matter in observable behavior from the hart, but can matter for any future formalizing of the attributes, for, say, a system architecture specification to describe or control PMAs.
There was a problem hiding this comment.
It seems like what gets us into trouble is that the current PMA text views there being only two type of regions (main memory and I/O), and views a 'vacant' region as being a special type of I/O region.
Either we stick with that (which goes against some of the proposed text changes in this PR), or we switch the text to a view that there are three types of regions - in which case a 'vacant' region is no longer a special type of I/O region.
The latter seems much cleaner, but I don't know if there is some relevant history behind why a vacant region needs to be described as a type of I/O region. @aswaterman What say you?
There was a problem hiding this comment.
The relevant original text is a little out of date with other changes to spec. I think it will actually be cleaner to describe vacant regions as a type of IO region, and that there are really only two types of memory range (main memory and IO), so regions are either main memory or I/O, and only I/O can have vacant regions. I think this hierarchy makes more sense from a platform perspective, where "vacancy" is really an absence of PMAs that can only exist in non-main-memory regions, with accesses to vacant regions returning access faults or bus-error interrupts. Also, while the text recommends PMA checks be done synchronously where possible, it is acknowledged in real platforms that some checks can only happen later in an instruction's life cycle but non-synchronous exceptions should only be for IO regions not main memory. One other aspect is that FENCE operations are only defined to distinguish memory and IO, and while vacant memory accesses do not cause an access, we might want to be able to order the effect of a bus-error interrupt wrt other accesses, so should use IO ordering for accesses to vacant regions (e.g., when probing for existence of devices).
There was a problem hiding this comment.
I support the notion of vacant regions being permissionless I/O regions. We already need to tolerate I/O regions having unusual characteristics, including re: permissions. So we can encode the vacancy property without needing any new mechanisms or characterizations.
src/machine.adoc
Outdated
| I/O; for example, regions that contain device control registers. Finally, there are | ||
| other address regions that are neither memory nor I/O. These regions are called vacant | ||
| regions. Vacant regions include attributes that specify access to these regions isn't | ||
| supported. Vacant regions can also be classified as I/O regions, but are never accessible. Any |
There was a problem hiding this comment.
Vacant regions include attributes that specify access to these regions isn't
supported. Vacant regions can also be classified as I/O regions, but are never accessible.
This is a slight change from the previous, but is it really clarifying? I had thought that {Main memory, I/O, Vacant} was a non-overlapping set.
src/machine.adoc
Outdated
| other address regions that are neither memory nor I/O. These regions are called vacant | ||
| regions. Vacant regions include attributes that specify access to these regions isn't | ||
| supported. Vacant regions can also be classified as I/O regions, but are never accessible. Any | ||
| attempt to access a vacant region causes an exception. |
There was a problem hiding this comment.
Is it right to say "access-fault exception" here?
There was a problem hiding this comment.
if the exception is an access-fault exception, then we can be specific!
There was a problem hiding this comment.
Yes, it is correct to specify that a violation results in an access-fault exception - especially since this is explicitly stated early on in the PMA section (but is worth repeating here).
Sorry. This had been lost in my data firehose. |
|
The document prefers Oxford commas. |
Signed-off-by: Kersten Richter <kersten@riscv.org>
Signed-off-by: Kersten Richter <kersten@riscv.org>
Signed-off-by: Kersten Richter <kersten@riscv.org>
Signed-off-by: Kersten Richter <kersten@riscv.org>
Updated description based on info from comments Signed-off-by: Kersten Richter <kersten@riscv.org>
|
@jscheid-ventana back to you. Is it more clear now based on all the comments? |
|
FYI, the AR Committee members (in this week's meeting) discussed this matter further (taking into account what is stated in both the Priv and Unpriv specs, the various considerations that have been raised, and what is the most appropriate way forward in clarifying the spec). One of the members will be posting and creating a PR to cleanly implement the resolution. |
Based on old PR - #665