diff --git a/.buildkite/common.py b/.buildkite/common.py index 6c86e26487d..3a397df6933 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -273,7 +273,7 @@ def __init__(self, with_build_step=True, **kwargs): if not args.binary_dir and with_build_step: build_cmds, self.shared_build = shared_build() self.build_group_per_arch( - "🏗ïļ Build", build_cmds, depends_on_build=False, set_key=self.shared_build + "build", build_cmds, depends_on_build=False, set_key=self.shared_build ) else: self.shared_build = None diff --git a/.buildkite/pipeline_coverage.py b/.buildkite/pipeline_coverage.py index 8ca73591c00..b137bde8345 100755 --- a/.buildkite/pipeline_coverage.py +++ b/.buildkite/pipeline_coverage.py @@ -9,7 +9,7 @@ pipeline = BKPipeline(with_build_step=False) pipeline.build_group( - ":coverage: Coverage", + "coverage", pipeline.devtool_test( devtool_opts="--no-build", pytest_opts="integration_tests/build/test_coverage.py", diff --git a/.buildkite/pipeline_cpu_template.py b/.buildkite/pipeline_cpu_template.py index 1f2d2644856..5edadeef29e 100755 --- a/.buildkite/pipeline_cpu_template.py +++ b/.buildkite/pipeline_cpu_template.py @@ -25,7 +25,7 @@ class BkStep(str, Enum): BkStep.COMMAND: [ "tools/devtool -y test --no-build -- -m no_block_pr -n4 --dist worksteal integration_tests/functional/test_cpu_features_x86_64.py -k 'test_cpu_rdmsr' " ], - BkStep.LABEL: "📖 rdmsr", + BkStep.LABEL: "rdmsr", "instances": [ "m5n.metal", "m6i.metal", @@ -39,7 +39,7 @@ class BkStep(str, Enum): BkStep.COMMAND: [ "tools/devtool -y test --no-build -- -m no_block_pr integration_tests/functional/test_cpu_template_helper.py -k test_guest_cpu_config_change", ], - BkStep.LABEL: "🖐ïļ fingerprint", + BkStep.LABEL: "fingerprint", }, "cpuid_wrmsr": { "snapshot": { @@ -48,7 +48,7 @@ class BkStep(str, Enum): "mkdir -pv tests/snapshot_artifacts_upload/{instance}_{os}_{kv}", "sudo mv tests/snapshot_artifacts/* tests/snapshot_artifacts_upload/{instance}_{os}_{kv}", ], - BkStep.LABEL: "ðŸ“ļ create snapshots", + BkStep.LABEL: "snapshot-create", BkStep.ARTIFACTS: "tests/snapshot_artifacts_upload/**/*", BkStep.TIMEOUT: 30, }, @@ -58,7 +58,7 @@ class BkStep(str, Enum): "mv tests/snapshot_artifacts_upload/{instance}_{os}_{kv} tests/snapshot_artifacts", "tools/devtool -y test --no-build -- -m nonci -n4 --dist worksteal integration_tests/functional/test_cpu_features_x86_64.py -k 'test_cpu_wrmsr_restore or test_cpu_cpuid_restore'", ], - BkStep.LABEL: "ðŸ“ļ load snapshot artifacts created on {instance} {snapshot_os} {snapshot_kv} to {restore_instance} {restore_os} {restore_kv}", + BkStep.LABEL: "snapshot-restore-src-{instance}-{snapshot_os}-{snapshot_kv}-dst-{restore_instance}-{restore_os}-{restore_kv}", BkStep.TIMEOUT: 30, }, "cross_instances": { @@ -137,7 +137,7 @@ def group_snapshot_restore(test_step): ) groups.append( - {"group": "ðŸ“ļ restores snapshots", "steps": steps, "depends_on": "snapshot"} + {"group": "snapshot-restore", "steps": steps, "depends_on": "snapshot"} ) return groups diff --git a/.buildkite/pipeline_cross.py b/.buildkite/pipeline_cross.py index f476fee76ad..bbe3a98b7cf 100755 --- a/.buildkite/pipeline_cross.py +++ b/.buildkite/pipeline_cross.py @@ -36,7 +36,7 @@ "tar cSvf snapshots/{instance}_{kv}.tar snapshot_artifacts", ] pipeline.build_group( - "ðŸ“ļ create snapshots", + "snapshot-create", commands, timeout=30, artifact_paths="snapshots/**/*", @@ -93,13 +93,13 @@ pytest_opts=f"-m nonci -n8 --dist worksteal {k_val} integration_tests/functional/test_snapshot_restore_cross_kernel.py", ), ], - "label": f"🎎 {src_instance} {src_kv} ➡ïļ {dst_instance} {dst_kv}", + "label": f"snapshot-restore-src-{src_instance}-{src_kv}-dst-{dst_instance}-{dst_kv}", "timeout": 30, "agents": {"instance": dst_instance, "kv": dst_kv, "os": dst_os}, **per_instance, } steps.append(step) pipeline.add_step( - {"group": "🎎 restore across instances and kernels", "steps": steps} + {"group": "snapshot-restore-across-instances-and-kernels", "steps": steps} ) print(pipeline.to_json()) diff --git a/.buildkite/pipeline_docker_popular.py b/.buildkite/pipeline_docker_popular.py index 2bcbe2eed56..92927588db2 100755 --- a/.buildkite/pipeline_docker_popular.py +++ b/.buildkite/pipeline_docker_popular.py @@ -13,7 +13,7 @@ ROOTFS_TAR = f"rootfs_$(uname -m)_{random_str(k=8)}.tar.gz" pipeline.build_group_per_arch( - ":ship: Rootfs build", + "rootfs-build", [ "sudo yum install -y systemd-container", "cd tools/test-popular-containers", @@ -26,7 +26,7 @@ ) pipeline.build_group( - ":whale: Docker Popular Containers", + "docker-popular-containers", [ "./tools/devtool download_ci_artifacts", f'buildkite-agent artifact download "{ROOTFS_TAR}" .', diff --git a/.buildkite/pipeline_pr.py b/.buildkite/pipeline_pr.py index 8744a0dcb6a..4edcc0b9037 100755 --- a/.buildkite/pipeline_pr.py +++ b/.buildkite/pipeline_pr.py @@ -27,7 +27,7 @@ pipeline.add_step( { "command": "./tools/devtool -y checkstyle", - "label": "ðŸŠķ Style", + "label": "style", }, depends_on_build=False, ) @@ -35,7 +35,7 @@ # run sanity build of devtool if Dockerfile is changed if any(x.parent.name == "devctr" for x in changed_files): pipeline.build_group_per_arch( - "🐋 Dev Container Sanity Build", + "dev-container-sanity-build", "./tools/devtool -y build_devctr && DEVCTR_IMAGE_TAG=latest ./tools/devtool test --no-build -- integration_tests/functional/test_api.py", ) @@ -44,7 +44,7 @@ for x in changed_files ): pipeline.build_group_per_arch( - "ðŸ“Ķ Release Sanity Build", + "release-sanity-build", "./tools/devtool -y make_release", depends_on_build=False, ) @@ -56,7 +56,7 @@ or any(x.name == "test_kani.py" for x in changed_files) ): kani_grp = pipeline.build_group( - "🔍 Kani", + "kani", "./tools/devtool -y test --no-build -- ../tests/integration_tests/test_kani.py -n auto", # Kani step default # Kani runs fastest on m6a.metal @@ -66,26 +66,23 @@ **DEFAULTS_PERF, depends_on_build=False, ) - # modify Kani steps' label - for step in kani_grp["steps"]: - step["label"] = "🔍 Kani" if run_all_tests(changed_files): pipeline.build_group( - "ðŸ“Ķ Build", + "build", pipeline.devtool_test(pytest_opts="integration_tests/build/"), depends_on_build=False, ) pipeline.build_group( - "⚙ Functional and security 🔒", + "functional-and-security", pipeline.devtool_test( pytest_opts="-n 16 --dist worksteal integration_tests/{{functional,security}}", ), ) pipeline.build_group( - "⏱ Performance", + "performance", pipeline.devtool_test( devtool_opts="--performance -c 1-10 -m 0", pytest_opts="../tests/integration_tests/performance/", diff --git a/.buildkite/pipeline_pr_no_block.py b/.buildkite/pipeline_pr_no_block.py index 5b0f3a0474d..c46aa536df5 100755 --- a/.buildkite/pipeline_pr_no_block.py +++ b/.buildkite/pipeline_pr_no_block.py @@ -19,7 +19,7 @@ ) pipeline.build_group( - "❓ Optional", + "optional", pipeline.devtool_test( devtool_opts="--performance -c 1-10 -m 0", pytest_opts="integration_tests/ -m no_block_pr --log-cli-level=INFO", diff --git a/.buildkite/pipeline_release_qa.py b/.buildkite/pipeline_release_qa.py index 7b25021812f..b892d8cf747 100755 --- a/.buildkite/pipeline_release_qa.py +++ b/.buildkite/pipeline_release_qa.py @@ -14,7 +14,7 @@ pipeline.add_step( { - "label": "Download release", + "label": "download-release", "if": 'build.env("VERSION") != "dev"', "command": [ "aws s3 sync --no-sign-request s3://spec.ccfc.min/firecracker-ci/firecracker/$$VERSION release-$$VERSION", @@ -25,7 +25,7 @@ ) pipeline.build_group_per_arch( - ":building_construction: Make release", + "make-release", # if is a keyword for python, so we need this workaround to expand it as a kwarg **{"if": 'build.env("VERSION") == "dev"'}, command=[ @@ -48,7 +48,7 @@ # The devtool expects the examples to be in the same folder as the binaries to run some tests # (for example, uffd handler tests). Build them and upload them in the same folder. pipeline.build_group_per_arch( - ":hammer_and_wrench: Build examples", + "build-examples", command=[ "CARGO_TARGET=$$(uname -m)-unknown-linux-musl", "./tools/devtool -y sh cargo build --target $$CARGO_TARGET --release --examples", @@ -63,7 +63,7 @@ pipeline.add_step( { - "label": ":pipeline: PR", + "label": "run-pr-pipeline", "command": ( ".buildkite/pipeline_pr.py --binary-dir release-$$VERSION " "| jq '(..|select(.priority? != null).priority) += 100' " diff --git a/rustfmt.toml b/rustfmt.toml new file mode 100644 index 00000000000..8ae608a6adb --- /dev/null +++ b/rustfmt.toml @@ -0,0 +1,8 @@ +comment_width = 100 +wrap_comments = true +format_code_in_doc_comments = true +format_strings = true +imports_granularity = "Module" +normalize_comments = true +normalize_doc_attributes = true +group_imports = "StdExternalCrate" diff --git a/src/cpu-template-helper/src/utils/x86_64.rs b/src/cpu-template-helper/src/utils/x86_64.rs index e83b387bb14..8eda70b2a94 100644 --- a/src/cpu-template-helper/src/utils/x86_64.rs +++ b/src/cpu-template-helper/src/utils/x86_64.rs @@ -194,7 +194,6 @@ mod tests { use vmm::cpu_config::x86_64::custom_cpu_template::CpuidRegisterModifier; use super::*; - use crate::utils::x86_64::{cpuid_leaf_modifier, cpuid_reg_modifier, msr_modifier}; macro_rules! cpuid_modifier_map { ($leaf:expr, $subleaf:expr, $flags:expr, $register:expr, $value:expr) => { diff --git a/src/firecracker/src/api_server/parsed_request.rs b/src/firecracker/src/api_server/parsed_request.rs index f98170ccbea..959aa1e5a0a 100644 --- a/src/firecracker/src/api_server/parsed_request.rs +++ b/src/firecracker/src/api_server/parsed_request.rs @@ -406,12 +406,12 @@ pub mod tests { "{} {} HTTP/1.1\r\nContent-Type: application/json\r\n", request_type, endpoint ); - if body.is_some() { + if let Some(body) = body { return format!( "{}Content-Length: {}\r\n\r\n{}", req_no_body, - body.unwrap().len(), - body.unwrap() + body.len(), + body ); } format!("{}\r\n", req_no_body,) diff --git a/src/vmm/src/arch/x86_64/interrupts.rs b/src/vmm/src/arch/x86_64/interrupts.rs index 8deb5a9d44e..4d51daa0256 100644 --- a/src/vmm/src/arch/x86_64/interrupts.rs +++ b/src/vmm/src/arch/x86_64/interrupts.rs @@ -111,7 +111,7 @@ mod tests { v.iter_mut() .for_each(|x| *x = set_apic_delivery_mode(*x, 2)); - let after: Vec = v.iter().map(|x| ((*x & !0x700) | ((2) << 8))).collect(); + let after: Vec = v.iter().map(|x| (*x & !0x700) | ((2) << 8)).collect(); assert_eq!(v, after); } diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index 332b1ac3cc3..97f2e92693f 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -31,6 +31,7 @@ use crate::device_manager::{ }; use crate::devices::virtio::balloon::Balloon; use crate::devices::virtio::block::device::Block; +use crate::devices::virtio::device::VirtioDevice; use crate::devices::virtio::mem::{VIRTIO_MEM_DEFAULT_SLOT_SIZE_MIB, VirtioMem}; use crate::devices::virtio::net::Net; use crate::devices::virtio::pmem::device::Pmem; @@ -682,7 +683,7 @@ fn attach_net_devices<'a, I: Iterator>> + Debug>( event_manager: &mut EventManager, ) -> Result<(), StartMicrovmError> { for net_device in net_devices { - let id = net_device.lock().expect("Poisoned lock").id().clone(); + let id = net_device.lock().expect("Poisoned lock").id().to_string(); event_manager.add_subscriber(net_device.clone()); // The device mutex mustn't be locked here otherwise it will deadlock. device_manager.attach_virtio_device(vm, id, net_device.clone(), cmdline, false)?; @@ -753,7 +754,7 @@ pub(crate) mod tests { use super::*; use crate::device_manager::tests::default_device_manager; use crate::devices::virtio::block::CacheType; - use crate::devices::virtio::generated::virtio_ids; + use crate::devices::virtio::device::VirtioDeviceType; use crate::devices::virtio::rng::device::ENTROPY_DEV_ID; use crate::devices::virtio::vsock::VSOCK_DEV_ID; use crate::mmds::data_store::{Mmds, MmdsVersion}; @@ -955,7 +956,7 @@ pub(crate) mod tests { assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_VSOCK, &vsock_dev_id) + .get_virtio_device(VirtioDeviceType::Vsock, &vsock_dev_id) .is_some() ); } @@ -980,7 +981,7 @@ pub(crate) mod tests { assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_RNG, ENTROPY_DEV_ID) + .get_virtio_device(VirtioDeviceType::Rng, ENTROPY_DEV_ID) .is_some() ); } @@ -1044,7 +1045,7 @@ pub(crate) mod tests { assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BALLOON, BALLOON_DEV_ID) + .get_virtio_device(VirtioDeviceType::Balloon, BALLOON_DEV_ID) .is_some() ); } @@ -1095,7 +1096,7 @@ pub(crate) mod tests { assert!(cmdline_contains(&cmdline, "root=/dev/vda ro")); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str()) + .get_virtio_device(VirtioDeviceType::Block, drive_id.as_str()) .is_some() ); } @@ -1116,7 +1117,7 @@ pub(crate) mod tests { assert!(cmdline_contains(&cmdline, "root=PARTUUID=0eaa91a0-01 rw")); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str()) + .get_virtio_device(VirtioDeviceType::Block, drive_id.as_str()) .is_some() ); } @@ -1138,7 +1139,7 @@ pub(crate) mod tests { assert!(!cmdline_contains(&cmdline, "root=/dev/vda")); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str()) + .get_virtio_device(VirtioDeviceType::Block, drive_id.as_str()) .is_some() ); } @@ -1175,17 +1176,17 @@ pub(crate) mod tests { assert!(cmdline_contains(&cmdline, "root=PARTUUID=0eaa91a0-01 rw")); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, "root") + .get_virtio_device(VirtioDeviceType::Block, "root") .is_some() ); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, "secondary") + .get_virtio_device(VirtioDeviceType::Block, "secondary") .is_some() ); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, "third") + .get_virtio_device(VirtioDeviceType::Block, "third") .is_some() ); @@ -1214,7 +1215,7 @@ pub(crate) mod tests { assert!(cmdline_contains(&cmdline, "root=/dev/vda rw")); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str()) + .get_virtio_device(VirtioDeviceType::Block, drive_id.as_str()) .is_some() ); } @@ -1235,7 +1236,7 @@ pub(crate) mod tests { assert!(cmdline_contains(&cmdline, "root=PARTUUID=0eaa91a0-01 ro")); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str()) + .get_virtio_device(VirtioDeviceType::Block, drive_id.as_str()) .is_some() ); } @@ -1256,7 +1257,7 @@ pub(crate) mod tests { assert!(cmdline_contains(&cmdline, "root=/dev/vda rw")); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_BLOCK, drive_id.as_str()) + .get_virtio_device(VirtioDeviceType::Block, drive_id.as_str()) .is_some() ); } @@ -1279,7 +1280,7 @@ pub(crate) mod tests { assert!(cmdline_contains(&cmdline, "root=/dev/pmem0 ro")); assert!( vmm.device_manager - .get_virtio_device(virtio_ids::VIRTIO_ID_PMEM, id.as_str()) + .get_virtio_device(VirtioDeviceType::Pmem, id.as_str()) .is_some() ); } diff --git a/src/vmm/src/device_manager/mmio.rs b/src/vmm/src/device_manager/mmio.rs index eea024d9692..8a75c9026e3 100644 --- a/src/vmm/src/device_manager/mmio.rs +++ b/src/vmm/src/device_manager/mmio.rs @@ -25,6 +25,7 @@ use crate::arch::{RTC_MEM_START, SERIAL_MEM_START}; #[cfg(target_arch = "aarch64")] use crate::devices::legacy::{RTCDevice, SerialDevice}; use crate::devices::pseudo::BootTimer; +use crate::devices::virtio::device::VirtioDeviceType; use crate::devices::virtio::transport::mmio::MmioTransport; use crate::vstate::bus::{Bus, BusError}; #[cfg(target_arch = "x86_64")] @@ -118,7 +119,7 @@ pub struct MMIODevice { #[derive(Debug, Default)] pub struct MMIODeviceManager { /// VirtIO devices using an MMIO transport layer - pub(crate) virtio_devices: HashMap<(u32, String), MMIODevice>, + pub(crate) virtio_devices: HashMap<(VirtioDeviceType, String), MMIODevice>, /// Boot timer device pub(crate) boot_timer: Option>, #[cfg(target_arch = "aarch64")] @@ -382,20 +383,20 @@ impl MMIODeviceManager { /// Gets the specified device. pub fn get_virtio_device( &self, - virtio_type: u32, + device_type: VirtioDeviceType, device_id: &str, ) -> Option<&MMIODevice> { self.virtio_devices - .get(&(virtio_type, device_id.to_string())) + .get(&(device_type, device_id.to_string())) } /// Run fn for each registered virtio device. pub fn for_each_virtio_device(&self, mut f: F) -> Result<(), E> where - F: FnMut(&u32, &String, &MMIODevice) -> Result<(), E>, + F: FnMut(&VirtioDeviceType, &String, &MMIODevice) -> Result<(), E>, { - for ((virtio_type, device_id), mmio_device) in &self.virtio_devices { - f(virtio_type, device_id, mmio_device)?; + for ((device_type, device_id), mmio_device) in &self.virtio_devices { + f(device_type, device_id, mmio_device)?; } Ok(()) } @@ -430,7 +431,7 @@ pub(crate) mod tests { use super::*; use crate::devices::virtio::ActivateError; - use crate::devices::virtio::device::VirtioDevice; + use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::queue::Queue; use crate::devices::virtio::transport::VirtioInterrupt; use crate::devices::virtio::transport::mmio::IrqTrigger; @@ -491,7 +492,11 @@ pub(crate) mod tests { } impl VirtioDevice for DummyDevice { - impl_device_type!(0); + impl_device_type!(VirtioDeviceType::Net); + + fn id(&self) -> &str { + "dummy" + } fn avail_features(&self) -> u64 { 0 @@ -575,15 +580,21 @@ pub(crate) mod tests { ) .unwrap(); - assert!(device_manager.get_virtio_device(0, "foo").is_none()); - let dev = device_manager.get_virtio_device(0, "dummy").unwrap(); + assert!( + device_manager + .get_virtio_device(VirtioDeviceType::Net, "foo") + .is_none() + ); + let dev = device_manager + .get_virtio_device(VirtioDeviceType::Net, "dummy") + .unwrap(); assert_eq!(dev.resources.addr, arch::MEM_32BIT_DEVICES_START); assert_eq!(dev.resources.len, MMIO_LEN); assert_eq!(dev.resources.gsi, Some(arch::GSI_LEGACY_START)); device_manager - .for_each_virtio_device(|virtio_type, device_id, mmio_device| { - assert_eq!(*virtio_type, 0); + .for_each_virtio_device(|device_type, device_id, mmio_device| { + assert_eq!(*device_type, VirtioDeviceType::Net); assert_eq!(device_id, "dummy"); assert_eq!(mmio_device.resources.addr, arch::MEM_32BIT_DEVICES_START); assert_eq!(mmio_device.resources.len, MMIO_LEN); @@ -642,7 +653,7 @@ pub(crate) mod tests { #[test] fn test_dummy_device() { let dummy = DummyDevice::new(); - assert_eq!(dummy.device_type(), 0); + assert_eq!(dummy.device_type(), VirtioDeviceType::Net); assert_eq!(dummy.queues().len(), QUEUE_SIZES.len()); } diff --git a/src/vmm/src/device_manager/mod.rs b/src/vmm/src/device_manager/mod.rs index fc245e05539..d91bd20e98e 100644 --- a/src/vmm/src/device_manager/mod.rs +++ b/src/vmm/src/device_manager/mod.rs @@ -31,7 +31,7 @@ use crate::devices::legacy::RTCDevice; use crate::devices::legacy::serial::SerialOut; use crate::devices::legacy::{IER_RDA_BIT, IER_RDA_OFFSET, SerialDevice}; use crate::devices::pseudo::BootTimer; -use crate::devices::virtio::device::VirtioDevice; +use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::transport::mmio::{IrqTrigger, MmioTransport}; use crate::resources::VmResources; use crate::snapshot::Persist; @@ -332,11 +332,11 @@ impl DeviceManager { /// Get a VirtIO device of type `virtio_type` with ID `device_id` pub fn get_virtio_device( &self, - virtio_type: u32, + device_type: VirtioDeviceType, device_id: &str, ) -> Option>> { if self.pci_devices.pci_segment.is_some() { - let pci_device = self.pci_devices.get_virtio_device(virtio_type, device_id)?; + let pci_device = self.pci_devices.get_virtio_device(device_type, device_id)?; Some( pci_device .lock() @@ -347,7 +347,7 @@ impl DeviceManager { } else { let mmio_device = self .mmio_devices - .get_virtio_device(virtio_type, device_id)?; + .get_virtio_device(device_type, device_id)?; Some( mmio_device .inner diff --git a/src/vmm/src/device_manager/pci_mngr.rs b/src/vmm/src/device_manager/pci_mngr.rs index d465482d8c1..a27d7110dda 100644 --- a/src/vmm/src/device_manager/pci_mngr.rs +++ b/src/vmm/src/device_manager/pci_mngr.rs @@ -16,8 +16,7 @@ use crate::devices::virtio::balloon::Balloon; use crate::devices::virtio::balloon::persist::{BalloonConstructorArgs, BalloonState}; use crate::devices::virtio::block::device::Block; use crate::devices::virtio::block::persist::{BlockConstructorArgs, BlockState}; -use crate::devices::virtio::device::VirtioDevice; -use crate::devices::virtio::generated::virtio_ids; +use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::mem::VirtioMem; use crate::devices::virtio::mem::persist::{VirtioMemConstructorArgs, VirtioMemState}; use crate::devices::virtio::net::Net; @@ -48,7 +47,7 @@ pub struct PciDevices { /// PCIe segment of the VMM, if PCI is enabled. We currently support a single PCIe segment. pub pci_segment: Option, /// All VirtIO PCI devices of the system - pub virtio_devices: HashMap<(u32, String), Arc>>, + pub virtio_devices: HashMap<(VirtioDeviceType, String), Arc>>, } #[derive(Debug, thiserror::Error, displaydoc::Display)] @@ -120,7 +119,7 @@ impl PciDevices { debug!("Allocating BDF: {pci_device_bdf:?} for device"); let mem = vm.guest_memory().clone(); - let device_type: u32 = device.lock().expect("Poisoned lock").device_type(); + let device_type = device.lock().expect("Poisoned lock").device_type(); // Allocate one MSI vector per queue, plus one for configuration let msix_num = @@ -172,7 +171,7 @@ impl PciDevices { ) -> Result<(), PciManagerError> { // We should only be reaching this point if PCI is enabled let pci_segment = self.pci_segment.as_ref().unwrap(); - let device_type: u32 = device.lock().expect("Poisoned lock").device_type(); + let device_type = device.lock().expect("Poisoned lock").device_type(); let virtio_device = Arc::new(Mutex::new(VirtioPciDevice::new_from_state( device_id.to_string(), @@ -207,7 +206,7 @@ impl PciDevices { /// Gets the specified device. pub fn get_virtio_device( &self, - device_type: u32, + device_type: VirtioDeviceType, device_id: &str, ) -> Option<&Arc>> { self.virtio_devices @@ -290,7 +289,7 @@ impl<'a> Persist<'a> for PciDevices { let pci_device_bdf = transport_state.pci_device_bdf.into(); match locked_virtio_dev.device_type() { - virtio_ids::VIRTIO_ID_BALLOON => { + VirtioDeviceType::Balloon => { let balloon_device = locked_virtio_dev .as_any() .downcast_ref::() @@ -305,7 +304,7 @@ impl<'a> Persist<'a> for PciDevices { transport_state, }); } - virtio_ids::VIRTIO_ID_BLOCK => { + VirtioDeviceType::Block => { let block_dev = locked_virtio_dev .as_mut_any() .downcast_mut::() @@ -326,7 +325,7 @@ impl<'a> Persist<'a> for PciDevices { }); } } - virtio_ids::VIRTIO_ID_NET => { + VirtioDeviceType::Net => { let net_dev = locked_virtio_dev .as_mut_any() .downcast_mut::() @@ -348,7 +347,7 @@ impl<'a> Persist<'a> for PciDevices { transport_state, }) } - virtio_ids::VIRTIO_ID_VSOCK => { + VirtioDeviceType::Vsock => { let vsock_dev = locked_virtio_dev .as_mut_any() // Currently, VsockUnixBackend is the only implementation of VsockBackend. @@ -379,7 +378,7 @@ impl<'a> Persist<'a> for PciDevices { transport_state, }); } - virtio_ids::VIRTIO_ID_RNG => { + VirtioDeviceType::Rng => { let rng_dev = locked_virtio_dev .as_mut_any() .downcast_mut::() @@ -393,7 +392,7 @@ impl<'a> Persist<'a> for PciDevices { transport_state, }) } - virtio_ids::VIRTIO_ID_PMEM => { + VirtioDeviceType::Pmem => { let pmem_dev = locked_virtio_dev .as_mut_any() .downcast_mut::() @@ -406,7 +405,7 @@ impl<'a> Persist<'a> for PciDevices { transport_state, }); } - virtio_ids::VIRTIO_ID_MEM => { + VirtioDeviceType::Mem => { let mem_dev = locked_virtio_dev .as_mut_any() .downcast_mut::() @@ -420,7 +419,6 @@ impl<'a> Persist<'a> for PciDevices { transport_state, }) } - _ => unreachable!(), } } diff --git a/src/vmm/src/device_manager/persist.rs b/src/vmm/src/device_manager/persist.rs index 2a0393e57f2..486fb69d09e 100644 --- a/src/vmm/src/device_manager/persist.rs +++ b/src/vmm/src/device_manager/persist.rs @@ -26,8 +26,7 @@ use crate::devices::virtio::balloon::{Balloon, BalloonError}; use crate::devices::virtio::block::BlockError; use crate::devices::virtio::block::device::Block; use crate::devices::virtio::block::persist::{BlockConstructorArgs, BlockState}; -use crate::devices::virtio::device::VirtioDevice; -use crate::devices::virtio::generated::virtio_ids; +use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::mem::VirtioMem; use crate::devices::virtio::mem::persist::{ VirtioMemConstructorArgs, VirtioMemPersistError, VirtioMemState, @@ -233,7 +232,7 @@ impl<'a> Persist<'a> for MMIODeviceManager { let mut locked_device = mmio_transport_locked.locked_device(); match locked_device.device_type() { - virtio_ids::VIRTIO_ID_BALLOON => { + VirtioDeviceType::Balloon => { let device_state = locked_device .as_any() .downcast_ref::() @@ -247,7 +246,7 @@ impl<'a> Persist<'a> for MMIODeviceManager { }); } // Both virtio-block and vhost-user-block share same device type. - virtio_ids::VIRTIO_ID_BLOCK => { + VirtioDeviceType::Block => { let block = locked_device.as_mut_any().downcast_mut::().unwrap(); if block.is_vhost_user() { warn!( @@ -265,7 +264,7 @@ impl<'a> Persist<'a> for MMIODeviceManager { }); } } - virtio_ids::VIRTIO_ID_NET => { + VirtioDeviceType::Net => { let net = locked_device.as_mut_any().downcast_mut::().unwrap(); if let (Some(mmds_ns), None) = (net.mmds_ns.as_ref(), states.mmds.as_ref()) { let mmds_guard = mmds_ns.mmds.lock().expect("Poisoned lock"); @@ -284,7 +283,7 @@ impl<'a> Persist<'a> for MMIODeviceManager { device_info, }); } - virtio_ids::VIRTIO_ID_VSOCK => { + VirtioDeviceType::Vsock => { let vsock = locked_device .as_mut_any() // Currently, VsockUnixBackend is the only implementation of VsockBackend. @@ -313,7 +312,7 @@ impl<'a> Persist<'a> for MMIODeviceManager { device_info, }); } - virtio_ids::VIRTIO_ID_RNG => { + VirtioDeviceType::Rng => { let entropy = locked_device .as_mut_any() .downcast_mut::() @@ -327,7 +326,7 @@ impl<'a> Persist<'a> for MMIODeviceManager { device_info, }); } - virtio_ids::VIRTIO_ID_PMEM => { + VirtioDeviceType::Pmem => { let pmem = locked_device.as_mut_any().downcast_mut::().unwrap(); let device_state = pmem.save(); states.pmem_devices.push(VirtioDeviceState { @@ -337,7 +336,7 @@ impl<'a> Persist<'a> for MMIODeviceManager { device_info, }) } - virtio_ids::VIRTIO_ID_MEM => { + VirtioDeviceType::Mem => { let mem = locked_device .as_mut_any() .downcast_mut::() @@ -351,7 +350,6 @@ impl<'a> Persist<'a> for MMIODeviceManager { device_info, }); } - _ => unreachable!(), }; Ok(()) diff --git a/src/vmm/src/devices/virtio/balloon/device.rs b/src/vmm/src/devices/virtio/balloon/device.rs index f3b3b7d13f7..e5e177871c2 100644 --- a/src/vmm/src/devices/virtio/balloon/device.rs +++ b/src/vmm/src/devices/virtio/balloon/device.rs @@ -28,9 +28,8 @@ use super::{ VIRTIO_BALLOON_S_OOM_KILL, VIRTIO_BALLOON_S_SWAP_IN, VIRTIO_BALLOON_S_SWAP_OUT, }; use crate::devices::virtio::balloon::BalloonError; -use crate::devices::virtio::device::ActiveState; +use crate::devices::virtio::device::{ActiveState, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_BALLOON; use crate::devices::virtio::queue::InvalidAvailIdx; use crate::devices::virtio::transport::{VirtioInterrupt, VirtioInterruptType}; use crate::logger::{IncMetric, log_dev_preview_warning}; @@ -393,7 +392,7 @@ impl Balloon { let max_len = MAX_PAGES_IN_DESC * SIZE_OF_U32; valid_descs_found = true; - if !head.is_write_only() && len % SIZE_OF_U32 == 0 { + if !head.is_write_only() && len.is_multiple_of(SIZE_OF_U32) { // Check descriptor pfn count. if len > max_len { error!( @@ -673,11 +672,6 @@ impl Balloon { Ok(()) } - /// Provides the ID of this balloon device. - pub fn id(&self) -> &str { - BALLOON_DEV_ID - } - fn trigger_stats_update(&mut self) -> Result<(), BalloonError> { // The communication is driven by the device by using the buffer // and sending a used buffer notification @@ -867,7 +861,11 @@ impl Balloon { } impl VirtioDevice for Balloon { - impl_device_type!(VIRTIO_ID_BALLOON); + impl_device_type!(VirtioDeviceType::Balloon); + + fn id(&self) -> &str { + BALLOON_DEV_ID + } fn avail_features(&self) -> u64 { self.avail_features @@ -954,16 +952,16 @@ impl VirtioDevice for Balloon { } fn kick(&mut self) { - // If device is activated, kick the balloon queue(s) to make up for any - // pending or in-flight epoll events we may have not captured in snapshot. - // Stats queue doesn't need kicking as it is notified via a `timer_fd`. if self.is_activated() { - info!("kick balloon {}.", self.id()); if self.free_page_hinting() { - // On restore we reset back to DONE to ensure everythign is freed + info!( + "[{:?}:{}] resetting free page hinting to DONE", + self.device_type(), + self.id() + ); self.update_free_page_hint_cmd(FREE_PAGE_HINT_DONE); } - self.process_virtio_queues(); + self.notify_queue_events(); } } } @@ -1129,7 +1127,7 @@ pub(crate) mod tests { for (reporting, hinting, deflate_on_oom, stats_interval) in combinations { let mut balloon = Balloon::new(0, *deflate_on_oom, *stats_interval, *hinting, *reporting).unwrap(); - assert_eq!(balloon.device_type(), VIRTIO_ID_BALLOON); + assert_eq!(balloon.device_type(), VirtioDeviceType::Balloon); let features: u64 = (1u64 << VIRTIO_F_VERSION_1) | (u64::from(*deflate_on_oom) << VIRTIO_BALLOON_F_DEFLATE_ON_OOM) diff --git a/src/vmm/src/devices/virtio/balloon/persist.rs b/src/vmm/src/devices/virtio/balloon/persist.rs index 333697499b2..11f6ab53648 100644 --- a/src/vmm/src/devices/virtio/balloon/persist.rs +++ b/src/vmm/src/devices/virtio/balloon/persist.rs @@ -10,8 +10,7 @@ use serde::{Deserialize, Serialize}; use super::*; use crate::devices::virtio::balloon::device::{BalloonStats, ConfigSpace, HintingState}; -use crate::devices::virtio::device::{ActiveState, DeviceState}; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_BALLOON; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDeviceType}; use crate::devices::virtio::persist::VirtioDeviceState; use crate::devices::virtio::queue::FIRECRACKER_MAX_QUEUE_SIZE; use crate::devices::virtio::transport::VirtioInterrupt; @@ -173,7 +172,7 @@ impl Persist<'_> for Balloon { .virtio_state .build_queues_checked( &constructor_args.mem, - VIRTIO_ID_BALLOON, + VirtioDeviceType::Balloon, num_queues, FIRECRACKER_MAX_QUEUE_SIZE, ) @@ -230,7 +229,7 @@ mod tests { ) .unwrap(); - assert_eq!(restored_balloon.device_type(), VIRTIO_ID_BALLOON); + assert_eq!(restored_balloon.device_type(), VirtioDeviceType::Balloon); assert_eq!(restored_balloon.acked_features, balloon.acked_features); assert_eq!(restored_balloon.avail_features, balloon.avail_features); diff --git a/src/vmm/src/devices/virtio/block/device.rs b/src/vmm/src/devices/virtio/block/device.rs index 13155efb31d..7eb775e3c71 100644 --- a/src/vmm/src/devices/virtio/block/device.rs +++ b/src/vmm/src/devices/virtio/block/device.rs @@ -12,8 +12,7 @@ use super::persist::{BlockConstructorArgs, BlockState}; use super::vhost_user::device::{VhostUserBlock, VhostUserBlockConfig}; use super::virtio::device::{VirtioBlock, VirtioBlockConfig}; use crate::devices::virtio::ActivateError; -use crate::devices::virtio::device::VirtioDevice; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_BLOCK; +use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::queue::{InvalidAvailIdx, Queue}; use crate::devices::virtio::transport::VirtioInterrupt; use crate::impl_device_type; @@ -96,13 +95,6 @@ impl Block { } } - pub fn id(&self) -> &str { - match self { - Self::Virtio(b) => &b.id, - Self::VhostUser(b) => &b.id, - } - } - pub fn root_device(&self) -> bool { match self { Self::Virtio(b) => b.root_device, @@ -133,7 +125,14 @@ impl Block { } impl VirtioDevice for Block { - impl_device_type!(VIRTIO_ID_BLOCK); + impl_device_type!(VirtioDeviceType::Block); + + fn id(&self) -> &str { + match self { + Self::Virtio(b) => b.id(), + Self::VhostUser(b) => b.id(), + } + } fn avail_features(&self) -> u64 { match self { @@ -215,18 +214,6 @@ impl VirtioDevice for Block { Self::VhostUser(b) => b.device_state.is_activated(), } } - - fn kick(&mut self) { - // If device is activated, kick the block queue(s) to make up for any - // pending or in-flight epoll events we may have not captured in - // snapshot. No need to kick Ratelimiters - // because they are restored 'unblocked' so - // any inflight `timer_fd` events can be safely discarded. - if self.is_activated() { - info!("kick block {}.", self.id()); - self.process_virtio_queues(); - } - } } impl MutEventSubscriber for Block { diff --git a/src/vmm/src/devices/virtio/block/vhost_user/device.rs b/src/vmm/src/devices/virtio/block/vhost_user/device.rs index dd08b8de7c8..580e2d8dd03 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/device.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/device.rs @@ -16,10 +16,9 @@ use vmm_sys_util::eventfd::EventFd; use super::{NUM_QUEUES, QUEUE_SIZE, VhostUserBlockError}; use crate::devices::virtio::ActivateError; use crate::devices::virtio::block::CacheType; -use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_blk::{VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_RO}; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_BLOCK; use crate::devices::virtio::generated::virtio_ring::VIRTIO_RING_F_EVENT_IDX; use crate::devices::virtio::queue::Queue; use crate::devices::virtio::transport::{VirtioInterrupt, VirtioInterruptType}; @@ -69,19 +68,20 @@ impl TryFrom<&BlockDeviceConfig> for VhostUserBlockConfig { type Error = VhostUserBlockError; fn try_from(value: &BlockDeviceConfig) -> Result { - if value.socket.is_some() - && value.is_read_only.is_none() - && value.path_on_host.is_none() - && value.rate_limiter.is_none() - && value.file_engine_type.is_none() - { + if let (Some(socket), None, None, None, None) = ( + &value.socket, + &value.is_read_only, + &value.path_on_host, + &value.rate_limiter, + &value.file_engine_type, + ) { Ok(Self { drive_id: value.drive_id.clone(), partuuid: value.partuuid.clone(), is_root_device: value.is_root_device, cache_type: value.cache_type, - socket: value.socket.as_ref().unwrap().clone(), + socket: socket.clone(), }) } else { Err(VhostUserBlockError::Config) @@ -288,7 +288,11 @@ impl VhostUserBlockImpl { } impl VirtioDevice for VhostUserBlockImpl { - impl_device_type!(VIRTIO_ID_BLOCK); + impl_device_type!(VirtioDeviceType::Block); + + fn id(&self) -> &str { + &self.id + } fn avail_features(&self) -> u64 { self.avail_features diff --git a/src/vmm/src/devices/virtio/block/virtio/device.rs b/src/vmm/src/devices/virtio/block/virtio/device.rs index ecdd8ee4f6d..ec7ec468505 100644 --- a/src/vmm/src/devices/virtio/block/virtio/device.rs +++ b/src/vmm/src/devices/virtio/block/virtio/device.rs @@ -25,12 +25,11 @@ use super::{BLOCK_QUEUE_SIZES, SECTOR_SHIFT, SECTOR_SIZE, VirtioBlockError, io a use crate::devices::virtio::ActivateError; use crate::devices::virtio::block::CacheType; use crate::devices::virtio::block::virtio::metrics::{BlockDeviceMetrics, BlockMetricsPerDevice}; -use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_blk::{ VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_RO, VIRTIO_BLK_ID_BYTES, }; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_BLOCK; use crate::devices::virtio::generated::virtio_ring::VIRTIO_RING_F_EVENT_IDX; use crate::devices::virtio::queue::{InvalidAvailIdx, Queue}; use crate::devices::virtio::transport::{VirtioInterrupt, VirtioInterruptType}; @@ -204,7 +203,7 @@ impl TryFrom<&BlockDeviceConfig> for VirtioBlockConfig { type Error = VirtioBlockError; fn try_from(value: &BlockDeviceConfig) -> Result { - if value.path_on_host.is_some() && value.socket.is_none() { + if let (Some(path_on_host), None) = (&value.path_on_host, &value.socket) { Ok(Self { drive_id: value.drive_id.clone(), partuuid: value.partuuid.clone(), @@ -212,7 +211,7 @@ impl TryFrom<&BlockDeviceConfig> for VirtioBlockConfig { cache_type: value.cache_type, is_read_only: value.is_read_only.unwrap_or(false), - path_on_host: value.path_on_host.as_ref().unwrap().clone(), + path_on_host: path_on_host.clone(), rate_limiter: value.rate_limiter, file_engine_type: value.file_engine_type.unwrap_or_default(), }) @@ -583,7 +582,11 @@ impl VirtioBlock { } impl VirtioDevice for VirtioBlock { - impl_device_type!(VIRTIO_ID_BLOCK); + impl_device_type!(VirtioDeviceType::Block); + + fn id(&self) -> &str { + &self.id + } fn avail_features(&self) -> u64 { self.avail_features @@ -790,7 +793,7 @@ mod tests { for engine in [FileEngineType::Sync, FileEngineType::Async] { let mut block = default_block(engine); - assert_eq!(block.device_type(), VIRTIO_ID_BLOCK); + assert_eq!(block.device_type(), VirtioDeviceType::Block); let features: u64 = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_RING_F_EVENT_IDX); diff --git a/src/vmm/src/devices/virtio/block/virtio/persist.rs b/src/vmm/src/devices/virtio/block/virtio/persist.rs index 380fe1de0e8..bfccc488503 100644 --- a/src/vmm/src/devices/virtio/block/virtio/persist.rs +++ b/src/vmm/src/devices/virtio/block/virtio/persist.rs @@ -12,9 +12,8 @@ use super::*; use crate::devices::virtio::block::persist::BlockConstructorArgs; use crate::devices::virtio::block::virtio::device::FileEngineType; use crate::devices::virtio::block::virtio::metrics::BlockMetricsPerDevice; -use crate::devices::virtio::device::{ActiveState, DeviceState}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_blk::VIRTIO_BLK_F_RO; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_BLOCK; use crate::devices::virtio::persist::VirtioDeviceState; use crate::rate_limiter::RateLimiter; use crate::rate_limiter::persist::RateLimiterState; @@ -102,7 +101,7 @@ impl Persist<'_> for VirtioBlock { .virtio_state .build_queues_checked( &constructor_args.mem, - VIRTIO_ID_BLOCK, + VirtioDeviceType::Block, BLOCK_NUM_QUEUES, FIRECRACKER_MAX_QUEUE_SIZE, ) @@ -230,7 +229,7 @@ mod tests { .unwrap(); // Test that virtio specific fields are the same. - assert_eq!(restored_block.device_type(), VIRTIO_ID_BLOCK); + assert_eq!(restored_block.device_type(), VirtioDeviceType::Block); assert_eq!(restored_block.avail_features(), block.avail_features()); assert_eq!(restored_block.acked_features(), block.acked_features()); assert_eq!(restored_block.queues(), block.queues()); diff --git a/src/vmm/src/devices/virtio/block/virtio/request.rs b/src/vmm/src/devices/virtio/block/virtio/request.rs index 8fc83cf43da..68857fa9444 100644 --- a/src/vmm/src/devices/virtio/block/virtio/request.rs +++ b/src/vmm/src/devices/virtio/block/virtio/request.rs @@ -297,7 +297,7 @@ impl Request { RequestType::In | RequestType::Out => { // Check that the data length is a multiple of 512 as specified in the virtio // standard. - if req.data_len % SECTOR_SIZE != 0 { + if !req.data_len.is_multiple_of(SECTOR_SIZE) { return Err(VirtioBlockError::InvalidDataLength); } let top_sector = req diff --git a/src/vmm/src/devices/virtio/device.rs b/src/vmm/src/devices/virtio/device.rs index 5f7d2170f21..4dfae488740 100644 --- a/src/vmm/src/devices/virtio/device.rs +++ b/src/vmm/src/devices/virtio/device.rs @@ -9,13 +9,15 @@ use std::fmt; use std::sync::Arc; use std::sync::atomic::AtomicU32; +use serde::{Deserialize, Serialize}; use vmm_sys_util::eventfd::EventFd; use super::ActivateError; use super::queue::{Queue, QueueError}; use super::transport::VirtioInterrupt; use crate::devices::virtio::AsAny; -use crate::logger::warn; +use crate::devices::virtio::generated::virtio_ids; +use crate::logger::{error, info, warn}; use crate::vstate::memory::GuestMemoryMmap; /// State of an active VirtIO device @@ -51,6 +53,22 @@ impl DeviceState { } } +/// Type of a virtio device +/// Represent it as u8 to give it a known size. +/// All used types fit in u8. +#[allow(clippy::cast_possible_truncation)] +#[repr(u8)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub enum VirtioDeviceType { + Net = virtio_ids::VIRTIO_ID_NET as u8, + Block = virtio_ids::VIRTIO_ID_BLOCK as u8, + Rng = virtio_ids::VIRTIO_ID_RNG as u8, + Balloon = virtio_ids::VIRTIO_ID_BALLOON as u8, + Vsock = virtio_ids::VIRTIO_ID_VSOCK as u8, + Mem = virtio_ids::VIRTIO_ID_MEM as u8, + Pmem = virtio_ids::VIRTIO_ID_PMEM as u8, +} + /// Trait for virtio devices to be driven by a virtio transport. /// /// The lifecycle of a virtio device is to be moved to a virtio transport, which will then query the @@ -75,14 +93,17 @@ pub trait VirtioDevice: AsAny + Send { } /// The virtio device type (as a constant of the struct). - fn const_device_type() -> u32 + fn const_device_type() -> VirtioDeviceType where Self: Sized; /// The virtio device type. /// /// It should be the same as returned by Self::const_device_type(). - fn device_type(&self) -> u32; + fn device_type(&self) -> VirtioDeviceType; + + /// Returns unique device id + fn id(&self) -> &str; /// Returns the device queues. fn queues(&self) -> &[Queue]; @@ -167,13 +188,33 @@ pub trait VirtioDevice: AsAny + Send { Ok(()) } + /// Notify all queues by writing to the eventfds. + fn notify_queue_events(&mut self) { + info!("[{:?}:{}] notifying queues", self.device_type(), self.id()); + for (i, eventfd) in self.queue_events().iter().enumerate() { + if let Err(err) = eventfd.write(1) { + error!( + "[{:?}:{}] error notifying queue {}: {}", + self.device_type(), + self.id(), + i, + err + ); + } + } + } + /// Kick the device, as if it had received external events. - fn kick(&mut self) {} + fn kick(&mut self) { + if self.is_activated() { + self.notify_queue_events(); + } + } } impl fmt::Debug for dyn VirtioDevice { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "VirtioDevice type {}", self.device_type()) + write!(f, "VirtioDevice type {:?}", self.device_type()) } } @@ -181,11 +222,11 @@ impl fmt::Debug for dyn VirtioDevice { #[macro_export] macro_rules! impl_device_type { ($const_type:expr) => { - fn const_device_type() -> u32 { + fn const_device_type() -> VirtioDeviceType { $const_type } - fn device_type(&self) -> u32 { + fn device_type(&self) -> VirtioDeviceType { Self::const_device_type() } }; @@ -202,7 +243,11 @@ pub(crate) mod tests { } impl VirtioDevice for MockVirtioDevice { - impl_device_type!(0); + impl_device_type!(VirtioDeviceType::Net); + + fn id(&self) -> &str { + "mock" + } fn avail_features(&self) -> u64 { self.avail_features diff --git a/src/vmm/src/devices/virtio/mem/device.rs b/src/vmm/src/devices/virtio/mem/device.rs index 35b56c9bef0..8417a81470a 100644 --- a/src/vmm/src/devices/virtio/mem/device.rs +++ b/src/vmm/src/devices/virtio/mem/device.rs @@ -16,9 +16,8 @@ use vmm_sys_util::eventfd::EventFd; use super::{MEM_NUM_QUEUES, MEM_QUEUE}; use crate::devices::virtio::ActivateError; -use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_MEM; use crate::devices::virtio::generated::virtio_mem::{ self, VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, virtio_mem_config, }; @@ -170,10 +169,6 @@ impl VirtioMem { }) } - pub fn id(&self) -> &str { - VIRTIO_MEM_DEV_ID - } - pub fn guest_address(&self) -> GuestAddress { GuestAddress(self.config.addr) } @@ -570,7 +565,7 @@ impl VirtioMem { return Err(VirtioMemError::DeviceNotActive); } - if requested_size % self.config.block_size != 0 { + if !requested_size.is_multiple_of(self.config.block_size) { return Err(VirtioMemError::InvalidSize(requested_size)); } if requested_size > self.config.region_size { @@ -602,7 +597,11 @@ impl VirtioMem { } impl VirtioDevice for VirtioMem { - impl_device_type!(VIRTIO_ID_MEM); + impl_device_type!(VirtioDeviceType::Mem); + + fn id(&self) -> &str { + VIRTIO_MEM_DEV_ID + } fn queues(&self) -> &[Queue] { &self.queues @@ -743,7 +742,7 @@ mod tests { use vm_memory::mmap::MmapRegionBuilder; use super::*; - use crate::devices::virtio::device::VirtioDevice; + use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::mem::device::test_utils::default_virtio_mem; use crate::devices::virtio::queue::VIRTQ_DESC_F_WRITE; use crate::devices::virtio::test_utils::test::VirtioTestHelper; @@ -757,7 +756,7 @@ mod tests { assert_eq!(mem.block_size_mib(), 2); assert_eq!(mem.plugged_size_mib(), 0); assert_eq!(mem.id(), VIRTIO_MEM_DEV_ID); - assert_eq!(mem.device_type(), VIRTIO_ID_MEM); + assert_eq!(mem.device_type(), VirtioDeviceType::Mem); let features = (1 << VIRTIO_F_VERSION_1) | (1 << VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE); assert_eq!(mem.avail_features(), features); diff --git a/src/vmm/src/devices/virtio/mem/persist.rs b/src/vmm/src/devices/virtio/mem/persist.rs index 345b464c48d..d3865a9865a 100644 --- a/src/vmm/src/devices/virtio/mem/persist.rs +++ b/src/vmm/src/devices/virtio/mem/persist.rs @@ -10,6 +10,7 @@ use serde::{Deserialize, Serialize}; use vm_memory::Address; use crate::Vm; +use crate::devices::virtio::device::VirtioDeviceType; use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_MEM; use crate::devices::virtio::generated::virtio_mem::virtio_mem_config; use crate::devices::virtio::mem::{MEM_NUM_QUEUES, VirtioMem, VirtioMemError}; @@ -74,7 +75,7 @@ impl Persist<'_> for VirtioMem { ) -> Result { let queues = state.virtio_state.build_queues_checked( constructor_args.vm.guest_memory(), - VIRTIO_ID_MEM, + VirtioDeviceType::Mem, MEM_NUM_QUEUES, FIRECRACKER_MAX_QUEUE_SIZE, )?; diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index 4ce89c342c7..fea0c8a916e 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -18,9 +18,8 @@ use vmm_sys_util::eventfd::EventFd; use super::NET_QUEUE_MAX_SIZE; use crate::devices::virtio::ActivateError; -use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_NET; use crate::devices::virtio::generated::virtio_net::{ VIRTIO_NET_F_CSUM, VIRTIO_NET_F_GUEST_CSUM, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6, VIRTIO_NET_F_HOST_UFO, @@ -344,11 +343,6 @@ impl Net { Self::new_with_tap(id, tap, guest_mac, rx_rate_limiter, tx_rate_limiter) } - /// Provides the ID of this net device. - pub fn id(&self) -> &String { - &self.id - } - /// Provides the MAC of this net device. pub fn guest_mac(&self) -> Option<&MacAddr> { self.guest_mac.as_ref() @@ -961,7 +955,11 @@ impl Net { } impl VirtioDevice for Net { - impl_device_type!(VIRTIO_ID_NET); + impl_device_type!(VirtioDeviceType::Net); + + fn id(&self) -> &str { + &self.id + } fn avail_features(&self) -> u64 { self.avail_features @@ -1058,17 +1056,6 @@ impl VirtioDevice for Net { fn is_activated(&self) -> bool { self.device_state.is_activated() } - - fn kick(&mut self) { - // If device is activated, kick the net queue(s) to make up for any - // pending or in-flight epoll events we may have not captured in snapshot. - // No need to kick Ratelimiters because they are restored 'unblocked' so - // any inflight `timer_fd` events can be safely discarded. - if self.is_activated() { - info!("kick net {}.", self.id()); - self.process_virtio_queues(); - } - } } #[cfg(test)] @@ -1153,7 +1140,7 @@ pub mod tests { fn test_virtio_device_type() { let mut net = default_net(); set_mac(&mut net, MacAddr::from_str("11:22:33:44:55:66").unwrap()); - assert_eq!(net.device_type(), VIRTIO_ID_NET); + assert_eq!(net.device_type(), VirtioDeviceType::Net); } #[test] diff --git a/src/vmm/src/devices/virtio/net/persist.rs b/src/vmm/src/devices/virtio/net/persist.rs index ba56cc39aac..24c8d99f166 100644 --- a/src/vmm/src/devices/virtio/net/persist.rs +++ b/src/vmm/src/devices/virtio/net/persist.rs @@ -10,8 +10,7 @@ use serde::{Deserialize, Serialize}; use super::device::{Net, RxBuffers}; use super::{NET_NUM_QUEUES, NET_QUEUE_MAX_SIZE, RX_INDEX, TapError}; -use crate::devices::virtio::device::{ActiveState, DeviceState}; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_NET; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDeviceType}; use crate::devices::virtio::persist::{PersistError as VirtioStateError, VirtioDeviceState}; use crate::devices::virtio::transport::VirtioInterrupt; use crate::mmds::data_store::Mmds; @@ -75,7 +74,7 @@ impl Persist<'_> for Net { fn save(&self) -> Self::State { NetState { - id: self.id().clone(), + id: self.id.clone(), tap_if_name: self.iface_name(), rx_rate_limiter_state: self.rx_rate_limiter.save(), tx_rate_limiter_state: self.tx_rate_limiter.save(), @@ -121,7 +120,7 @@ impl Persist<'_> for Net { net.queues = state.virtio_state.build_queues_checked( &constructor_args.mem, - VIRTIO_ID_NET, + VirtioDeviceType::Net, NET_NUM_QUEUES, NET_QUEUE_MAX_SIZE, )?; @@ -181,7 +180,7 @@ mod tests { ) { Ok(restored_net) => { // Test that virtio specific fields are the same. - assert_eq!(restored_net.device_type(), VIRTIO_ID_NET); + assert_eq!(restored_net.device_type(), VirtioDeviceType::Net); assert_eq!(restored_net.avail_features(), virtio_state.avail_features); assert_eq!(restored_net.acked_features(), virtio_state.acked_features); assert_eq!(restored_net.is_activated(), virtio_state.activated); diff --git a/src/vmm/src/devices/virtio/persist.rs b/src/vmm/src/devices/virtio/persist.rs index 85c4940f305..3572107369c 100644 --- a/src/vmm/src/devices/virtio/persist.rs +++ b/src/vmm/src/devices/virtio/persist.rs @@ -11,7 +11,7 @@ use serde::{Deserialize, Serialize}; use super::queue::{InvalidAvailIdx, QueueError}; use super::transport::mmio::IrqTrigger; -use crate::devices::virtio::device::VirtioDevice; +use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_ring::VIRTIO_RING_F_EVENT_IDX; use crate::devices::virtio::queue::Queue; use crate::devices::virtio::transport::mmio::MmioTransport; @@ -114,10 +114,10 @@ impl Persist<'_> for Queue { } /// State of a VirtioDevice. -#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] pub struct VirtioDeviceState { /// Device type. - pub device_type: u32, + pub device_type: VirtioDeviceType, /// Available virtio features. pub avail_features: u64, /// Negotiated virtio features. @@ -145,7 +145,7 @@ impl VirtioDeviceState { pub fn build_queues_checked( &self, mem: &GuestMemoryMmap, - expected_device_type: u32, + expected_device_type: VirtioDeviceType, expected_num_queues: usize, expected_queue_max_size: u16, ) -> Result, PersistError> { @@ -290,31 +290,45 @@ mod tests { #[test] fn test_virtiodev_sanity_checks() { let max_size = DEFAULT_QUEUE_MAX_SIZE; - let mut state = VirtioDeviceState::default(); + let mut state = VirtioDeviceState { + device_type: VirtioDeviceType::Net, + avail_features: 0, + acked_features: 0, + queues: vec![], + activated: false, + }; let mem = default_mem(); // Valid checks. - state.build_queues_checked(&mem, 0, 0, max_size).unwrap(); + state + .build_queues_checked(&mem, VirtioDeviceType::Net, 0, max_size) + .unwrap(); // Invalid dev-type. state - .build_queues_checked(&mem, 1, 0, max_size) + .build_queues_checked(&mem, VirtioDeviceType::Block, 0, max_size) .unwrap_err(); // Invalid num-queues. state - .build_queues_checked(&mem, 0, 1, max_size) + .build_queues_checked(&mem, VirtioDeviceType::Net, 1, max_size) .unwrap_err(); // Unavailable features acked. state.acked_features = 1; state - .build_queues_checked(&mem, 0, 0, max_size) + .build_queues_checked(&mem, VirtioDeviceType::Net, 0, max_size) .unwrap_err(); // Validate queue sanity checks. - let mut state = VirtioDeviceState::default(); + let mut state = VirtioDeviceState { + device_type: VirtioDeviceType::Net, + avail_features: 0, + acked_features: 0, + queues: vec![], + activated: false, + }; let good_q = QueueState::default(); state.queues = vec![good_q]; // Valid. state - .build_queues_checked(&mem, 0, state.queues.len(), max_size) + .build_queues_checked(&mem, VirtioDeviceType::Net, state.queues.len(), max_size) .unwrap(); // Invalid max queue size. @@ -324,7 +338,7 @@ mod tests { }; state.queues = vec![bad_q]; state - .build_queues_checked(&mem, 0, state.queues.len(), max_size) + .build_queues_checked(&mem, VirtioDeviceType::Net, state.queues.len(), max_size) .unwrap_err(); // Invalid: size > max. @@ -335,7 +349,7 @@ mod tests { state.queues = vec![bad_q]; state.activated = true; state - .build_queues_checked(&mem, 0, state.queues.len(), max_size) + .build_queues_checked(&mem, VirtioDeviceType::Net, state.queues.len(), max_size) .unwrap_err(); // activated && !q.is_valid() @@ -343,7 +357,7 @@ mod tests { state.queues = vec![bad_q]; state.activated = true; state - .build_queues_checked(&mem, 0, state.queues.len(), max_size) + .build_queues_checked(&mem, VirtioDeviceType::Net, state.queues.len(), max_size) .unwrap_err(); } diff --git a/src/vmm/src/devices/virtio/pmem/device.rs b/src/vmm/src/devices/virtio/pmem/device.rs index ce426f2db47..e05a3291c9d 100644 --- a/src/vmm/src/devices/virtio/pmem/device.rs +++ b/src/vmm/src/devices/virtio/pmem/device.rs @@ -15,9 +15,8 @@ use vm_memory::{GuestAddress, GuestMemoryError}; use vmm_sys_util::eventfd::EventFd; use crate::devices::virtio::ActivateError; -use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_PMEM; use crate::devices::virtio::pmem::PMEM_QUEUE_SIZE; use crate::devices::virtio::pmem::metrics::{PmemMetrics, PmemMetricsPerDevice}; use crate::devices::virtio::queue::{DescriptorChain, InvalidAvailIdx, Queue, QueueError}; @@ -331,7 +330,11 @@ impl Pmem { } impl VirtioDevice for Pmem { - impl_device_type!(VIRTIO_ID_PMEM); + impl_device_type!(VirtioDeviceType::Pmem); + + fn id(&self) -> &str { + &self.config.id + } fn avail_features(&self) -> u64 { self.avail_features diff --git a/src/vmm/src/devices/virtio/pmem/persist.rs b/src/vmm/src/devices/virtio/pmem/persist.rs index a8089433b86..d27c2356979 100644 --- a/src/vmm/src/devices/virtio/pmem/persist.rs +++ b/src/vmm/src/devices/virtio/pmem/persist.rs @@ -6,8 +6,7 @@ use vm_memory::GuestAddress; use super::device::{ConfigSpace, Pmem, PmemError}; use crate::Vm; -use crate::devices::virtio::device::DeviceState; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_PMEM; +use crate::devices::virtio::device::{DeviceState, VirtioDeviceType}; use crate::devices::virtio::persist::{PersistError as VirtioStateError, VirtioDeviceState}; use crate::devices::virtio::pmem::{PMEM_NUM_QUEUES, PMEM_QUEUE_SIZE}; use crate::snapshot::Persist; @@ -57,7 +56,7 @@ impl<'a> Persist<'a> for Pmem { ) -> Result { let queues = state.virtio_state.build_queues_checked( constructor_args.mem, - VIRTIO_ID_PMEM, + VirtioDeviceType::Pmem, PMEM_NUM_QUEUES, PMEM_QUEUE_SIZE, )?; @@ -120,7 +119,7 @@ mod tests { .unwrap(); // Test that virtio specific fields are the same. - assert_eq!(restored_pmem.device_type(), VIRTIO_ID_PMEM); + assert_eq!(restored_pmem.device_type(), VirtioDeviceType::Pmem); assert_eq!(restored_pmem.avail_features(), pmem.avail_features()); assert_eq!(restored_pmem.acked_features(), pmem.acked_features()); assert_eq!(restored_pmem.queues(), pmem.queues()); diff --git a/src/vmm/src/devices/virtio/rng/device.rs b/src/vmm/src/devices/virtio/rng/device.rs index 2f9efd80909..2c599f49ac9 100644 --- a/src/vmm/src/devices/virtio/rng/device.rs +++ b/src/vmm/src/devices/virtio/rng/device.rs @@ -14,9 +14,8 @@ use super::metrics::METRICS; use super::{RNG_NUM_QUEUES, RNG_QUEUE}; use crate::devices::DeviceError; use crate::devices::virtio::ActivateError; -use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_RNG; use crate::devices::virtio::iov_deque::IovDequeError; use crate::devices::virtio::iovec::IoVecBufferMut; use crate::devices::virtio::queue::{FIRECRACKER_MAX_QUEUE_SIZE, InvalidAvailIdx, Queue}; @@ -85,10 +84,6 @@ impl Entropy { }) } - pub fn id(&self) -> &str { - ENTROPY_DEV_ID - } - fn signal_used_queue(&self) -> Result<(), DeviceError> { self.interrupt_trigger() .trigger(VirtioInterruptType::Queue(RNG_QUEUE.try_into().unwrap())) @@ -254,7 +249,11 @@ impl Entropy { } impl VirtioDevice for Entropy { - impl_device_type!(VIRTIO_ID_RNG); + impl_device_type!(VirtioDeviceType::Rng); + + fn id(&self) -> &str { + ENTROPY_DEV_ID + } fn queues(&self) -> &[Queue] { &self.queues @@ -313,13 +312,6 @@ impl VirtioDevice for Entropy { self.device_state = DeviceState::Activated(ActiveState { mem, interrupt }); Ok(()) } - - fn kick(&mut self) { - if self.is_activated() { - info!("kick entropy {}.", self.id()); - self.process_virtio_queues(); - } - } } #[cfg(test)] @@ -328,7 +320,7 @@ mod tests { use super::*; use crate::check_metric_after_block; - use crate::devices::virtio::device::VirtioDevice; + use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::queue::VIRTQ_DESC_F_WRITE; use crate::devices::virtio::test_utils::test::{ VirtioTestDevice, VirtioTestHelper, create_virtio_mem, @@ -366,7 +358,7 @@ mod tests { #[test] fn test_device_type() { let entropy_dev = default_entropy(); - assert_eq!(entropy_dev.device_type(), VIRTIO_ID_RNG); + assert_eq!(entropy_dev.device_type(), VirtioDeviceType::Rng); } #[test] diff --git a/src/vmm/src/devices/virtio/rng/persist.rs b/src/vmm/src/devices/virtio/rng/persist.rs index 27df145eb81..361e943ddda 100644 --- a/src/vmm/src/devices/virtio/rng/persist.rs +++ b/src/vmm/src/devices/virtio/rng/persist.rs @@ -7,7 +7,7 @@ use std::sync::Arc; use serde::{Deserialize, Serialize}; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_RNG; +use crate::devices::virtio::device::VirtioDeviceType; use crate::devices::virtio::persist::{PersistError as VirtioStateError, VirtioDeviceState}; use crate::devices::virtio::queue::FIRECRACKER_MAX_QUEUE_SIZE; use crate::devices::virtio::rng::{Entropy, EntropyError, RNG_NUM_QUEUES}; @@ -56,7 +56,7 @@ impl Persist<'_> for Entropy { ) -> Result { let queues = state.virtio_state.build_queues_checked( &constructor_args.mem, - VIRTIO_ID_RNG, + VirtioDeviceType::Rng, RNG_NUM_QUEUES, FIRECRACKER_MAX_QUEUE_SIZE, )?; @@ -98,7 +98,7 @@ mod tests { ) .unwrap(); - assert_eq!(restored.device_type(), VIRTIO_ID_RNG); + assert_eq!(restored.device_type(), VirtioDeviceType::Rng); assert_eq!(restored.id(), ENTROPY_DEV_ID); assert!(!restored.is_activated()); assert!(!entropy.is_activated()); diff --git a/src/vmm/src/devices/virtio/transport/mmio.rs b/src/vmm/src/devices/virtio/transport/mmio.rs index d98dd4ce365..1a252c4d000 100644 --- a/src/vmm/src/devices/virtio/transport/mmio.rs +++ b/src/vmm/src/devices/virtio/transport/mmio.rs @@ -241,7 +241,7 @@ impl BusDevice for MmioTransport { let v = match offset { 0x0 => MMIO_MAGIC_VALUE, 0x04 => MMIO_VERSION, - 0x08 => self.locked_device().device_type(), + 0x08 => self.locked_device().device_type() as u32, 0x0c => VENDOR_ID, // vendor id 0x10 => { let mut features = self @@ -486,6 +486,7 @@ pub(crate) mod tests { use super::*; use crate::devices::virtio::ActivateError; + use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::device_status::DEVICE_NEEDS_RESET; use crate::impl_device_type; use crate::test_utils::single_region_mem; @@ -528,7 +529,11 @@ pub(crate) mod tests { } impl VirtioDevice for DummyDevice { - impl_device_type!(123); + impl_device_type!(VirtioDeviceType::Rng); + + fn id(&self) -> &str { + "dummy" + } fn avail_features(&self) -> u64 { self.avail_features @@ -656,7 +661,10 @@ pub(crate) mod tests { assert_eq!(read_le_u32(&buf[..]), MMIO_VERSION); d.read(0x0, 0x08, &mut buf[..]); - assert_eq!(read_le_u32(&buf[..]), d.locked_device().device_type()); + assert_eq!( + read_le_u32(&buf[..]), + d.locked_device().device_type() as u32, + ); d.read(0x0, 0x0c, &mut buf[..]); assert_eq!(read_le_u32(&buf[..]), VENDOR_ID); diff --git a/src/vmm/src/devices/virtio/transport/pci/device.rs b/src/vmm/src/devices/virtio/transport/pci/device.rs index f0cc8bdefc7..ef0e3fe0fcd 100644 --- a/src/vmm/src/devices/virtio/transport/pci/device.rs +++ b/src/vmm/src/devices/virtio/transport/pci/device.rs @@ -28,7 +28,7 @@ use vmm_sys_util::errno; use vmm_sys_util::eventfd::EventFd; use crate::Vm; -use crate::devices::virtio::device::VirtioDevice; +use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_ids; use crate::devices::virtio::queue::Queue; use crate::devices::virtio::transport::pci::common_config::{ @@ -299,16 +299,16 @@ impl Debug for VirtioPciDevice { impl VirtioPciDevice { fn pci_configuration( - virtio_device_type: u32, + device_type: VirtioDeviceType, msix_config: &Arc>, ) -> PciConfiguration { - let pci_device_id = VIRTIO_PCI_DEVICE_ID_BASE + u16::try_from(virtio_device_type).unwrap(); - let (class, subclass) = match virtio_device_type { - virtio_ids::VIRTIO_ID_NET => ( + let pci_device_id = VIRTIO_PCI_DEVICE_ID_BASE + device_type as u16; + let (class, subclass) = match device_type { + VirtioDeviceType::Net => ( PciClassCode::NetworkController, &PciNetworkControllerSubclass::EthernetController as &dyn PciSubclass, ), - virtio_ids::VIRTIO_ID_BLOCK => ( + VirtioDeviceType::Block => ( PciClassCode::MassStorage, &PciMassStorageSubclass::MassStorage as &dyn PciSubclass, ), @@ -959,10 +959,9 @@ mod tests { use super::{PciCapabilityType, VirtioPciDevice}; use crate::arch::MEM_64BIT_DEVICES_START; use crate::builder::tests::default_vmm; - use crate::devices::virtio::device::VirtioDevice; + use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::device_status::{ACKNOWLEDGE, DRIVER, DRIVER_OK, FEATURES_OK}; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; - use crate::devices::virtio::generated::virtio_ids; use crate::devices::virtio::rng::Entropy; use crate::devices::virtio::transport::pci::device::{ COMMON_CONFIG_BAR_OFFSET, COMMON_CONFIG_SIZE, DEVICE_CONFIG_BAR_OFFSET, DEVICE_CONFIG_SIZE, @@ -995,7 +994,7 @@ mod tests { fn get_virtio_device(vmm: &Vmm) -> Arc> { vmm.device_manager .pci_devices - .get_virtio_device(virtio_ids::VIRTIO_ID_RNG, "rng") + .get_virtio_device(VirtioDeviceType::Rng, "rng") .unwrap() .clone() } diff --git a/src/vmm/src/devices/virtio/vsock/device.rs b/src/vmm/src/devices/virtio/vsock/device.rs index 7fe10d158ad..e914a03bdbe 100644 --- a/src/vmm/src/devices/virtio/vsock/device.rs +++ b/src/vmm/src/devices/virtio/vsock/device.rs @@ -32,9 +32,8 @@ use super::defs::uapi; use super::packet::{VSOCK_PKT_HDR_SIZE, VsockPacketRx, VsockPacketTx}; use super::{VsockBackend, defs}; use crate::devices::virtio::ActivateError; -use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_config::{VIRTIO_F_IN_ORDER, VIRTIO_F_VERSION_1}; -use crate::devices::virtio::generated::virtio_ids::VIRTIO_ID_VSOCK; use crate::devices::virtio::queue::{InvalidAvailIdx, Queue as VirtQueue}; use crate::devices::virtio::transport::{VirtioInterrupt, VirtioInterruptType}; use crate::devices::virtio::vsock::VsockError; @@ -122,11 +121,6 @@ where Self::with_queues(cid, backend, queues) } - /// Provides the ID of this vsock device as used in MMIO device identification. - pub fn id(&self) -> &str { - defs::VSOCK_DEV_ID - } - /// Retrieve the cid associated with this vsock device. pub fn cid(&self) -> u64 { self.cid @@ -284,7 +278,11 @@ impl VirtioDevice for Vsock where B: VsockBackend + Debug + 'static, { - impl_device_type!(VIRTIO_ID_VSOCK); + impl_device_type!(VirtioDeviceType::Vsock); + + fn id(&self) -> &str { + defs::VSOCK_DEV_ID + } fn avail_features(&self) -> u64 { self.avail_features @@ -387,7 +385,11 @@ where // The only reason we still `kick` it is to make guest process // `TRANSPORT_RESET_EVENT` event we sent during snapshot creation. if self.is_activated() { - info!("kick vsock {}.", self.id()); + info!( + "[{:?}:{}] signaling event queue", + self.device_type(), + self.id() + ); self.signal_used_queue(EVQ_INDEX).unwrap(); } } @@ -412,7 +414,7 @@ mod tests { (driver_features & 0xffff_ffff) as u32, (driver_features >> 32) as u32, ]; - assert_eq!(ctx.device.device_type(), VIRTIO_ID_VSOCK); + assert_eq!(ctx.device.device_type(), VirtioDeviceType::Vsock); assert_eq!(ctx.device.avail_features_by_page(0), device_pages[0]); assert_eq!(ctx.device.avail_features_by_page(1), device_pages[1]); assert_eq!(ctx.device.avail_features_by_page(2), 0); diff --git a/src/vmm/src/devices/virtio/vsock/persist.rs b/src/vmm/src/devices/virtio/vsock/persist.rs index acf330a3e71..ed98563d3ae 100644 --- a/src/vmm/src/devices/virtio/vsock/persist.rs +++ b/src/vmm/src/devices/virtio/vsock/persist.rs @@ -9,8 +9,7 @@ use std::sync::Arc; use serde::{Deserialize, Serialize}; use super::*; -use crate::devices::virtio::device::{ActiveState, DeviceState}; -use crate::devices::virtio::generated::virtio_ids::{self, VIRTIO_ID_VSOCK}; +use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDeviceType}; use crate::devices::virtio::persist::VirtioDeviceState; use crate::devices::virtio::queue::FIRECRACKER_MAX_QUEUE_SIZE; use crate::devices::virtio::transport::VirtioInterrupt; @@ -112,7 +111,7 @@ where .virtio_state .build_queues_checked( &constructor_args.mem, - VIRTIO_ID_VSOCK, + VirtioDeviceType::Vsock, defs::VSOCK_NUM_QUEUES, FIRECRACKER_MAX_QUEUE_SIZE, ) @@ -197,7 +196,7 @@ pub(crate) mod tests { ) .unwrap(); - assert_eq!(restored_device.device_type(), VIRTIO_ID_VSOCK); + assert_eq!(restored_device.device_type(), VirtioDeviceType::Vsock); assert_eq!(restored_device.avail_features_by_page(0), device_pages[0]); assert_eq!(restored_device.avail_features_by_page(1), device_pages[1]); assert_eq!(restored_device.avail_features_by_page(2), 0); diff --git a/src/vmm/src/dumbo/pdu/mod.rs b/src/vmm/src/dumbo/pdu/mod.rs index fc6a24760a7..06a6035cbb8 100644 --- a/src/vmm/src/dumbo/pdu/mod.rs +++ b/src/vmm/src/dumbo/pdu/mod.rs @@ -96,7 +96,7 @@ fn compute_checksum( sum += usize::from(bytes.ntohs_unchecked(i * 2)); } - if len % 2 != 0 { + if !len.is_multiple_of(2) { sum += usize::from(bytes[len - 1]) << 8; } diff --git a/src/vmm/src/rate_limiter/mod.rs b/src/vmm/src/rate_limiter/mod.rs index abef851d251..2e411b8ca5a 100644 --- a/src/vmm/src/rate_limiter/mod.rs +++ b/src/vmm/src/rate_limiter/mod.rs @@ -152,7 +152,7 @@ impl TokenBucket { // we would allow some fraction of a nano second to be used twice, allowing // for the generation of one extra token in extreme circumstances). let mut time_adjustment = tokens * processed_refill_time / processed_capacity; - if tokens * processed_refill_time % processed_capacity != 0 { + if !(tokens * processed_refill_time).is_multiple_of(processed_capacity) { time_adjustment += 1; } diff --git a/src/vmm/src/resources.rs b/src/vmm/src/resources.rs index e2faeffaf56..a7af85fbc99 100644 --- a/src/vmm/src/resources.rs +++ b/src/vmm/src/resources.rs @@ -9,6 +9,7 @@ use serde::{Deserialize, Serialize}; use vm_memory::GuestAddress; use crate::cpu_config::templates::CustomCpuTemplate; +use crate::devices::virtio::device::VirtioDevice; use crate::logger::info; use crate::mmds; use crate::mmds::data_store::{Mmds, MmdsVersion}; @@ -293,7 +294,9 @@ impl VmResources { for net_dev in net_devs_with_mmds { let net = net_dev.lock().unwrap(); - inner_mmds_config.network_interfaces.push(net.id().clone()); + inner_mmds_config + .network_interfaces + .push(net.id().to_string()); // Only need to get one ip address, as they will all be equal. if inner_mmds_config.ipv4_address.is_none() { // Safe to unwrap the mmds_ns as the filter() explicitly checks for @@ -434,8 +437,7 @@ impl VmResources { if !network_interfaces.iter().all(|id| { self.net_builder .iter() - .map(|device| device.lock().expect("Poisoned lock").id().clone()) - .any(|x| &x == id) + .any(|device| device.lock().expect("Poisoned lock").id() == id) }) { return Err(MmdsConfigError::InvalidNetworkInterfaceId); } @@ -448,7 +450,7 @@ impl VmResources { // network interface ID list. for net_device in self.net_builder.iter() { let mut net_device_lock = net_device.lock().expect("Poisoned lock"); - if network_interfaces.contains(net_device_lock.id()) { + if network_interfaces.contains(&net_device_lock.id) { net_device_lock.configure_mmds_network_stack(ipv4_addr, mmds.clone()); } else { net_device_lock.disable_mmds_network_stack(); @@ -554,6 +556,7 @@ mod tests { use crate::cpu_config::templates::{CpuTemplateType, StaticCpuTemplate}; use crate::devices::virtio::block::virtio::VirtioBlockError; use crate::devices::virtio::block::{BlockError, CacheType}; + use crate::devices::virtio::device::VirtioDevice; use crate::devices::virtio::vsock::VSOCK_DEV_ID; use crate::resources::VmResources; use crate::utils::net::mac::MacAddr; diff --git a/src/vmm/src/vmm_config/drive.rs b/src/vmm/src/vmm_config/drive.rs index adf8083f74d..2d3fddac830 100644 --- a/src/vmm/src/vmm_config/drive.rs +++ b/src/vmm/src/vmm_config/drive.rs @@ -12,6 +12,7 @@ use crate::VmmError; use crate::devices::virtio::block::device::Block; pub use crate::devices::virtio::block::virtio::device::FileEngineType; use crate::devices::virtio::block::{BlockError, CacheType}; +use crate::devices::virtio::device::VirtioDevice; /// Errors associated with the operations allowed on a drive. #[derive(Debug, thiserror::Error, displaydoc::Display)] diff --git a/src/vmm/src/vmm_config/machine_config.rs b/src/vmm/src/vmm_config/machine_config.rs index e337a5a9dcd..a975f5217ad 100644 --- a/src/vmm/src/vmm_config/machine_config.rs +++ b/src/vmm/src/vmm_config/machine_config.rs @@ -52,7 +52,7 @@ impl HugePageConfig { HugePageConfig::Hugetlbfs2M => 2, }; - mem_size_mib % divisor == 0 + mem_size_mib.is_multiple_of(divisor) } /// Returns the flags required to pass to `mmap`, in addition to `MAP_ANONYMOUS`, to diff --git a/src/vmm/src/vmm_config/net.rs b/src/vmm/src/vmm_config/net.rs index 81a2db49f33..8bbae0f60db 100644 --- a/src/vmm/src/vmm_config/net.rs +++ b/src/vmm/src/vmm_config/net.rs @@ -9,6 +9,7 @@ use serde::{Deserialize, Serialize}; use super::RateLimiterConfig; use crate::VmmError; +use crate::devices::virtio::device::VirtioDevice; use crate::devices::virtio::net::{Net, TapError}; use crate::utils::net::mac::MacAddr; @@ -34,7 +35,7 @@ impl From<&Net> for NetworkInterfaceConfig { let rx_rl: RateLimiterConfig = net.rx_rate_limiter().into(); let tx_rl: RateLimiterConfig = net.tx_rate_limiter().into(); NetworkInterfaceConfig { - iface_id: net.id().clone(), + iface_id: net.id().to_string(), host_dev_name: net.iface_name(), guest_mac: net.guest_mac().copied(), rx_rate_limiter: rx_rl.into_option(), @@ -108,7 +109,7 @@ impl NetBuilder { let mac_conflict = |net: &Arc>| { let net = net.lock().expect("Poisoned lock"); // Check if another net dev has same MAC. - Some(mac_address) == net.guest_mac() && &netif_config.iface_id != net.id() + Some(mac_address) == net.guest_mac() && netif_config.iface_id != net.id() }; // Validate there is no Mac conflict. // No need to validate host_dev_name conflict. In such a case, @@ -124,7 +125,7 @@ impl NetBuilder { if let Some(index) = self .net_devices .iter() - .position(|net| net.lock().expect("Poisoned lock").id() == &netif_config.iface_id) + .position(|net| net.lock().expect("Poisoned lock").id() == netif_config.iface_id) { self.net_devices.swap_remove(index); } diff --git a/src/vmm/src/vmm_config/vsock.rs b/src/vmm/src/vmm_config/vsock.rs index 920e4a4d217..b51ba4b7946 100644 --- a/src/vmm/src/vmm_config/vsock.rs +++ b/src/vmm/src/vmm_config/vsock.rs @@ -115,6 +115,7 @@ pub(crate) mod tests { use vmm_sys_util::tempfile::TempFile; use super::*; + use crate::devices::virtio::device::VirtioDevice; use crate::devices::virtio::vsock::VSOCK_DEV_ID; pub(crate) fn default_config(tmp_sock_file: &TempFile) -> VsockDeviceConfig { diff --git a/tests/README.md b/tests/README.md index ae1a2d7756d..7aa88430677 100644 --- a/tests/README.md +++ b/tests/README.md @@ -251,26 +251,6 @@ schedule an A/B-Test in buildkite, the `REVISION_A` and `REVISION_B` environment variables need to be set in the "Environment Variables" field under "Options" in buildkite's "New Build" modal. -### A/B visualization - -To create visualization of A/B runs use `tools/ab_plot.py` script. It supports -creating `pdf` and `table` outputs with multiple directories as inputs. Example -usage: - -```sh - ./tools/ab_plot.py a_path b_path --output_type pdf -``` - -Alternatively using `devtool` running the script in the dev container with -pre-installed dependencies. - -```sh - ./tools/devtool sh ./tools/ab_plot.py a_path b_path --output_type pdf -``` - -> [!NOTE] Generating `pdf` output may take some time for tests with a lot of -> permutations. - ### Beyond commit comparisons While our automated A/B-Testing suite only supports A/B-Tests across commit @@ -279,26 +259,37 @@ arbitrary environment (such as comparison how the same Firecracker binary behaves on different hosts). For this, run the desired tests in your environments using `devtool` as you -would for a non-A/B test. The only difference to a normal test run is you should -set two environment variables: `AWS_EMF_ENVIRONMENT=local` and -`AWS_EMF_NAMESPACE=local`: +would for a non-A/B test. This will produce `test_results` directories which +will contain `metrics.json` files for each run test. + +The `tools/ab_test.py` script can find and use these `metrics.json` files in the +provided directories to compare runs: ```sh -AWS_EMF_ENVIRONMENT=local AWS_EMF_NAMESPACE=local tools/devtool -y test -- integration_tests/performance/test_boottime.py::test_boottime +tools/ab_test.py analyze ``` -This instructs `aws_embedded_metrics` to dump all data series that our A/B-Test -orchestration would analyze to `stdout`, and pytest will capture this output -into a file stored at `./test_results/test-report.json`. +This will then print the same analysis described in the previous sections. -The `tools/ab_test.py` script can consume these test reports, so next collect -your two test report files to your local machine and run +#### Visualization + +To create visualization of A/B runs use `tools/ab_plot.py` script. It supports +creating `pdf` and `table` outputs using same `metrics.json` files used by +`tools/ab_test.py`. Example usage: ```sh -tools/ab_test.py analyze + ./tools/ab_plot.py --output_type pdf ``` -This will then print the same analysis described in the previous sections. +Alternatively using `devtool` running the script in the dev container with +pre-installed dependencies. + +```sh + ./tools/devtool sh ./tools/ab_plot.py --output_type pdf +``` + +> [!NOTE] Generating `pdf` output may take some time for tests with a lot of +> permutations. #### Troubleshooting diff --git a/tests/fmt.toml b/tests/fmt.toml deleted file mode 100644 index 003e0604fc3..00000000000 --- a/tests/fmt.toml +++ /dev/null @@ -1,8 +0,0 @@ -comment_width=100 -wrap_comments=true -format_code_in_doc_comments=true -format_strings=true -imports_granularity=Module -normalize_comments=true -normalize_doc_attributes=true -group_imports=StdExternalCrate \ No newline at end of file diff --git a/tests/integration_tests/style/test_rust.py b/tests/integration_tests/style/test_rust.py index a2278ebf936..f5247cf79cf 100644 --- a/tests/integration_tests/style/test_rust.py +++ b/tests/integration_tests/style/test_rust.py @@ -18,11 +18,8 @@ def test_rust_order(): def test_rust_style(): """Test that rust code passes style checks.""" - - # ../src/io_uring/src/bindings.rs - config = open("fmt.toml", encoding="utf-8").read().replace("\n", ",") # Check that the output is empty. - _, stdout, _ = utils.check_output(f"cargo fmt --all -- --check --config {config}") + _, stdout, _ = utils.check_output("cargo fmt --all -- --check") # rustfmt prepends `"Diff in"` to the reported output. assert "Diff in" not in stdout diff --git a/tools/ab_test.py b/tools/ab_test.py index 8998f457f25..cbba534390d 100755 --- a/tools/ab_test.py +++ b/tools/ab_test.py @@ -206,11 +206,7 @@ def collect_data(tag: str, binary_dir: Path, pytest_opts: str): test_report_path = f"{test_path}/test-report.json" subprocess.run( f"./tools/test.sh --binary-dir={binary_dir} {pytest_opts} -m '' --json-report-file=../{test_report_path}", - env=os.environ - | { - "AWS_EMF_ENVIRONMENT": "local", - "AWS_EMF_NAMESPACE": "local", - }, + env=os.environ, check=True, shell=True, ) diff --git a/tools/devtool b/tools/devtool index dd95860695c..0316f916e9b 100755 --- a/tools/devtool +++ b/tools/devtool @@ -686,6 +686,8 @@ cmd_test() { do_build=1 do_archive=1 do_kvm_check=1 + do_build_dir_check=1 + do_ci_artifacts_check=1 # Parse any command line args. while [ $# -gt 0 ]; do case "$1" in @@ -713,6 +715,12 @@ cmd_test() { "--no-kvm-check") do_kvm_check=0 ;; + "--no-build-dir-check") + do_build_dir_check=0 + ;; + "--no-ci-artifacts-check") + do_ci_artifacts_check=0 + ;; "--") { shift; break; } ;; *) die "Unknown argument: $1. Please use --help for help." @@ -724,8 +732,8 @@ cmd_test() { # Check prerequisites. [ $do_kvm_check != 0 ] && ensure_kvm ensure_devctr - ensure_build_dir - ensure_ci_artifacts + [ $do_build_dir_check != 0 ] && ensure_build_dir + [ $do_ci_artifacts_check != 0 ] && ensure_ci_artifacts if [ $do_build != 0 ]; then cmd_build --release if [ -n "$BUILDKITE_PULL_REQUEST_BASE_BRANCH" ]; then @@ -942,7 +950,7 @@ cmd_test_debug() { # Example: `devtool fmt` # cmd_fmt() { - cmd_sh "cargo fmt --all -- --config $(tr '\n' ','