[herd,litmus] Initial support for translating addresses of instructions#1518
[herd,litmus] Initial support for translating addresses of instructions#1518artkhyzha merged 14 commits intoherd:masterfrom
Conversation
801e860 to
c13b49b
Compare
|
That's good idea, @murzinv . It seems that this commit passes UPD. I suppose some extra care could be taken to check if |
murzinv
left a comment
There was a problem hiding this comment.
Just few nitpicks I noticed while trying to add support in litmus7
| {} | ||
| P0 ; | ||
| .pagealign ; | ||
| ADD X0,X0,#1; |
There was a problem hiding this comment.
Nitpick: can we please use W registers (here and in other tests) so litmus7 won't complain about different types for register
Register x0 has different types: <int> and <int64_t>
| @@ -0,0 +1,9 @@ | |||
| AArch64 A005 | |||
| @@ -0,0 +1,13 @@ | |||
| AArch64 A011 | |||
| @@ -0,0 +1,10 @@ | |||
| AArch64 A008 | |||
| @@ -0,0 +1,9 @@ | |||
| AArch64 A007 | |||
|
JFYI, |
af7919c to
f41a95a
Compare
187b063 to
922c043
Compare
herd/AArch64Sem.ml
Outdated
| (get_exported_labels test)) | ||
| labels in | ||
|
|
||
| let needs_vmsa_for_ifetch = |
There was a problem hiding this comment.
Checking understanding: needs_vmsa_for_ifetch might be needed when the PTE fields change, while needs_ifetch is needed for labels that would enable changing the instruction at an address?
herd/mem.ml
Outdated
| with | ||
| | Some (V.Var _) -> true | ||
| | Some (V.Val (Symbolic (Virtual {name=n;_}))) when Symbol.is_label n -> true | ||
| | Some (V.Val (Symbolic (Physical (s,_)))) when (s |> Symbol.of_string |> Symbol.is_label) -> true (* FIXME: this benefits from a proper exposition of a label as a physical address *) |
There was a problem hiding this comment.
If this is future work, can it be added in the PR description similar to the other items?
There was a problem hiding this comment.
Fair. With a bit of thinking, I wonder if the PAs could use Symbols instead of strings too. That would make the interface here a bit clearer. However, it's not necessary for the functionality of this PR, so I am tempted to leave it as a FIXME.
Specifically, right now we have:
In mem.ml:
let solve_mem_non_mixed test es rfm cns kont res =
...
| Some (V.Val (Symbolic (Physical (s,_)))) when (s |> Symbol.of_string |> Symbol.is_label) -> true
...
In semExtra.ml:
let is_non_mixed_symbol test =
...
| Physical (s,_) when (s |> Symbol.of_string |> Symbol.is_label) -> true
...
In constant.ml:
let is_non_mixed_symbol = function
| Physical (s,_) when (s |> Symbol.of_string |> Symbol.is_label)
...
In all of these cases, I am slightly surprised by how I need to analyse the string to understand if it is a label. This is in constract to VAs, where I just look at the type of the name. The need to provide logic for PAs of labels will likely remain, but perhaps the interface for checking if a constant is a PA of a label could become simpler by letting the type of PA's symbol be Constant.Symbol rather than a string.
|
Thanks @artkhyzha, great work. I have reviewed this PR in detail with the exception of |
0f5d9ec to
6ff5781
Compare
|
@relokin @fsestini One of the commits of this PR, [herd] Modify tests to match new fault syntax, is concerned with modifying the output of some of the existing tests that have generic faults in their postcondition. This does not impact the hash of the test. This is required as a result of the introduction of Another issue I'd like your opinion on is excluding the new execute bit in the PTE from the computation of the hash. This required as, by default, we have agreed |
relokin
left a comment
There was a problem hiding this comment.
Thanks for this Artem. A couple of thoughts:
- I think you can squash my commits into one, they don't stand on their own.
- You would have to squash the last commit into the relevant commits as well.
- This commit "Modify tests to match new fault syntax" should be squash with the commit that introduces the new syntax.
Overall, we should ensure that we can pass the regressions with every commit.
I am a little on the fence about D-MMU and I-MMU have we discussed and agreed on this syntax? In existing tests that we use MMU that implies D-MMU?
For litmus7 can we please check that we can run all catalogues in vmsa+ifetch mode?
Finally, variant=vmsa,ifetch now enables translation for data and for instructions. Do we see the need to have a mode where we have ifetch and translation only for data? Does this even make sense to support?
| let mk_imm_branch off = Some (I_B (BranchTarget.Offset off)) | ||
|
|
||
| (* Nop is used by litmus7 as marker for start/stop of litmus assembly sequence *) | ||
| let nop = Some (I_MOV(V32,ZR,K 0)) |
There was a problem hiding this comment.
I am not entirely sure about this change. We can go with this for now, but there are two alternatives:
- Just like we count NOPs we could have counted .pagealign as the adding the corresponding number of NOPs.
- A better alternative in my opinion would be to not rely on NOP for markers at all, what can't litmus7 emits internal labels instead of counting NOPs?
Perhaps we can leave a note with a TODO here. Or otherwise something for @diaolo01's backlog.
I think it's perfectly fine to change .expected files.
Very good question and I am glad we are not brushing over this! Could we ignore the x bit for the purposes of computing the hash only when it has it's default value? Meaning if x=1, it is not used in the hash calculation, otherwise if x=0 we include it? It would be great if @maranget could have a look at this question as well. |
6ff5781 to
78f3a1c
Compare
|
As a status update and an interim response to @relokin's feedback (thanks!):
The current idea is to let There is an open question whether
Thank you for this suggestion. This is revealing some issues that will be investigated.
I agree that turning on/off translation for data accesses and/or instruction fetches could be helpful. This functionality is more about user interface, I think, and it strikes me as an "expert" feature that could be not enabled by default. We could support variants That said, I wonder if this idea is okay to implement separately, in a follow-up PR. |
78f3a1c to
cfe1421
Compare
@relokin, thank you again for this suggestion. It has revealed some issues that were investigated:
Overall, the open question is whether we would like to allow running PEs from EL0 and EL1 simultaneously. We do not currently expect to run litmus tests that execute from the same address both in EL0 and EL1, but in principle we could want to run litmus tests where some PEs are EL0 and others are EL1. One of the questions that comes up is the meaning of the "x" bit in TTD tuples: does it correspond to PXN or UXN? To which point the suggestion could be that both of them get set with identical values. Are there any other potential conceptual issues with mixing EL0 and EL1 in vmsa+ifetch tests? Do we consider this a blocking issue, or could it be left as future work? |
I've checked the behaviour I'd introduced and it already omits x when it is default (x=1) and includes it when it's different (x=0). No further action required on this. |
fsestini
left a comment
There was a problem hiding this comment.
Thanks @artkhyzha . I’m not too familiar with the VMSA/ifetch specifics, but the changes look good from my side.
I left a few inline comments. They’re mostly general OCaml/style notes rather than feature concerns.
One additional follow-up idea (not for this PR): pseudo.ml publicly exposes constructors like Macro and Skip that are treated as “illegal” in several places (via assert false, like, say gen/top_gen.ml:044). That "contract" isn’t explicit from the public Pseudo.S signature alone, even though lib and pseudo.ml are shared across the repo. It might be worth a follow‑up refactor to split pseudo into more specific types or to hide those constructors behind narrower interfaces, so the API and types better reflect what’s actually supported. I appreciate this PR is building on existing structures, but since the pseudo type is being touched here I thought I’d mention it as something to keep in mind.
lib/faultType.ml
Outdated
| let matches actual expected = | ||
| match actual, expected with | ||
| | MMU (_, m_actual), MMU (DISide.Any, m_expected) -> | ||
| m_actual = m_expected |
There was a problem hiding this comment.
Not a big deal in this particular case since mmu_t is a small variant, but as a general rule I try to avoid polymorphic equality (=) on custom datatypes since it’s a bit fragile if the type ever grows to include non-comparable data. Perhaps using an explicit let mmu_t_equal = (=) could keep the intent clear.
There was a problem hiding this comment.
Interesting. Does the same concern apply to the FaultType.t itself? Would it be a good practice to have equal : t -> t -> bool in its interface?
There was a problem hiding this comment.
It would apply to FaultType.t as well, yes, especially considering FaultType already exposes a compare function.
In general I would consider good practice for any module M exposing any type t that is intended to be compared for equality to also expose an explicit equality predicate.
cfe1421 to
7c61683
Compare
… for instruction fetches Changes: - simplified is_non_mixed_symbol - added parser support for referencing PTEs of labels - added address translation for instruction fetches Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
`litmus_pte_unset_el0` takes start and end addresses, but `code_init` calls it with end address mistakenly set to `start + number of pages`, which result in only single page being processed. Let's fix that by setting end to `start + size`. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
Instruction Abort comes with it's own Exception Code (EC), use that to wire up with our fault_handler. Note, that we do not have any symbol associated with the code, only lables can be used for that, thus do not attempt to match fault address due to instruction abort with any symbol. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
`litmus7` (when using `-variant self`) needs to identify the boundaries of the litmus assembly in the final binary. To achieve this, it wraps the inline assembly with special markers and provides runtime infrastructure to locate them. Currently, the NOP instruction is used as a marker. Although NOPs can also appear in the litmus assembly, `litmus7` handles this by counting them so that they do not interfere with the markers. However, we are about to emit an `.align` directive into the inline assembly. This directive introduces NOPs as padding, which are outside the `litmus7`’s visibility and can therefore interfere with the marker detection. To avoid this issue, switch the marker instruction from NOP to a less ambiguous one, such as `MOV WZR, #0`. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
Map the `.pagealign` directive to the `.align PAGE_SHIFT` assembler attribute. Since the page geometry is not known upfront, we use stringification tricks to defer the decision until compile time. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
Translating instruction addresses requires the litmus assembly to reside at page-size granularity. We already control alignment within the litmus assembly using the `.pagealign` directive, but we must also handle alignment outside of it. First, we need to ensure that the start of the litmus assembly is page-aligned and that the first instruction of the litmus test is placed at the beginning of the page. To achieve this, we implicitly insert a `.pagealign` directive at the start of the litmus assembly - between the start marker and the first instruction of the litmus test. We also align the `prelude_size` to the page boundary to account for the offset between the start marker and the first instruction. Second, when a fault handler is present, we must ensure safe recovery from faults. For this, we align everything following the litmus assembly to page granularity by implicitly inserting a `.pagealign` directive just before the stop marker. Third, the compiler-emitted code is copied into a dedicated page-aligned buffer for execution and modification. Copying the code as-is could break page alignment guarantees since the emitted code is not guaranteed to be page-aligned. Therefore, we enforce alignment of the generated code. Additionally, introduce logic to collect, save, manipulate, and restore page table entries for labels present in the litmus test. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
- Update faultType to contain MMU domain (instruction/data) - Update CAT sets for MMU and PTE sets to distinguish between data and instruction
-Introduce CAT sets for Data and Instruction PTE accesses
7c61683 to
6a603ae
Compare
|
Thank you, @fsestini, for the comments -- I have tried to address them, so the PR should be up to date. |
relokin
left a comment
There was a problem hiding this comment.
Thanks everyone for your efforts on this.
@artkhyzha feel free to merge when you are ready.
This PR adds basic support for address translation for instruction fetch into
herd7andlitmus7.The syntax of litmus tests is extended with:
.pagealignthat aligns the next instruction to the new page and inserts an immediate branch to that instruction;TTD(P0:Page0)=...wherePage0is a label on P0;Functionality-wise, semantics of instruction fetch is reworked to benefit from the pre-existing support for address translation. Prior to the instruction fetch, the address of the instruction is translated, and a possibility of the address having been remapped onto a different page is considered.
Ideas for future work.
UI-wise, recommended, but not urgent:
-variant ifetch,iico_ctrledges may need to be displayed without '-showevents all'.Refactoring-wise, optional and just for consideration:
loader.mlnow makes use of a pseudoinstructionSkip <imm>denoting the offset of a padding until the next page. To make use of it, the Loader module does yet another transformation of the internal data structure for a litmus test. Wasted transformations aside, it would be an improvement to maintainability if the organisation of the transformations was clearer.semExtra.mlimplements checks for the purposes of mixed-size mode, which are then used bymem.mlduring therfconstruction. One of such checks interacted with the way offsets are created for instruction addresses. That interaction received "duct-tape" treatment (a special case got added) and is marked as "FIXME". Ideally, offsets for instruction addresses would pass the usual mixed-size checks.