This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][builtins][RISCV] Port __clear_cache to RISC-V baremetal
Needs RevisionPublic

Authored by sequencer on Dec 1 2020, 11:37 PM.

Details

Summary

Implements __clear_cache for RISC-V baremetal. We can't just use
SBI call on baremetal, this uses fence.i instruction flushing the
icache of the current hart, corresponding to riscv64-unknown-elf
toolchain.

Diff Detail

Event Timeline

sequencer created this revision.Dec 1 2020, 11:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 11:37 PM
Herald added subscribers: Restricted Project, NickHung, evandro and 15 others. · View Herald Transcript
sequencer requested review of this revision.Dec 1 2020, 11:37 PM
lenary accepted this revision.Dec 2 2020, 3:21 AM

Looking at the documentation in compiler-rt/lib/builtins/README.txt, I'm happy that this is doing the right thing: right now we don't have a (good) way of working out whether the core supports Zifencei or not, but I think it should be a no-op on systems which don't have caches.

This revision is now accepted and ready to land.Dec 2 2020, 3:21 AM

I'm concerned by the fact that this port doesn't clear the cache for other cores, which this builtin generally does. Furthermore, the #elif doesn't reliably detect baremetal, since it could be other non-Linux OSes, such as BSD. It's also not immediately obvious who actually requires / benefits from this builtin. Lastly, the existing test won't run for baremetal, so this won't get tested.

This revision now requires review to proceed.Dec 3 2020, 4:21 PM
lenary added a comment.Dec 3 2020, 4:25 PM

@luismarques Sorry for missing those details. I've now marked you as a blocking reviewer.

@lenary @luismarques Thank you for your review!

I'm concerned by the fact that this port doesn't clear the cache for other cores, which this builtin generally does.

I$ in RISCV core always doesn't maintain coherence usually, that's the reason why fence.i is used to flush cache. It also have no mechanism to flush all I$ in the SMP system. So SBI is designed to do this job. However bare metal may or may not have SBI, which means this job should be handled by programmer, not compiler.

Furthermore, the #elif doesn't reliably detect baremetal, since it could be other non-Linux OSes, such as BSD.

I think that's the problem is coming from current software architecture, basically, SBI be used for different supervisor programs, like Linux, BSD. So we may should define a SBI header and flag in compiler itself, something like __riscv_supervisor__, rather than __linux__. I think this refactor should be introduce in the future PRs.

Lastly, the existing test won't run for baremetal, so this won't get tested.

I really want to add test for this! But I don't know where to test this and how to test? I noticed in this commit, test is not added too.
Hope there are some inductions to me to test this!

mhorne added a subscriber: mhorne.Dec 7 2020, 1:11 PM

To add to the discussion a little, I have recently been looking at this __clear_cache problem on FreeBSD. I don't really see a viable workaround other than adding a new system call like Linux has done. It's unfortunate that a shortcoming in the ISA will force a change of this nature to be made on every platform, at least until better cache maintenance instructions are defined.

I share Luís's concern that this will negatively affect platforms other than the bare-metal target. I became aware of the issue due to the fact that __clear_cache currently expands to an abort. What happens if NetBSD, for example, starts building user programs that use __clear_cache? They may never become aware that the implementation only provides fence.i semantics, which could end up as a very difficult bug to track down.

Is there a particular motivation for porting this function to the bare-metal target? It seems to me that whatever bare-metal project is using this would be better off using a plain fence.i or other mechanism, at least at this point in time.

Is there a particular motivation for porting this function to the bare-metal target? It seems to me that whatever bare-metal project is using this would be better off using a plain fence.i or other mechanism, at least at this point in time.

Exactly. That's what I meant when I said "not immediately obvious who actually requires / benefits from this builtin". IMO, the point of these builtins is to abstract away several problems, but in most bare-metal environments you have enough control and knowledge of the platform that you'd be better off doing it yourself, in a way that's fully tailored to your environment. Still, if there is some reasonable use case, I'm still somewhat open to the idea of adding support for RISC-V bare-metal, just be sure to address the other review concerns.

lenary resigned from this revision.Jan 14 2021, 11:32 AM

@sequencer What's the plan for this patch?

lenary removed a subscriber: lenary.Mar 26 2021, 12:03 PM
luismarques requested changes to this revision.Jul 20 2021, 3:13 AM

As previously discussed, this is probably not the way to go. Since the patch is stalled but wasn't marked as abandoned, I'm explicitly "requesting changes" so that it doesn't stay in my "must review" queue.

This revision now requires changes to proceed.Jul 20 2021, 3:13 AM