RFR: 8309258: RISC-V: Add riscv_hwprobe syscall [v2]

[Thomas Stuefe]

On Tue, 13 Jun 2023 12:34:28 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hi, please consider.
>> 
>> Linux kernel 6.4 will come with the new syscall https://docs.kernel.org/riscv/hwprobe.html to determine CPU and features.
>> RV cpus features/capabilities can vastly differ and it is not feasible for users to manually enable the correct feature set.
>> Today the VM uses the ELF aux vector and cpuinfo to gather some information about CPU capabilities.
>> 
>> Currently features are track with a bit field, this is insufficient.
>> There are many capabilities and these can have values attached to them.
>> CPU features should also be possible to turn if we can determine vendor (hwprobe).
>> 
>> This patchs adds the syscall, uses the syscall in combination with the aux and cpuinfo to enable features by default.
>> If there is a vendor specific path it calls that in addition.
>> Then we build the feature string(and bit field) and update flags accordingly.
>> 
>> Tested t1 and hwprobe with:
>> https://lore.kernel.org/qemu-devel/7f8d733df6e9b6151e9efb843d55441348805e70.camel@rivosinc.com/
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   free uarch string

I just got curious. Small nits and questions.

It seems odd to add individual vendor capabilities, but since its democratic and every vendor can add theirs, I think this is fine.

src/hotspot/cpu/riscv/vm_version_riscv.hpp line 47:

> 45:     uint64_t    _feature_bit;
> 46:     bool        _enabled;
> 47:     int64_t     _value;

all but _enabled and _value could be const, ditto the accessor routines

src/hotspot/os_cpu/linux_riscv/vm_version_linux_riscv.cpp line 40:

> 38: 
> 39: #ifndef HWCAP_ISA_I
> 40: #define HWCAP_ISA_I  (1ULL << ('I' - 'A'))

nit: use the nth_bit() macro?

src/hotspot/os_cpu/linux_riscv/vm_version_linux_riscv.cpp line 90:

> 88:   assert(ext_D.feature_bit() == HWCAP_ISA_D, "Bit for D must follow Linux HWCAP");
> 89:   assert(ext_C.feature_bit() == HWCAP_ISA_C, "Bit for C must follow Linux HWCAP");
> 90:   assert(ext_V.feature_bit() == HWCAP_ISA_V, "Bit for V must follow Linux HWCAP");

I'm somewhat confused about the hw_cap feature bits. Kernel asm/hwcap.h seems only to define caps up to C for riscv. This code mentions (in VMVersion) Q, H, V. 

Are these future flags?

Here we don't check for Q, should we check that too?

src/hotspot/os_cpu/linux_riscv/vm_version_linux_riscv.cpp line 99:

> 97:   char buf[1024] = {};
> 98:   if (uarch != nullptr && strcmp(uarch, "") != 0) {
> 99:     snprintf(buf, sizeof(buf), "%s,", uarch);

maybe assert for length < (sizeof(buf) - safety zone for the follow up strcat)

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

Marked as reviewed by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14445#pullrequestreview-1477270130
PR Review Comment: https://git.openjdk.org/jdk/pull/14445#discussion_r1228190142
PR Review Comment: https://git.openjdk.org/jdk/pull/14445#discussion_r1229033338
PR Review Comment: https://git.openjdk.org/jdk/pull/14445#discussion_r1229085082
PR Review Comment: https://git.openjdk.org/jdk/pull/14445#discussion_r1229021853