wasmtime / PR #4997 Flush Icache on AArch64 Windows · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #4997 Flush Icache on AArch64 Windows

Wasmtime GitHub notifications bot (Oct 02 2022 at 18:52):

afonso360 opened PR #4997 from windows-aarch64-icache to main:

:wave: Hey,

I tried to run the cranelift filetest suite on a aarch64-pc-windows-msvc machine, and it crashes with STATUS_ILLEGAL_INSTRUCTION. All of the tests pass individually, and if I run it on a single core, it sometimes passes the entire test suite.

I think this is due to us not clearing the icache after writing the new code as required by arm. This PR adds a call to FlushInstructionCache where we already have a membarrier on linux (See #3426).

I'm not too knowledgeable about this, but Its was what I saw recommended in a Arm Community blog post, although I would really appreciate if someone could doublecheck if this is the correct approach. This also seems to be what firefox does for their jit.

With this patch we can now pass the entire filetest suite without crashing!

I applied the same solution to the wasmtime side of things, but it's worth noting that I was never able to get a STATUS_ILLEGAL_INSTRUCTION there!
I tested with cargo test -p wasmtime-cli wast::Cranelift::spec::simd_i and all 48 tests pass, no matter how many times I try to run them.

I can't test the entire test suite since that fails due to #4992 (I think)

cc: @cfallin @akirilov-arm

Wasmtime GitHub notifications bot (Oct 02 2022 at 19:01):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2022 at 19:01):

bjorn3 created PR review comment:

Maybe call this from set_readonly_and_executable? That will also allow skipping parts that have already been flushed previously when doing multiple rounds of defining a function and finalizing everything, like for hot reloading.

Wasmtime GitHub notifications bot (Oct 02 2022 at 19:49):

afonso360 edited PR #4997 from windows-aarch64-icache to main:

:wave: Hey,

I tried to run the cranelift filetest suite on a aarch64-pc-windows-msvc machine, and it crashes with STATUS_ILLEGAL_INSTRUCTION. All of the tests pass individually, and if I run it on a single core, it sometimes passes the entire test suite.

I think this is due to us not clearing the icache after writing the new code as required by arm. This PR adds a call to FlushInstructionCache where we already have a membarrier on linux (See #3426).

I'm not too knowledgeable about this, but it was what I saw recommended in a Arm Community blog post, although I would really appreciate if someone could doublecheck if this is the correct approach. This also seems to be what firefox does for their jit.

With this patch we can now pass the entire filetest suite without crashing!

I applied the same solution to the wasmtime side of things, but it's worth noting that I was never able to get a STATUS_ILLEGAL_INSTRUCTION there!
I tested with cargo test -p wasmtime-cli wast::Cranelift::spec::simd_i and all 48 tests pass, no matter how many times I try to run them.

I can't test the entire test suite since that fails due to #4992 (I think)

cc: @cfallin @akirilov-arm

Wasmtime GitHub notifications bot (Oct 03 2022 at 09:28):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 03 2022 at 09:35):

afonso360 created PR review comment:

I've done that and I've also switched the prepare_icache_flush calls to be done in allocate(). Calling one inside Memory and the other outside I think would be a kind of weird API quirk. I added a MemoryUse flag to ensure that we always call the icache flushes/prepares in the appropriate place, and only for memory intended for execution.

Another thing that I noticed and think is probably a bug, is that we only call MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE once, when initially allocating the memory, but then can call MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE multiple times when hotswapping, I'm not totally sure this is allowed and what happens if we do this.

If it is, then maybe we want to call prepare_icache_flush in new instead of allocate.

I tested this patch on aarch64-linux and aarch64-msvc and it seems to work on both.

Wasmtime GitHub notifications bot (Oct 03 2022 at 09:35):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 03 2022 at 09:36):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 03 2022 at 09:36):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 03 2022 at 09:39):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 03 2022 at 09:40):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 03 2022 at 09:45):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:43):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:43):

akirilov-arm created PR review comment:

One minor suggestion - perform the cache maintenance before making the memory read-only; the architecture permits either option, but I think that there have been cases in the past where the operations failed on non-writable pages due to a CPU erratum.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:43):

akirilov-arm created PR review comment:

Similarly here - do this before the call on line 161.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:43):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:43):

akirilov-arm created PR review comment:

Refer to my comment in #4987, but the gist of it is that MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE should be executed only once for the whole lifetime of the process and subsequent calls are just unnecessary overhead, yet harmless. So, yes, you shouldn't have moved the code.

