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.

Sign up for GitHub

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

Jump to bottom

Handle address translation for misaligned loads and stores better #467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle address translation for misaligned loads and stores better #467

wants to merge 1 commit into from

Conversation

Alasdair
Copy link
Collaborator

Refactor the LOAD and STORE instruction so they split misaligned accesses into multiple sub-accesses and perform address translation separately. This means we should handle the case where a misaligned access straddles a page boundary in a sensible way, even if we don't yet cover the full range of possibilities allowed for any possible RISC-V implementation.

In addition tidy up the implementation in a few ways:

  • Very long lines on the LOAD encdec clause were fixed by adding a helper function

  • Add some line breaks in the code so it reads less claustrophobic

  • Ensure we use the same names for arguments in encdec/execute/assembly. Previously we used 'size' and 'width'. I opted for 'width' consistently

I tested this using the test in #49. Additionally I ran the tests in this repository on a tweaked version of model that would split apart even correctly aligned loads and stores using the misaligned logic to test that it was at least doing something sensible.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 14, 2024 •
edited
Loading

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 91a16b4. ± Comparison against base commit 1559013.

♻️ This comment has been updated with latest results.

@Timmmm
Copy link
Collaborator

Timmmm commented May 15, 2024

I'm not sure this is the right approach, for a few reasons.

  1. We actually only need to split on page boundaries in order to fix the address translation issue.
  2. It's very unlikely that a chip will actually split accesses into memory operations like this. Eventually we want it to be configurable but until then I think it should at least be a likely implementation.
  3. There's no way you are going to want to copy & paste this throughout the float and vector load/stores!
  4. I think having all these details in the execute function itself is probably not ideal from an including-code-in-the-spec point of view.

Did you see this commit? I think it's a lot nicer to read something that abstracts away the virtual address translation a bit:

function clause execute(LOAD(imm, rs1, rd, is_unsigned, width, aq, rl)) = {
  let offset : xlenbits = sign_extend(imm);
  let width_bytes = size_bytes(width);

  // This is checked during decoding.
  assert(width_bytes <= sizeof(xlen_bytes));

  /* Get the address, X(rs1) + offset.
     Some extensions perform additional checks on address validity. */
  match ext_data_get_addr(rs1, offset, Read(Data), width_bytes) {
    Ext_DataAddr_Error(e)  => { ext_handle_data_check_error(e); RETIRE_FAIL },
    Ext_DataAddr_OK(vaddr) => {
      if   check_misaligned(vaddr, width)
      then { handle_mem_exception(vaddr, E_Load_Addr_Align()); RETIRE_FAIL }
      else match vmem_read(Read(Data), vaddr, width_bytes, aq, rl, false) {
        match value {
          Ok(result)    => { X(rd) = extend_value(is_unsigned, result); RETIRE_SUCCESS },
          Err(vaddr, e) => { handle_mem_exception(vaddr, e); RETIRE_FAIL }
        }
    }
  }
}

I couldn't quite do it as cleanly for stores because of the mem_write_ea() thing unfortunately. If we didn't have to worry about that then it could be as simple as that LOAD, just with vmem_write(..., X(rs1), ...).

@Alasdair
Copy link
Collaborator Author

We actually only need to split on page boundaries in order to fix the address translation issue.

I think the question is: should be the semantics of this be the same as splitting misaligned accesses into separate operations? If the observable semantics is the same then the model could choose to do things in a less efficient way.

I considered trying to only split only on page boundaries, but then I thought that there are probably a bunch of other cases where misaligned access straddle other things like PMP regions, and just splitting into separate operations might be cleaner.

@Alasdair
Copy link
Collaborator Author

On the other points regarding abstracting this detail behind a helper function, I agree. I'll take a closer look at that commit.

We've thought a bit more in general about misaligned and virtual memory for ARM, and I think the semantics there is that you split into byte sized accesses (unless you have some feature flags etc... it always gets more complicated).

@Timmmm
Copy link
Collaborator

Timmmm commented May 15, 2024

If the observable semantics is the same then the model could choose to do things in a less efficient way.

That's true.

I think the semantics there is that you split into byte sized accesses.

That seems sensible! I think the PMP requirement that all bytes of a memory access be in one PMP region screws this up for RISC-V.

@Alasdair
Copy link
Collaborator Author

The interesting case would be: Misaligned access that straddles a page boundary, where each page is in a different PMP region. What is the envelope of allowed behaviour?

@Timmmm
Copy link
Collaborator

Timmmm commented May 15, 2024

I made a diagram for these cases. In that case you can either split into separate memory operations in which case everything will be fine (first example in the image), or you are technically allowed to keep it as one discontinuous (!) memory operation in which case it will fail (second example).

I can't imagine any real systems that would do the latter but Andrew Waterman said it is allowed. The thing that makes it a single memory operation is that it cannot be observed to be partially complete.

image

Also if you're not being pedantic a device could just have the second case fail too, because nothing can observe that you actually did one memory operation if you claim you did two. I don't think the difference is observable.

Kind of feels like the PMP spec is just a bit broken tbh. I wonder if they even thought about this stuff.

@Alasdair
Copy link
Collaborator Author

Thanks, that's very useful. I think as a first go it would be reasonable to support the 0, 2, and 3 cases and not support 1 for the time being.

@PeterSewell
Copy link
Collaborator

PeterSewell commented May 15, 2024 via email

@Alasdair Alasdair force-pushed the ldst_misaligned branch 2 times, most recently from 7db08ac to 1f0312e Compare May 15, 2024 15:34
@Alasdair
Copy link
Collaborator Author

Ok, I refactored the pull request so there are separate vmem_read and vmem_write_from_register functions. I also added options to the C simulator that change the order in which address translations occur for misaligned accesses and control whether misaligned accesses are split into the largest possible aligned size, or always into bytes.

@allenjbaum
Copy link
Collaborator

allenjbaum commented May 15, 2024 via email

@jrtc27
Copy link
Collaborator

jrtc27 commented May 15, 2024

The privileged spec is very clear that alignment faults take precedence over access faults.

@Alasdair
Copy link
Collaborator Author

  • do you support misaligned at all (and at which granularity - didn't
    include that in the truth table
  • do you split up the accesses, and if so, do you first access the lower
    address(es) or the higher address(es).

Ok good to know, those are essentially the two command line flags I have implemented.

@allenjbaum
Copy link
Collaborator

allenjbaum commented May 15, 2024 via email

@jrtc27
Copy link
Collaborator

jrtc27 commented May 15, 2024

Not exactly: The RISC-V Instruction Set Manual: Volume II: Privileged Architecture Table 15. Synchronous exception priority in decreasing priority order. Priority Exc.Code Description Highest 3 Instruction address breakpoint 12, 1 During instruction address translation: First encountered page fault or access fault 1 With physical address for instruction: Instruction access fault 2 0 8,9,11 3 3 Illegal instruction Instruction address misaligned Environment call Environment break Load/store/AMO address breakpoint 4,6 Optionally: Load/store/AMO address misaligned 13, 15, 5, 7 During address translation for an explicit memory access: First encountered page fault or access fault 5,7 With physical address for an explicit memory access: Load/store/AMO access fault Lowest 4,6 If not higher priority: Load/store/AMO address misaligned
…
On Wed, May 15, 2024 at 12:34 PM Jessica Clarke @.> wrote: The privileged spec is very clear that alignment faults take precedence over access faults. — Reply to this email directly, view it on GitHub <#467 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJWOVH7E5IGBK3ULTY3ZCO2D3AVCNFSM6AAAAABHW7D3ESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJTGMYTQMZWGA . You are receiving this because you commented.Message ID: @.>

That was exactly the table I was looking at, but I missed the final row and interpreted the higher-priority "Optionally" as meaning "if you don't support misaligned accesses"... unhelpful wording.

@allenjbaum
Copy link
Collaborator

allenjbaum commented May 15, 2024 via email

@billmcspadden-riscv billmcspadden-riscv added the tgmm-agenda Tagged for the next Golden Model meeting agenda. label May 23, 2024
@Alasdair Alasdair force-pushed the ldst_misaligned branch 3 times, most recently from eb67458 to 4bdc440 Compare July 29, 2024 15:13
@Alasdair
Copy link
Collaborator Author

Should be rebased onto the latest master now.

@Alasdair
Copy link
Collaborator Author

Alasdair commented Aug 5, 2024

If we want I could update this PR to handle the Zama16b extension/option we discussed today as the splitting function I add here would be the ideal place to add this option.

@billmcspadden-riscv
Copy link
Collaborator

billmcspadden-riscv commented Aug 5, 2024 via email

Timmmm
Timmmm reviewed Oct 8, 2024
View reviewed changes
Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I finally got around to reading this (sorry!). Some thoughts:

  1. It also needs to handle float loads/stores and a gazillion vector loads/stores. I don't know how you do that nicely without first class functions though. vmem_write_from_float_reg, vmem_write_from_vector_reg, etc...

  2. This means the misaligned accesses will always be split into aligned accesses, but that isn't necessary for the Sail emulator (since the underlying memory natively supports misaligned accesses) and it happens to not match our hardware (which also natively supports misaligned accesses). The only situations where we do need to split is at page boundaries.

This doesn't matter too much but if you support fine grained (i.e. low G) PMPs and PMAs then it might. Also it makes tracing more difficult because it's physical accesses that are logged and now we have way more.

I think we should add a flag to only split at some grain size, which would be a maximum of the page size (where you must split, unless we make it very complicated and check pages are contiguously mapped). I can imagine implementations where you might want to set it to the cache line size too.

Does that make sense? I feel like we might need a table where the rows are example addresses, the columns are the options and the cells are the expected accesses.

c_emulator/riscv_sim.c
break;
case OPT_MISALIGNED_TO_BYTE:
fprintf(stderr,
"misaligned accesses will be split into individual "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for this option? I know Spike does it but I assumed that was just for implementation simplicity. Does real hardware do this? I would maybe say YAGNI for now unless there's some use case I've missed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's how Arm models it in ASL afaik (although they have a bunch of toggles and other options too). That doesn't necessarily constrain what implementations do, it just means the observable behaviour when you split into larger accesses shouldn't be different.

The most relaxed permissible weak-memory behaviour is also given by splitting into individual bytes.

model/riscv_vmem_utils.sail
repeat {
let offset = i;
let vaddr = vaddr + (offset * bytes);
match translateAddr(vaddr, Write(Data)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be tricky but our hardware (and I presume most) does all the address translation first and then the memory access. Could we support at least that ordering as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it would be fine to do all the address translations first then the accesses.

model/riscv_vmem_utils.sail
} else {
i = offset + step
}
} until finished;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh TIL Sail has do..while! And also while .. do .. apparently. Might be worth mentioning those in the manual I guess?

model/riscv_vmem_utils.sail Outdated
_: "sys_misaligned_to_byte"
} : unit -> bool

val split_misaligned : forall 'width, 'width in {1, 2, 4, 8}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there's also cbo.zero which does 32 byte writes (by default). I think you can do this without a match anyway though.

val count_trailing_zeros : forall 'n, 'n >= 0. (bits('n)) -> range(0, 'n)
function count_trailing_zeros(x) = {
    foreach (i from 0 to ('n - 1)) {
        if x[i] == bitone then return i;
    };
    'n
}

val split_misaligned : forall 'width, 'width in {1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096} .
  (xlenbits, int('width)) -> {'n 'bytes, 'width == 'n * 'bytes & 'bytes > 0. (int('n), int('bytes))}
function split_misaligned(vaddr, width) = {
    if is_aligned_addr(vaddr, width) then (1, width)
    else if sys_misaligned_to_byte() then (width, 1)
    else {
        let vaddr_alignment_bytes = 2 ^ count_trailing_zeros(vaddr);
        assert(vaddr_alignment_bytes <= width);
        (vaddr_alignment_bytes, width / vaddr_alignment_bytes)
    }
}

I couldn't figure out how to get "must be a power of two" into the type system though. :-(

Copy link
Collaborator Author

@Alasdair Alasdair Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't, as it would require an existential quantifier ("M is a power of two" becomes "there exists an N such that M = 2 ^ N"). We could add some syntactic sugar for a set of powers of two up to some bound though.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Oct 9, 2024 via email

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 9, 2024

Yeah I agree. I thought about this a bit more and I think in the fullness of time we probably want something like this:

image

Any of those steps on their own (1, 2a, 2b or 2c) solve the original page boundary problem. The minimum needed to solve it is 1, so I think we should do that first. 2a, 2b and 2c are only required if you want to precisely match PMA/PMP exceptions to a hardware implementation (and that implementation supports PMA/PMPs finer grained than a page; I'm not sure how common that is - our hardware doesn't but I can imagine some does).

Supporting all of those options may be a bit tricky to code in Sail tbf. Probably doable though; I might have a go...

I think actually the biggest issue is mem_write_ea. We don't really want to duplicate this function 3 or 4 times, with just one line in the middle different. :-/

@Alasdair
Copy link
Collaborator Author

Alasdair commented Oct 9, 2024

It also needs to handle float loads/stores and a gazillion vector loads/stores. I don't know how you do that nicely without first class functions though. vmem_write_from_float_reg, vmem_write_from_vector_reg, etc...

I think I was trying too hard to not change the existing order in the code. Realistically if we ditch the write_mem_ea call things become a lot easier and we can just have a vmem_write that everything uses.

The idea is that for the operational concurrency model, an instruction computes the address (and announces it) before it reads the value its writing for re-ordering reasons. However, I think there is an equivalent formalisation of the operational model where the write_mem_ea event takes a set of registers as an additional parameter, and this would remove the need to rely on any ordering in the Sail spec (which has the additional advantage of matching how the axiomatic model works w.r.t. ordering between Sail statements). Although to be clear, this equivalent formalisation only exists in my head and I haven't proven it equivalent.

I think we should add a flag to only split at some grain size, which would be a maximum of the page size (where you must split, unless we make it very complicated and check pages are contiguously mapped). I can imagine implementations where you might want to set it to the cache line size too.

The Zama16b extension mentioned previously is essentially guaranteeing that you can do misaligned accesses in hardware up to 16 byte boundaries, so it makes sense to implement something like this. I wrote a function that implements that, but the logic is quite twisty when you have a different alignments and access width parameters, and you have to be careful when at the the top or bottom of the address space to avoid wrapping when computing the top or bottom byte address used by the access (I might be missing some easy way of doing it though).

@allenjbaum
Copy link
Collaborator

allenjbaum commented Oct 10, 2024 via email

@Alasdair
Copy link
Collaborator Author

Updated with a plain vmem_write function that can be used everywhere, including in the vector extension (but I still need to update all those instructions, there aren't that many though). I also updated STORECON and LOADRES even though that may not be needed as they are always aligned.

@Alasdair Alasdair force-pushed the ldst_misaligned branch 2 times, most recently from b9ba55d to ef84c7c Compare October 28, 2024 19:16
@Alasdair
Handle address translation for misaligned loads and stores better
91a16b4 
Refactor the LOAD and STORE instruction so they split misaligned
accesses into multiple sub-accesses and perform address translation
separately. This means we should handle the case where a misaligned
access straddles a page boundary in a sensible way, even if we don't
yet cover the full range of possibilities allowed for any RISC-V
implementation.

There are options for the order in which misaligned happen, i.e. from
high-to-low or from low-to-high as well as the granularity of the splitting,
either all the way to bytes or to the largest aligned size. The splitting
can also be disabled if an implementation supports misaligned accesses in hardware.

The Zama16b extension is support with an --enable-zama16b flag on the simulator.

In addition tidy up the implementation in a few ways:

- Very long lines on the LOAD encdec were fixed by adding a helper

- Add some linebreaks in the code so it reads as less claustrophobic

- Ensure we use the same names for arguments in encdec/execute/assembly.
  Previously we used 'size' and 'width'. I opted for 'width' consistently.
@Alasdair
Copy link
Collaborator Author

I have now added the option to allow misaligned accesses to be atomic provided they exist within some region with a --misaligned-allowed-within flag, and added the Zama16b extension with the --enable-zama16b flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Timmmm Timmmm Timmmm left review comments

Assignees
No one assigned
Labels
tgmm-agenda Tagged for the next Golden Model meeting agenda.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Alasdair @Timmmm @PeterSewell @allenjbaum @jrtc27 @billmcspadden-riscv