RFR: 8310656: RISC-V: __builtin___clear_cache can fail silently. [v2]

[Fei Yang]

On Fri, 30 Jun 2023 08:56:02 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hi, please consider.
>> 
>> We recently had a bug where user were missing permissions to use this syscall.
>> Which caused crashing on, according to hs_err on things like "addi x11, x24, 0" with SIGILL.
>> If it fails it is even possible to execute valid but 'old' instruction which may not lead to a crash, instead the program misbehaves.
>> 
>> To avoid this mess I suggest that we first test the syscall during vm init and we use it directly.
>> This way we can make sure it never fails.
>> 
>> Tested failing syscall with qemu, tested t1 in qemu, t1 on jh7110 in-progress.
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   added back data barrier
>   
>   Signed-off-by: Robbin Ehn <rehn at rivosinc.com>

Overall LGTM. Two nits remain.

src/hotspot/cpu/riscv/icache_riscv.cpp line 49:

> 47: 
> 48: void ICacheStubGenerator::generate_icache_flush(ICache::flush_icache_stub_t* flush_icache_stub) {
> 49:   // Only riscv_flush_icache is supported as I-cache synhronization.

Typo: s/synhronization/synchronization/

src/hotspot/os_cpu/linux_riscv/riscv_flush_icache.cpp line 62:

> 60: bool RiscvFlushIcache::test() {
> 61:   long ret;
> 62:   intptr_t end = ((intptr_t)&ret) + 64;

It looks a little bit odd to me here as `ret` is only 8 bytes. Maybe define a local array of 64 bytes and operate on that?

-------------

Marked as reviewed by fyang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14670#pullrequestreview-1507993451
PR Review Comment: https://git.openjdk.org/jdk/pull/14670#discussion_r1248353481
PR Review Comment: https://git.openjdk.org/jdk/pull/14670#discussion_r1248354948