Conversation
e983ba4 to
d3045d5
Compare
There was a problem hiding this comment.
Pull request overview
Enables SST importers to compile on non-amd64 architectures while keeping SST hardware access amd64-only.
Changes:
- Splits SST detection and ioctl transport by architecture.
- Generates compile-only non-amd64 ABI declarations.
- Adds amd64/arm64 tests and native arm64 CI coverage.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pkg/sst/platform.go |
Moves architecture-specific detection out of shared code. |
pkg/sst/platform_amd64.go |
Retains device-based SST detection on amd64. |
pkg/sst/platform_nonamd64.go |
Reports SST unsupported on non-amd64. |
pkg/sst/platform_amd64_test.go |
Tests amd64 device detection. |
pkg/sst/platform_nonamd64_test.go |
Tests non-amd64 rejection. |
pkg/sst/internal/isst/isst.go |
Removes shared ioctl transport. |
pkg/sst/internal/isst/ioctl_amd64.go |
Provides the real amd64 ioctl transport. |
pkg/sst/internal/isst/ioctl_nonamd64.go |
Rejects non-amd64 ioctl requests. |
pkg/sst/internal/isst/ioctl_nonamd64_test.go |
Tests deterministic ioctl rejection. |
pkg/sst/internal/isst/_types_amd64.go |
Defines the shared Linux/amd64 generation source. |
pkg/sst/internal/isst/types_nonamd64.go |
Supplies compile-only ABI declarations. |
pkg/sst/internal/isst/gen_types.sh |
Generates both architecture outputs. |
.github/workflows/verify.yaml |
Adds cross-build and native arm64 validation. |
Files not reviewed (1)
- pkg/sst/internal/isst/types_nonamd64.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@klihub this is mostly lift and shift from |
|
/cc @marquiz |
klihub
left a comment
There was a problem hiding this comment.
@TonyxSun This looks fine to me.
I have one micro-nit regarding the commit message itself: the message body is 423 characters long, formatted as a single line. Could you reformat that to multiple lines with <= 72 characters each.
d3045d5 to
acd6faa
Compare
|
@klihub done! ty for reminder about convention |
acd6faa to
c6a5788
Compare
Generated amd64-only SST UAPI declarations prevent packages that import SST from compiling on other architectures. Generate the ISST UAPI declarations into an architecture-independent file from the Linux/amd64 ABI source. They are fixed-width structs and constants, so they compile everywhere. Gate support detection and ioctl execution by architecture so non-amd64 callers fail deterministically, without changing amd64 SST behavior. Build, lint and test natively on arm64 in CI with the same make verify and make test targets as amd64. Signed-off-by: Wuyue (Tony) Sun <68176295+TonyxSun@users.noreply.github.com>
c6a5788 to
d17a9f5
Compare
Summary
The SST package generates Linux ioctl request declarations in an amd64-only Go file. Importers therefore fail to compile on other architectures before SST support detection can report that the Intel-only feature is unavailable.
This change generates the ISST UAPI declarations into a single architecture-independent file from the Linux/amd64 ABI source. They are fixed-width structs and constants, so they compile everywhere. Device detection and the real ioctl transport remain amd64-only. Non-amd64 builds report SST as unsupported and reject ioctl calls before accessing the device path. It does not emulate SST or enable Intel SST features on ARM.
Native arm64 CI now runs the same
make verifyandmake testtargets as amd64. This unblocks ARM64 consumers such as containers/nri-plugins in containers/nri-plugins#461.Test plan
make verifymake testin a Linux/arm64 containerCGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build ./...CGO_ENABLED=0 GOOS=linux GOARCH=arm64 go build ./...go test ./pkg/sst/...on Linux/amd64 and Linux/arm64