Skip to content

armv7#52

Open
samcday wants to merge 4 commits into
mainfrom
armv7
Open

armv7#52
samcday wants to merge 4 commits into
mainfrom
armv7

Conversation

@samcday
Copy link
Copy Markdown
Owner

@samcday samcday commented May 14, 2026

No description provided.

samcday added 2 commits May 14, 2026 15:14
Signed-off-by: Sam Day <me@samcday.com>
Signed-off-by: Sam Day <me@samcday.com>
@samcday samcday force-pushed the armv7 branch 3 times, most recently from ffbb694 to c5bb19f Compare May 14, 2026 06:03
Signed-off-by: Sam Day <me@samcday.com>
Signed-off-by: Sam Day <me@samcday.com>
@samcday
Copy link
Copy Markdown
Owner Author

samcday commented May 14, 2026

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 14, 2026

Claude finished @samcday's task in 2m 7s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze changes
  • Post review feedback

This PR does two things: (1) eliminates the bindgen + ublk_cmd.h build-time dependency in favor of hand-written Rust bindings, and (2) adds ARMv7 as a cross-compile target. Both are well-executed. Here are my observations:

What's good

Bindgen removal (build.rs, ublk_cmd.h deleted; sys.rs rewritten): Replacing generated bindings with hand-written structs + const assertions is a substantial improvement for cross-compilation. bindgen requires a C toolchain and target headers to be available at build time — exactly what makes ARMv7 cross-compilation painful. The compile-time assertions on sizes, alignments, and field offsets in sys.rs provide equivalent correctness guarantees that are target-aware (Rust evaluates size_of::<T>() in const context for the compile target, not the build host).

rust-lld for cross-linking: Using the LLVM linker (already part of the Rust toolchain) means no gcc-arm-linux-gnueabihf or similar cross-toolchain packages needed. The empty extra_packages: "" for armv7 is correct given this approach.

upload_host: false and -p smoo-gadget-cli: Correctly scoping the armv7 build to just the gadget binary, since smoo-host is meant for the host side and has USB library dependencies unsuitable for an ARMv7 embedded target.


Observations

--cfg=io_uring_skip_arch_check appears in two places

It's in .cargo/config.toml:16 (for local dev builds) and in build.yml:72 matrix.rustflags (picked up via RUSTFLAGS: "-D warnings ${{ matrix.rustflags }}" at line 105). This duplication is intentional — RUSTFLAGS env var overrides the config.toml target-specific rustflags, so both are needed. It's not wrong, but a brief comment on either side explaining why ARMv7 needs this flag (io-uring crate has an architecture allowlist; ARMv7 kernels 5.1+ do support io_uring) would help the next person who wonders if one of these can be removed.

Musl kernel headers step uses $(uname -m) (host arch)

.github/workflows/build.yml:91 creates symlinks under /usr/include/$(uname -m)-linux-musl/, which on the ubuntu-24.04 runner will resolve to x86_64, not arm. For the armv7 job this is harmless since smoo-gadget-cli doesn't pull in libusb1-sys (which is what needs these headers). But if a libusb-depending crate ever lands in the gadget binary's transitive deps, the cross-compile job would silently fail to find headers. This is a pre-existing issue in the workflow, not introduced here.

sys.rs defines more structs than the old bindings used

The new sys.rs includes ublk_param_discard, ublk_param_devt, ublk_param_zoned, ublk_param_dma_align, ublk_param_segment — these weren't in the public API before (they were defined in the header but only referenced through ublk_params). The assertions for all of them are present, so this is purely additive and correct. No issue, just noting the scope expansion.

ublksrv_io_cmd.addr is u64 on a 32-bit target

This is intentional: the kernel ABI for ublk deliberately uses __u64 for addresses even in the 32-bit userspace ABI, so the gadget can pass 32-bit pointers up-cast to u64. The compile-time assertions will catch any alignment breakage at cross-compile time.


Summary

Clean, well-reasoned PR. The bindgen removal is the right long-term call for a project targeting multiple architectures. The compile-time assertions are the correct mitigation for losing bindgen's static analysis. No blocking issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant