This is an archive of the discontinued LLVM Phabricator instance.

[NFC][RISCV][builtins] remove some hard-coded values from i-cache clear routine
ClosedPublic

Authored by EccoTheDolphin on Sep 12 2020, 6:13 PM.

Details

Summary

remove some hard-coded values from i-cache clear routine

Diff Detail

Event Timeline

EccoTheDolphin created this revision.Sep 12 2020, 6:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2020, 6:13 PM
Herald added subscribers: Restricted Project, evandro, luismarques and 12 others. · View Herald Transcript
EccoTheDolphin requested review of this revision.Sep 12 2020, 6:13 PM

You'll notice that there is already an implementation of __clear_cache for RISC-V in the file you are contributing to, in line 160. That existing implementation directly performs a syscall. Your new implementation calls a (RISC-V-specific) GNU libc function __riscv_flush_icache, which internally either uses a vDSO (if available) or performs the syscall.

We don't need to have the two implementations, and in fact with this patch you'll probably get the cache cleared twice. Still, there might be some opportunities for improving the existing code. Some more specific comments:

(1) Do we want to depend on the glibc / versions of libc that implement __riscv_flush_icache? Some older versions of glibc didn't implement that, I think. The main advantage is that we would get the vDSO support, although we could also implement that directly, without depending on glibc.
(2) The existing code doesn't check for RV64. I wrote that code, but I don't remember if that was intentional (i.e. I had confirmed that the syscall should also work for RV32 Linux) or an oversight. So the check for __riscv_xlen == 64 might be a good idea.
(3) For the direct syscall I had avoided #including the headers by directly defining a few syscall constants, but we can revisit that decision if it's too hardcoded. Still, I think you might be #including some headers you don't actually need for the function call you are using.

compiler-rt/lib/builtins/clear_cache.c
49–53

Do we actually need all of these headers? Isn't sys/cachectl.h sufficient?

93–95

I think the option that we really want to document is the 0 (clear for all threads), not the one we don't use (SYS_RISCV_FLUSH_ICACHE_LOCAL).

smd removed a subscriber: smd.Sep 14 2020, 9:38 AM
smd added a subscriber: smd.

You'll notice that there is already an implementation of __clear_cache for RISC-V in the file you are contributing to, in line 160. That existing implementation directly performs a syscall. Your new implementation calls a (RISC-V-specific) GNU libc function __riscv_flush_icache, which internally either uses a vDSO (if available) or performs the syscall.

We don't need to have the two implementations, and in fact with this patch you'll probably get the cache cleared twice. Still, there might be some opportunities for improving the existing code. Some more specific comments:

(1) Do we want to depend on the glibc / versions of libc that implement __riscv_flush_icache? Some older versions of glibc didn't implement that, I think. The main advantage is that we would get the vDSO support, although we could also implement that directly, without depending on glibc.
(2) The existing code doesn't check for RV64. I wrote that code, but I don't remember if that was intentional (i.e. I had confirmed that the syscall should also work for RV32 Linux) or an oversight. So the check for __riscv_xlen == 64 might be a good idea.
(3) For the direct syscall I had avoided #including the headers by directly defining a few syscall constants, but we can revisit that decision if it's too hardcoded. Still, I think you might be #including some headers you don't actually need for the function call you are using.

Thank you very much for your review. As for the code duplication - sorry once again, at the time this was written we had no such code in the mainline (we used llvm 10 as the initial codebase). We'll look into the suggested adjustments.

We don't need to have the two implementations, and in fact with this patch you'll probably get the cache cleared twice. Still, there might be some opportunities for improving the existing code. Some more specific comments:

I agree. Once again, thank you for pointing that out. Also it should be noted that we don't really need icache_clear implementation for ASAN to function. We stumbled upon an unimplemented syscall when we were debugging our port of HWASAN (which we have plans to publish, but it requires an additional ISA extention). As such we decided to isolate this patch (that is the modified version with the suggested improvements to the existing codebase) from the main patch bundle and will re-upload it as a standalone patch.

(1) Do we want to depend on the glibc / versions of libc that implement __riscv_flush_icache? Some older versions of glibc didn't implement that, I think. The main advantage is that we would get the vDSO support, although we could also implement that directly, without depending on glibc.

I don't have enough knowledge about the consequences of such decision. As such I would prefer to not avoid introducing an additional dependency for now (till we get a better understanding of that).

(2) The existing code doesn't check for RV64. I wrote that code, but I don't remember if that was intentional (i.e. I had confirmed that the syscall should also work for RV32 Linux) or an oversight. So the check for __riscv_xlen == 64 might be a good idea.

I've scanned through the source code of Linux kernel - and I believe that we don't need to check for encoding. Looks like the respected syscall is available on both 32-bit and 64 bit versions.

(3) For the direct syscall I had avoided #including the headers by directly defining a few syscall constants, but we can revisit that decision if it's too hardcoded. Still, I think you might be #including some headers you don't actually need for the function call you are using.

I believe that the hard-coding of "__NR_riscv_flush_icache" constant is excessive, since it should be available if we include <sys/syscall.h> header (this define is provided by the Linux kernel). Other header files are not needed.

addressing improvements suggested by Luís Marques

EccoTheDolphin retitled this revision from [RISCV][builtins] implementation of i-cache clear routine to [NFC][RISCV][builtins] remove some hard-coded values from i-cache clear routine.Sep 23 2020, 11:48 PM
EccoTheDolphin edited the summary of this revision. (Show Details)
luismarques accepted this revision.Sep 24 2020, 2:01 AM

LGTM. Thanks for the patch.

This revision is now accepted and ready to land.Sep 24 2020, 2:01 AM

I can commit this for you. Is assume this the correct author? Alexey Baturo <space.monkey.delivers@gmail.com>

IMO, I think you could request commit access. You have contributed a whole patch series and have been thoughfully participating in its review. Since most commits should go through pre-review anyway, the bar to gain commit access probably shouldn't be too high. IMO it should just be just high enough that random people aren't always screwing everything and disrupting other's work. If you do gain commit access, just keep an eye on the buildbots when you commit a patch, and revert the patch if builds start to fail and you can't immediately fix the issue.

I can commit this for you. Is assume this the correct author? Alexey Baturo <space.monkey.delivers@gmail.com>

IMO, I think you could request commit access. You have contributed a whole patch series and have been thoughfully participating in its review. Since most commits should go through pre-review anyway, the bar to gain commit access probably shouldn't be too high. IMO it should just be just high enough that random people aren't always screwing everything and disrupting other's work. If you do gain commit access, just keep an eye on the buildbots when you commit a patch, and revert the patch if builds start to fail and you can't immediately fix the issue.

Please, do. Yes, please, change the author is Alexey Baturo - he is the one who debugged this and provided the original patch.

IMO, I think you could request commit access. You have contributed a whole patch series and have been thoughtfully participating in its review. Since most commits should go through pre-review anyway, the bar to gain commit access probably shouldn't be too high.

Thanks. I'll try to request one in the meantime.