search

riscv / sail-riscv

Sail RISC-V model
https://lists.riscv.org/g/tech-golden-model
Other
407 stars 148 forks source link

Don't read or write 8 bytes for 4-byte PTEs #461

Closed Timmmm closed 1 month ago

Timmmm commented 2 months ago

For Sv32 Page Table Entries are only 4 bytes, but the old code was unconditionally reading and writing 8 bytes.

Fixes #459

github-actions[bot] commented 2 months ago

Test Results

712 tests  ±0   712 :white_check_mark: ±0   0s :stopwatch: ±0s   6 suites ±0     0 :zzz: ±0    1 files   ±0     0 :x: ±0 

Results for commit 4d607432. ± Comparison against base commit e1242d85.

:recycle: This comment has been updated with latest results.

Timmmm commented 1 month ago

Actually it's worse than I thought; it also writes 8 bytes for Sv32 when updating the accessed/dirty bits. This pretty much means Sv32 is broken at the moment.

jrtc27 commented 1 month ago

How was the original PR tested if we’re finding this now?

Alasdair commented 1 month ago

Are you planning on making a separate PR for the writes, or adjust this one? Otherwise LGTM, and we should get this fix in quickly.

Alasdair commented 1 month ago

How was the original PR tested if we’re finding this now?

We ran it on the tests in this repo, which I thought does include RV32 with virtual memory enabled. We also had a member of the golden model meeting run it on an extra set of virtual memory tests their company had, and we re-ran the basic 'boot FreeBSD' smoke test. It might be the case that there are no tests that actually do anything with the PTE dirty bits though.

jrtc27 commented 1 month ago

How was the original PR tested if we’re finding this now?

We ran it on the tests in this repo, which I thought does include RV32 with virtual memory enabled. We also had a member of the golden model meeting run it on an extra set of virtual memory tests their company had, and we re-ran the basic 'boot FreeBSD' smoke test. It might be the case that there are no tests that actually do anything with the PTE dirty bits though.

--enable-dirty-update is off by default

Timmmm commented 1 month ago

Are you planning on making a separate PR for the writes, or adjust this one?

I'll update this one.

Timmmm commented 1 month ago

Ok I updated it to include the writes too. I am also working on refactoring the virtual memory code to use Sail's type system properly in all of this code, rather than resorting to bit(64).