petri: qemu backend - #4471
petri: qemu backend#4471Trevor Jones (tjones60) wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate build, artifact-resolution, and QEMU runtime issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds a Linux QEMU backend for AArch64 emulation in Petri, with multi-architecture artifact handling, validation tests, and CI integration.
Changes:
- Adds QEMU runtime, device configuration, and emulator tests.
- Refactors Petri logging, builder, backend, and initrd APIs.
- Extends artifact resolution, downloads, and CI workflows for multi-architecture QEMU tests.
File summaries
| File | Change summary | Final review comments |
|---|---|---|
vmm_tests/vmm_tests/tests/tests/ttrpc.rs |
Updates logging API usage. | — |
vmm_tests/vmm_tests/tests/tests/qemu.rs |
Adds QEMU boot, sharing, and VFIO validation tests. | nit · 3 votes — The guest-side output-file error should describe an inside-to-outside write rather than reading from outside. |
vmm_tests/vmm_tests/tests/tests/multiarch.rs |
Registers QEMU boot coverage. | — |
vmm_tests/vmm_tests/tests/tests/main.rs |
Includes the QEMU test module. | — |
vmm_tests/vmm_test_macros/src/lib.rs |
Adds QEMU macro/backend support. | — |
vmm_tests/prep_steps/src/main.rs |
Updates Petri test parameters. | — |
vmm_tests/petri_artifacts_vmm_test/src/lib.rs |
Declares the QEMU artifact. | — |
vmm_tests/petri_artifact_resolver_openvmm_known_paths/src/lib.rs |
Resolves QEMU and multi-architecture artifacts. | critical · 2 votes — Prefixing pipette paths with a target triple breaks Windows and root-level bundled pipette resolution; probe the multi-architecture layout before retaining the root/Cargo-target fallback. |
vmm_tests/cca_tests/tests/cca.rs |
Updates logging API usage. | — |
tmk/tmk_tests/src/lib.rs |
Updates logging API usage. | critical · 2 votes — The Windows-only block still accesses params.logger; change it to params.log_source so tmk_tests compiles on Windows. |
petri/src/vm/qemu/mod.rs |
Implements the QEMU backend and runtime handling. | moderate · 3 votes — Restrict compatibility checks to Linux, AArch64, and Firmware::LinuxDirect.moderate · 1 vote — Ensure spawned QEMU processes are killed and reaped on later error paths.moderate · 3 votes — Reap QEMU after killing it during teardown.critical · 1 vote — Replace public-operation todo!() calls for update_command_line, reset, and get_guest_state_file with implementations or explicit unsupported errors; use Ok(None) for guest state. Also applies at lines 312, 320, and 324.critical · 2 votes — Avoid panicking in run_without_agent() when no initrd was prepared; return an error or support the original firmware initrd.nit · 3 votes — Identify the pipe as qemu stderr, not qemu stdout.nit · 1 vote — Correct the “Add a devices” grammar in the documentation.nit · 2 votes — Correct the “topologies” spelling. |
petri/src/vm/qemu/devices.rs |
Adds QEMU device configuration types. | — |
petri/src/vm/openvmm/mod.rs |
Updates OpenVMM backend capabilities and init handling. | — |
petri/src/vm/openvmm/construct.rs |
Propagates prebuilt-initrd rdinit parameters. |
— |
petri/src/vm/mod.rs |
Refactors shared resources, initrd APIs, and backend traits. | critical · 2 votes — Update the exhaustive Hyper-V PetriVmResources destructure for the new prebuilt_initrd field.nit · 3 votes — Update documentation describing prepare_initrd() to reference PetriInitrd, rdinit_param, and with_prebuilt_initrd.nit · 3 votes — Rename the misspelled public type NoPetriVmFramebufferAcces.moderate · 1 vote — Include the backend custom init script when preparing prebuilt initrds so QEMU receives the correct connection configuration. |
petri/src/vm/hyperv/mod.rs |
Updates the Hyper-V backend implementation. | — |
petri/src/test.rs |
Renames the log-source parameter. | — |
petri/src/requirements.rs |
Adds QEMU requirements. | — |
petri/burette/src/tests/virtio_fs.rs |
Adapts minimal builder usage. | — |
petri/burette/src/tests/scale_boot.rs |
Uses the new initrd type. | — |
petri/burette/src/tests/network.rs |
Adapts minimal builder usage. | — |
petri/burette/src/tests/memory.rs |
Uses the new initrd type. | — |
petri/burette/src/tests/disk_io.rs |
Adapts minimal builder usage. | — |
petri/burette/src/tests/boot_time.rs |
Uses the new initrd and builder APIs. | — |
flowey/flowey_lib_hvlite/src/init_vmm_tests_content_dir.rs |
Handles selected multi-architecture artifacts. | moderate · 3 votes — The AArch64 initrd request is incorrectly gated on x64 selection, so QEMU-only filters can omit the guest initrd.moderate · 1 vote — The x64 Linux artifact builder does not publish the required AArch64 pipette; wire it into the builder and shared producer.moderate · 1 vote — Create every selected architecture directory before copying AArch64 initrd or Image files. |
flowey/flowey_lib_hvlite/src/download_vmm_tests_built_artifacts.rs |
Downloads architecture-specific pipette artifacts. | — |
flowey/flowey_lib_hvlite/src/_jobs/local_build_and_run_nextest_vmm_tests.rs |
Builds selected test artifacts. | — |
flowey/flowey_lib_hvlite/src/_jobs/download_and_run_nextest_vmm_tests.rs |
Updates downloaded artifact selections. | — |
flowey/flowey_lib_hvlite/src/_jobs/consume_and_test_nextest_vmm_tests_archive.rs |
Passes prebuilt artifact selections. | — |
flowey/flowey_hvlite/src/pipelines/vmm_tests_run.rs |
Resolves QEMU and architecture-specific requirements. | — |
flowey/flowey_hvlite/src/pipelines/checkin_gates.rs |
Configures CI artifact selections. | — |
ci-flowey/openvmm-pr.yaml |
Updates generated Flowey execution steps. | — |
.github/workflows/openvmm-pr.yaml |
Updates generated PR workflow steps. | — |
.github/workflows/openvmm-pr-release.yaml |
Updates generated release workflow steps. | — |
.github/workflows/openvmm-ci.yaml |
Updates generated CI workflow steps. | — |
Review details
Suppressed comments (8)
flowey/flowey_lib_hvlite/src/init_vmm_tests_content_dir.rs:728
- The x64 Linux CI artifact builder is used by the
x64-linux-*jobs, and the newqemu_linux_direct_aarch64test requiresPIPETTE_LINUX_AARCH64. This builder only publishespipette_linux_musl_x64, so the QEMU test's guest pipette is absent and content initialization cannot satisfy the artifact. Wire the AArch64 pipette into this builder and its shared artifact producer.
pipette_linux_musl_x64 => PipetteOutput,
flowey/flowey_lib_hvlite/src/init_vmm_tests_content_dir.rs:644
- QEMU tests are x64 test binaries that boot an AArch64 guest, so both architecture-specific directories can be selected while
arch_diris only the test target's directory. After the initrd selection above is corrected, copyingaarch64/initrdoraarch64/Imagewill fail because that directory was never created; create every selected architecture directory before these copies.
fs_err::create_dir_all(test_content_dir.join(arch_dir))?;
petri/src/vm/mod.rs:570
prepare_initrd()is the public API used withwith_prebuilt_initrd, but it always omits the backend init script. For QEMU this yieldsrdinit=/pipette, so pipette defaults to vsock while the backend only exposes TCP forwarding; using the prebuilt-initrd optimization then cannot connect. Use the backend's custom-init-script hook here (OpenVMM and Hyper-V returnNone).
self.prepare_custom_initrd(|_| None)
petri/src/vm/qemu/mod.rs:163
- After
spawn, every later?can return while thestd::process::Childis not owned by a kill-on-drop guard. Since droppingChilddoes not terminate it, failures creatingPolledChildor the logging pipes, and test failures after startup, can leave an orphaned QEMU process. Add an ownership/drop guard or ensure every error path kills and reaps it.
let mut qemu_process = cmd.spawn().context("failed to launch QEMU")?;
let qemu_stdout = qemu_process.stdout.take().expect("stdout should be piped");
let qemu_stderr = qemu_process.stderr.take().expect("stderr should be piped");
let qemu_process = PolledChild::<std::process::Child>::new(driver, qemu_process)
petri/src/vm/qemu/mod.rs:324
get_guest_state_fileis an optional capability whose trait default returnsOk(None), but this backend replaces that behavior with a panic. ReturnOk(None)for QEMU, which has no guest-state file.
todo!()
petri/src/vm/qemu/mod.rs:312
- This placeholder panics whenever a caller uses the supported
PetriVmRuntime::update_command_lineAPI with QEMU. Other unsupported QEMU operations return an error, so return an explicit unsupported-operation error here instead of aborting the test process.
todo!()
petri/src/vm/qemu/mod.rs:320
- This placeholder panics whenever a caller uses the required
PetriVmRuntime::resetAPI with QEMU. Since reset is not implemented, return an error consistent with the other unsupported operations rather than aborting the test process.
todo!()
petri/src/vm/qemu/mod.rs:208
- The new doc comment has a grammatical error: it should say that the method adds devices, not "Add a devices".
/// Add a devices to the emulator configuration.
- Files reviewed: 35/35 changed files
- Comments generated: 13
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical CI artifact issues and backend/API correctness issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (19)
Previously missed (3) — in code that hasn't changed since the last review.
petri/src/vm/qemu/mod.rs:68
QemuPetriRuntimeowns a rawPolledChild, but it has noDropcleanup. Any test failure or panic that dropsPetriVmbefore callingteardowntherefore leaves QEMU running, orphaning the emulator and its forwarded port; the existing ttrpc process wrapper explicitly kills and reaps children on drop. Add an RAII kill/reap guard for this process.
This issue also appears on line 223 of the same file.
petri/src/vm/qemu/mod.rs:81
qemu-system-aarch64can only boot the AArch64 Linux-direct configuration represented bybuild_qemu_command, but this check accepts x64 guests, non-Linux hosts, and non-Linux-direct firmware. Those configurations resolve successfully and fail later at launch (or try to use a Linux-only artifact). Restrict compatibility to Linux hosts, AArch64 guests, andFirmware::LinuxDirect.
petri/src/vm/qemu/mod.rs:109- The shared test initrd relies on BusyBox applets being installed before invoking commands (the existing serial-agent and incubator init scripts do this), but this new script calls
ipandroutedirectly. On a fresh initrd those applets may not exist, leaving the interface unconfigured while pipette still starts;wait_for_agentthen retries until its timeout. Install the BusyBox applets before the network setup (or reuse the complete incubator setup).
flowey/flowey_lib_hvlite/src/init_vmm_tests_content_dir.rs:644
- Only the target architecture directory is created here, but this PR now copies both x64 and AArch64 kernels/initrds whenever both selections are enabled (as the CI pipelines do). On an AArch64 target the x64 copy, or on an x64 target the AArch64 copy, fails because its parent directory does not exist.
let arch_dir = match arch {
CommonArch::X86_64 => "x64",
CommonArch::Aarch64 => "aarch64",
};
fs_err::create_dir_all(test_content_dir.join(arch_dir))?;
petri/src/vm/mod.rs:2315
- The new public type name is misspelled as
NoPetriVmFramebufferAcces; it is missing the finalsinAccess. Rename the type and its uses so the associated API does not expose a typo.
pub struct NoPetriVmFramebufferAcces;
petri/src/vm/mod.rs:569
- The
prepare_initrddocumentation still says this method returns a path and that callers should pass the path towith_prebuilt_initrd, but the signature now returnsPetriInitrdand the builder accepts that struct. Update the public API docs to describe the returned path plusrdinit_param; otherwise the documented usage no longer compiles.
pub fn prepare_initrd(&self) -> anyhow::Result<PetriInitrd> {
petri/src/vm/mod.rs:341
- Adding
prebuilt_initrdas a requiredPetriVmResourcesfield leaves the Hyper-V backend'slet PetriVmResources { driver, log_source } = resources;pattern incomplete, so the workspace fails to compile with a missing-field error. Update that destructuring to include the field or use...
pub prebuilt_initrd: Option<PetriInitrd>,
petri/src/vm/qemu/mod.rs:226
- This teardown path sends
killbut never waits for the child afterward. When teardown is used while QEMU is still running (for example after a test error), the killed process can remain unreaped as a zombie; the existing incubator cleanup waits after killing its QEMU child. Reap the child here while handling the already-exited case.
qemu_process
.get_mut()
.kill()
.context("unable to kill qemu process")?;
petri/src/vm/qemu/mod.rs:312
- This
todo!()panics if a caller uses the public runtime API to update QEMU's command line. Unsupported backend operations elsewhere return ananyhowerror, so this should report that QEMU does not support the operation instead of unwinding the test process.
todo!()
petri/src/vm/qemu/mod.rs:428
- The new TODO comment spells
topologisincorrectly.
// TODO: more complex CPU topologis
petri/src/vm/qemu/mod.rs:320
- This
todo!()panics when a generic Petri test requests a QEMU reset. Return an explicit unsupported-operation error (or implement reset) so an unsupported capability is reported as a test failure rather than an unconditional panic.
todo!()
petri/src/vm/qemu/mod.rs:324
- QEMU has no guest state file, but overriding the trait default with
todo!()makes a normal query panic. Preserve the trait's unsupported-backend behavior by returningOk(None).
todo!()
petri/src/vm/qemu/mod.rs:178
- This error context is attached to the stderr pipe, but says QEMU stdout. If stderr setup fails, the diagnostic points at the wrong stream; change the context to identify stderr.
.context("failed to create polled pipe for qemu stdout")?;
petri/src/vm/qemu/mod.rs:208
- The public method documentation has the grammatical error
Add a devices.
/// Add a devices to the emulator configuration.
petri/src/vm/qemu/mod.rs:52
- This adds a public Petri VMM backend and a new emulator test path, but
Guide/src/dev_guide/tests/vmm.mdcurrently documents the existing VMM test workflow without QEMU or the nested/emulator limitations and usage. Please update that guide (or link a follow-up) so test authors can discover and use the new backend.
/// The QEMU Petri backend
#[derive(Debug)]
pub struct QemuPetriBackend {
petri/src/vm/qemu/mod.rs:164
- After QEMU is spawned, failures creating
PolledPipeor log files return before aQemuPetriRuntimeis constructed. The localPolledChildis then dropped without killing the process, so setup errors can also leave an orphaned emulator; use a kill-and-reap guard around post-spawn initialization.
let mut qemu_process = cmd.spawn().context("failed to launch QEMU")?;
let qemu_stdout = qemu_process.stdout.take().expect("stdout should be piped");
let qemu_stderr = qemu_process.stderr.take().expect("stderr should be piped");
let qemu_process = PolledChild::<std::process::Child>::new(driver, qemu_process)
.context("failed to create PolledChild")?;
tmk/tmk_tests/src/lib.rs:226
- The
PetriTestParamsfield was renamed tolog_source, but the Windowshyperv_openhcl_tmksimplementation below still readsparams.logger(line 263). That leaves the Windows build referring to a nonexistent field; update that remaining use as part of this rename.
let logger = params.log_source.clone();
vmm_tests/petri_artifact_resolver_openvmm_known_paths/src/lib.rs:351
- The resolver now looks for pipette at
<repo>/<target>/pipettewhenVMM_TESTS_CONTENT_DIRis unset, whereas the previous fallback was Cargo'starget/<target>/<profile>/pipette. Direct Petri/burette runs that rely on the documented fallback can no longer find their locally built pipette; retain the Cargo-target fallback after checking the test-content layout.
match get_path(
".",
PathBuf::from(&target).join(binary),
vmm_tests/vmm_tests/tests/tests/qemu.rs:87
- This context describes the opposite operation:
agent.write_fileis writing from the guest to the host share, not reading from outside into the guest. The misleading message makes failures in the new round-trip check difficult to diagnose.
- Files reviewed: 35/35 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate review findings remain in artifact selection, runtime behavior, and security.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (11)
Previously missed (2) — in code that hasn't changed since the last review.
petri/src/vm/qemu/mod.rs:68
- The runtime owns a raw
PolledChildwithout aDropguard. Petri catches test errors and panics without guaranteeing thatteardownruns, andstd::process::Childdeliberately does not kill its process on drop; errors during the fallible setup afterspawncan leak it before this runtime is even returned. Wrap the child in an RAII kill-and-reap guard immediately after spawning, as the ttrpc test harness does.
petri/src/vm/qemu/mod.rs:112 rdinit=/custom-init.shruns this as PID 1, bypassing the initrd's normal/init. Unlike the existing QEMU init script inpetri/incubator/src/qemu.rs, this script does not install busybox commands or mount/dev,/proc, and/sys(and does not create/etc). Theipcommands, the latermount -t 9p, and the resolv.conf write used by the new test can consequently fail before pipette starts; add the basic initrd setup before configuring networking.
flowey/flowey_lib_hvlite/src/init_vmm_tests_content_dir.rs:650
- Only the directory for the target architecture is created, but the selections can request both x64 and AArch64 kernels/initrds. For the x64 job, the AArch64 copy below therefore fails because
test_content_dir/aarch64does not exist (and the reverse occurs for an AArch64 target). Create both architecture directories or each copy's parent.
fs_err::create_dir_all(test_content_dir.join(arch_dir))?;
petri/src/vm/qemu/devices.rs:59
- This field's documentation promises that
vfio: truebinds the device after boot, butQemuPetriBackendonly emits the QEMU device command and never performs VFIO provisioning. The only binding implementation is the test-localvalidate_vfio_deviceshelper, so callers usingwith_devicescannot rely on the documented behavior or obtain an assigned device for nested tests. Implement the runtime provisioning path or remove/reword this flag until it exists.
/// If true, bind the device to vfio-pci after boot, making it available
/// for passthrough into the L2 guest.
pub vfio: bool,
petri/src/vm/qemu/mod.rs:234
- When teardown is called while QEMU is still running (for example after a test error), this sends the kill signal and then drops the child without waiting for it. That leaves the process unreaped as a zombie; the repository's
OpenvmmChildexplicitly waits after killing. Check whether the child has already exited, kill it if necessary, and awaitqemu_process.wait()after a kill.
let mut qemu_process = self.qemu_process.lock().await;
qemu_process
.get_mut()
.kill()
.context("unable to kill qemu process")?;
petri/src/vm/qemu/mod.rs:240
allow_resetis ignored, and-no-rebootmakes a guest reboot terminate QEMU rather than restart it.PetriVmRuntimerequireswait_for_halt(true)to restart on reset, soPetriVm::wait_for_resetwill report a normal halt/error instead of completing for this backend. Implement the restart path or reject reset explicitly rather than silently treating it as a halt.
async fn wait_for_halt(&mut self, _allow_reset: bool) -> anyhow::Result<PetriHaltReasonDetail> {
let status = self.qemu_process.lock().await.wait().await?;
Ok(PetriHaltReasonDetail {
petri/src/vm/qemu/mod.rs:320
todo!()is reachable through the publicPetriVm::update_command_lineAPI and will panic for QEMU, unlike the other backends' unsupported-operation errors. Return an error (or implement the operation) so a test cannot panic the harness.
async fn update_command_line(&mut self, _command_line: &str) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:328
resetis another public runtime operation implemented astodo!(), so callingPetriVm::wait_for_resetor a direct reset path can panic instead of returning an unsupported-operation result. Replace this with a real QEMU reset or an error.
async fn reset(&mut self) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:332
- The trait's default implementation for
get_guest_state_filereturnsOk(None)when a backend has no state file. Overriding it withtodo!()makes a harmless capability query panic for QEMU; returnOk(None)or remove the override.
async fn get_guest_state_file(&self) -> anyhow::Result<Option<PathBuf>> {
todo!()
petri/src/vm/qemu/mod.rs:409
- The listener is dropped before QEMU is spawned, so concurrent QEMU Petri VMs can select the same ephemeral port and one of their
hostfwdbindings will fail. Coordinate allocation with launch or retry on an address-in-use failure.
std::net::TcpListener::bind("127.0.0.1:0").context("failed to bind ephemeral port")?;
let port = listener
.local_addr()
.context("failed to get local addr")?
.port();
vmm_tests/petri_artifact_resolver_openvmm_known_paths/src/lib.rs:350
- Changing the lookup to only
<target>/pipettefixes the new Flowey content layout but removes the existing fallbacks. Cargo-built pipette is undertarget/<target>/<profile>/pipette, whileburette cmd_packagestages the packaged pipette at the bundle root viaresolve_bundle_name; with no Flowey target-subdirectory, normal local/perf runs therefore fail to resolve pipette. Probe the new content-dir path first, then preserve the Cargo/package fallback or update the bundle layout together.
match get_path(
".",
PathBuf::from(&target).join(binary),
- Files reviewed: 35/35 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect artifact selection, backend compatibility, networking, and process cleanup.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (18)
Previously missed (4) — in code that hasn't changed since the last review.
petri/src/vm/mod.rs:579
prepare_initrdalways returnsrdinit=/pipette, but QEMU's TCP setup is supplied only byT::build_custom_init_scriptduring the automatic preparation inrun_core. A caller following the documented prepare/with_prebuilt_initrdpath for a QEMU builder will therefore start pipette with its default transport instead of TCP, sowait_for_agentcannot connect.
petri/src/vm/qemu/mod.rs:81check_compatonly checks the host OS/CPU and ignores both the guest architecture and firmware, so a configuration such asqemu_linux_direct_x64reachesnewand panics at theassert_eq!(arch, Aarch64), while UEFI/PCAT variants reach a backend that cannot boot them. Restrict compatibility to the supported Linux-direct AArch64 guest (or return a normal error for unsupported configurations) before constructing the backend.
petri/src/vm/qemu/mod.rs:108- The custom
rdinitscript invokesipbefore installing the BusyBox applets. The shared test initrd is used elsewhere with an explicit/bin/busybox --install /bininitialization (petri/src/linux_direct_serial_agent.rs:10-10andpetri/incubator/src/qemu.rs:180-196); without it,ipis not guaranteed to exist as a command, so the network setup fails and the TCP pipette connection times out.
petri/src/vm/qemu/mod.rs:233 - Dropping a
PetriVmon an error path drops thisstd::process::Child, butChilddoes not kill the process when dropped. VMM tests commonly return early with?after boot, so a failed QEMU test can leave an orphaned emulator and its guest running; add a synchronous child guard/Dropcleanup (including the setup path beforeQemuPetriRuntimeis returned), as the ttrpc test does for its child process.
flowey/flowey_lib_hvlite/src/init_vmm_tests_content_dir.rs:482
- The old Linux-pipette path explicitly called
make_executable()after copying. The shared helper now omits that step, so a pipeline artifact whose executable bit was not preserved is copied as a non-executablepipetteand cannot be launched from the content directory.
fs_err::copy(&src, &dst)?;
petri/src/vm/hyperv/mod.rs:89
- Removing these firmware checks now lets
hyperv_linux_direct_x64passPetriVmArtifacts::neweven though the Hyper-V backend has no Linux-direct kernel/initrd launch path; the macro parser accepts that configuration and it used to be rejected here. Restore the Linux-direct (and AArch64 PCAT) compatibility checks, or reject those invalid VMM/config combinations during parsing, so they do not proceed into an unusable Hyper-V VM.
fn check_compat(_firmware: &Firmware, arch: MachineArch) -> bool {
arch == MachineArch::host()
}
petri/src/vm/mod.rs:10
- The new public Petri backend and
qemu_...test configurations are not reflected inGuide/src/dev_guide/tests/vmm.md, which is the repository's documented entry point for Petri VMM tests and currently lists only the existing backends/targets. Please update that guide (including QEMU's Linux-host/AArch64-guest limitation and artifact setup) so the new backend is discoverable and the supported-platform information stays accurate.
/// QEMU full machine emulation management
pub mod qemu;
petri/src/vm/mod.rs:573
- This public return type is
PetriInitrdcontaining a temporaryTempPath, not aTempFiletype. The wording is misleading for callers deciding what must be kept alive.
/// Returns a [`PetriInitrd`] struct that contains a `TempFile` that
/// must not be dropped until after the VM boots.
petri/src/vm/qemu/mod.rs:318
- These
todo!()implementations panic when a caller uses a supported runtime API such as command-line update, reset, or guest-state access. The other unsupported QEMU operations returnanyhowerrors, so these should do the same rather than aborting the test process; the same replacement is needed for theresetandget_guest_state_filemethods immediately below.
async fn update_command_line(&mut self, _command_line: &str) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:330
- The runtime trait explicitly allows backends without a guest-state file to return
Ok(None), but this implementation panics withtodo!()when queried. ReturnOk(None)for QEMU rather than crashing the test process.
async fn get_guest_state_file(&self) -> anyhow::Result<Option<PathBuf>> {
todo!()
petri/src/vm/qemu/mod.rs:327
- This unsupported operation uses
todo!(), sowait_for_reseton a QEMU VM panics instead of returning a backend capability error. Match the other QEMU methods and returnanyhow::bail!so a test can handle unsupported reset behavior without taking down the runner.
async fn reset(&mut self) -> anyhow::Result<()> {
todo!()
}
petri/src/vm/qemu/mod.rs:452
- The QEMU command line replaces the guest kernel command line with only
rdinit. Since the backend also attaches-serial mon:stdio, omitting the AArch64 serial console means kernel/init-script diagnostics never reach theguestlog (the incubator profile usesconsole=ttyAMA0). Include the console arguments here.
cmd.arg("-append").arg(format!("rdinit={rdinit_param}"));
petri/src/vm/qemu/mod.rs:232
- This kills QEMU but never waits for it. On failure paths the child therefore remains unreaped and can accumulate as a zombie; the existing QEMU/incubator cleanup pattern kills and then waits on the child.
qemu_process
.get_mut()
.kill()
.context("unable to kill qemu process")?;
petri/src/vm/qemu/mod.rs:118
- This new comment has a spelling error:
bachendshould bebackend.
// QEMU bachend only supports linux X64 with an aarch64 guest
petri/src/vm/qemu/mod.rs:214
- This new documentation comment has an extra article:
Add a devicesshould beAdd devices.
/// Add a devices to the emulator configuration.
petri/src/vm/qemu/mod.rs:227
- The log tasks are cancelled before the QEMU process is stopped or reaped. On normal shutdown,
wait_for_haltmay have observed exit while the pipe tasks are still draining, so this can discard the tail of guest/stderr logs and make failures harder to diagnose. Stop/reap QEMU first, then await the logging tasks as the incubator backend does.
async fn teardown(mut self) -> anyhow::Result<()> {
futures::future::join_all(self.log_tasks.into_iter().map(|t| t.cancel())).await;
vmm_tests/petri_artifact_resolver_openvmm_known_paths/src/lib.rs:350
- The fallback path was changed from Cargo's
target/{triple}/{profile}/pipettelayout to{triple}/pipetteat the repository root. WhenVMM_TESTS_CONTENT_DIRis not set, local known-path resolution can no longer find either Linux or Windows pipette builds; keep the content-dir lookup but restore the existing Cargo fallback.
match get_path(
".",
PathBuf::from(&target).join(binary),
vmm_tests/petri_artifact_resolver_openvmm_known_paths/src/lib.rs:350
- The new pipette lookup now expects
<target-triple>/pipettein the content/bundle root, butresolve_bundle_namestill maps both Linux pipette artifacts to the single pathpipette. A self-contained bundle containing both architectures can therefore not satisfy both artifact handles. Update the per-architecture bundle names together with this lookup.
".",
PathBuf::from(&target).join(binary),
- Files reviewed: 35/35 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved QEMU lifecycle, compatibility, port allocation, and artifact-handling issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (14)
Previously missed (1) — in code that hasn't changed since the last review.
petri/src/vm/qemu/mod.rs:68
- Storing the raw
PolledChildleaves QEMU unmanaged once this runtime is dropped. A test panic, an error aftercmd.spawn()but before this runtime is returned, or an early teardown can leave an orphaned/zombie QEMU; use a kill-and-reap drop guard likeOpenvmmChildand retain it during startup error paths.
This issue also appears on line 228 of the same file.
flowey/flowey_lib_hvlite/src/init_vmm_tests_content_dir.rs:266
- The QEMU resolver's architecture argument is the host architecture, but
archhere comes fromvmm_tests_targetand denotes the target/guest architecture. On an x86_64 Linux runner building the AArch64 target this requests an AArch64 QEMU binary, which cannot execute on the host; usectx.arch()as the incubator path does.
let qemu_system_aarch64 = prebuilt_artifacts.qemu_system_aarch64.then(|| {
ctx.reqv(|v| {
crate::resolve_openvmm_qemu::Request::Get(
crate::resolve_openvmm_qemu::QemuFile::SystemAarch64,
arch,
petri/src/vm/hyperv/mod.rs:89
- The compatibility check no longer rejects
Firmware::LinuxDirect, but Hyper-V's run path only configures a firmware/IGVM VM and never passes a Linux kernel or initrd to Hyper-V. Ahyperv_linux_direct_*configuration now reaches this backend and fails with an invalid VM setup instead of being rejected as before; restore the LinuxDirect guard (and the existing AArch64 PCAT guard if that combination remains unsupported).
fn check_compat(_firmware: &Firmware, arch: MachineArch) -> bool {
arch == MachineArch::host()
}
petri/src/vm/mod.rs:573
TempFileis not the type returned here;prepare_initrdreturns aPetriInitrdcontaining aTempPath. This rustdoc should link the actual lifetime-bearing type so callers understand what must remain alive.
/// Returns a [`PetriInitrd`] struct that contains a `TempFile` that
/// must not be dropped until after the VM boots.
petri/src/vm/mod.rs:10
- This adds a new Petri VMM backend, but the VMM test guide still documents the macro as covering Hyper-V and OpenVMM only (
Guide/src/dev_guide/tests/vmm.md:21-25). Update the guide with QEMU's supported host/guest combination and backend selection so the public test-framework documentation matches the new behavior.
pub mod qemu;
petri/src/vm/qemu/mod.rs:80
- This predicate ignores both the firmware and guest architecture, while
newasserts AArch64 andbuild_qemu_commandonly accepts LinuxDirect. Consequently,qemu_x64orqemu_openhcl_*configurations can passcheck_compatand then panic or fail with a missing initrd; check those constraints here.
fn check_compat(_firmware: &Firmware, _arch: MachineArch) -> bool {
// Our QEMU bachend only supports linux X64 at this time
MachineArch::X86_64 == MachineArch::host() && cfg!(target_os = "linux")
petri/src/vm/qemu/mod.rs:318
- This
todo!()panics if a test uses the generic command-line update API with QEMU. Other unsupported backend operations return ananyhowerror, so return an explicit unsupported-operation error here instead of aborting the test process.
async fn update_command_line(&mut self, _command_line: &str) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:326
- This
todo!()panics if a test requests a reset through the generic Petri API. QEMU does not implement reset, so report that limitation as an error like the other unsupported operations rather than panicking.
async fn reset(&mut self) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:330
PetriVmRuntime::get_guest_state_filealready definesOk(None)as the default for backends without a state file. Leavingtodo!()here makes a normal capability query panic for QEMU; returnOk(None)instead.
async fn get_guest_state_file(&self) -> anyhow::Result<Option<PathBuf>> {
todo!()
petri/src/vm/qemu/mod.rs:232
wait_for_clean_teardownwaits for QEMU before calling this method, so this unconditionalkillcan run after the child has already exited. On failure paths it also kills the child without waiting/reaping it. Checktry_waitbefore killing and awaitPolledChild::wait()after a kill, as the existing process guards do, to avoid PID-reuse hazards and zombie children.
let mut qemu_process = self.qemu_process.lock().await;
qemu_process
.get_mut()
.kill()
.context("unable to kill qemu process")?;
petri/src/vm/qemu/mod.rs:156
- When a test uses
noagent/run_without_agent,run_coredeliberately does not prepare a prebuilt initrd because no pipette binary is requested. This unconditional requirement therefore makes every no-agent QEMU configuration fail before launch; either boot the original firmware initrd in that mode or explicitly reject this configuration.
prebuilt_initrd
.as_ref()
.context("QEMU requires a prebuilt initrd")?,
petri/src/vm/qemu/mod.rs:118
- The comment says
bachend; correct this typo while touching the backend description.
// QEMU bachend only supports linux X64 with an aarch64 guest
petri/src/vm/qemu/mod.rs:214
- This rustdoc has a grammatical error: "Add a devices" should be "Add devices" or "Add a device".
/// Add a devices to the emulator configuration.
vmm_tests/vmm_test_macros/src/lib.rs:315
- This adds QEMU as a third public Petri VMM, but
Guide/src/dev_guide/tests/vmm.md:21-25still describes the macro as supporting VMMs including only Hyper-V. Update the VMM test guide to mention the QEMU backend and its Linux/architecture limitations so test authors can discover the new form.
- Files reviewed: 35/35 changed files
- Comments generated: 1
- Review effort level: Lite
|
|
||
| /// This test is a step toward full native support testing openvmm using | ||
| /// the qemu aarch64 system emulator. It demonstrates that petri can setup | ||
| /// the same emulator that incubator creates in our existing tests. |
There was a problem hiding this comment.
we're actually booting a vm here?
There was a problem hiding this comment.
We are just booting the emulated machine here and setting up all the emulated devices, but not launching openvmm nested yet.
| /// This test is a step toward full native support testing openvmm using | ||
| /// the qemu aarch64 system emulator. It demonstrates that petri can setup | ||
| /// the same emulator that incubator creates in our existing tests. | ||
| #[vmm_test_with(qemu, configs(linux_direct_aarch64))] |
There was a problem hiding this comment.
how does the shorthand vs the vmm_tests_with work? like i imagine we will have:
qemu
qemu-cca
fvp
fvp-cca
is the first qualifier the "backend" flavor, and the 2nd is the test?
There was a problem hiding this comment.
I don't think there should be multiple qemu or fvp backends. Whether CCA is enabled should be a configuration option. It might even make sense to have it be a backend-agnostic option that would enable the appropriate hardware on the emulator or enable confidential VM extensions to be forwarded to the guest for VMMs (for nested SNP tests, for example).
There was a problem hiding this comment.
To clarify, whether CCA is enabled in the L1 (emulator or VM) should be a configuration option. Whether CCA is enabled in the L2 will be part of the config in the nested test, like TDX/SNP/VBS is selected today.
| Ok(()) | ||
| } | ||
|
|
||
| /// Set up VFIO devices. |
There was a problem hiding this comment.
this is setting it up in the "host" right?
There was a problem hiding this comment.
Yes.
| overrides.vmm = match ident_string.as_str() { | ||
| "openvmm" => Some(Vmm::OpenVmm), | ||
| "hyperv" => Some(Vmm::HyperV), | ||
| "qemu" => Some(Vmm::Qemu), |
There was a problem hiding this comment.
i do wonder if we should call this "qemu-tcg" because just "qemu" could mean qemu as a virtstack? we want to make it clear this is an emulated cpu i think?
There was a problem hiding this comment.
I think it is ok that it is ambiguous. From the docs, it looks like using QEMU as a VMM involves passing -accel whpx to the same qemu-system-<guest_arch> binary. Whether the CPU is emulated or "accelerated" using a hypervisor seems like something that should just be a backend-specific config parameter. Maybe we could add the option with only one enum variant for now to make it more clear?
|
|
||
| fn qemu_system_aarch64_path() -> anyhow::Result<PathBuf> { | ||
| get_path( | ||
| ".packages/underhill-deps-private", |
There was a problem hiding this comment.
why is this deps private, i thought we have it under openvmm-deps or do i misunderstand something here
There was a problem hiding this comment.
I think this is something leftover from pre-oss days but I'm not sure. I plan to refactor and simplify all of this to mostly get rid of these magic paths.
| @@ -0,0 +1,527 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
| // Licensed under the MIT License. | |||
|
|
|||
There was a problem hiding this comment.
🟡 Changes recommended
Critical extraction, boot initialization, and teardown issues, along with unresolved backend/runtime issues, remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (13)
Previously missed (3) — in code that hasn't changed since the last review.
petri/src/vm/qemu/mod.rs:66
- Once
run()has returned, a test error or panic dropsPetriVmwithout calling this runtime's asyncteardown. BecausePolledChild/std::process::Childhas no drop-time kill, the QEMU process can be left orphaned; add a kill-and-reap guard for failure paths.
petri/src/vm/qemu/mod.rs:81 - This compatibility check ignores the guest architecture, so
qemu_linux_direct_x64on an x86_64 Linux host returns true and thennew()panics at theassert_eq!(arch, MachineArch::Aarch64)below. Include the supported AArch64 guest constraint here so unsupported configurations are skipped before backend construction.
petri/src/vm/qemu/mod.rs:118 - Typo:
bachendshould bebackend.
petri/src/vm/hyperv/mod.rs:89
- The new check now accepts plain
Firmware::LinuxDirect, but the Hyper-V backend still discardsprebuilt_initrdandHyperVNewCustomVMArgs::from_configdoes not provide the Linux kernel/initrd to Hyper-V. Ahyperv_linux_direct_*configuration therefore reaches an unsupported boot path instead of being rejected; exclude the non-OpenHCL Linux-direct variant or implement its boot support.
fn check_compat(_firmware: &Firmware, arch: MachineArch) -> bool {
arch == MachineArch::host()
}
petri/src/vm/mod.rs:578
- The return type is now
PetriInitrd, but the surrounding public documentation still tells callers to pass a path towith_prebuilt_initrd. Update the wording to say that callers pass the returnedPetriInitrdvalue.
/// Call this once before timing, then pass the path to
/// [`with_prebuilt_initrd`](Self::with_prebuilt_initrd) for each
/// iteration.
pub fn prepare_initrd(&self) -> anyhow::Result<PetriInitrd> {
petri/src/vm/mod.rs:10
- The new public QEMU Petri backend and
qemutest selector are not described in the existing VMM test guide, which is where Petri backends and test authoring are documented. Please updateGuide/src/dev_guide/tests/vmm.mdwith the backend's supported host/guest combinations and setup/artifact requirements, or track that documentation follow-up explicitly.
pub mod qemu;
petri/src/vm/qemu/devices.rs:59
- These
vfioflags are documented as causing the QEMU backend to bind devices tovfio-pci, butQemuPetriBackend::runnever consumes them; only the new test helper reads the flags after boot. A caller using this public configuration will not get a VFIO device unless it duplicates that helper, so the binding must be part of the backend lifecycle or the API must make the caller-side step explicit.
/// If true, bind the device to vfio-pci after boot, making it available
/// for passthrough into the L2 guest.
pub vfio: bool,
petri/src/vm/qemu/mod.rs:318
- This public runtime operation panics whenever a caller uses it with the QEMU backend. Other unsupported backend operations return an
anyhowerror, so return an explicit unsupported error instead of leaving atodo!()panic.
async fn update_command_line(&mut self, _command_line: &str) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:326
- A reset request against a QEMU VM currently panics through
todo!()rather than returning a recoverable unsupported-operation error. This is reachable through the publicPetriVm::resetAPI.
async fn reset(&mut self) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:330
- The trait already defines
get_guest_state_fileas an optional capability, but this override panics whenever the public API is called. ReturnOk(None)for QEMU, matching the trait's default and the backend's lack of a guest-state file.
async fn get_guest_state_file(&self) -> anyhow::Result<Option<PathBuf>> {
todo!()
petri/src/vm/qemu/mod.rs:214
- Grammar:
Add a devicesshould beAdd devices(orAdd a device).
/// Add a devices to the emulator configuration.
petri/src/vm/qemu/mod.rs:318
- These
todo!()implementations make operations exposed through the generic Petri runtime API panic. A generic test can callupdate_command_lineorreset, and unlike the explicit unsupported-operation methods below, QEMU would abort the test process instead of returning an error. Implement the operations or return explicitanyhow::bail!errors until they are supported.
async fn update_command_line(&mut self, _command_line: &str) -> anyhow::Result<()> {
todo!()
vmm_tests/petri_artifact_resolver_openvmm_known_paths/src/lib.rs:350
- When
VMM_TESTS_CONTENT_DIRis unset, this now looks for<repo>/<target>/pipette, but Cargo places a locally built binary attarget/<target>/<profile>/pipette; the existingcargo_build_profile()fallback was removed. This breaks direct local Petri/burette/test runs that use the known-path resolver. Keep the content-directory layout first, then fall back to the Cargo target path.
match get_path(
".",
PathBuf::from(&target).join(binary),
- Files reviewed: 36/36 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical bootstrap and additional compatibility, lifecycle, API, VFIO, Hyper-V, and artifact issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (13)
Previously missed (1) — in code that hasn't changed since the last review.
petri/src/vm/qemu/mod.rs:68
QemuPetriRuntimeowns astd::process::Child, but it has no drop-time cleanup. If setup fails afterspawn, orwait_for_agenttimes out and thePetriVmis dropped, the child is neither killed nor reaped, leaving an orphaned QEMU process (and potentially its forwarded port) behind. Add an RAII kill-and-reap guard for failure paths.
flowey/flowey_lib_hvlite/src/init_vmm_tests_content_dir.rs:267
resolve_openvmm_qemu::Request::Gettakes the host architecture, but this passes the test target architecture. Theaarch64-linux-tcgjob runs on an x86_64 host, so this requests the aarch64 QEMU executable; if an emulator test is selected there, the downloaded binary cannot run on the host. Usectx.arch()for this request, as the incubator setup below does.
let qemu_system_aarch64 = prebuilt_artifacts.qemu_system_aarch64.then(|| {
ctx.reqv(|v| {
crate::resolve_openvmm_qemu::Request::Get(
crate::resolve_openvmm_qemu::QemuFile::SystemAarch64,
arch,
v,
petri/src/vm/hyperv/mod.rs:89
- Removing the
!firmware.is_linux_direct()check makeshyperv_linux_direct_x64resolve and enter the Hyper-V backend, even though this backend explicitly ignoresprebuilt_initrdbecause Hyper-V does not support Linux-direct boot. This turns an unsupported configuration that was previously skipped into a failing VM launch; retain the firmware guard (or add an explicit rejection).
fn check_compat(_firmware: &Firmware, arch: MachineArch) -> bool {
arch == MachineArch::host()
}
petri/src/vm/qemu/mod.rs:80
check_compatcurrently accepts every firmware and guest architecture on a Linux x86-64 host, butnewimmediately asserts that the guest is AArch64 andbuild_qemu_commandonly acceptsFirmware::LinuxDirect. A valid macro input such asqemu_linux_direct_x64orqemu_uefi_x64therefore panics instead of being skipped/rejected. Restrict compatibility to the exact supported firmware and guest architecture.
fn check_compat(_firmware: &Firmware, _arch: MachineArch) -> bool {
// Our QEMU bachend only supports linux X64 at this time
MachineArch::X86_64 == MachineArch::host() && cfg!(target_os = "linux")
petri/src/vm/qemu/mod.rs:145
- The
vfiosetting is never consumed by this backend:runonly turns the devices into QEMU command-line PCI functions, while the actual unbind/rebind step exists only in the test helper. Callers that configurevfio: truetherefore do not receive VFIO devices despite the public field documenting that behavior. Provision these devices in the backend/runtime (and advertise capabilities), or make binding explicitly caller-owned.
let mut qemu_config = QemuPetriConfig::default();
if let Some(f) = modify_vmm_config {
qemu_config = f.0(qemu_config);
}
petri/src/vm/qemu/mod.rs:232
teardownkills QEMU but never waits for it. Direct callers ofvm.teardown()therefore leave the killed child unreaped (a zombie on Unix), and can also return before QEMU has released its resources. After checking whether it has already exited, kill if necessary and awaitwait()to reap it.
let mut qemu_process = self.qemu_process.lock().await;
qemu_process
.get_mut()
.kill()
.context("unable to kill qemu process")?;
petri/src/vm/qemu/mod.rs:318
- This public runtime operation panics with
todo!()when called. Unsupported QEMU operations should return a regular error, as the Hyper-V backend does, so a test cannot abort the harness via an API call.
async fn update_command_line(&mut self, _command_line: &str) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:326
- This public runtime operation panics with
todo!()when called. Return an unsupported-operation error until QEMU reset is implemented rather than unwinding the test process.
async fn reset(&mut self) -> anyhow::Result<()> {
todo!()
petri/src/vm/qemu/mod.rs:330
- The trait already defines
get_guest_state_fileas an optional operation with a defaultOk(None), but this backend replaces that safe result withtodo!(). ReturnOk(None)for QEMU until guest-state support exists.
async fn get_guest_state_file(&self) -> anyhow::Result<Option<PathBuf>> {
todo!()
petri/src/vm/qemu/mod.rs:118
- Typo in this new comment: “bachend” should be “backend”.
// QEMU bachend only supports linux X64 with an aarch64 guest
petri/src/vm/qemu/mod.rs:214
- The new documentation says “Add a devices”; use “Add devices” instead.
/// Add a devices to the emulator configuration.
vmm_tests/petri_artifact_resolver_openvmm_known_paths/src/lib.rs:350
- This removes the Cargo-output fallback for locally built pipette binaries. When
VMM_TESTS_CONTENT_DIRis unset,get_pathnow looks for<repo>/<target>/pipette, but Cargo still writes the binary undertarget/<target>/<profile>/pipette; direct local VMM test runs will therefore fail to resolve pipette. Keep the new content-directory layout as the first lookup while retaining the existing Cargo target/profile fallback.
match get_path(
".",
PathBuf::from(&target).join(binary),
vmm_tests/vmm_tests/tests/tests/main.rs:31
- This adds a new Petri VMM backend and a supported test target, but
Guide/src/dev_guide/tests/vmm.mdstill describes only the existing Hyper-V/OpenVMM scenarios and target matrix. Update the VMM test guide with the QEMU backend's Linux x86-64-host/AArch64-guest limitations and invocation so the documented test surface matches the new API.
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Lite
| "#!/bin/sh\n\ | ||
| ip link set eth0 up\n\ | ||
| ip addr add 10.0.2.15/24 dev eth0\n\ | ||
| ip route add default via 10.0.2.2\n\ | ||
| echo 'nameserver 10.0.2.3' > /etc/resolv.conf\n\ | ||
| exec '/{}' --transport tcp\n", | ||
| pipette_path.replace('\'', "'\\''") | ||
| )) |
This PR is the first step towards native nested/emulator support in Petri. It adds a QEMU "VMM" backend, allowing test authors to launch the emulator using Petri with the same semantics as Hyper-V or OpenVMM.
Also includes:
Most of the logic in the Petri QEMU backend and the validate_emulator test was copied from the incubator crate and adapted for the petri context. In the future, I will add native support for nested tests to petri (allows for qemu + openvmm or openvmm + hyperv tests, for example), at which point we can remove incubator.