8374056: RISC-V: Fix argument passing for the RiscvFlushIcache::flush

[ikarostsin]

Based on the implementation of RiscvFlushIcache::flush, the second parameter should be the end address rather than the size of the memory region.

void RiscvFlushIcache::flush(uintptr_t start, uintptr_t end) {
  long ret = sys_flush_icache(start, end, SYS_RISCV_FLUSH_ICACHE_ALL);
  guarantee_with_errno(ret == 0, "riscv_flush_icache failed");

So this commit corrects the erroneous parameter passed to RiscvFlushIcache::flush in the icache_flush function call.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Error

 ⚠️ OCA signatory status must be verified

Issue

• JDK-8374056: RISC-V: Fix argument passing for the RiscvFlushIcache::flush (Bug - P4)

Reviewers

• Fei Yang (@RealFYang - Reviewer)
• Robbin Ehn (@robehn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28886/head:pull/28886
$ git checkout pull/28886

Update a local copy of the PR:
$ git checkout pull/28886
$ git pull https://git.openjdk.org/jdk.git pull/28886/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28886

View PR using the GUI difftool:
$ git pr show -t 28886

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28886.diff

[bridgekeeper]

Hi @ikarostsin, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user ikarostsin" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@ikarostsin The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[ikarostsin]

/signed

[bridgekeeper]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[RealFYang]

This seems related to https://bugs.openjdk.org/browse/JDK-8310656.

Before JDK-8310656, we used to do a __builtin___clear_cache(addr, addr + (lines << ICache::log2_line_size));.

@robehn : Could you please confirm that?

[RealFYang]

LGTM.

[RealFYang]

I checked latest kernel code (6.6 - 6.18), the syscall doesn't currently do anything with the address range.
I think that's why this doesn't affect functionality. But this needs to be fixed to avoid possible confusion.
(Created new JBS: https://bugs.openjdk.org/browse/JDK-8374056)

/*
 * Allows the instruction cache to be flushed from userspace.  Despite RISC-V
 * having a direct 'fence.i' instruction available to userspace (which we
 * can't trap!), that's not actually viable when running on Linux because the
 * kernel might schedule a process on another hart.  There is no way for
 * userspace to handle this without invoking the kernel (as it doesn't know the
 * thread->hart mappings), so we've defined a RISC-V specific system call to
 * flush the instruction cache.
 *
 * sys_riscv_flush_icache() is defined to flush the instruction cache over an
 * address range, with the flush applying to either all threads or just the
 * caller.  We don't currently do anything with the address range, that's just
 * in there for forwards compatibility.
 */
SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
        uintptr_t, flags)
{
        /* Check the reserved flags. */
        if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_ALL))
                return -EINVAL;

        flush_icache_mm(current->mm, flags & SYS_RISCV_FLUSH_ICACHE_LOCAL);

        return 0;
}

[robehn]

Yes, so it looks like I got careless with the argument as I did know they did not matter...

My confusion came from:

Glibc usual one:
extern int cacheflush (void *__addr, const int __nbytes, const int __op)

Risc-V:
extern int __riscv_flush_icache (void *__start, void *__end, unsigned long int __flags);
SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end, uintptr_t, flags)

Thanks for fixing !