Skip to content

libvirt: Disable firmware debug log (isa-debugcon) by default #229

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

Open
cgwalters wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −56
Open

libvirt: Disable firmware debug log (isa-debugcon) by default #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion crates/integration-tests/src/tests/libvirt_verb.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,21 @@ fn test_libvirt_run_no_storage_opts_without_bind_storage() -> Result<()> {
"Domain XML should NOT contain bind-storage-ro metadata when flag is not used. Found in XML."
);
println!("✓ Domain XML does not contain bind-storage-ro metadata");
println!("✓ Test passed: STORAGE_OPTS credentials are correctly excluded when --bind-storage-ro is not used");

// Verify that firmware debug log (isa-debugcon) is NOT present by default.
// Verbose OVMF firmware causes debug spam (e.g. VirtioSerialIoGetControl)
// when this device is present, so it must be opt-in via --firmware-log.
if std::env::consts::ARCH == "x86_64" {
assert!(
!domain_xml.contains("isa-debugcon"),
"Domain XML should NOT contain isa-debugcon by default (use --firmware-log to enable)"
);
println!(
"✓ Domain XML does not contain isa-debugcon (firmware debug log disabled by default)"
);
}

println!("✓ Test passed: default domain config has no unexpected extras");
Ok(())
}
integration_test!(test_libvirt_run_no_storage_opts_without_bind_storage);
Expand Down
55 changes: 1 addition & 54 deletions crates/kit/src/libvirt/domain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ impl DomainBuilder {
ovmf_code_format: None,
nvram_template: None,
nvram_format: None,
firmware_log: Some(FirmwareLogOutput::Console), // Default to pty for virsh console access
firmware_log: None,
}
}

Expand Down Expand Up @@ -776,57 +776,4 @@ mod tests {
assert!(xml_ro.contains("source dir=\"/host/storage\""));
assert!(xml_ro.contains("target dir=\"hoststorage\""));
}

Copy link
Collaborator

@gursewak1997 gursewak1997 Mar 18, 2026

Choose a reason for hiding this comment

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

The firmware log functionality still exists (via --firmware-log flag), but all 3 tests were deleted rather than adapted. We should have tests for it?

Copy link
Collaborator Author

@cgwalters cgwalters Mar 19, 2026

Choose a reason for hiding this comment

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

IMO these type of unit tests were pretty silly. What we probably could do instead is have an integration test for the libvirt config.

#[test]
fn test_firmware_log_default() {
// By default, firmware log should be enabled (pty/console mode)
let xml = DomainBuilder::new()
.with_name("test-firmware-log-default")
.build_xml()
.unwrap();

// On x86_64, should have isa-debugcon serial device
if std::env::consts::ARCH == "x86_64" {
assert!(xml.contains("serial type=\"pty\""));
assert!(xml.contains("target type=\"isa-debug\""));
assert!(xml.contains("model name=\"isa-debugcon\""));
assert!(xml.contains("address type=\"isa\" iobase=\"0x402\""));
}
}

#[test]
fn test_firmware_log_file() {
// Test firmware log to file
let xml = DomainBuilder::new()
.with_name("test-firmware-log-file")
.with_firmware_log(FirmwareLogOutput::File("/tmp/ovmf-debug.log".to_string()))
.build_xml()
.unwrap();

// On x86_64, should have isa-debugcon with file output
if std::env::consts::ARCH == "x86_64" {
assert!(xml.contains("serial type=\"file\""));
assert!(xml.contains("source path=\"/tmp/ovmf-debug.log\""));
assert!(xml.contains("target type=\"isa-debug\""));
assert!(xml.contains("model name=\"isa-debugcon\""));
assert!(xml.contains("address type=\"isa\" iobase=\"0x402\""));
}
}

#[test]
fn test_firmware_log_disabled() {
// Test disabling firmware log by setting firmware_log to None after construction
// Note: There's no public API to disable it once set, but we can test the XML
// generation doesn't include it on non-x86 architectures
let xml = DomainBuilder::new()
.with_name("test-firmware-log")
.build_xml()
.unwrap();

// On non-x86_64, should NOT have isa-debugcon (it's x86-only)
if std::env::consts::ARCH != "x86_64" {
assert!(!xml.contains("isa-debugcon"));
assert!(!xml.contains("isa-debug"));
}
}
}
11 changes: 10 additions & 1 deletion crates/kit/src/libvirt/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,10 @@ pub struct LibvirtRunOpts {
#[clap(long)]
pub disable_tpm: bool,

/// Enable firmware debug log (captures OVMF/EDK2 DEBUG output via isa-debugcon)
#[clap(long)]
pub firmware_log: bool,

/// Directory containing secure boot keys (required for uefi-secure)
#[clap(long)]
pub secure_boot_keys: Option<Utf8PathBuf>,
Expand Down Expand Up @@ -1099,7 +1103,12 @@ fn create_libvirt_domain_from_disk(
.with_transient_disk(opts.transient)
.with_network("none") // Use QEMU args for SSH networking instead
.with_firmware(opts.firmware)
.with_tpm(!opts.disable_tpm)
.with_tpm(!opts.disable_tpm);
if opts.firmware_log {
domain_builder =
domain_builder.with_firmware_log(crate::libvirt::domain::FirmwareLogOutput::Console);
}
domain_builder = domain_builder
.with_metadata("bootc:source-image", &opts.image)
.with_metadata("bootc:memory-mb", &memory.to_string())
.with_metadata("bootc:vcpus", &cpus.to_string())
Expand Down
Loading