multikernel: add isolated SR-IOV VF assignment - #4

Code review of the SR-IOV VF assignment series. 8 findings inline: 3 that look like real breakage (link failure, atomic-context deadlock, use-after-free), 4 medium, 1 cleanup.

I read through the full series. The design is sound: the host keeps device ownership, the IOMMU domain is the real isolation boundary rather than config filtering, and the docs say that plainly instead of overclaiming. Generation and epoch tagging is applied consistently, and encoding owner CPU plus slot in the producer gate (so a force-halt NMI cannot strand it permanently) is a genuinely good idea.

Below are correctness findings, then a request about how the series is packaged.

Findings

1. Refcount-0 resurrection in mk_instance_release() (kernel/multikernel/core.c)

The new early return in the kref release callback:

ret = mk_instance_release_resources(instance);
if (WARN_ON_ONCE(ret)) {
        pr_crit("Retaining multikernel instance %d ...");
        return;                       /* not freed, refcount is already 0 */
}

kref_put() has already driven the count to zero, but the instance is still in mk_instance_idr and on mk_instance_list, because mk_instance_destroy() only unlinks after a successful release. The next mk_instance_find() does idr_find() followed by kref_get() on a zero refcount, which trips refcount_warn_saturate("addition on 0"), and the following put frees an object other callers still hold.

This is reachable whenever mk_pci_release_assignments() returns -ETIMEDOUT or -EBUSY, which the series deliberately makes possible (mailbox quiesce timeout, VF still driver-bound). Suggested fix: re-init the reference before returning, or unlink from the idr and list first so the object is unreachable.

2. mk_ipi_ring_publish() spins on a cross-kernel gate with interrupts disabled (kernel/multikernel/ipi.c)

preempt_disable(); local_irq_save(flags);
for (retry = 0; retry < MK_IPI_PRODUCER_RETRIES; retry++) {   /* 10000 */
        old = atomic64_cmpxchg_acquire(&ring->producer_gate, 0, token);
        ...
        cpu_relax();
}

producer_gate lives in shared memory, so the owner may be a CPU belonging to a different kernel. A host CPU can therefore spin 10000 cpu_relax() iterations with local interrupts off waiting on a spawn that is stalled, descheduled, or NMI-parked. Console output reaches this path through vsock, as the old comment in this function noted.

This converts a lock-free MPSC ring that admitted up to 63 concurrent producers into a single global gate, and the failure mode is a dropped message (-EAGAIN) rather than backpressure. The recoverability argument for the gate design is convincing; holding it with IRQs off and spinning on a remote kernel is the part I would push back on.

3. mk_reply_cancel() can permanently strand reply slots (kernel/multikernel/ipi.c)

In the cancel loop, any state that is neither WRITING nor EXECUTING (notably COMMITTED, which an earlier timeout installs) falls out of the loop into the cancelled: label, bumps cancelled_slots, and returns 0, so the caller reports -ETIMEDOUT while the slot is never returned to FREE.

A COMMITTED slot is only reclaimed by mk_reply_publish(). If the host worker never reaches publish (instance force-halted between mk_reply_begin_execute() and publish), that slot is gone for the rest of the epoch. There are only MK_REPLY_SLOTS == 16 and every PCI config access consumes one, so a small number of these wedges config access with -ENOSPC until respawn. occupied_failures will grow with no recovery path.

4. mk_do_cpu_add() can leave a CPU online but untracked (kernel/multikernel/hotplug.c)

After add_cpu() succeeds:

mk_cpu_ownership_lock();
if (root_instance->cpus) {
        ret = mk_cpu_set_add(root_instance->cpus, cpu_id);
        if (ret)
                pr_warn(...);
}
mk_cpu_ownership_unlock();
if (ret)
        goto unlock_transaction;

Two issues. On mk_cpu_set_add() failure the function returns an error with the CPU already online and the rollback mk_hotplug_op never recorded, so nothing will undo it. And when root_instance->cpus is NULL, the if (ret) tests the leftover value from the CPU bring-up call rather than a fresh result. The previous code warned and continued, which was at least self-consistent.

The pre-reserve added just above is the right idea; it just is not carried through to the failure path.

5. XLF_MULTIKERNEL_IPI_V2 through V7 claim six bits of xloadflags

arch/x86/include/uapi/asm/bootparam.h now takes 0x0100 through 0x2000 in the documented x86 boot protocol UAPI word for a private host/spawn handshake, with five of the six values unused. struct mk_shared_data already carries abi_magic and abi_version, and the ELF note now carries ipi_abi_version, so the boot-protocol bit is only needed as a coarse "this bzImage speaks the protocol at all" gate. One bit would do, and I would expect the x86 maintainers to say the same when this eventually goes upstream.

Smaller notes

• mk_pci_msi_prepare() accepts MK_PCI_MSI_FAILED as a valid starting state alongside IDLE, so it re-prepares after a teardown that is known to have failed. The host-side SETUP does retry mk_pci_release_irqs() first, but "failed teardown" is exactly the case where that retry is least likely to succeed.
• x86_multikernel_pci_init() calls panic() when pci_scan_single_device() returns NULL. Fail-closed is right for assignment generally, but a transient enumeration miss now kills the spawn rather than failing the instance.
• mk_instance_confirm_parked() gained an early if (!instance->cpus_on_slot) return 0; ahead of the new mk_ipi_ring_recover_halted() call, so that path reports success without recovering a gate a dead producer may still own.
• DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, mk_pci_restore_resources) runs on every device on every CONFIG_PCI kernel. It is inert on the host, but a global ANY_ID fixup for a subarch-specific feature would be better gated on X86_SUBARCH_MULTIKERNEL.
• Two counters exist but are unreachable: the spawn's config-access latency (mk_pci_cfg_count / total_ns / max_ns) is only printed once via pr_notice during enumeration, and mk_pci_control_pool_exhausted is incremented and never read. Both belong in the new stats file, especially the latency one given that config access is the hot path.
• kernel/multikernel/pci.c at ~2600 lines covers leases, IOMMU domains, IRQ forwarding, the control-plane message handler, config access, and the transaction machinery. Splitting it along those lines would make it much easier to review. The host/spawn split against arch/x86/multikernel/pci.c is already clean.

Request: split the IPI transport rework out

The commit split within the PR is good, but three commits are standalone infrastructure that happens to ride along with the feature:

• multikernel: make resource reservation atomic
• multikernel: serialize CPU ownership transfers
• multikernel: make IPI publication ordered and recoverable

I would like at least the IPI transport work to land as its own series. It rewrites the shared ring, adds the reply table, bumps the shared ABI to v7, burns a boot-protocol flag bit, and breaks every existing DTB through MK_FDT_COMPATIBLE. That is a larger and riskier change than the SR-IOV feature it is supporting, it affects every multikernel user rather than only those assigning VFs, and findings 2 and 3 are both in it. Reviewed on its own it would get the attention it deserves, and SR-IOV could then be reviewed as a feature rather than as a feature plus a transport rewrite.

The CPU ownership serialization is a similar case on a smaller scale. The mk_cpu_set locking rework in particular (adding the raw_spinlock_t, out-of-lining the three inline accessors, rewriting reserve/add/copy as retry loops) changes an API used throughout the subsystem and stands on its own merits. Worth at least its own commit, separate from the route-pinning changes it enables.

To be clear on why the CPU work is here at all: mk_pci_request_route_stale() makes CPU ownership a security-relevant fact, which does require the mk_cpu_set locking, the route migration in mk_send_cpu_remove(), and the CPU 0 exclusion. That chain is sound. But mk_do_cpu_add() and mk_do_cpu_remove() operate on root_instance->cpus, which the route validation never reads, so the transaction and ownership locking there is consistency work rather than a prerequisite. That is also where finding 4 crept in.