sail-riscv
sail-riscv copied to clipboard
Fix htif memory range check
Previously an access that just touched the bottom of HTIF would have incorrectly been counted as within HTIF. E.g. if HTIF is at 0x8 and you do a 4-byte access to 0x4.
I don't think the old condition was wrong was it? These are just two different options for what to do when there's a misaligned access that overlaps the edge of the HTIF device. The current option will say it is in HTIF memory and then give you an access fault. This version will say it is outside HTIF and then do a normal write to invisible memory "behind" HTIF.
I think the current behaviour is the better option.
For example
addr = 8002004, width = 4 base = 8002008, htif_tohost_size = 8
This is legal for the current behavior
(addr <_u base + htif_tohost_size) & (addr + width >=_u base)
8002004 < 8002008 + 8 & 8002004 + 4 = 8002008
But addr should >= base, isn't it?
Oh, I got your idea, this will be checked inside htif_load/store
I found this during hypervisor testing. There is a test case doing a lot of sw operations to set the entire BSS region to 0. This process triggered HTIF. I'm not entirely sure what happened. I need to check it again.
@Timmmm I think we should change it to
- (addr <_u base + htif_tohost_size) & (addr + width >=_u base)
+ (addr <_u base + htif_tohost_size) & (addr + width >_u base)
^ remove = here
|htif|
| | this is not within htif
found it
Test Results
2 115 tests ±0 2 115 ✅ ±0 20m 43s ⏱️ +43s 1 suites ±0 0 💤 ±0 1 files ±0 0 ❌ ±0
Results for commit 7b22b6fd. ± Comparison against base commit 28988dbe.
Ah yes that looks correct.
I don't think the old condition was wrong was it? These are just two different options for what to do when there's a misaligned access that overlaps the edge of the HTIF device. The current option will say it is in HTIF memory and then give you an access fault. This version will say it is outside HTIF and then do a normal write to invisible memory "behind" HTIF.
Shouldn't this behavior fall under an access that overlaps two PMAs? I might have missed it, but I could not find information in the spec on how that should be handled. The two possibilities you mention indicate the need for a config option. We could discuss this at the meeting.
I think normally you would have one PMA for your entire MMIO region that wouldn't support misaligned accesses at all.
I added #1389 to track making this all make sense, but this fix is at least correct - are you happy to merge it and defer the rest of the work @pmundkur ?