BTW another subtle issue - the membarrier() calls are completely unnecessary for single-threaded processes and multi-threaded programs, provided that each thread uses its own generated code. However, I can't think of any easy way to distinguish these use cases from the regular multi-threaded one, and I am not sure if adding an option to JITModule::new() is the right approach.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:43):

akirilov-arm created PR review comment:

Nit - I think the phrasing of this comment gives the wrong impression because there is no obligation really to call icache_flush() at some point after this call; my suggestion is to just state that it is a prerequisite for any usage of the latter.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:54):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:54):

afonso360 created PR review comment:

Got it! I'll move it to new and add a comment with that description. Thanks!

Wasmtime GitHub notifications bot (Oct 03 2022 at 10:56):

afonso360 deleted PR review comment.

Wasmtime GitHub notifications bot (Oct 03 2022 at 11:01):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 03 2022 at 11:01):

afonso360 created PR review comment:

Got it! I'd like to try and keep all the membarrier calls contained to the Memory struct, otherwise I think we get a really weird interface where you need to do some stuff outside of Memory and some other stuff happens automatically. I think moving this to new effectively gets us back to where we were before while keeping these details contained.

I'm not really seeing a way of cleaning the issue you mentioned in https://github.com/bytecodealliance/wasmtime/pull/4987#discussion_r984806226 where we only really need to do this once per process.

Wasmtime GitHub notifications bot (Oct 03 2022 at 11:03):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 03 2022 at 13:04):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 03 2022 at 13:04):

akirilov-arm created PR review comment:

Sorry, I might have given the wrong impression - I am not really concerned with where exactly the source code is located, rather with the sequence of operations at runtime. So, as long as the registration happens only at the creation of the JITModule object (which seems to be the "entry point" into the cranelift-jit crate), then everything is fine from my point of view.

As for the other issue - on second thought, you might be able to just execute MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE and then, assuming that my reading of the documentation is correct, if you fail with EPERM, do the registration and repeat. BTW you will probably have to coordinate with @cfallin because he should touch the same code paths to fix the issue he is working on, so I'd expect merge conflicts.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:24):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:46):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:46):

cfallin created PR review comment:

A minor thing, but would it make sense here to call this wasmtime-jit-icache-coherence instead and version it along with the other Wasmtime-related crates (so 2.0.0 currently)?

I honestly don't have too many strong opinions here, but absent any other reasons it does seem like a crate that lives in our repo in crates/ and is published with all the others should probably be named/versioned in line with the others as well.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:46):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:46):

cfallin created PR review comment:

Ah, here I see you've considered RISC-V, cool (comments above).

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:46):

cfallin created PR review comment:

Not strictly necessary for this PR, but I wonder how RISC-V fits into this -- it looks like at the ISA level it has a fence.i instruction, so it is closer to AArch64 in this regard (weaker coherence by default). Is it enough to do the same membarrier calls as on aarch64? (cc @yuyang-ok)

In the absence of any other information, perhaps we could perform the same membarrier calls on RISC-V as we do on aarch64?

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:54):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:54):

afonso360 created PR review comment:

I don't mind the naming, but I'm a bit unsure about publishing.

The main reason is that our cache_clear is a lie (on linux), and only works with the pipeline_flush. We do mention in the docs that callers must always call pipeline_flush but I'm worried that someone might not do that.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:57):

cfallin created PR review comment:

Ah, so it actually has to be published if it's a dependency of wasmtime-jit. If the API safety is a concern, perhaps we could work out how to wrap it in a higher-level interface (e.g. some sort of object with methods to cache-clear and pipeline-flush, and only allows one to pipeline-flush once cache-clear has been invoked at least once)?

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:59):

alexcrichton created PR review comment:

Also, how come there's a rustix and a libc implementation? Would it be reasonable to pick one as the only non-windows implementation?

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:59):

alexcrichton created PR review comment:

One way I'd recommend writing this to make this more easily maintainable over time is:

cfg_if::cfg_if! {
    if #[cfg(target_os = "windows")] {
        mod win;
        use win as imp;
    } else if #[cfg(feature = "rustix")] {
        mod rustix;
        use rustix as imp;
    } else {
        mod libc;
        use libc as imp;
    }
}

and below just use imp::the_method() instead of duplicating the #[cfg]

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 18:59):

alexcrichton created PR review comment:

Could the functions in this crate use #[inline] instead of #[inline(always)]?

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:09):

afonso360 created PR review comment:

Well, for cranelift-jit we don't want to import rustix since that adds a bunch of dependencies for a couple of membarriers (https://github.com/bytecodealliance/wasmtime/pull/3395). For wasmtime I think we do want the safety guarantees of rustix.

So we ended up with both, one implementation in wasmtime-jit and another in cranelift-jit

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:09):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:11):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:14):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:15):

alexcrichton created PR review comment:

From a maintainability point of view though I don't think it makes sense to have two different versions of this code. If cranelift-jit doesn't want to use rustix because it's too big of a dependency then that seems like an equivalent argument could be made for wasmtime dropping rustix, but the same arguments for why wasmtime uses rustix I feel can be used in reverse as well to motivate the usage of rustix.

Overall I assume the actual compiled-down code is basically the same modulo what function does the syscall instruction so at least form my perspective I would prefer to only have one implementation to maintain rather than two.

Also a bit more broadly I feel that the cranelift-jit crate doesn't really fit well with this repository right now. Nothing in Wasmtime uses it and it does not see heavy usage in tests I believe, but it's quite a complicated an nontrivial crate at the same time. Basically the support/maintenance story for it seems somewhat unclear but I ideally don't want it to place further burdens on other code elsewhere.

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:31):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:31):

bjorn3 created PR review comment:

Cranelift-jit is used and tested by cg_clif.

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:56):

afonso360 created PR review comment:

Overall I assume the actual compiled-down code is basically the same modulo what function does the syscall instruction so at least form my perspective I would prefer to only have one implementation to maintain rather than two.

I hope so too! Or its probably a bug. :big_smile: I don't really have an opinion on what we should do here, but I'm happy with whatever.

Also a bit more broadly I feel that the cranelift-jit crate doesn't really fit well with this repository right now. Nothing in Wasmtime uses it and it does not see heavy usage in tests I believe, but it's quite a complicated an nontrivial crate at the same time. Basically the support/maintenance story for it seems somewhat unclear but I ideally don't want it to place further burdens on other code elsewhere.

We do use the jit for all runtests in cranelift (in the filetest suite), that ends up being quite a few and it also runs on all targets that cranelift supports. And we also use the jit when fuzzing the cranelift-fuzzgen target.

I do think we could go the other way and try to use cranelift-jit in wasmtime instead of wasmtime-jit. That would probably be a big project, and I'm not sure if there is anything that would be fundamentally incompatible. But I think it would make sense in terms of a code sharing perspective, and as a bonus cranelift users would get a better JIT!

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:56):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 04 2022 at 19:58):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 04 2022 at 20:00):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 04 2022 at 20:11):

bjorn3 created PR review comment:

I think we could share the core for handling mapping as executable and relocating, but the user facing interface of cranelift-module is designed for the C linkage model and is not compatible with the wasm linkage model.

Wasmtime GitHub notifications bot (Oct 04 2022 at 20:11):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 08:27):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 08:50):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 08:51):

afonso360 created PR review comment:

I've implemented clear_cache as unsafe, and added another note on the doc comments for that function. I like the current API as it allows authors to reorder the clear_cache and pipeline_flush as they see fit. Which should be valid as long as no one tries to execute code in the mean time.

Once we add an actual clear_cache implementation it should also be easier to just drop the unsafe and keep the same API.

What do you think about that?

Wasmtime GitHub notifications bot (Oct 05 2022 at 08:51):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 08:51):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:07):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:22):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:22):

afonso360 created PR review comment:

I agree that we do need to do something, from what I've read RISCV is allowed to have incoherent I and D caches. From this documentation of the kernel, it looks like CORE_SYNC is not yet implemented for RISCV.

I've tried to read the kernel a bit and I've found that they support a

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:23):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:25):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:31):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:32):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:46):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 10:22):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 14:23):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 14:41):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 14:41):

alexcrichton created PR review comment:

The not(target_os = "windows") condition here can be removed since it's already implied by the if branch above

Wasmtime GitHub notifications bot (Oct 05 2022 at 14:54):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:12):

akirilov-arm created PR review comment:

Isn't this redundant now?

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:13):

akirilov-arm created PR review comment:

See above.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:13):

akirilov-arm created PR review comment:

Note that this blog post is about AArch32, though I think that it is still correct at a high level (but the reference to CP15, for example, is irrelevant).

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:13):

akirilov-arm created PR review comment:

all processors that are executing threads in the current process or something like that is probably a more useful statement? in the same coherence domain is technically correct, but perhaps superfluous - I am not aware of any operating system that runs in a different environment, at least any OS that might plausibly execute Wasmtime.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:13):

akirilov-arm created PR review comment:

IMHO we need a longer explanation here because the logic is a bit subtle. To an unfamiliar reader the implementation might be confusing because by the time we reach this point we will have already executed FlushInstructionCache(), so why are we flushing the write buffers now? The reason is that we do not care about the flushing of the write buffers per se, but about the fact that, as documented by Microsoft, this call results in an inter-processor interrupt that affects all processors running a thread of the current process, which would act as a core serializing operation.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:14):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:14):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:14):

akirilov-arm created PR review comment:

Stray over?

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:14):

akirilov-arm created PR review comment:

No, if it is necessary to call pipeline_flush() (i.e. the application is multi-threaded), then it must be called after clear_cache() (think speculative instruction prefetching).

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:14):

akirilov-arm created PR review comment:

cache instead of cash

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:14):

akirilov-arm created PR review comment:

See the comment on line 19.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:15):

akirilov-arm created PR review comment:

IMHO the name should emphasize that this function is necessary only for multi-threaded applications, so something like pipeline_flush_mt() or sync_core_mt()?

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:15):

akirilov-arm created PR review comment:

We need a comment here that this is required only for multi-threaded programs, and that it must be executed after clear_cache().

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:17):

akirilov-arm created PR review comment:

No, we rely on the mprotect() call that switches the memory from readable and writable to readable and executable.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:17):

akirilov-arm created PR review comment:

See my comment on libc::clear_cache.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:17):

akirilov-arm created PR review comment:

From an AArch64 perspective, this is a requirement only for multi-threaded code, i.e. code that does not do thread-local code generation and execution.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:57):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 05 2022 at 15:57):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 16:00):

akirilov-arm created PR review comment:

This is an architectural detail - I am not familiar with RISC-V at all, but it is possible that the architecture specifies that if instruction caches are flushed, then the pipeline might be flushed as well if necessary, hence no need to do anything in addition; on AArch64 these actions are decoupled. Or to put it another way - an architecture having incoherent data and instruction caches does not imply that it behaves in exactly the same way as the 64-bit Arm architecture (and hence requiring exactly the same sequence of actions); possibly there are nuances.

BTW the system call you have linked to says that it can be made to apply to all threads in the process, not just the caller, which might be what you are looking for.

Wasmtime GitHub notifications bot (Oct 05 2022 at 16:16):

cfallin created PR review comment:

That sounds good to me!

Wasmtime GitHub notifications bot (Oct 05 2022 at 16:16):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 16:59):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 17:22):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:16):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:16):

akirilov-arm created PR review comment:

Nit - you can mention the "broadcast" ISB bit here too for clarity, e.g.:

... core serializing operation, equivalent to a "broadcast" `ISB` that the architecture does not provide and which is what ...

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:16):

akirilov-arm created PR review comment:

I am sorry, but now I remember why I put the membarrier() call after changing the page permissions originally - so that it is ordered after the implicit cache cleaning done by mprotect(). Do you mind moving this line (and the preceding comment) right before line 214?

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:16):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:16):

akirilov-arm created PR review comment:

Similarly here - do you mind moving this call after line 166?

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:17):

akirilov-arm edited PR review comment.

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:46):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:46):

afonso360 created PR review comment:

Ah right! that makes sense since we are technically not doing anything in the clear_cache operation!

Wasmtime GitHub notifications bot (Oct 05 2022 at 19:48):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 07 2022 at 10:29):

afonso360 created PR review comment:

Or to put it another way - an architecture having incoherent data and instruction caches does not imply that it behaves in exactly the same way as the 64-bit Arm architecture (and hence requiring exactly the same sequence of actions); possibly there are nuances.

Yeah, that's right, we should go and double check that!

I've opened https://github.com/bytecodealliance/wasmtime/issues/5033 to track this, but I'm going to look at the ISA manual to check if they guarantee anything like that.

Wasmtime GitHub notifications bot (Oct 07 2022 at 10:29):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 11:03):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 11 2022 at 11:33):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 11:33):

akirilov-arm created PR review comment:

Nit - you can omit this.

Wasmtime GitHub notifications bot (Oct 11 2022 at 11:33):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 11:34):

akirilov-arm edited PR review comment.

Wasmtime GitHub notifications bot (Oct 11 2022 at 12:03):

afonso360 updated PR #4997 from windows-aarch64-icache to main.

Wasmtime GitHub notifications bot (Oct 11 2022 at 12:41):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:42):

afonso360 requested cfallin for a review on PR #4997.

Wasmtime GitHub notifications bot (Oct 12 2022 at 18:15):

cfallin merged PR #4997.

Last updated: Nov 22 2024 at 16:03 UTC