Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception policy for misaligned LC/SC is odd #59

Closed
sorear opened this issue Jan 26, 2024 · 3 comments · Fixed by #66
Closed

Exception policy for misaligned LC/SC is odd #59

sorear opened this issue Jan 26, 2024 · 3 comments · Fixed by #66

Comments

@sorear
Copy link
Contributor

sorear commented Jan 26, 2024

The base ISA defines "load misaligned" and "store misaligned" as exception causes distinct from access faults to allow them to be quickly dispatched to an emulation handler, and has language (in privileged §3.6.3.3 Alignment) saying that in at least one case misaligned exceptions should only be generated if emulation is intended, and access-faults should be generated otherwise.

LC/SC (and compressed and capability-mode versions thereof) are defined to always check alignment and throw exceptions. Do we intend for those instructions to be emulated, and appear to succeed in U-mode?

If yes, the spec should be loosened so that implementations supporting misaligned access for integer types can execute misaligned LC/SC without an exception. (Naturally, any such operation must not create a tagged capability from misaligned data or allow a tagged capability to be partially overwritten without untagging.)

If no, the exception should be changed to an access-fault.

Do we expect compilers to use LC and SC for inline small struct copies of unknown alignment?

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 26, 2024

They are not emulatable. But having misalignment show up as an access fault really sucks for an OS, because it can't know to report BUS_ADRALN for the SIGBUS. Versus a misaligned access handler, where you already need to decode the instruction to know what you need to emulate, at which point it's really easy to see that it's something you shouldn't be emulating. So I dislike that the privileged spec says that for atomics, and don't believe we should perpetuate that misfeature.

@tariqkurd-repo tariqkurd-repo added the question Further information is requested label Jan 26, 2024
@andresag01
Copy link
Collaborator

@sorear : Thanks for the comment, it is a very good observation!

The specific section in the RISC-V privileged spec you are referring to says this:

Implementations may raise access-fault exceptions instead of address-misaligned exceptions for some
misaligned accesses, indicating the instruction should not be emulated by a trap handler. If, for a
given address and access width, all misaligned LRs/SCs and AMOs generate access-fault exceptions,
then regular misaligned loads and stores using the same address and access width are not required to
execute atomically.

I agree with @jrtc27 that this is not ideal in the privileged spec. Thankfully, the RISC-V spec is also using the word "may" which I read as a "suggestion" rather than a hard requirement. IMHO, if we have a dedicated exception code for misaligned, we should use it regardless of whether the operation is emulatable or not.

I suggest closing this ticket as "wont fix" unless there is another very strong reason to change LC/SC to access-fault when performing misaligned accesses.

@sorear
Copy link
Contributor Author

sorear commented Jan 26, 2024

The ticket needs to be fixed with improved documentation. I'll make a PR "soon".

sorear added a commit to sorear/riscv-cheri that referenced this issue Jan 26, 2024
Make it clear in the spec that this overrides Zicclsm and should not
be emulated.

Resolves riscv#59.
@tariqkurd-repo tariqkurd-repo added RISC-V integration changes and removed question Further information is requested labels Jan 31, 2024
sorear added a commit to sorear/riscv-cheri that referenced this issue Jan 31, 2024
Make it clear in the spec that this overrides Zicclsm and should not
be emulated.

Resolves riscv#59.
sorear added a commit to sorear/riscv-cheri that referenced this issue Jan 31, 2024
Make it clear in the spec that this overrides Zicclsm and should not
be emulated.

Resolves riscv#59.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants