Skip to content

feat: Fix acpi and cpuid #179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
junyu0312 merged 2 commits into from
Jun 19, 2026
Merged

feat: Fix acpi and cpuid #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
977b06d
fix: Introduce HW_REDUCED to avoid pm1a
junyu0312 Jun 19, 2026
f7146e3
fix: Fix cpuid
junyu0312 Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion crates/vm-core/src/virtualization/kvm/vcpu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ use crate::virtualization::vcpu::command::VcpuCommandRequest;
use crate::virtualization::vcpu::command::VcpuCommandResponse;
use crate::virtualization::vcpu::error::VcpuError;

#[cfg(target_arch = "x86_64")]
mod cpu_id;
mod vm_exit;

pub struct KvmVcpuInternal<'a> {
Expand Down Expand Up @@ -57,7 +59,17 @@ impl KvmVcpu {
) -> Result<Self, VcpuError> {
let mut vcpu_fd = vm_fd.create_vcpu(vcpu_id)?;
#[cfg(target_arch = "x86_64")]
vcpu_fd.set_cpuid2(supported_cpuid)?;
{
use crate::virtualization::kvm::vcpu::cpu_id::update_cpuid;

let cpuid = update_cpuid(
supported_cpuid,
vcpu_id
.try_into()
.map_err(|_| VcpuError::UpdateCpuid("vcpu_id too large"))?,
);
Comment on lines +65 to +70

@coderabbitai coderabbitai Bot Jun 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== KvmVcpu::new call sites =="
rg -nP -C3 '\bKvmVcpu::new\s*\(' --type rust

echo
echo "== vCPU count/id config symbols =="
rg -nP -C3 '\b(vcpu_id|vcpu_count|max_vcpus|num_vcpus|cpu_count)\b' --type rust

echo
echo "== Existing upper-bound checks related to 255/u8::MAX =="
rg -nP -C3 '\b(255|u8::MAX)\b' --type rust

echo
echo "== Other narrowing conversions on vcpu_id =="
rg -nP -C3 '\bvcpu_id\b.*\btry_into\s*\(|\bvcpu_id\b.*\bas\s*u8\b' --type rust

Repository: junyu0312/rust-vm

Length of output: 29867

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== VmConfig definition and vcpus field =="
rg -nP 'struct\s+VmConfig|vcpus\s*:' --type rust -B2 -A5

echo
echo "== Config validation logic =="
rg -nP 'validate|check.*vcpu|max.*vcpu' --type rust -B2 -A2

echo
echo "== Machine config or startup validation =="
fd -e rs -e toml | xargs rg -l 'max.*vcpu|validate.*config|vcpu.*limit' 2>/dev/null | head -5

echo
echo "== KVM create_vcpu constraints (to understand the u64 parameter) =="
rg -nP 'create_vcpu' --type rust -B1 -A3

Repository: junyu0312/rust-vm

Length of output: 9022

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== ACPI set_vcpus usage =="
rg -nP 'set_vcpus|\.vcpus' --type rust -B3 -A3

echo
echo "== x86_64 bootloader ACPI setup =="
cat -n crates/vm-bootloader/src/boot_loader/arch/x86_64.rs | sed -n '50,100p'

echo
echo "== Where ACPI is built for x86_64 =="
rg -nP 'AcpiTableBuilder|setup_acpi' --type rust -B2 -A5

Repository: junyu0312/rust-vm

Length of output: 18220

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Aarch64 bootloader load function (full) =="
cat -n crates/vm-bootloader/src/boot_loader/arch/aarch64.rs | sed -n '60,130p'

echo
echo "== Error types for vCPU validation =="
rg -nP 'VcpuExceedsAcpiCapability|VcpuError|enum.*Error' --type rust -B1 -A3 | head -60

Repository: junyu0312/rust-vm

Length of output: 6317

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== VM initialization order: bootloader vs vcpu creation =="
cat -n crates/vm-vmm/src/vm/config.rs | sed -n '90,140p'

echo
echo "== Bootloader load error path =="
rg -nP 'bootloader.*load|load.*bootloader' --type rust -B2 -A5

echo
echo "== Check if bootloader failure prevents vcpu creation =="
rg -nP 'bootloader|vcpu_manager' --type rust crates/vm-vmm/src/vm/config.rs -B1 -A1

Repository: junyu0312/rust-vm

Length of output: 7111

Confirm the new u8 vCPU-ID cap is enforced upstream.

This introduces a hard failure for vcpu_id >= 256. vCPU creation (lines 67-69) has no upstream validation—VmConfig::vcpus is unchecked and passed directly to the vCPU creation loop. For x86_64, the bootloader does validate vcpus after vCPU creation (redundant), and aarch64 has no validation outside this try_into. Consider adding config-level validation to fail earlier with a clearer error.

🤖 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/src/virtualization/kvm/vcpu.rs` around lines 65 - 70, Add
upstream validation in the VmConfig to enforce the u8 vCPU-ID cap before vCPU
creation occurs. The current code in the update_cpuid call relies on a try_into
conversion that fails at vCPU creation time if vcpu_id exceeds 255, but
VmConfig::vcpus has no validation to prevent this. Add a validation check when
VmConfig is created or validated to ensure vcpus does not exceed 256 (the
maximum value representable in u8), providing a clear configuration-level error
message rather than a runtime error during vCPU creation.

vcpu_fd.set_cpuid2(&cpuid)?;
}

let (command_tx, mut command_rx) = mpsc::channel(8);
let is_running = Arc::new(AtomicBool::new(false));
Expand Down
19 changes: 19 additions & 0 deletions crates/vm-core/src/virtualization/kvm/vcpu/cpu_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
use kvm_bindings::CpuId;

pub fn update_cpuid(cpuid: &CpuId, vcpu_id: u8) -> CpuId {
let mut cpuid = cpuid.clone();

for entry in cpuid.as_mut_slice() {
match entry.function {
// Version and Features
0x01 => {
entry.ebx &= 0xffffff;
// Update INITIAL_APIC_ID
entry.ebx |= (vcpu_id as u32) << 24;
}
_ => continue,
}
}

cpuid
}
3 changes: 3 additions & 0 deletions crates/vm-core/src/virtualization/vcpu/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ use crate::cpu::vm_exit::VmExitHandlerError;

#[derive(Error, Debug)]
pub enum VcpuError {
#[error("Failed to update cpuid, err: {0}")]
UpdateCpuid(&'static str),

#[error("Vcpu command channel disconnected")]
VcpuCommandDisconnected,

Expand Down
11 changes: 4 additions & 7 deletions crates/vm-device/src/device/dummy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ pub struct Dummy;

impl Dummy {
pub fn new(pio_allocator: &mut RangeAllocator<u16>) -> Result<Self, DeviceError> {
let _ = pio_allocator.reserve(0x1004, 1)?;
let _ = pio_allocator.reserve(0x1006, 1)?;
let _ = pio_allocator.reserve(0x87, 1)?;

Ok(Dummy)
Expand All @@ -33,12 +31,11 @@ impl Device for Dummy {

impl PioDevice for Dummy {
fn ports(&self) -> Vec<Range<u16>> {
let range = 0x87..0x88;

vec![
// acpi pm1a
0x1004..0x1005,
0x1006..0x1007,
// TODO
0x87..0x88,
// TODO: What's this
range,
]
}

Expand Down
22 changes: 11 additions & 11 deletions crates/vm-firmware/src/acpi/type/fadt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ use crate::acpi::r#type::common_header::CommonHeader;
use crate::acpi::r#type::generic_address_structure_format::GenericAddressStructureFormat;
use crate::acpi::utils::checksum;

// A zero indicates the power button is handled as a fixed feature programming model;
// a one indicates the power button is handled as a control method device.
// If the system does not have a power button, this value would be “1” and no power button device would be present.
const ACPI_FADT_POWER_BUTTON: u32 = 1 << 4; /* 04: [V1] Power button is handled as a control method device */
// A zero indicates the sleep button is handled as a fixed feature programming model;
// a one indicates the sleep button is handled as a control method device.
// If the system does not have a sleep button, this value would be “1” and no sleep button device would be present.
const ACPI_FADT_SLEEP_BUTTON: u32 = 1 << 5; /* 05: [V1] Sleep button is handled as a control method device */
const FADT_F_HW_REDUCED_ACPI: u32 = 1 << 20; /* 20: [V5] ACPI hardware is not implemented (ACPI 5.0) */

#[derive(Default, Immutable, IntoBytes)]
#[repr(C, packed)]
pub struct Fadt {
Expand Down Expand Up @@ -89,20 +99,10 @@ impl Fadt {
creator_id: CREATOR_ID,
creator_revision: CREATOR_REVISION,
},
flags: 0,
flags: ACPI_FADT_POWER_BUTTON | ACPI_FADT_SLEEP_BUTTON | FADT_F_HW_REDUCED_ACPI,
fadt_minor_version: 5, // ACPI 6.6 specification says it is 5.
x_dsdt,
hypervisor_vendor_id: HYPERVISOR_VENDOR_ID,
// TODO
pm1a_cnt_blk: 0x1000,
// TODO
pm1_evt_len: 16,
// TODO
pm1a_evt_blk: 0x1004,
// TODO
pm1_cnt_len: 32,
// TODO
sci_int: 9,
..Default::default()
};

Expand Down
Loading