misaligned load/store on page crossing doesn't tablewalk for second page

[scottj97]

An 8-byte store to address 0xffc, for example, should store 4 bytes to one page and 4 to the next page. When paging is enabled, two separate translations are required.

The Sail model is only translating the first page, then storing 8 bytes to contiguous physical addresses.

• No tablewalk occurs for the second page
• No exceptions are taken if the second page has unacceptable permissions
• The physical address for the second half of the store is incorrect

I created a repo with a test to recreate the issue. Instruction 128 shows the problem. There is one tablewalk, then 8 bytes loaded from 0x80003FFC.

This should take an exception (with mtval 0x0000003a177df000) because the second page's access bit is 0. The handler will repair the access bit, then re-execute the instruction. The second attempt should succeed, loading the second half of the access from a different physical page.

(Fetches across pages seem to work correctly.)

[pmundkur]

this is a great catch and test case. i'm pondering the cleanest way to fix this.

[rmn30]

Unfortunately looks like we still need a fix for this. How about adding a width parameter to translateAddr and adding a TR_Straddling case to the TR_Result union? Will need to handle this in quite a lot of places.

[PeterSewell]

It should do two independent accesses, separately translated, no?

…

On Mon, 28 Jun 2021, 10:54 Robert Norton, ***@***.***> wrote: Unfortunately looks like we still need a fix for this. How about adding a width parameter to translateAddr and adding a TR_Straddling case to the TR_Result union? Will need to handle this in quite a lot of places. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#49 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFMZZVQGSATSIK4SJC6EMDTVBBE3ANCNFSM4M4MQK2Q> .

[rmn30]

Yes, I'm proposing that translateAddr would do two translations if necessary and then the caller would have to deal with an additional case for page-straddling by performing multiple accesses and merging the result. Possibly the single-page case would go away and become a degenerate case.

[PeterSewell]

I don't know exactly how the model is structured, but my point is that the decomposition of misaligned accesses into multiple accesses should happen slightly higher up, before one even gets to translateAddr. (Also, when one gets to the concurrency behaviour, they're really independent accesses)

…

On Mon, 28 Jun 2021 at 12:05, Robert Norton ***@***.***> wrote: Yes, I'm proposing that translateAddr would do two translations if necessary and then the caller would have to deal with an additional case for page-straddling by performing multiple accesses and merging the result. Possibly the single-page case would go away and become a degenerate case. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#49 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFMZZR7XYVTQRV3FR5Z6V3TVBJPNANCNFSM4M4MQK2Q> .

[rmn30]

I haven't checked the spec but I imagine you'd want to do both of the translations first to check for faults before doing the accesses otherwise you could end up doing half a write then realising you have to take a fault. The sail model does the translation first then the memory access uses the resulting physical address (unless there is a fault) e.g. https://github.com/rems-project/sail-riscv/blob/master/model/riscv_insts_base.sail#L326 The higher up place you posit doesn't currently exist although it might be useful to introduce a translate-and-handle-misalignment layer.

[PeterSewell]

On Mon, 28 Jun 2021 at 13:30, Robert Norton ***@***.***> wrote: I haven't checked the spec but I imagine you'd want to do both of the translations first to check for faults before doing the accesses otherwise you could end up doing half a write then realising you have to take a fault.

apparently some real h/w (on other archs) does do the latter

…

The sail model does the translation first then the memory access uses the resulting physical address (unless there is a fault) e.g. https://github.com/rems-project/sail-riscv/blob/master/model/riscv_insts_base.sail#L326 The higher up place you posit doesn't currently exist although it might be useful to introduce a translate-and-handle-misalignment layer. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#49 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFMZZU2ZGT52WEKV4QIAVDTVBTNVANCNFSM4M4MQK2Q> .

[rmn30]

I can't see anywhere in the RISC-V specs. that directly addresses this case although it is permitted to emulate unaligned accesses in software implying that partial writes are possible at least in that case.

[scottj97]

Partial writes are allowed by the spec. Spike does partial writes in this case.

However, some implementations will not, and I'd like Sail to be able to model that implementation choice without requiring extensive rework of this logic.

[allenjbaum]

This is a bug that will interfere with a fair number of tests - has there been any progress?
Does that mean that we need to configure not only whether Sail supports unaligned accesses, but that if it does, it checks both halves before writing anything (or reading)?
E.g. translate, write, translate, write vs Translate, Translate, Write, Write to ensure we can configure it like a real implementation?

[scottj97]

Does that mean that we need to configure not only whether Sail supports unaligned accesses, but that if it does, it checks both halves before writing anything (or reading)? E.g. translate, write, translate, write vs Translate, Translate, Write, Write to ensure we can configure it like a real implementation?

I'm not opposed to making source code edits to change this behavior if required, but I hope it can be a small change and not a complete rewrite of the logic.

There are many implementation-dependent things that require source code edits today.

[allenjbaum]

I'm not sure what you mean by source code edits - as opposed to what?

My concern here is that we are comparing the behavior of a Sail model with that of some vendor's legal RISC-V implementation. If more than one result is legally possible, I want to be have the vendor indicate which behavior they implement (and I'm going to make the assumption in this case it is one order or the other), and be able to configure sail to mimic that behavior.

It's a little worse in this case, because we already know that
"doesn't support unaligned data access" means an implementation will always trap, while
"does support unaligned data access" means that sometimes it will just perform the access,
and sometimes it will trap (based on non-architectural state values such as TLB state).

So results can be deterministic (that is, based on known architectural state or architectural options that are communicated via a YAML configuration file), and coild be non-deterministic (non-architectural state dependent).
If the number of legal non-deterministic results is small (e.g. 2, possibly 3), we intend to configure Sail to use one behavior, and then rerun with the other behavior, and allow matches on either behavior to be considered a match.

If I understand this case correctly, there are 3 legal results:

• return or write the value,
• access nothing and trap,
• access something and trap,
  I think can be reduced to 2 sets of two reaulta, which the pair is selected via a YAML configuration option.
  But, this requires that all 3 behaviors can be configured in the Sail model.

It's not the only option that doesn't have a CSR bit or some other means of determining what it does, but this particular example is the most challenging - and one of the more important ones to implement.

[scottj97]

Editing the source *.sail files, as opposed to editing a config setting in a YAML file. Since no such config system exists today, I've made dozens of source edits to match my team's implementation choices. If you intend to support all legal implementation choices via the YAML then yes you'd need a setting for this.

In this particular test case, we're doing an 8-byte store across two pages, the second of which does not have its PTE's dirty bit set. There are three legal results, depending on implementation choices. There is no nondeterministic behavior here.

• write the second PTE's dirty bit to 1, write 4 bytes to the first page, write 4 bytes to the second page (not necessarily in that order)
• write 4 bytes to the first page, then trap because the second page's dirty bit is 0
• write nothing; trap because the second page's dirty bit is 0

Regarding nondeterminism, I'll send you email about that, since it's getting off-topic for this issue report.

[allenjbaum]

Those are basically the 3 cases the ACTs will know about. It either executes without trapping, does a partial execution, and then traps, or does no execution and traps. In your specific case, it may be deterministic, but some implementations might trap if the 2nd half crosses a page boundary but doesn't have a translation in the TLB, but not trap if it doesn't cross a page boundary even if the translation is not in the TLB. For others it might also depend on whether there was a cache miss or not. That is nondeterministic, because it depends on TLB state which is microarchitectural. regarding source vs YAML: The intent is certainly to support all (well, within limits) legal implementations by configuration using the YAML files. Generally, those implementation choices depend on the value of CSR bits and their writability. That support (with those limits) is what is being added. Every CSR that has optional behavior will describe that behavior (e.g. which bits are read only, and their value). There is documentation on the schema syntax in the riscv-conf repo, and there is a prototype implementation that RIOS labs has developed. There are a few other optional behaviors that have no CSR controls, and misalign support is at the very top of the list. IF you have a list, I'd love to see it! (beyond: we implement these extensions that don't have MISA bits). There are two other privspec 1.11 features that I'm aware of that aren't implemented: writable BigEndian, and writable XLEN. I'm hoping no one ever tries to support them, because it will be nasty - but they're easily described using the defined syntax and YAML, and if the model can define its behavior based on the value of CSR bits, then the support is almost all there (modulo the changes that have to take effect when you change those CSR bit values....)

…

On Fri, Oct 1, 2021 at 11:49 PM Scott Johnson ***@***.***> wrote: Editing the source *.sail files, as opposed to editing a config setting in a YAML file. Since no such config system exists today, I've made dozens of source edits to match my team's implementation choices. If you intend to support all legal implementation choices via the YAML then yes you'd need a setting for this. In this particular test case, we're doing an 8-byte store across two pages, the second of which does not have its PTE's dirty bit set. There are three legal results, depending on implementation choices. There is no nondeterministic behavior here. - write the second PTE's dirty bit to 1, write 4 bytes to the first page, write 4 bytes to the second page (not necessarily in that order) - write 4 bytes to the first page, then trap because the second page's dirty bit is 0 - write nothing; trap because the second page's dirty bit is 0 Regarding nondeterminism, I'll send you email about that, since it's getting off-topic for this issue report. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#49 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJVRIBZ2GNX2V4HDMSDUE2TO5ANCNFSM4M4MQK2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[Timmmm]

I am going to fix this.

For memory reads I think it isn't too bad to solve. I will just change the code to a single mem_read() that takes a virtual address, and does virtual address translation and the memory read. For reads I can't see any reason to split up address translation and physical memory access.

Putting everything in one function means it is very easy to handle split accesses, and also means the virtual address is readily accessible for logging which is something we require for our verif system.

For writes, that doesn't quite work because of the requirement for the write value to be calculated after mem_write_ea() but before mem_write_value(). In most languages you could pass a get_value() callback that would calculate the write value, but Sail doesn't support first class functions so that isn't an option.

My proposal is to do the same as read - have a single mem_write() that takes a virtual address. However split it into two functions with an opaque type passed between them:

let opaque_data = mem_write_prepare(vaddr, width, etc.);
let write_value = X(rs1);
mem_write_commit(opaque_data, write_value);

I think this will work and be fairly elegant. It also allows you to fairly easily modify the code that it does partial writes. I will implement the case that does both translations up front though, since that's what the chip I'm working on does and I think it is the simplest & most likely case in most implementations.

If anyone thinks this is a bad idea let me know!