Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR migrates hypervisor selection and conditional compilation from Cargo feature flags ( ChangesFeature-to-target_os cfg migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
45-56: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider adding explicit
permissions:blocks.zizmor flags these jobs as running with default (broad)
GITHUB_TOKENpermissions since nopermissions:block is set. Pre-existing and unrelated to this feature-flag removal, but worth tightening while touching these jobs.Also applies to: 103-116, 152-167
🤖 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 @.github/workflows/ci.yaml around lines 45 - 56, The GitHub Actions jobs currently rely on the default broad GITHUB_TOKEN scope because they do not define an explicit permissions block. Add a minimal permissions: section to the affected job definitions in ci.yaml, including build_arm64_hvp and the other referenced jobs, and keep the scopes as restrictive as possible for the steps they run. Use the existing job names to locate each workflow block and apply the same tightening consistently across them.Source: Linters/SAST tools
🤖 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 `@crates/vm-cli/src/main.rs`:
- Around line 22-27: The macOS branch in the hypervisor selection logic needs to
be limited to Apple Silicon builds because `AppleHypervisor` depends on
`crate::arch::aarch64::*` and will not compile on x86_64 macOS. Update the
platform gating in the `select_hypervisor` path so the
`vm_core::virtualization::hvp::AppleHypervisor` arm only matches when both
`target_os = "macos"` and `target_arch = "aarch64"` are true, and ensure the
other hypervisor arms remain unchanged.
In `@crates/vm-core/Cargo.toml`:
- Around line 24-38: The macOS virtualization dependencies are currently enabled
for every macOS build, which pulls in Apple Silicon-only crates on x86_64.
Update the dependency gate in Cargo.toml so the applevisor, applevisor-sys, and
strum entries are only included when both target_os is macos and target_arch is
aarch64, and keep the existing vm-core/vm-aarch64 related paths aligned with
that architecture-specific gating.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 45-56: The GitHub Actions jobs currently rely on the default broad
GITHUB_TOKEN scope because they do not define an explicit permissions block. Add
a minimal permissions: section to the affected job definitions in ci.yaml,
including build_arm64_hvp and the other referenced jobs, and keep the scopes as
restrictive as possible for the steps they run. Use the existing job names to
locate each workflow block and apply the same tightening consistently across
them.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 991b9cc6-b993-480a-8428-69f7eef671af
📒 Files selected for processing (10)
.github/workflows/ci.yamlcrates/vm-cli/Cargo.tomlcrates/vm-cli/src/main.rscrates/vm-core/Cargo.tomlcrates/vm-core/src/virtualization.rscrates/vm-core/src/virtualization/hypervisor/error.rscrates/vm-core/src/virtualization/vcpu/error.rscrates/vm-core/src/virtualization/vm/error.rscrates/vm-vmm/Cargo.tomlscripts/run_hvp.sh
💤 Files with no reviewable changes (1)
- crates/vm-cli/Cargo.toml
| target_os = "macos" => { | ||
| Ok(Box::new(vm_core::virtualization::hvp::AppleHypervisor)) | ||
| } | ||
| feature = "kvm" => { | ||
| target_os = "linux" => { | ||
| Ok(Box::new(vm_core::virtualization::kvm::KvmHypervisor::new()?)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files crates/vm-cli/src/main.rs crates/vm-core/Cargo.toml crates/vm-core/src/virtualization.rs crates/vm-core/src/virtualization/* 2>/dev/null || true
printf '\n== main.rs outline ==\n'
ast-grep outline crates/vm-cli/src/main.rs --view expanded || true
printf '\n== Cargo.toml outline ==\n'
ast-grep outline crates/vm-core/Cargo.toml --view expanded || true
printf '\n== Search for AppleHypervisor and target_arch gates ==\n'
rg -n 'AppleHypervisor|target_arch|aarch64|x86_64-apple-darwin|target_os = "macos"|cfg_select!' crates/vm-core crates/vm-cli -SRepository: junyu0312/rust-vm
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== main.rs around the relevant lines ==\n'
sed -n '1,120p' crates/vm-cli/src/main.rs
printf '\n== vm-core Cargo.toml around dependency/configuration lines ==\n'
sed -n '1,220p' crates/vm-core/Cargo.toml
printf '\n== virtualization.rs relevant slice ==\n'
sed -n '1,240p' crates/vm-core/src/virtualization.rsRepository: junyu0312/rust-vm
Length of output: 3356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Determine whether AppleHypervisor is gated by arch in code or docs ==\n'
rg -n 'AppleHypervisor|Hypervisor.framework|aarch64|arm64|Intel Macs|x86_64' -S .Repository: junyu0312/rust-vm
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow, read-only inspection for the exact branches and cfgs.
printf '\n== crates/vm-cli/src/main.rs ==\n'
nl -ba crates/vm-cli/src/main.rs | sed -n '1,120p'
printf '\n== crates/vm-core/Cargo.toml ==\n'
nl -ba crates/vm-core/Cargo.toml | sed -n '1,220p'
printf '\n== crates/vm-core/src/virtualization.rs ==\n'
nl -ba crates/vm-core/src/virtualization.rs | sed -n '1,260p'Repository: junyu0312/rust-vm
Length of output: 228
🌐 Web query:
Hypervisor.framework Intel Macs supported Apple Silicon only macOS Hypervisor.framework availability
💡 Result:
The Hypervisor.framework is available and supported on both Intel-based Macs and Apple Silicon Macs [1][2][3]. It is not restricted to Apple Silicon [1][3]. On Intel-based Mac computers, the framework requires a processor that supports the Intel VT-x feature set, including Extended Page Tables (EPT) and Unrestricted Mode [1][4][5]. These requirements generally align with Intel processors from the Westmere generation (released in 2010) or newer [4][6]. Developers can verify if a specific Intel-based Mac supports the framework by checking if the command sysctl kern.hv_support returns 1 [4][5][7]. On Apple Silicon Macs, the Hypervisor.framework is fully supported to build virtualization solutions [8][9]. Furthermore, starting with macOS 15.0, Apple added support for nested virtualization on Apple Silicon through the framework, enabling a hypervisor to run inside another virtual machine [10]. For high-level tasks, Apple also provides the Virtualization.framework, which is available on both Intel and Apple Silicon architectures and provides a higher-level API for running virtual machines [3].
Citations:
- 1: https://developer.apple.com/documentation/hypervisor
- 2: https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00963.html
- 3: https://developer.apple.com/documentation/virtualization?changes=latest_min__7_8_1&language=objc
- 4: https://apple.stackexchange.com/questions/220323/what-are-the-hardware-requirements-for-hypervisor-framework
- 5: https://docs.rs/hypervisor/latest/hypervisor/
- 6: https://github.com/moby/hyperkit
- 7: https://github.com/cloud-hypervisor/hypervisor-framework/blob/269219e730e1e78afecb09873cecf5dc44c0bada/README.md
- 8: Apple M1 / Hypervisor.framework support canonical/multipass#1857
- 9: https://developer.apple.com/documentation/hypervisor/apple-silicon
- 10: Support the hypervisor.framework on Apple-silicon actions/runner-images#13505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' crates/vm-core/src/virtualization/hvp.rs
printf '\n==\n'
sed -n '1,220p' crates/vm-core/src/virtualization/hvp/vm.rs
printf '\n==\n'
sed -n '1,260p' crates/vm-core/src/virtualization/hvp/vcpu.rsRepository: junyu0312/rust-vm
Length of output: 16904
Gate the macOS hypervisor path on target_arch = "aarch64". AppleHypervisor pulls in crate::arch::aarch64::*, so x86_64-apple-darwin will hit this branch and fail to compile.
🤖 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 `@crates/vm-cli/src/main.rs` around lines 22 - 27, The macOS branch in the
hypervisor selection logic needs to be limited to Apple Silicon builds because
`AppleHypervisor` depends on `crate::arch::aarch64::*` and will not compile on
x86_64 macOS. Update the platform gating in the `select_hypervisor` path so the
`vm_core::virtualization::hvp::AppleHypervisor` arm only matches when both
`target_os = "macos"` and `target_arch = "aarch64"` are true, and ensure the
other hypervisor arms remain unchanged.
| strum_macros = { workspace = true } | ||
| vm-aarch64 = { workspace = true } | ||
|
|
||
| [target.'cfg(target_arch = "x86_64")'.dependencies] | ||
| vm-firmware = { workspace = true } | ||
|
|
||
| [target.'cfg(target_os = "linux")'.dependencies] | ||
| kvm-bindings = { workspace = true } | ||
| kvm-ioctls = { workspace = true } | ||
| vmm-sys-util = { workspace = true } | ||
|
|
||
| [features] | ||
| kvm = ["kvm-bindings", "kvm-ioctls"] | ||
| hvp = ["applevisor", "applevisor-sys", "strum"] | ||
| [target.'cfg(target_os = "macos")'.dependencies] | ||
| applevisor = { workspace = true } | ||
| applevisor-sys = { workspace = true } | ||
| strum = { workspace = true } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Cargo.toml ==\n'
sed -n '1,220p' crates/vm-core/Cargo.toml
printf '\n== virtualization.rs outline ==\n'
ast-grep outline crates/vm-core/src/virtualization.rs --view expanded || true
printf '\n== main.rs outline ==\n'
ast-grep outline crates/vm-cli/src/main.rs --view expanded || true
printf '\n== Search for applevisor / hvp / aarch64 gates ==\n'
rg -n --hidden --glob '!target' --glob '!**/node_modules/**' \
'applevisor|applevisor-sys|hv_vm_config_set_el2_enabled|target_arch\s*=\s*"aarch64"|cfg_select!|feature\s*=\s*"hvp"|hvp' \
crates/vm-core crates/vm-cliRepository: junyu0312/rust-vm
Length of output: 14450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== crates/vm-core/src/virtualization.rs (relevant slice) ==\n'
sed -n '1,260p' crates/vm-core/src/virtualization.rs
printf '\n== crates/vm-core/src/virtualization/hvp.rs (relevant slice) ==\n'
sed -n '1,260p' crates/vm-core/src/virtualization/hvp.rs
printf '\n== crates/vm-cli/src/main.rs (relevant slice) ==\n'
sed -n '1,260p' crates/vm-cli/src/main.rsRepository: junyu0312/rust-vm
Length of output: 3528
🌐 Web query:
applevisor crate supported architectures macOS x86_64 aarch64 Hypervisor.framework
💡 Result:
The applevisor Rust crate provides safe bindings specifically for the Apple Silicon (aarch64) Hypervisor.framework [1][2]. According to its documentation and metadata, the crate is designed for and explicitly focuses on support for aarch64 (Apple Silicon) architectures [1][2]. It does not support x86_64 architectures because it acts as a wrapper for the Apple Silicon-specific Hypervisor.framework APIs [1][3]. Key points regarding its architecture support: - Primary Architecture: aarch64 (Apple Silicon) [1][2]. - Platform Limitation: The crate is intended for macOS on Apple Silicon [1][2]. - API Focus: It provides bindings to the Hypervisor.framework as it exists on Apple Silicon [1][3]. Consequently, it is incompatible with x86_64 macOS environments [1][2]. Developers using this crate must ensure their project is targeting the appropriate Apple Silicon hardware and that their binary has been granted the necessary com.apple.security.hypervisor entitlement to interact with the framework [3].
Citations:
- 1: https://docs.rs/applevisor/latest/applevisor/
- 2: https://docs.rs/crate/applevisor/latest/source/Cargo.toml
- 3: https://docs.rs/applevisor/latest/applevisor/index.html
Gate the macOS virtualization deps by target_arch = "aarch64". applevisor/applevisor-sys are Apple Silicon-only, but hvp is still compiled for every macOS target, so x86_64-apple-darwin will pull in the ARM-only path and fail the build. strum can move with the same gate since it’s only used there.
🤖 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 `@crates/vm-core/Cargo.toml` around lines 24 - 38, The macOS virtualization
dependencies are currently enabled for every macOS build, which pulls in Apple
Silicon-only crates on x86_64. Update the dependency gate in Cargo.toml so the
applevisor, applevisor-sys, and strum entries are only included when both
target_os is macos and target_arch is aarch64, and keep the existing
vm-core/vm-aarch64 related paths aligned with that architecture-specific gating.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores