microsoft / cheriot-ibex

cheriot-ibex is a RTL implementation of CHERIoT ISA based on LowRISC's Ibex core.
Apache License 2.0
73 stars 14 forks source link

MSHWM/MSHWMB alignment #14

Closed mndstrmr closed 5 months ago

mndstrmr commented 11 months ago

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:

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 commented 11 months ago

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 commented 11 months ago

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 commented 5 months ago

As per https://github.com/microsoft/cheriot-sail/pull/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 commented 5 months ago

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