openvmm: a chipset device whose add fails stays a target of its line sets

When a chipset device's arc_mutex_device(..).add(..) fails because its state unit's name is already in use, the device stays a target of the line sets it asked for. If a caller goes on with the builder anyway, a later start of those line-set units drives the device, changing its state with no state unit to save or restore it. No caller does this: BaseChipsetBuilder::build and the microVM's load both return the add's error, so this is a latent lifecycle hazard off the microVM path, not a reachable failure.

Reproduction

Register a unit named ioapic, then add an IO-APIC device of the same name with add_line_target(IRQ_LINE_SET, 0..=23, 0). The add returns NameInUse and should leave nothing behind, but the IRQ line set's inspection still holds "ioapic:0-23":"0-23", and the PIT's line 2 still lists ioapic among its targets. The test a_failed_spawn_leaves_no_line_target that fix.patch adds fails at the pin.

Cause

ArcMutexChipsetServicesFinalize::finalize adds the device to its line sets before it spawns the device's unit:

// vmm_core/vmotherboard/src/chipset/backing/arc_mutex/services.rs
    for (id, source_range, target_start) in self.line_set_targets {
        // ...
        line_set.add_target(source_range, target_start, dev_name.clone(), device.clone());
        builder = builder.dependency_of(handle);
    }
}
// ...
let unit = builder
    .spawn(driver.clone(), |recv| device_unit.run(recv))
    .map_err(FinalizeError::NameInUse)?;

When the spawn returns NameInUse, nothing removes the targets already added.

Fix

fix.patch keeps the coverage assert and the dependency_of edges before the spawn. It moves add_target into a second loop that runs only after the spawn succeeds, using the same line sets and the same order. On the success path the line sets are still stopped at build time, so the result is identical. Two tests are added in vmm_core/vmotherboard/src/chipset/builder/mod.rs: a_failed_spawn_leaves_no_line_target (it fails before the fix and passes after), and the control a_normal_build_keeps_its_line_sets (the PIT, PIC, IO-APIC and BSP LINT mappings and target order are unchanged). For the tests' inspections, the patch adds inspect with its initiate feature as a dev-dependency of vmotherboard.

Proposed patch

Patch against OpenVMM 3cf58a5234ed824435825c9f87187f5129219724. Save the contents below as fix.patch and run git apply fix.patch.

fix.patch

diff --git a/vmm_core/vmotherboard/Cargo.toml b/vmm_core/vmotherboard/Cargo.toml
index b71f155..a834bf1 100644
--- a/vmm_core/vmotherboard/Cargo.toml
+++ b/vmm_core/vmotherboard/Cargo.toml
@@ -58,6 +58,7 @@ thiserror.workspace = true
 tracing.workspace = true
 
 [dev-dependencies]
+inspect = { workspace = true, features = ["initiate"] }
 test_with_tracing.workspace = true
 
 [lints]
diff --git a/vmm_core/vmotherboard/src/chipset/backing/arc_mutex/services.rs b/vmm_core/vmotherboard/src/chipset/backing/arc_mutex/services.rs
index 7baae76..b350d0a 100644
--- a/vmm_core/vmotherboard/src/chipset/backing/arc_mutex/services.rs
+++ b/vmm_core/vmotherboard/src/chipset/backing/arc_mutex/services.rs
@@ -126,18 +126,20 @@ fn finalize(
 
         if !self.line_set_targets.is_empty() {
             let valid_lines = line_target.ok_or(FinalizeError::NoInterruptTarget)?;
-            let device = Arc::new(LineSetTargetDevice::new(device.clone()));
-            for (id, source_range, target_start) in self.line_set_targets {
+            // Each target's line set, and the edge from its unit to the
+            // device's, here; the targets themselves only once the device's
+            // unit spawns (below), so that a failed spawn leaves none.
+            for (id, source_range, target_start) in &self.line_set_targets {
                 assert!(valid_lines.iter().any(|range| {
-                    *range.start() <= target_start
+                    *range.start() <= *target_start
                         && *range.end()
-                            >= target_start + (source_range.end() - source_range.start())
+                            >= *target_start + (source_range.end() - source_range.start())
                 }));
-                let (line_set, handle) =
-                    inner
-                        .line_sets
-                        .line_set(self.builder.driver_source, self.builder.units, id);
-                line_set.add_target(source_range, target_start, dev_name.clone(), device.clone());
+                let (_, handle) = inner.line_sets.line_set(
+                    self.builder.driver_source,
+                    self.builder.units,
+                    id.clone(),
+                );
                 builder = builder.dependency_of(handle);
             }
         }
@@ -149,6 +151,17 @@ fn finalize(
             .spawn(driver.clone(), |recv| device_unit.run(recv))
             .map_err(FinalizeError::NameInUse)?;
 
+        if !self.line_set_targets.is_empty() {
+            let target = Arc::new(LineSetTargetDevice::new(device.clone()));
+            for (id, source_range, target_start) in self.line_set_targets {
+                let (line_set, _) =
+                    inner
+                        .line_sets
+                        .line_set(self.builder.driver_source, self.builder.units, id);
+                line_set.add_target(source_range, target_start, dev_name.clone(), target.clone());
+            }
+        }
+
         inner
             .arc_mutex_device_units
             .push(DynamicDeviceUnit::new(unit, driver, device.clone()));
diff --git a/vmm_core/vmotherboard/src/chipset/builder/mod.rs b/vmm_core/vmotherboard/src/chipset/builder/mod.rs
index ea51f5c..eb7e4e2 100644
--- a/vmm_core/vmotherboard/src/chipset/builder/mod.rs
+++ b/vmm_core/vmotherboard/src/chipset/builder/mod.rs
@@ -414,3 +414,179 @@ async fn restore(&mut self, _buffer: SavedStateBlob) -> Result<(), RestoreError>
         }
     }
 }
+
+
+#[cfg(test)]
+mod tests {
+    use super::ChipsetBuilder;
+    use crate::DebugEventHandler;
+    use chipset_device_resources::BSP_LINT_LINE_SET;
+    use chipset_device_resources::IRQ_LINE_SET;
+    use chipset_device_resources::LineSetId;
+    use pal_async::DefaultDriver;
+    use pal_async::async_test;
+    use state_unit::StateUnits;
+    use std::sync::Arc;
+    use vmcore::vm_task::VmTaskDriverSource;
+    use vmcore::vm_task::thread::ThreadDriverBackend;
+    use vmcore::vmtime::VmTime;
+    use vmcore::vmtime::VmTimeKeeper;
+
+    struct NoDebugEvents;
+
+    impl DebugEventHandler for NoDebugEvents {
+        fn on_debug_break(&self, _vp: Option<u32>) {}
+    }
+
+    struct NoRouting;
+
+    impl chipset_resources::ioapic::IoApicRouting for NoRouting {
+        fn assert(&self, _irq: u8) {}
+        fn set_route(&self, _irq: u8, _request: Option<(u64, u32)>) {}
+    }
+
+    /// The JSON of the inspection of the builder's line set `id` (whose
+    /// inspection completes at once, under the builder's lock).
+    fn line_set_json(builder: &ChipsetBuilder<'_>, id: LineSetId) -> String {
+        use futures::FutureExt;
+        let mut inner = builder.inner.lock();
+        let (set, _) = inner
+            .line_sets
+            .line_set(builder.driver_source, builder.units, id);
+        let mut inspection = inspect::inspect("", set);
+        inspection.resolve().now_or_never().unwrap();
+        inspection.results().json().to_string()
+    }
+
+    /// The targets of the builder's line set `id` in their order, for each
+    /// of `vectors`: a probe line's `Display` at each vector.
+    fn line_set_orders(
+        builder: &ChipsetBuilder<'_>,
+        id: LineSetId,
+        vectors: std::ops::RangeInclusive<u32>,
+    ) -> String {
+        let mut inner = builder.inner.lock();
+        let (set, _) = inner
+            .line_sets
+            .line_set(builder.driver_source, builder.units, id);
+        vectors
+            .map(|vector| format!("{vector}:{}", set.new_line(vector, "probe").unwrap()))
+            .collect::<Vec<_>>()
+            .join(";")
+    }
+
+    /// A device whose unit cannot spawn (its name is already a unit's) adds
+    /// no line target: the IRQ line set holds no mapping to it, nor does the
+    /// PIT's line on it.
+    #[async_test]
+    async fn a_failed_spawn_leaves_no_line_target(driver: DefaultDriver) {
+        let driver_source = VmTaskDriverSource::new(ThreadDriverBackend::new(driver.clone()));
+        let units = StateUnits::new();
+        let keeper = VmTimeKeeper::new(&driver, VmTime::from_100ns(0));
+        let vmtime = keeper.builder().build(&driver).await.unwrap();
+        let (send, _recv) = mesh::channel();
+        let vmtime_unit = units.add("vmtime").build(send).unwrap();
+        let (send, _recv) = mesh::channel();
+        let _taken = units.add("ioapic").build(send).unwrap();
+        let builder = ChipsetBuilder::new(
+            &driver_source,
+            &units,
+            Arc::new(NoDebugEvents),
+            &vmtime,
+            &vmtime_unit,
+            false,
+            false,
+            None,
+        );
+        builder
+            .arc_mutex_device("pit")
+            .add(|services| {
+                chipset::pit::PitDevice::new(
+                    services.new_line(IRQ_LINE_SET, "timer0", 2),
+                    services.register_vmtime().access("pit"),
+                )
+            })
+            .unwrap();
+        let failed = builder.arc_mutex_device("ioapic").add(|services| {
+            services.add_line_target(IRQ_LINE_SET, 0..=23, 0);
+            chipset::ioapic::IoApicDevice::new(24, Box::new(NoRouting))
+        });
+        assert!(failed.is_err());
+        let json = line_set_json(&builder, IRQ_LINE_SET);
+        assert!(!json.contains("ioapic"), "{json}");
+    }
+
+    /// A normal build's line sets: with the PIT, PIC and IO-APIC added as
+    /// their resolvers add them and the BSP's LINT target, each line set
+    /// holds these mappings, lines and targets, in this order (as before
+    /// the fix).
+    #[async_test]
+    async fn a_normal_build_keeps_its_line_sets(driver: DefaultDriver) {
+        let driver_source = VmTaskDriverSource::new(ThreadDriverBackend::new(driver.clone()));
+        let units = StateUnits::new();
+        let keeper = VmTimeKeeper::new(&driver, VmTime::from_100ns(0));
+        let vmtime = keeper.builder().build(&driver).await.unwrap();
+        let (send, _recv) = mesh::channel();
+        let vmtime_unit = units.add("vmtime").build(send).unwrap();
+        let builder = ChipsetBuilder::new(
+            &driver_source,
+            &units,
+            Arc::new(NoDebugEvents),
+            &vmtime,
+            &vmtime_unit,
+            false,
+            false,
+            None,
+        );
+        builder
+            .arc_mutex_device("pit")
+            .add(|services| {
+                chipset::pit::PitDevice::new(
+                    services.new_line(IRQ_LINE_SET, "timer0", 2),
+                    services.register_vmtime().access("pit"),
+                )
+            })
+            .unwrap();
+        builder
+            .arc_mutex_device("pic")
+            .add(|services| {
+                services.add_line_target(IRQ_LINE_SET, 1..=1, 1);
+                services.add_line_target(IRQ_LINE_SET, 2..=2, 0);
+                services.add_line_target(IRQ_LINE_SET, 3..=15, 3);
+                chipset::pic::DualPic::new(
+                    services.new_line(BSP_LINT_LINE_SET, "ready", 0),
+                    &mut services.register_pio(),
+                )
+            })
+            .unwrap();
+        builder
+            .arc_mutex_device("ioapic")
+            .add(|services| {
+                services.add_line_target(IRQ_LINE_SET, 0..=23, 0);
+                chipset::ioapic::IoApicDevice::new(24, Box::new(NoRouting))
+            })
+            .unwrap();
+        builder.add_external_line_target(
+            BSP_LINT_LINE_SET,
+            0..=1,
+            0,
+            "bsp",
+            vmcore::line_interrupt::test_helpers::TestLineInterruptTarget::new_arc(),
+        );
+        let got = [
+            line_set_json(&builder, IRQ_LINE_SET),
+            line_set_json(&builder, BSP_LINT_LINE_SET),
+            line_set_orders(&builder, IRQ_LINE_SET, 0..=23),
+            line_set_orders(&builder, BSP_LINT_LINE_SET, 0..=1),
+        ];
+        assert_eq!(
+            got,
+            [
+                r#"{"lines":{"2":{"lines":{"pit:timer0":false},"targets":{"ioapic":2,"pic":0}}},"targets":{"ioapic:0-23":"0-23","pic:0-0":"2-2","pic:1-1":"1-1","pic:3-15":"3-15"}}"#,
+                r#"{"lines":{"0":{"lines":{"pic:ready":false},"targets":{"bsp":0}}},"targets":{"bsp:0-1":"0-1"}}"#,
+                r#"0:targets[ioapic(0)],lines[probe(false)];1:targets[pic(1),ioapic(1)],lines[probe(false)];2:targets[pic(0),ioapic(2)],lines[pit:timer0(false),probe(false)];3:targets[pic(3),ioapic(3)],lines[probe(false)];4:targets[pic(4),ioapic(4)],lines[probe(false)];5:targets[pic(5),ioapic(5)],lines[probe(false)];6:targets[pic(6),ioapic(6)],lines[probe(false)];7:targets[pic(7),ioapic(7)],lines[probe(false)];8:targets[pic(8),ioapic(8)],lines[probe(false)];9:targets[pic(9),ioapic(9)],lines[probe(false)];10:targets[pic(10),ioapic(10)],lines[probe(false)];11:targets[pic(11),ioapic(11)],lines[probe(false)];12:targets[pic(12),ioapic(12)],lines[probe(false)];13:targets[pic(13),ioapic(13)],lines[probe(false)];14:targets[pic(14),ioapic(14)],lines[probe(false)];15:targets[pic(15),ioapic(15)],lines[probe(false)];16:targets[ioapic(16)],lines[probe(false)];17:targets[ioapic(17)],lines[probe(false)];18:targets[ioapic(18)],lines[probe(false)];19:targets[ioapic(19)],lines[probe(false)];20:targets[ioapic(20)],lines[probe(false)];21:targets[ioapic(21)],lines[probe(false)];22:targets[ioapic(22)],lines[probe(false)];23:targets[ioapic(23)],lines[probe(false)]"#,
+                r#"0:targets[bsp(0)],lines[pic:ready(false),probe(false)];1:targets[bsp(1)],lines[probe(false)]"#,
+            ]
+        );
+    }
+}

Navigation Menu

Repository navigation

openvmm: a chipset device whose add fails stays a target of its line sets #1387

Reproduction

Cause

Fix

Proposed patch

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

openvmm: a chipset device whose add fails stays a target of its line sets #1387

Description

Reproduction

Cause

Fix

Activity

tangruize commented on Nov 8, 2026

Proposed patch

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions