Clarify CJALR operation order #10
Conversation
src/insns/cjalr_jalr_32bit.adoc
Outdated
| length violation check is a stricter condition. The sealing check can also be | ||
| skipped because the capability will always have been unsealed. |
There was a problem hiding this comment.
I'm not sure about the sealing part? We still want to raise an error if cs1 is a sentry and the offset is non-zero?
There was a problem hiding this comment.
Agreed, we would like to keep the sealing check
There was a problem hiding this comment.
I meant the sealing check in the invalid address conversion procedure. I'll reword it to make that clearer.
There was a problem hiding this comment.
I reworded it. Hopefully clearer.
Timmmm
force-pushed
the
user/timh/clarify_cjalr
branch
from
ccc54a1 to
1ae67d2
Compare
src/insns/cjalr_jalr_32bit.adoc
Outdated
| When performing invalid address conversion the tag never needs to be cleared because | ||
| the earlier length violation check is a stricter condition than the representability | ||
| check, and the capability is always unsealed before invalid address conversion. | ||
| ==== |
There was a problem hiding this comment.
I don't fully agree with this note for a couple of reasons:
- The tag is never cleared because the instruction excepts in all cases instead of changing PCC when something goes wrong
- The capability is not always unsealed. As @arichardson pointed out, there is an exception when the immediate is not 0 because you may be jumping to an instruction that is not the entry point.
I would prefer to remove this note altogether.
There was a problem hiding this comment.
- Yes.. that's what I said isn't it?
- I said "the capability is always unsealed before invalid address conversion" which I think is true, no?
I think there's enough confusion about this that a note is warranted. Maybe you could come up with better wording?
There was a problem hiding this comment.
@Timmmm: How about something like this?
If
cs1is sealed, then it is only unsealed when the jump target address equalscs1's address, i.e.immmust be 0, otherwise a CHERI exception is raised. Also, CJALR checks that the target address is withincs1's bounds, so a representability check is not needed when performing invalid address conversion.
There was a problem hiding this comment.
I'll just remove this so we can get the important bit landed.
src/insns/cjalr_jalr_32bit.adoc
Outdated
| ==== | ||
|
|
||
| The capability is then installed in <<pcc>>. The <<pcc>> of the next instruction | ||
| following the jump (<<pcc>> + 4) is sealed and written to `cd`. |
There was a problem hiding this comment.
Please note that that this test is wrapped under the Synopsis:: header, but it is not currently indented properly because it is after the note and it is also missing the + at the end of the previous paragraph.
src/insns/cjalr_jalr_32bit.adoc
Outdated
| of the next instruction following the jump (<<pcc>> + 4) is sealed and written | ||
| to `cd`. | ||
| performed. The check for length violation is done next using the converted | ||
| address and the original bounds. |
There was a problem hiding this comment.
I think this text is better placed as a note in the Exceptions:: section where the actual failure cases are highlighted.
Update readme.adoc
Clarify that length violations are checked on the address after it has undergone invalid address conversion. Also fix a typo in the title. Fixes riscv#9
Timmmm
force-pushed
the
user/timh/clarify_cjalr
branch
from
1ae67d2 to
9c862a6
Compare
Clarify that length violations are checked on the address after it has undergone invalid address conversion.
Also fix a typo in the title.
Fixes #9