Add riscv64 CPU feature detection#173
Conversation
klauspost
left a comment
klauspost
left a comment
There was a problem hiding this comment.
Looks very clean. No objections.
There was a problem hiding this comment.
Pull request overview
Adds initial riscv64 CPU feature detection support to the cpuid package, integrating Linux hwprobe-based detection (with /proc/cpuinfo fallback) and extending the public feature/vendor enums and docs to include RISC-V.
Changes:
- Add
riscv64OS/CPU detection (riscv_hwprobesyscall on Linux;/proc/cpuinfofallback). - Extend
FeatureIDandVendorenums (and generated string mappings) with RISC-V features/vendors. - Update documentation and CLI package comments to list
riscv64as supported.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents riscv64 detection approach and lists new RISC-V feature flags. |
| os_other_riscv64.go | Adds non-Linux riscv64 OS detection stub (core counts only). |
| os_linux_riscv64.go | Implements Linux riscv64 feature/vendor detection via riscv_hwprobe and /proc/cpuinfo parsing. |
| featureid_string.go | Updates generated stringer output for new FeatureID/Vendor constants. |
| detect_riscv64.go | Adds riscv64 entry point wiring into the library’s detection pipeline. |
| detect_ref.go | Updates fallback build constraints to exclude riscv64. |
| cpuid.go | Adds RISC-V vendors/features and updates supported-arch comment; extends vendor mapping. |
| cmd/cpuid/main.go | Updates supported-arch comment for the CLI wrapper. |
Files not reviewed (1)
- featureid_string.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I updated CI and a few other minor things. Looks like there is a minor conflict. Please take a look at that, and the AI feedback, which looks legit to me. |
mengzhuo
force-pushed
the
riscv64
branch
2 times, most recently
from
43bab01 to
17841f8
Compare
| c.featureSet.setIf(extMap["ztso"], RV_ZTSO) | ||
| c.featureSet.setIf(extMap["zacas"], RV_ZACAS) | ||
|
|
||
| hasCrypto := extMap["zknd"] || extMap["zkne"] || extMap["zknh"] || |
There was a problem hiding this comment.
Just wondering - aren't these also useful individually? I don't know about the arch, but I imagine each refers to specific features and not the same, or?
There was a problem hiding this comment.
Yes... RISC-V has a lots of feature sets, I choice some instead of All ratified extensions.
For example, ZACAS you can do Compare-And-Swap without branching.
https://github.com/zte-riscv/go/pull/68/changes
Right now golang.org/x/sys only add vector/bit related.
There was a problem hiding this comment.
I can only add minimum extension sets for rva20u64 profile if you want
There was a problem hiding this comment.
OK. I would prefer the detection to be as fine-grained as possible, meaning keeping RV_ZKND, etc as a single feature.
You are welcome to add this:
var RV_CRYPTO_FEATS = CombineFeatures(RV_ZKND, ...)
// HasOneOf returns whether the CPU supports one or more of the requested features.
func (c *CPUInfo) HasOneOf(f Features) bool {
return c.featureSet.hasOneOf(f)
}...so callers can do cpuid.CPU.HasOneOf(cpuid.RV_CRYPTO_FEATS)). But I am not sure I understand the value of having one, but not all of these features. Did you mean to AND them?
There was a problem hiding this comment.
No, zvk (crypto) is not mandatory all AES/SHA (NIST suit, N) SM (Chinese suits, S) so that's possible to have only N or S.
There was a problem hiding this comment.
OK, but we need to be able to detect them individually, not just "some of these may be available".
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis PR adds complete RISC-V 64-bit (riscv64) support to the cpuid package, including vendor constants, ISA feature detection via hwprobe syscall or ChangesRISC-V Feature Detection
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Thanks for reviewing, I'm will commit new code based on your review ASAP. |
|
Tip For best results, initiate chat on the files or code changes.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
os_linux_riscv64.go (1)
138-143: 💤 Low valueConsider clarifying why RV_IMA is set unconditionally.
Line 143 sets
RV_IMAunconditionally without checking hwprobe base_behavior. While this assumption is correct—Linux/riscv64 requires IMA support to boot—a brief comment would make the rationale explicit.Optional clarifying comment
c.featureSet.setIf(imaExt&uint64(cryptoMask) != 0, RV_CRYPTO) + // RV64IMA is mandatory for Linux; all riscv64 systems support it c.featureSet.set(RV_IMA) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@os_linux_riscv64.go` around lines 138 - 143, Add a short clarifying comment before the unconditional c.featureSet.set(RV_IMA) explaining that Linux/riscv64 requires IMA support to boot, which is why RV_IMA is set regardless of hwprobe/base_behavior; reference the surrounding logic that computes imaExt & cryptoMask and sets RV_CRYPTO so readers know RV_IMA is intentionally unconditional (symbols: RV_IMA, c.featureSet.set, imaExt, cryptoMask, RV_CRYPTO).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpuid.go`:
- Line 348: The RV_CRYPTO comment currently lists only five extensions
(Zknd/Zkne/Zknh/Zksed/Zksh) but the detection logic in riscv_isa.go for
RV_CRYPTO checks eight extensions; update the RV_CRYPTO comment to enumerate all
extensions that the code detects (add Zbkb, Zbkc, Zbkx) or adjust the detection
logic to match the documented list, referencing the RV_CRYPTO constant and the
detection code in riscv_isa.go so the comment and implementation stay
consistent.
In `@detect_riscv64.go`:
- Around line 1-3: This file lacks the riscv64 build constraint and is being
compiled on non-riscv64 targets causing duplicate symbols; add a build tag
header above the package declaration exactly like other detect_* files by
inserting the modern and legacy build constraints for riscv64 (e.g., "//go:build
riscv64" and the corresponding "// +build riscv64") before "package cpuid" so
the file (detect_riscv64.go) is only built on riscv64 targets.
In `@README.md`:
- Line 542: Remove the undocumented RV_RNG table row from the README: locate the
table entry that lists "RV_RNG" (the undocumented feature ID) and delete that
table row so the README matches the implemented feature IDs in
featureid_string.go (e.g., RV_IMA, RV_C, RV_F, RV_D, RV_V, RV_ZBA–RV_ZBS,
RV_ZICOND, RV_ZIHINTPAUSE, RV_ZICBOM, RV_ZICBOZ, RV_ZICBOP, RV_ZFA, RV_ZFH,
RV_ZFHMIN, RV_ZTSO, RV_ZACAS, RV_CRYPTO, RV_VECTOR_CRYPTO); ensure no other
references to RV_RNG remain in the documentation.
---
Nitpick comments:
In `@os_linux_riscv64.go`:
- Around line 138-143: Add a short clarifying comment before the unconditional
c.featureSet.set(RV_IMA) explaining that Linux/riscv64 requires IMA support to
boot, which is why RV_IMA is set regardless of hwprobe/base_behavior; reference
the surrounding logic that computes imaExt & cryptoMask and sets RV_CRYPTO so
readers know RV_IMA is intentionally unconditional (symbols: RV_IMA,
c.featureSet.set, imaExt, cryptoMask, RV_CRYPTO).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6310fa7b-e27b-4a36-b809-edad53a22084
📒 Files selected for processing (10)
README.mdcmd/cpuid/main.gocpuid.gocpuid_test.godetect_ref.godetect_riscv64.gofeatureid_string.goos_linux_riscv64.goos_other_riscv64.goriscv_isa.go
| RV_ZFHMIN // Minimal half-precision FP | ||
| RV_ZTSO // Total store ordering | ||
| RV_ZACAS // Atomic CAS | ||
| RV_CRYPTO // Scalar cryptography (Zknd/Zkne/Zknh/Zksed/Zksh) |
There was a problem hiding this comment.
RV_CRYPTO comment is incomplete.
The comment lists five extensions (Zknd/Zkne/Zknh/Zksed/Zksh) but the actual detection logic in riscv_isa.go:44-46 checks eight extensions: the five listed plus Zbkb, Zbkc, and Zbkx.
📝 Proposed fix
- RV_CRYPTO // Scalar cryptography (Zknd/Zkne/Zknh/Zksed/Zksh)
+ RV_CRYPTO // Scalar cryptography (Zknd/Zkne/Zknh/Zksed/Zksh/Zbkb/Zbkc/Zbkx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RV_CRYPTO // Scalar cryptography (Zknd/Zkne/Zknh/Zksed/Zksh) | |
| RV_CRYPTO // Scalar cryptography (Zknd/Zkne/Zknh/Zksed/Zksh/Zbkb/Zbkc/Zbkx) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpuid.go` at line 348, The RV_CRYPTO comment currently lists only five
extensions (Zknd/Zkne/Zknh/Zksed/Zksh) but the detection logic in riscv_isa.go
for RV_CRYPTO checks eight extensions; update the RV_CRYPTO comment to enumerate
all extensions that the code detects (add Zbkb, Zbkc, Zbkx) or adjust the
detection logic to match the documented list, referencing the RV_CRYPTO constant
and the detection code in riscv_isa.go so the comment and implementation stay
consistent.
| | RV_ZTSO | Total store ordering | | ||
| | RV_ZACAS | Atomic CAS | | ||
| | RV_CRYPTO | Scalar cryptography (Zknd/Zkne/Zknh/Zksed/Zksh/Zbkb/etc) | | ||
| | RV_RNG | Entropy source (Zkr) | |
There was a problem hiding this comment.
Remove undocumented RV_RNG feature.
The RV_RNG feature ID is not defined in the codebase. According to the featureid_string.go constants, the implemented RISC-V features are: RV_IMA, RV_C, RV_F, RV_D, RV_V, RV_ZBA–RV_ZBS, RV_ZICOND, RV_ZIHINTPAUSE, RV_ZICBOM, RV_ZICBOZ, RV_ZICBOP, RV_ZFA, RV_ZFH, RV_ZFHMIN, RV_ZTSO, RV_ZACAS, RV_CRYPTO, and RV_VECTOR_CRYPTO. RV_RNG is absent.
Fix: remove the RV_RNG row
| RV_ZACAS | Atomic CAS |
| RV_CRYPTO | Scalar cryptography (Zknd/Zkne/Zknh/Zksed/Zksh/Zbkb/etc) |
-| RV_RNG | Entropy source (Zkr) |
| RV_VECTOR_CRYPTO | Vector crypto (Zvbb/Zvbc/Zvkg/Zvkned/Zvknhb/etc) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | RV_RNG | Entropy source (Zkr) | | |
| | RV_ZACAS | Atomic CAS | | |
| | RV_CRYPTO | Scalar cryptography (Zknd/Zkne/Zknh/Zksed/Zksh/Zbkb/etc) | | |
| | RV_VECTOR_CRYPTO | Vector crypto (Zvbb/Zvbc/Zvkg/Zvkned/Zvknhb/etc) | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 542, Remove the undocumented RV_RNG table row from the
README: locate the table entry that lists "RV_RNG" (the undocumented feature ID)
and delete that table row so the README matches the implemented feature IDs in
featureid_string.go (e.g., RV_IMA, RV_C, RV_F, RV_D, RV_V, RV_ZBA–RV_ZBS,
RV_ZICOND, RV_ZIHINTPAUSE, RV_ZICBOM, RV_ZICBOZ, RV_ZICBOP, RV_ZFA, RV_ZFH,
RV_ZFHMIN, RV_ZTSO, RV_ZACAS, RV_CRYPTO, RV_VECTOR_CRYPTO); ensure no other
references to RV_RNG remain in the documentation.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests