server: VmmHdl appears to be leaked during normal VM shutdown

[gjcolombo]

Repro steps:

1. Start an ad hoc propolis-server instance
2. Send it an ensure request
3. Ask to stop the VM instance (you don't have to start it first, though you can)

Observed: The DESTROY_SELF vmm ioctl is issued (and a probe set on vmm_destroy_locked fires), but the kernel VMM persists until the process is killed. The stack on the resulting call to vmm_destroy_finish shows it originated from genunix!proc_exit. Writing a simple Drop impl for VmmHdl that just prints to stderr shows that this drop impl is apparently never reached.

Expected: there is at least some way to convince Propolis to fully close the kernel VMM fd on VM destruction.

---

We've discussed this in the past and concluded that in at least some cases it's useful for the kernel VMM to outlive the Propolis instance that owns it so that the VMM can be inspected with tools like mdb -b. I would at least like to consider avoiding this for production builds, though, for reasons related to this Omicron issue comment: it's useful for sled-agent to be able to say "Propolis reported that it's in the Destroyed state, so all its reservoir memory is released," because this helps give it the ability to tell Nexus that a VMM is gone and then do long-running zone cleanup operations afterward.

Even absent that motivation, I'd at least like to understand exactly which paths aren't fully dropping their VmmHdl references so that we can adjust the behavior if/when we need to.

[gjcolombo]

I dusted off the TracingArc I created the last time I had to manage one of these and created a VM with no devices other than its chipset and vCPUs, then immediately stopped it without starting it. According to the instrumentation there are six VmmHdl refs left after shutdown. Now to look at the stacks to see which references I can and can't account for.

[gjcolombo]

Gentle reader, I will spare you the exact call stacks and instead subject you to my extremely rough notes:

1   hdl created
2   cloned in machine::Builder (inner_hdl)
3   Builder::finalize (acc_msi)
4   MsiAccessor::new (for tree)
5   Builder::finalize (guest-HV interface)
4   Builder::finalize (bhyve doesn't need it)
5   Builder::finalize (vCPU 0)
6   Builder::finalize (vCPU 1)
7   Builder::finalize (clones into return value)
6   Builder::finalize (drops local)
7   pci::topology::Builder::finish (MSI accessor)
8   initialize_chipset (Piix3Lpc)
9   Piix3Lpc::create (IrqConfig)
10  Piix3Lpc::create (BhyveAtPic)
11  Piix3Lpc::create (BhyveAtPit)
12  Piix3Lpc::create (BhyveIoApic)
13  Piix3Lpc::create (BhyveRtc)
12  drop from Piix3Lpc::create (local created in initialize_chipset)
13   initialize_chipset (Piix3PM)
14  attach to I440FxHostBridge (MSI accessor)
15  attach to I440FxHostBridge (MSI accessor)
16  attach to I440FxHostBridge (MSI accessor)
17  attach to I440FxHostBridge (MSI accessor)
18  attach to I440FxHostBridge (MSI accessor)
19  attach to I440FxHostBridge (MSI accessor)
20  initialize_hpet
19  MsiAccessor::poison
18  MsiAccessor::poison
17  MsiAccessor::poison
16  MsiAccessor::poison
15  MsiAccessor::poison
14  MsiAccessor::poison
13  MsiAccessor::poison
12  MsiAccessor::poison
11  Machine::do_destroy
12  Machine::do_destroy
11  Machine::do_destroy
10  drop Machine
9   drop PhysMap from drop of Machine
8   drop HPET
7   drop vCPU
6   drop vCPU

I'm pretty sure the unaccounted-for references are from the PIIX3 objects (the one for Piix3PM ends up being moved into a BhyvePmTimer). Next step is to figure out what's up with those.

[gjcolombo]

My guess (still need to confirm more explicitly) is that the problem is a circular reference to the i440FX host bridge:

propolis/bin/propolis-server/src/lib/initializer.rs

Lines 314 to 347 in ba4d338

	let chipset_hb = i440fx::I440FxHostBridge::create(
	pci_topology,
	i440fx::Opts {
	power_pin: Some(power_pin),
	reset_pin: Some(reset_pin),
	enable_pcie: i440fx.enable_pcie,
	},
	);
	let chipset_lpc =
	i440fx::Piix3Lpc::create(self.machine.hdl.clone());
	let chipset_pm = i440fx::Piix3PM::create(
	self.machine.hdl.clone(),
	chipset_hb.power_pin(),
	self.log.new(slog::o!("device" => "piix3pm")),
	);
	let do_pci_attach = |bdf, dev: Arc<dyn pci::Endpoint>| {
	chipset_hb.pci_attach(
	bdf,
	dev,
	chipset_lpc.route_lintr(bdf),
	);
	};
	// Attach chipset devices to PCI and buses
	do_pci_attach(i440fx::DEFAULT_HB_BDF, chipset_hb.clone());
	chipset_hb.attach(self.machine);
	do_pci_attach(i440fx::DEFAULT_LPC_BDF, chipset_lpc.clone());
	chipset_lpc.attach(&self.machine.bus_pio);
	do_pci_attach(i440fx::DEFAULT_PM_BDF, chipset_pm.clone());
	chipset_pm.attach(&self.machine.bus_pio);

In this snippet, an Arc<I440FxHostBridge> is created on line 314 and takes ownership of the VM's PCI topology. Not long after this the machine initializer tries to attach the chipset PCI devices to that topology. On line 340, it clones the host bridge and passes the clone to do_pci_attach, which creates a self-reference: the HB owns the PCI topology, which holds a strong reference back to the HB. Oops.

I'll need to mull over how to deal with this. I think one possibility is to define a I440FxHostBridgePciDevice that impls pci::Device and that I440FxHostBridge owns a reference to; that would give it something other than itself that it could attach to the PCI topology. But I'm open to other suggestions here.

[gjcolombo]

As an aside, I've pushed the TracingArc code (but not the bits to use it to track this down) to this branch: https://github.com/gjcolombo/propolis/tree/gjcolombo/tracing-arc

[pfmooney]

A couple drive-by thoughts on this:

1. Do we want to put the VmmHdl behind an Accessor, at least for the bits of device emulation which require it?
2. Splitting up the chipset duty into the HostBridge (pci::)device, and then some overarching thing (which owns the PCI topology) sounds reasonable.