Skip to content

Fix so all cache control functions should only exist for RISC-V on unix#118

Merged
CensoredUsername merged 1 commit into
CensoredUsername:devfrom
wakabat:new-feature-to-customize-flush-icache
Sep 12, 2025
Merged

Fix so all cache control functions should only exist for RISC-V on unix#118
CensoredUsername merged 1 commit into
CensoredUsername:devfrom
wakabat:new-feature-to-customize-flush-icache

Conversation

@wakabat

@wakabat wakabat commented Sep 11, 2025

Copy link
Copy Markdown
Contributor

Right now in riscv targets, only when compiling on unix configuration will the riscv_flush_icache be available. In other cases the compilation fails.

I do understand that only Linux provides __riscv_flush_icache making it work, there are other bare-metal environments, where we can simply have a no-op __riscv_flush_icache function.

This change adds a feature so we can plugin a customized __riscv_flush_icache, making the code suitable to more environments.

@Techcable

Copy link
Copy Markdown

The feature name does not reflect the fact that this is specific to RISC-V. It may be more appropriate as a cfg(...) flag, given that this is a niche feature and requires the cooperation of the final binary.

@wakabat

wakabat commented Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

I could of course rename the feature to something like riscv-custom-flush-icache, if that works for you.

So the goal here is really to be able to stub a riscv_flush_icache on non-Unix platform. There could be multiple ways achieving the goal, personally, I'm okay with any of them. I'm not sure what cfg(...) flag you would lean towards here. Let me know what you think is a more proper way, I can adjust the change accordingly.

@wakabat

wakabat commented Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

Personally, I would disagree that this is a niche feature. For RISC-V, I do believe people tweak it to run on more weird environments than just Linux. For x86 / arm64, maybe Linux could be more mainstream, and non-OS environment is a niche case, but with respect to RISC-V, I personally do believe those non-OS environments could actually be the majority.

@Techcable

Copy link
Copy Markdown

I could of course rename the feature to something like riscv-custom-flush-icache, if that works for you.

So the goal here is really to be able to stub a riscv_flush_icache on non-Unix platform. There could be multiple ways achieving the goal, personally, I'm okay with any of them. I'm not sure what cfg(...) flag you would lean towards here. Let me know what you think is a more proper way, I can adjust the change accordingly.

Well, I am not the maintainer, so my opinions don't really matter here. 😉

I think maybe a feature flag custom-flush-cache would make more sense if it unconditionally delegated to a dynasm-specific function like dynasm_flush_icache. This would make custom flush behavior possible regardless of operating system or architecture.

@CensoredUsername

Copy link
Copy Markdown
Owner

Hi! You've unfortunately ran into one of the rough edges of the RISC-V specification, which is that with the currently ratified standards there's no portable way of doing icache invalidation independent of the platform.

Just for some context, originally this was supposed to be handled by the fence.i instruction, but in the spec this instruction has some major issues. Namely, it only handles synchronisation of the current hart. Meaning that if you use it and the code then gets moved to another hart by the OS, you are out of luck. You do not know when your code gets moved around by the OS, which is why currently in RISC-V icache invalidation has to be handled by the OS, so it knows that it needs to execute a fence.i after moving the thread around.

I'm a bit confused on why this feature is needed though. On unix platforms we automatically call __riscv_flush_icache. On non-unix RISC-V dynasm-rs simply doesn't handle the icache invalidation by itself (it is already a no-op, so there shouldn't be a need to stub it out). If you do need to do something else (like call fence.i), you can simply manually do that before jumping into the generated code. There's no reason to actually have this separate feature for that?

Meanwhile, if you do have a RISC-V core with a more advanced icache management scheme, similar to aarch64, this single hook likely wouldn't be enough to begin with.

@wakabat

wakabat commented Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

On unix platforms we automatically call __riscv_flush_icache. On non-unix RISC-V dynasm-rs simply doesn't handle the icache invalidation by itself (it is already a no-op, so there shouldn't be a need to stub it out).

I think this is where we might not be on the same page. While the full riscv module will be compiled in RISC-V, the 2 functions riscv_flush_icache are enforce_ordering_dcache_icache under different cfg flags: riscv_flush_icache is only there for unix targets, while enforce_ordering_dcache_icache always exists.

As a result of this, when we try compiling dynasmrt with a target that is not unix, the following error will happen:

error[E0425]: cannot find function `riscv_flush_icache` in this scope
   --> runtime/src/cache_control.rs:204:18
    |
204 |             rv = riscv_flush_icache(start, end, flags);
    |                  ^^^^^^^^^^^^^^^^^^ not found in this scope

This is the rationale behind this PR. It's really not about a different icache management scheme, but the ability to avoid it when not needed.

Looking at your comments, perhaps it makes more sense to also hide enforce_ordering_dcache_icache behind cfg(unix) flag?

@CensoredUsername

CensoredUsername commented Sep 12, 2025 via email

Copy link
Copy Markdown
Owner

@wakabat wakabat changed the title Add feature to inject custom flush icache function for RISC-V Fix so all cache control functions should only exist for RISC-V on unix Sep 12, 2025
@wakabat

wakabat commented Sep 12, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for your reply, I've revised this PR to ensure that enforce_ordering_dache_icache is only available and called when the code is compiled for RISC-V on unix platforms. I've updated the PR description as well. Please let me know if you have any more comments.

@wakabat wakabat force-pushed the new-feature-to-customize-flush-icache branch from 4e8a6a1 to 4fb2864 Compare September 12, 2025 03:03
@CensoredUsername CensoredUsername changed the base branch from master to dev September 12, 2025 22:44
@CensoredUsername CensoredUsername merged commit ef8410c into CensoredUsername:dev Sep 12, 2025
1 check passed
@CensoredUsername

Copy link
Copy Markdown
Owner

Looks good, thanks! It should be released as 4.0.1 now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants