Clarify CBO for unaligned rs1 #1433
Conversation
Rework the text to be much more explicit about the behaviour when rs1 is not aligned to the cache block size. I also moved & slightly reworded the note about the assembly syntax since it's not relevant to the instruction semantics.
| relevant *tval CSR is written with the faulting effective address (i.e. the same | ||
| faulting address value as for other causes of these exceptions). | ||
| relevant *tval CSR is written with the faulting effective address (i.e. the | ||
| value of _rs1_). |
There was a problem hiding this comment.
This is the part that I endorse. My initial reaction is the other changes are not necessary, and maybe not even desirable, but I'll leave that decision to @dkruckemyer-ventana.
There was a problem hiding this comment.
I think at least the bits about not requiring rs1 to be aligned are needed because that isn't specified anywhere currently (unless I missed it).
There was a problem hiding this comment.
I did not think there was an intent to support a CMO for flush/invalidate/clean to operate on multiple cache blocks. Its do not think that was the intent when saying access size is a cache block - I understand the intent was to state that the CMO affects all of the cache block. From that perspective it is more reasonable to think of a CMO.flush/invalidate/clean targets the cache block addressed by rs1 and that cache block may be addressed by specifying any byte of that cache block - but a given CMO operates on the one cache block mapped to the address in rs1
There was a problem hiding this comment.
I agree, that's what I meant it to say. Does it sound like it's saying it would operate on multiple cache blocks?
| that contains the address specified in _rs1_. It is not required that _rs1_ is | ||
| aligned to the size of a cache block. On faults the address stored in the *tval | ||
| CSR is _rs1_; not the base address of the cache block. The instruction operates | ||
| on the set of coherent caches accessed by the agent executing the instruction. |
There was a problem hiding this comment.
A CMO to flush/invalidate/clean should take effect even if the cache block is cached in a coherent cache that is not directly accessible to the agent executing the corresponding CMO. For instance, the cache block may be cached in the private cache of a hart should be flushed/invalidated/cleaned if a CMO addressing that cache block was executed by another hart.
There was a problem hiding this comment.
It does say this elsewhere:
Additionally, if a coherent agent in the set executes a CBO instruction that specifies the cache block, the resulting operation shall apply to any and all of the copies in the caches that can be accessed by the load and store operations from the coherent agents.
So I think it's reasonably clear, but if you want to clarify it more here I think it can go in a separate PR.
There was a problem hiding this comment.
"that can be accessed by the load and store operations" - it could be interpreted that CMO will not operate on private caches of other harts because they cannot be "accessed" by load/store operations performed on the hart that executes the CMO. The other interpretation would be that "accessed" includes forwarding of data from private caches of remote harts to the private cache of the hart executing the load/store. I would like to hear @dkruckemyer-ventana interpretation.
There was a problem hiding this comment.
Yeah I think that's just an oversight here. The other bit I quoted is much more explicit and clearly agrees with what you said.
There was a problem hiding this comment.
Please rewrite "On faults the address stored in the *tval CSR is rs1; not the base address of the cache block." as "On faults, the faulting virtual address is considered to be the value in rs1, rather than the base address of the cache block."
This improves separation of concerns, because faulting virtual addresses might end up in places other than *tval.
|
@ved-rivos @dkruckemyer-ventana any objection to merging this? I think the points Ved raised above are pre-existing issues (unless I've misunderstood) and therefore can be addressed in another PR. |
| operation. | ||
| that contains the address specified in _rs1_. It is not required that _rs1_ is | ||
| aligned to the size of a cache block. On faults the address stored in the *tval | ||
| CSR is _rs1_; not the base address of the cache block. The instruction operates |
There was a problem hiding this comment.
| corresponding to the cache block that contains the address specified in _rs1_. | ||
| It is not required that _rs1_ is aligned to the size of a cache block. On | ||
| faults the address stored in the *tval CSR is _rs1_; not the base address of | ||
| the cache block. An implementation may or may not update the entire set of |
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm OK with it after the minor suggestions I made, but I still want @dkruckemyer-ventana to sign off.
Rework the text to be much more explicit about the behaviour when rs1 is not aligned to the cache block size.
I also moved & slightly reworded the note about the assembly syntax since it's not relevant to the instruction semantics.
Fixes #1263