Add riscv64 CPU feature detection#173
Merged
Merged
Conversation
klauspost
approved these changes
Jun 1, 2026
klauspost
left a comment
klauspost
left a comment
Owner
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.
Owner
|
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
klauspost
approved these changes
Jun 3, 2026
klauspost
reviewed
Jun 3, 2026
Owner
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis PR adds complete RISC-V 64-bit (riscv64) support to the cpuid package, including vendor and feature ID constants, ISA string parsing, Linux hwprobe syscall detection with 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 |
Contributor
Author
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
Owner
|
@mengzhuo PTAL at the remaining comments. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@riscv_isa.go`:
- Around line 73-77: The combined crypto-suite flags (RV_ZKN, RV_ZKS, RV_ZVKNG,
RV_ZVKSG) are only being set based on whether their constituent individual
features are present, but they should also be set when the bundle token itself
is directly included in the ISA string. Modify each of the four setIf calls to
check an OR condition: in addition to checking hasSetP for the feature set
(rvZKNFeatures, rvZKSFeatures, rvZVKNFeatures, rvZVKSFeatures), also check if
the corresponding bundle token constant (RV_ZKN, RV_ZKS, RV_ZVKNG, RV_ZVKSG) is
already set in the featureSet, so that the combined flag is set whether it comes
from individual features or is specified directly as a bundle token.
🪄 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: 2f7bdced-3cba-4fd3-8e6f-56160a8eecac
📒 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
✅ Files skipped from review due to trivial changes (1)
- featureid_string.go
🚧 Files skipped from review as they are similar to previous changes (5)
- detect_ref.go
- cpuid_test.go
- os_other_riscv64.go
- cpuid.go
- detect_riscv64.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@riscv_isa.go`:
- Around line 10-19: The code stores raw ISA tokens in extMap without
normalizing versioned extensions like "zba1p0", causing exact feature lookups to
fail. After trimming whitespace from each extension token (following the `ext =
strings.TrimSpace(ext)` line), normalize the extension by stripping trailing
version information (numeric characters and version markers) so that "zba1p0"
becomes "zba" before storing in extMap. Apply this normalization consistently to
both the single-character extensions extracted from rv64 tokens in the for loop
over ext[4:] and to the regular extension tokens handled in the else branch.
🪄 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: 215ad2ac-4e36-4dd8-a820-b921ac2d794a
📒 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
✅ Files skipped from review due to trivial changes (1)
- featureid_string.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/cpuid/main.go
- os_other_riscv64.go
- detect_ref.go
- detect_riscv64.go
- cpuid_test.go
- os_linux_riscv64.go
- cpuid.go
klauspost
approved these changes
Jun 16, 2026
Owner
|
@mengzhuo Thanks for the contribution! |
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
/proc/cpuinfofallback, and an expanded RISC-V features table.