From f475eb8ee7c9a3e360b2f1210ed71b629243cd2a Mon Sep 17 00:00:00 2001 From: Hamlin Li Date: Thu, 16 Oct 2025 14:04:45 +0000 Subject: [PATCH] 8368950: RISC-V: fail to catch out of order declarations among dependent cpu extensions/flags Reviewed-by: fyang, luhenry --- src/hotspot/cpu/riscv/vm_version_riscv.hpp | 100 +++++++++++------- .../os_cpu/linux_riscv/riscv_hwprobe.cpp | 87 +++++++-------- 2 files changed, 108 insertions(+), 79 deletions(-) diff --git a/src/hotspot/cpu/riscv/vm_version_riscv.hpp b/src/hotspot/cpu/riscv/vm_version_riscv.hpp index 346ca35dc1e..3d555d47e9f 100644 --- a/src/hotspot/cpu/riscv/vm_version_riscv.hpp +++ b/src/hotspot/cpu/riscv/vm_version_riscv.hpp @@ -64,41 +64,12 @@ class VM_Version : public Abstract_VM_Version { virtual void disable_feature() { _value = -1; } - const char* pretty() { return _pretty; } - uint64_t feature_bit() { return _linux_feature_bit; } - bool feature_string() { return _feature_string; } - int64_t value() { return _value; } + const char* pretty() { return _pretty; } + uint64_t feature_bit() { return _linux_feature_bit; } + bool feature_string() { return _feature_string; } + int64_t value() { return _value; } virtual bool enabled() = 0; virtual void update_flag() = 0; - - protected: - bool deps_all_enabled(RVFeatureValue* dep0, ...) { - assert(dep0 != nullptr, "must not"); - - va_list va; - va_start(va, dep0); - RVFeatureValue* next = dep0; - bool enabled = true; - while (next != nullptr && enabled) { - enabled = next->enabled(); - next = va_arg(va, RVFeatureValue*); - } - va_end(va); - return enabled; - } - - void deps_string(stringStream& ss, RVFeatureValue* dep0, ...) { - assert(dep0 != nullptr, "must not"); - ss.print("%s (%s)", dep0->pretty(), dep0->enabled() ? "enabled" : "disabled"); - - va_list va; - va_start(va, dep0); - RVFeatureValue* next = nullptr; - while ((next = va_arg(va, RVFeatureValue*)) != nullptr) { - ss.print(", %s (%s)", next->pretty(), next->enabled() ? "enabled" : "disabled"); - } - va_end(va); - } }; #define UPDATE_DEFAULT(flag) \ @@ -117,8 +88,9 @@ class VM_Version : public Abstract_VM_Version { #define UPDATE_DEFAULT_DEP(flag, dep0, ...) \ void update_flag() { \ assert(enabled(), "Must be."); \ + DEBUG_ONLY(verify_deps(dep0, ##__VA_ARGS__)); \ if (FLAG_IS_DEFAULT(flag)) { \ - if (this->deps_all_enabled(dep0, ##__VA_ARGS__)) { \ + if (deps_all_enabled(dep0, ##__VA_ARGS__)) { \ FLAG_SET_DEFAULT(flag, true); \ } else { \ FLAG_SET_DEFAULT(flag, false); \ @@ -149,11 +121,16 @@ class VM_Version : public Abstract_VM_Version { class RVExtFeatureValue : public RVFeatureValue { const uint32_t _cpu_feature_index; + public: RVExtFeatureValue(const char* pretty, int linux_bit_num, uint32_t cpu_feature_index, bool fstring) : RVFeatureValue(pretty, linux_bit_num, fstring), _cpu_feature_index(cpu_feature_index) { } + int cpu_feature_index() { + // Can be used to check, for example, v is declared before Zvfh in RV_EXT_FEATURE_FLAGS. + return _cpu_feature_index; + } bool enabled() { return RVExtFeatures::current()->support_feature(_cpu_feature_index); } @@ -165,6 +142,57 @@ class VM_Version : public Abstract_VM_Version { RVFeatureValue::disable_feature(); RVExtFeatures::current()->clear_feature(_cpu_feature_index); } + + protected: + bool deps_all_enabled(RVExtFeatureValue* dep0, ...) { + assert(dep0 != nullptr, "must not"); + + va_list va; + va_start(va, dep0); + RVExtFeatureValue* next = dep0; + bool enabled = true; + while (next != nullptr && enabled) { + enabled = next->enabled(); + next = va_arg(va, RVExtFeatureValue*); + } + va_end(va); + return enabled; + } + + void deps_string(stringStream& ss, RVExtFeatureValue* dep0, ...) { + assert(dep0 != nullptr, "must not"); + ss.print("%s (%s)", dep0->pretty(), dep0->enabled() ? "enabled" : "disabled"); + + va_list va; + va_start(va, dep0); + RVExtFeatureValue* next = nullptr; + while ((next = va_arg(va, RVExtFeatureValue*)) != nullptr) { + ss.print(", %s (%s)", next->pretty(), next->enabled() ? "enabled" : "disabled"); + } + va_end(va); + } + +#ifdef ASSERT + void verify_deps(RVExtFeatureValue* dep0, ...) { + assert(dep0 != nullptr, "must not"); + assert(cpu_feature_index() >= 0, "must"); + + va_list va; + va_start(va, dep0); + RVExtFeatureValue* next = dep0; + while (next != nullptr) { + assert(next->cpu_feature_index() >= 0, "must"); + // We only need to check depenency relationship for extension flags. + // The dependant ones must be declared before this, for example, v must be declared + // before Zvfh in RV_EXT_FEATURE_FLAGS. The reason is in setup_cpu_available_features + // we need to make sure v is `update_flag`ed before Zvfh, so Zvfh is `update_flag`ed + // based on v. + assert(cpu_feature_index() > next->cpu_feature_index(), "Invalid"); + next = va_arg(va, RVExtFeatureValue*); + } + va_end(va); + } +#endif // ASSERT }; class RVNonExtFeatureValue : public RVFeatureValue { @@ -282,14 +310,14 @@ class VM_Version : public Abstract_VM_Version { decl(marchid , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \ /* A unique encoding of the version of the processor implementation. */ \ decl(mimpid , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \ + /* Manufactory JEDEC id encoded, ISA vol 2 3.1.2.. */ \ + decl(mvendorid , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \ /* SATP bits (number of virtual addr bits) mbare, sv39, sv48, sv57, sv64 */ \ decl(satp_mode , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \ /* Performance of misaligned scalar accesses (unknown, emulated, slow, fast, unsupported) */ \ decl(unaligned_scalar , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \ /* Performance of misaligned vector accesses (unknown, unspported, slow, fast) */ \ decl(unaligned_vector , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \ - /* Manufactory JEDEC id encoded, ISA vol 2 3.1.2.. */ \ - decl(mvendorid , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \ decl(zicboz_block_size , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \ #define DECLARE_RV_NON_EXT_FEATURE(PRETTY, LINUX_BIT, FSTRING, FLAGF) \ diff --git a/src/hotspot/os_cpu/linux_riscv/riscv_hwprobe.cpp b/src/hotspot/os_cpu/linux_riscv/riscv_hwprobe.cpp index 017d8a43666..ec756c44fe6 100644 --- a/src/hotspot/os_cpu/linux_riscv/riscv_hwprobe.cpp +++ b/src/hotspot/os_cpu/linux_riscv/riscv_hwprobe.cpp @@ -167,27 +167,20 @@ static bool is_set(int64_t key, uint64_t value_mask) { void RiscvHwprobe::add_features_from_query_result() { assert(rw_hwprobe_completed, "hwprobe not init yet."); - if (is_valid(RISCV_HWPROBE_KEY_MVENDORID)) { - VM_Version::mvendorid.enable_feature(query[RISCV_HWPROBE_KEY_MVENDORID].value); - } - if (is_valid(RISCV_HWPROBE_KEY_MARCHID)) { - VM_Version::marchid.enable_feature(query[RISCV_HWPROBE_KEY_MARCHID].value); - } - if (is_valid(RISCV_HWPROBE_KEY_MIMPID)) { - VM_Version::mimpid.enable_feature(query[RISCV_HWPROBE_KEY_MIMPID].value); - } + // ====== extensions ====== + // if (is_set(RISCV_HWPROBE_KEY_BASE_BEHAVIOR, RISCV_HWPROBE_BASE_BEHAVIOR_IMA)) { + VM_Version::ext_a.enable_feature(); VM_Version::ext_i.enable_feature(); VM_Version::ext_m.enable_feature(); - VM_Version::ext_a.enable_feature(); - } - if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_IMA_FD)) { - VM_Version::ext_f.enable_feature(); - VM_Version::ext_d.enable_feature(); } if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_IMA_C)) { VM_Version::ext_c.enable_feature(); } + if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_IMA_FD)) { + VM_Version::ext_d.enable_feature(); + VM_Version::ext_f.enable_feature(); + } if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_IMA_V)) { // Linux signal return bug when using vector with vlen > 128b in pre 6.8.5. long major, minor, patch; @@ -202,21 +195,29 @@ void RiscvHwprobe::add_features_from_query_result() { VM_Version::ext_v.enable_feature(); } } + +#ifndef PRODUCT + if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZACAS)) { + VM_Version::ext_Zacas.enable_feature(); + } +#endif if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZBA)) { VM_Version::ext_Zba.enable_feature(); } if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZBB)) { VM_Version::ext_Zbb.enable_feature(); } +#ifndef PRODUCT + if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZBKB)) { + VM_Version::ext_Zbkb.enable_feature(); + } +#endif if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZBS)) { VM_Version::ext_Zbs.enable_feature(); } #ifndef PRODUCT - if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZICBOZ)) { - VM_Version::ext_Zicboz.enable_feature(); - } - if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZBKB)) { - VM_Version::ext_Zbkb.enable_feature(); + if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZFA)) { + VM_Version::ext_Zfa.enable_feature(); } #endif if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZFH)) { @@ -226,15 +227,28 @@ void RiscvHwprobe::add_features_from_query_result() { VM_Version::ext_Zfhmin.enable_feature(); } #ifndef PRODUCT + if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZICBOZ)) { + VM_Version::ext_Zicboz.enable_feature(); + } + // Currently tests shows that cmove using Zicond instructions will bring + // performance regression, but to get a test coverage all the time, will + // still prefer to enabling it in debug version. + if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZICOND)) { + VM_Version::ext_Zicond.enable_feature(); + } + if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZTSO)) { + VM_Version::ext_Ztso.enable_feature(); + } if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZVBB)) { VM_Version::ext_Zvbb.enable_feature(); } -#endif -#ifndef PRODUCT if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZVBC)) { VM_Version::ext_Zvbc.enable_feature(); } #endif + if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZVFH)) { + VM_Version::ext_Zvfh.enable_feature(); + } #ifndef PRODUCT if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZVKNED) && is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZVKNHB) && @@ -243,30 +257,18 @@ void RiscvHwprobe::add_features_from_query_result() { VM_Version::ext_Zvkn.enable_feature(); } #endif - if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZVFH)) { - VM_Version::ext_Zvfh.enable_feature(); + + // ====== non-extensions ====== + // + if (is_valid(RISCV_HWPROBE_KEY_MARCHID)) { + VM_Version::marchid.enable_feature(query[RISCV_HWPROBE_KEY_MARCHID].value); } -#ifndef PRODUCT - if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZFA)) { - VM_Version::ext_Zfa.enable_feature(); + if (is_valid(RISCV_HWPROBE_KEY_MIMPID)) { + VM_Version::mimpid.enable_feature(query[RISCV_HWPROBE_KEY_MIMPID].value); } -#endif -#ifndef PRODUCT - if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZTSO)) { - VM_Version::ext_Ztso.enable_feature(); + if (is_valid(RISCV_HWPROBE_KEY_MVENDORID)) { + VM_Version::mvendorid.enable_feature(query[RISCV_HWPROBE_KEY_MVENDORID].value); } -#endif -#ifndef PRODUCT - if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZACAS)) { - VM_Version::ext_Zacas.enable_feature(); - } - // Currently tests shows that cmove using Zicond instructions will bring - // performance regression, but to get a test coverage all the time, will - // still prefer to enabling it in debug version. - if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZICOND)) { - VM_Version::ext_Zicond.enable_feature(); - } -#endif // RISCV_HWPROBE_KEY_CPUPERF_0 is deprecated and returns similar values // to RISCV_HWPROBE_KEY_MISALIGNED_SCALAR_PERF. Keep it there for backward // compatibility with old kernels. @@ -277,7 +279,6 @@ void RiscvHwprobe::add_features_from_query_result() { VM_Version::unaligned_scalar.enable_feature( query[RISCV_HWPROBE_KEY_MISALIGNED_SCALAR_PERF].value); } - if (is_valid(RISCV_HWPROBE_KEY_MISALIGNED_VECTOR_PERF)) { VM_Version::unaligned_vector.enable_feature( query[RISCV_HWPROBE_KEY_MISALIGNED_VECTOR_PERF].value);