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

MSHWM/MSHWMB alignment #14

Closed
mndstrmr opened this issue Sep 27, 2023 · 4 comments
Closed

MSHWM/MSHWMB alignment #14

mndstrmr opened this issue Sep 27, 2023 · 4 comments

Comments

@mndstrmr
Copy link
Contributor

In the Sail MSHWM and MSHWMB both receive unaligned values, both in CSR writes and when bumping MSHWM. In CherIoT-ibex both are 16 byte aligned in both cases.

Split misaligned loads lead to another issue, where if the first request would have been below MSHWMB and the second request was above MSHWMB (and below MSHWM) then MSHWM will be updated to the original, misaligned address + 4. The Sail only checks the original address and does not consider the access width.

My suggestion would be the following:

  • Add the 16 byte alignment enforcement to the Sail
  • If the access address is below MSHWMB check if address + width would cross into the stack area (this check is likely made easier by the 16 byte alignment since provided MSHWMB != MSHWM, address < MSHWMB < address + width ==> overlap). If it does, set MSHWM = MSHWMB to mark the entire stack as accessed. This change would need to be reflected in the System Verilog too.

I appreciate that all of this is very unlikely to ever be seen during real execution, but it is a disparity between spec and implementation, hence our checks do catch it.

@rmn30
Copy link

rmn30 commented Oct 4, 2023

I think I was originally reluctant to embed the 16-byte stack alignment in the ISA, but it is probably a good idea so this is a case of the implementation getting a little ahead of the spec.

The unaligned access case is more interesting. The way the RTOS currently works it will never cause a problem because we will never hand out a capability that permits an unaligned access across the base of the stack. Is Ibex even configured to support unaligned stores? If the extra check is not too onerous for hardware we should add it but I'm not sure what the impact of the extra add + compare might be.

@kliuMsft
Copy link
Contributor

kliuMsft commented Oct 5, 2023

ibex does support unaligned stores. The way MSHWM is implemented, we actually compare addresses of both words if it is an unaligned access. However since MSHWM grows downwards, only the 1st comparison (lower word) will actually update MSHWM (to the lower 16-byte boundary).

The CSP bound checking is independent of this though (which is completely based on byte address). To me if we have MSHWMB == CSP.base32, and both 16-byte aligned, I don't really see a issue - or maybe I am still missing something?

@rmn30
Copy link

rmn30 commented Apr 4, 2024

As per CHERIoT-Platform/cheriot-sail#49 I have updated the Sail to use 16-byte alignment and documented the unaligned write issue. I elected not to require the hardware to handle this case as it should never arise in practice. Can we close this now?

@mndstrmr
Copy link
Contributor Author

mndstrmr commented Apr 9, 2024

Apologies for the wait, the store instruction non error case proves correct now, thanks.

@mndstrmr mndstrmr closed this as completed Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants