Conversation
WalkthroughRemoved kernel-version branching and compile-time feature checks from RISC-V feature detection; Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as Kernel init
participant Features as riscv_check_features
participant HW as Hardware HWCAP
participant Compile as Compile-time checks (old)
rect `#f0f8ff`
Note over Init,Features: Old flow (before change)
Init->>Features: call riscv_check_features
alt kernel >= 6.5
Features->>Compile: run compile-time checks
Compile-->>Features: compile-time results
else kernel < 6.5
Features->>HW: read runtime HWCAP
HW-->>Features: hw capability bits
end
Features-->>Init: feature bitmap
end
rect `#f7fff0`
Note over Init,Features: New flow (after change)
Init->>Features: call riscv_check_features
Features->>HW: read runtime HWCAP (RVV from HWCAP)
HW-->>Features: hw capability bits
Features-->>Init: feature bitmap (ZBC disabled)
end
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (100)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1999 +/- ##
===========================================
- Coverage 83.31% 82.78% -0.53%
===========================================
Files 161 161
Lines 12960 12952 -8
Branches 3149 3145 -4
===========================================
- Hits 10797 10722 -75
- Misses 1133 1170 +37
- Partials 1030 1060 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Calling something downstream "bogus" is not going to help getting this PR approved... Only way this PR is going to get approved is to combine it with adding support for hwprobe. |
That would have to be after the first 2.3 stable release. This PR disables ZBC completely, and while that is certainly not ideal, I am fine with that for now if that fixes a real problem. |
|
I've discussed the change privately earlier with Icenowy, and would prefer this PR over #1996 since it keeps the detection of RVV version (the asm volatile block) and thus won't break problematic downstream kernels that set HWCAP_ISA_V even with RVV v0.7.1. We could drop the extra detection later when hwprobe support is introduced: these problematic kernels don't have hwprobe support backported, so hwprobe will fail and we could simply fallback to GC. But serving as a quick fix, keeping most of the logic and only fixing the real problem sounds better to me. |
Disabling ZBC detection would discourage downstream projects from switching to zlib-ng until we implement hwprobe, which is more than just not ideal. It makes the project look unprofessional. This PR can be split in two by removing any controversial changes and delaying adding support for hwprobe to later zlib-ng release. We can list in 2.3.0 release notes that minimum supported kernel version for optimised variants is 6.5 as that is already established as safe requirement. |
Even after hwprobe syscall support is added in zlib-ng, there will be "short" gap before we change the minimum kernel version requirement... It might be possible when 2.4.0 is eventually released, but not before... This gives downstream projects enough time to switch to kernel that implements hwprobe syscall. |
|
I am okay to strip the Zbc commit out, although technically this is incorrect and may lead to problems in the future (but at least not now). |
The HWCAP facility comes at day 1 of Linux RISC-V support (date back to 4.15), only the V bit definition is added in 6.5 (because proper vector support is added in that version too). There should be no need to test kernel version number before accessing hwcap, only the V bit will never be present on kernel older than 6.5 (except dirty patched downstream ones). For Xtheadvector systems that bogusly announce V bit in HWCAP, the assembly code should be able to factor them out. This is tested on a Sophgo SG2042 machine with 6.1 kernel. Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
b989f77 to
331f8c4
Compare
|
Dropped the Zbc patch and fixed the problem that riscv_check_features_compile_time() is forgot to be removed . |
This PR drops the code that checks uname before accessing AT_HWCAP (because this auxval always exist in RISC-V Linux), and drops the bogus HWCAP bit for Zbc (because it's never part of upstream Linux UAPI).
cc @ziyao233