Conversation
b715339 to
0ff04f5
Compare
|
Hi @nickolaev Nice work, thanks for the PR! I finally have time to look into it. A few high-level comments:
|
|
@congwang-mk thanks for the high-level review. Indeed, this is a naive implementation of "let's make ICMP pass"; It is very fragile as it is. The questions you asked are setting a proper direction. I have some ideas; let me try to clean things up and will ping you once I have a better patch series. |
8d8960b to
4e05ab7
Compare
|
@congwang-mk I have updated the commit history with the revised work and the PR message reflects the new state. |
b2d1b55 to
64608bc
Compare
Thank you for doing it. I will take a look during the weekend, since I am still working on the refactoring to prepare for ARM/RISCV (which is also why you got merge conflicts). |
64608bc to
5efa61c
Compare
|
Thanks for the update! The layering is a clear improvement: synthetic roots instead of physical bridges, manifest-owned BAR/identity metadata,
|
|
Separate from the item list above, a thought about the shape rather than the bugs. SR-IOV VFs are built assuming something mediates. VF config space is impoverished by design because a hypervisor is expected to synthesize the missing parts from the PF's SR-IOV capability. Multikernel's premise is that there is no mediator, so the series meets a mediator-shaped hole and fills it two ways: the manifest for BARs and identity, which works because those are static, and a config-op filter for everything else, which does not, because the filter lives inside the kernel it is meant to constrain. The interrupt gap is the same mismatch surfacing where it cannot be papered over. But multikernel already has the right mechanism: a host that owns global state and a message ring to reach it. That suggests splitting along the axis that actually exists:
Keep the synthetic roots and the IOMMU domain either way. The identity-mapped domain in particular fits well: with no second-level translation it range-restricts a device rather than virtualizing an address space, which is simpler than what VFIO needs. The obvious cost is latency on mediated config access, and I have not measured what that does to something like igbvf at probe time. Worth a quick experiment before committing. |
5efa61c to
75e1053
Compare
Thanks for the review @congwang-mk |
1e25d2f to
a3db37b
Compare
|
OK, so the host-mediated PCI config is now in place and IRQ ownership is resolved. |
congwang-mk
left a comment
There was a problem hiding this comment.
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.
| return ret; | ||
| } | ||
|
|
||
| bool mk_pci_msi_controlled(struct pci_dev *dev) |
There was a problem hiding this comment.
Link failure when CONFIG_PCI_MMCONFIG=n. The five mk_pci_msi_* functions (230, 238, 271, 285, 306) are inside the #ifdef CONFIG_PCI_MMCONFIG block; the #else arm only re-provides mk_arch_snapshot_pci_host_bridges and the x86_init.pci hooks. multikernel.h declares them unconditionally under MULTIKERNEL && X86 && PCI, and drivers/pci/msi/msi.c:244 + irqdomain.c:15,19,31,34,42 call them unconditionally.
PCI_MMCONFIG depends on ACPI || JAILHOUSE_GUEST, so a spawn kernel with CONFIG_ACPI=n (what this series targets) fails to link vmlinux with five undefined references.
| goto out; | ||
|
|
||
| /* Pairs with the response handler's publication of status and value. */ | ||
| while (!smp_load_acquire(&pending.done)) { |
There was a problem hiding this comment.
Synchronous RPC re-dispatches the whole IPI ring from atomic context. mk_pci_remote_config() and mk_pci_send_irq_request() (:214) busy-wait up to 1s in mk_poll_ipi_messages(), which drains the ring and runs every handler, including mk_pci_irq_forward_handler() -> generic_handle_irq_safe() -> the assigned device's ISR.
Both callers run IRQs-off under a raw spinlock: config accesses come via pci_bus_read/write_config_* holding pci_lock; mk_pci_msi_bind() is reached from __pci_write_msi_msg() under desc->lock with IRQs off during irq_startup().
Deadlock: driver probe does pci_read_config_word(), the poll dispatches a forwarded MSI, that ISR issues a config read -> self-deadlock on the non-recursive pci_lock, same CPU. Even without re-entry, spinning 1s IRQs-off trips the hard-lockup watchdog.
| pr_info("Releasing multikernel instance %d (%s), returning resources to root\n", | ||
| instance->id, instance->name); | ||
| ret = mk_instance_release_resources(instance); | ||
| WARN_ON_ONCE(ret); |
There was a problem hiding this comment.
Instance freed after failed resource release -> use-after-free. This only WARN_ON_ONCE()s the error, then kfree(instance) two lines down.
On failure mk_pci_release_assignments() (kernel/multikernel/pci.c:1697-1717) breaks out of its loop, leaving the mk_pci_assignment linked on both instance->pci_assignments (now freed) and the global active list, with assignment->instance dangling. A VF whose FLR times out during teardown gets there; the next BUS_NOTIFY_UNBOUND_DRIVER runs mk_pci_assignment_failure_work(), which dereferences assignment->instance->state and ->name.
mk_instance_destroy() and mk_create_instance_from_dtb() do honor this error; the kref path is the outlier.
| tail = atomic_read(&ring->tail); | ||
|
|
||
| slot = &root_instance->ipi_data->ring.entries[tail]; | ||
| for (scanned = 0; scanned < MK_IPI_RING_SIZE; scanned++) { |
There was a problem hiding this comment.
Ring consumption is no longer FIFO. The new drain scans all MK_IPI_RING_SIZE slots from tail and consumes anything READY, skipping WRITING. Slot order no longer equals delivery order: A claims slot 5, B claims 6, B publishes first -> 6 is delivered before 5.
mk_vsock_ipi_handler() turns each payload into an skb on a byte-stream socket, so two concurrent senders now corrupt the stream rather than just delaying it. The goal here (an interrupted producer must not block others) is reachable without breaking ordering in the common case.
| { | ||
| struct pci_dev *dev = msi_desc_to_pci_dev(entry); | ||
|
|
||
| if (mk_pci_msi_write_msg(dev, entry->msi_index, entry->irq, |
There was a problem hiding this comment.
MSI bind failure is swallowed, leaving a silently dead vector. mk_pci_msi_write_msg() (arch/x86/multikernel/pci.c:271) logs the mk_pci_msi_bind() error with pr_err_ratelimited() and then returns true unconditionally, so __pci_write_msi_msg() stores entry->msg and returns as if programmed.
On RPC timeout or -ESTALE/-ENODEV, pci_alloc_irq_vectors() and request_irq() both succeed while the host never enabled the corresponding vector. The VF driver then waits forever for an interrupt that never arrives, with no error visible to it.
| instance = mk_instance_find(irq_work->request.sender_instance_id); | ||
| if (!instance) | ||
| goto out; | ||
| mk_cpu_ownership_lock(); |
There was a problem hiding this comment.
CPU-ownership mutex held across sleeping MSI work. This holds mk_cpu_ownership_lock() across mk_pci_irq_access(), which can msleep(MK_PCI_FLR_SETTLE_MS + 1), pci_alloc_irq_vectors(), request_irq() and free_irq() (synchronize_irq). mk_pci_cfg_work_fn() (:505) does the same.
Every mk_instance_transfer_cpus() / mk_send_cpu_add() / mk_send_cpu_remove() on any instance stalls behind an unrelated instance's MSI setup, and the requester's own 1s deadline can expire while the host is still in the FLR sleep -> spurious -ETIMEDOUT. Only the mk_pci_request_route_stale check actually needs the ownership lock.
| pr_info("Forwarding host IRQ %u as instance IRQ %u for %s vector %u\n", | ||
| irq, local_irq, pci_name(assignment->vf), | ||
| payload.vector); | ||
| if (mk_send_message_to_instance(assignment->instance, MK_MSG_IO, |
There was a problem hiding this comment.
Forwarded MSI silently dropped on allocation/backpressure failure. This hardirq handler forwards via mk_send_message_to_instance() -> __mk_send_message(), which does a kzalloc(GFP_ATOMIC) per interrupt and returns -ENOSPC from mk_ipi_ring_claim_slot() when the 64-slot ring is full. The handler only pr_warn_ratelimited()s and returns IRQ_HANDLED.
Edge-triggered MSI never re-asserts, so under memory pressure or ring backpressure the spawn kernel permanently loses that completion (e.g. an igbvf TX/RX cleanup interrupt) and the queue hangs. No retry, coalescing, or pending-interrupt fallback.
| return 0; | ||
| } | ||
|
|
||
| raw_pci_ops = &pci_mmcfg; |
There was a problem hiding this comment.
Dead stores; the "saved backend" does not exist. raw_pci_ops/raw_pci_ext_ops are set to &pci_mmcfg and immediately overwritten by the filtered ops, and nothing captures pci_mmcfg anywhere.
The commit message says accepted accesses are "forwarded to the saved backend", but mk_pci_raw_read/write always go out as a host RPC. The ECAM windows registered by pci_mmconfig_add() and mapped by pci_mmcfg_arch_init() just above are therefore mapped and never used. Either wire the local backend up or drop the ECAM mapping and these two stores, since as written it reads like there is a local fast path.
| unsigned long boot_lps; /* Host delay loops per second */ | ||
| unsigned long boot_cpu_khz; /* Host CPU frequency calibration */ | ||
| unsigned long boot_tsc_khz; /* Host TSC frequency calibration */ | ||
| unsigned long boot_apic_hz; /* Host local APIC timer frequency */ |
There was a problem hiding this comment.
I am wondering why you have to handle clock in this PR? If anything is wrong with clock, please separate it out
9cd7a06 to
5916a11
Compare
4ff65f4 to
20902d6
Compare
d206aae to
1249380
Compare
1249380 to
b051716
Compare
Replace the owner-and-slot-state producer protocol with one spinlocked producer per duplex direction and one release/acquire ready flag per slot. Reset a parent/child link only after the previous child is confirmed parked, and close endpoints before their backing image pages are released. Carry the parent and doorbell identities in the shared link, restore them through the boot device tree design, and route nested console output through the actual parent instance. Fail incompatible boot contexts and legacy image notes with explicit errors while keeping the vmlinux descriptor as the original u64 entry. Preserve host timer calibration for spawned x86 kernels and declare the OF dependency required by the upstream boot-tree manifest. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Describe only the PCI functions assigned to a spawn and retain the BAR shape discovered by the host. Validate exact BDF syntax, reject duplicate functions, and preserve the inventory across the baseline and instance device trees. Keep PCI support behind CONFIG_PCI so the control plane remains buildable without PCI. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Create one synthetic root bus for each assigned domain and bus, then scan only the explicitly assigned devfns. Supply an exact bus-number resource when x86 root resources do not provide one. Spawn kernels do not inherit or map host ECAM windows. Configuration is mediated by the filtered backend introduced with the assigned roots. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Synthetic root filtering alone leaves raw x86 configuration backends able to reach functions outside the spawn inventory. Apply the assigned-BDF filter to both raw configuration entry points. Identity reads come from validated metadata while other accesses use the selected backend only for an assigned function. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Reserve assigned VFs transactionally under a host-wide lease lock. Detach the host driver, mark the device assigned, and publish ownership only when the complete request succeeds. Propagate host-driver restoration failures and retain failed instance state when cleanup cannot be completed safely. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Instance creation reserves memory, CPUs, PCI devices, host-bridge metadata, and platform devices. Returning an error after any one of those transfers can otherwise expose a partially populated instance and leak resources from the root. Validate configuration counts and lists before transfer, acquire each resource class in a fixed order, and unwind every completed step in reverse order. Centralize release so create failure, instance deletion, and final reference teardown share the same all-or-nothing semantics. Balance references acquired for remote memory add and remove operations on every success and error path so resource hotplug cannot pin a deleted instance. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Exclusive VF ownership does not constrain DMA. Allocate a host-owned domain for each assignment and map only the memory owned by the instance. Update mappings with memory hotplug under the lease lifetime. Detach and destroy the domain before returning the VF to the host. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Returning a VF while it can still issue DMA races the IOMMU teardown and host-driver reprobe. Unexpected PF or VF removal must also stop an active instance instead of silently losing its assigned device. Clear bus mastering, wait for pending transactions, and issue function-level reset while the assignment domain is still attached. Then detach and free the domain, restore the saved driver override and host driver, and only afterwards return the inventory to the root. Lease-loss notifications force an active instance to halt and mark it failed. Rollback entries that were prepared but never committed skip device quiesce and driver restoration, releasing only their prepared IOMMU resources. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
A stopped or force-halted instance can leave bus mastering enabled and DMA in flight while its next boot rewrites the same memory. Lease teardown resets the VF, but a respawn retains the lease and previously skipped that protection. Require assigned VFs to support FLR. After confirming that all instance CPUs are parked, clear bus mastering, drain pending transactions, and reset every leased VF while its restrictive IOMMU domain remains attached. Abort the restart if any device cannot be made safe. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Protect CPU membership and pool ownership with a dedicated mutex nested inside an operation-wide transaction lock. Keep add and remove transactions serialized across reservation, acknowledgment, and repark so a CPU cannot be transferred twice. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Serialize shared-ring producers with a bounded owner-aware gate. Preserve FIFO publication and recover a gate only after its producer CPU is known to be parked. The gate and slot-state protocol change the private shared transport layout. Add an exact pre-launch layout and protocol check at this boundary, then require a transport initialization acknowledgment after both rings have been validated and before marking the instance active. Fail invalid manifests and missing acknowledgments closed. A spawn started by a host without the pre-launch check validates the boot-context anchor before using any shifted field and enters a local interrupt-disabled halt loop on mismatch without trusting shared park state or touching reset and APIC hardware. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Cache the IRQ forwarding CPU for hardirq-safe routing. Serialize route, ownership, reload, halt, and teardown mutations while active users hold a route reference. Drain assignment IRQ producers before publishing a replacement route or reparking the old CPU. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Proxy spawn PCI configuration through the host so only a live VF lease selected by an assigned BDF can access hardware. Return results through preallocated generation-tagged reply slots. Atomic callers wait only on their slot with a bounded deadline and never drain the general ring. Bump the exact transport ABI to version 4. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Keep MSI and MSI-X programming out of irqchip callbacks. Setup, bind, activation, restore, and teardown use the process-context PCI control path; write_msg only updates the cached message. Route controlled VF FLR through a separate process-context direct reply, with an independent spawn epoch and serial generation. Reject raw config-space FLR writes so reset cannot run inside the bounded atomic config path. Reject stale operations without side effects and fail closed on incomplete activation or reset. Pre-mask controlled MSI-X tables before host activation. Bump the exact transport ABI to version 6. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
A bounded shared ring cannot guarantee delivery when a host interrupt arrives in hardirq context. Record assigned interrupts in preallocated per-instance pending slots and use the IPI only as a doorbell. Protect each slot with spawn epoch, lifecycle generation, and an atomic pending, masked, and consuming token. Coalesce while masked and retry lost doorbells until the guest drains the slot. Bump the exact transport ABI to version 7. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Expose a versioned per-instance snapshot for the ordered IPI ring, direct reply table, and pending IRQ mailbox. Document every counter, gauge, reset boundary, and the non-atomic modulo-u32 snapshot semantics. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Treat COMMITTED as a terminal timeout state during cancellation and publish replies only while the slot still holds the matching request and generation. A racing timeout can then reclaim the exact abandoned or committed slot without allowing a late reply to overwrite a reused slot. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Keep restart available when IOMMU support is disabled and provide fail-closed PCI assignment stubs when PCI support is absent. This preserves the base MultiKernel build across both supported configuration boundaries. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Pin control-route selection through message publication and the physical doorbell with a raw spinlock. Use lifetime-stable instance pointers in atomic console and PCI paths, and keep ID lookup APIs process-context only. Drain PCI retry work before shared-ring teardown and serialize every mailbox access against publication and removal. Replace hard-IRQ mempool fallback and waiter wakeups with preallocated atomic work slots and deferred process-context wakeups. Reuse the image-owned instance during spawn and establish one instance, CPU transaction, route, and park lock order across manifest generation and pool teardown. Re-kick a full ring before reporting it through ratelimited deferred printk. Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
b051716 to
d0dd880
Compare
Depends on #7.
Summary
This 17-commit SR-IOV series is stacked directly on the reviewed IPI transport branch. It assigns SR-IOV VFs to multikernel instances while the host retains privileged device, DMA, reset, and interrupt control.
The earlier link, atomic-context, lifetime, FIFO, MSI, CPU-ownership, interrupt-loss, and file-structure review findings are addressed in the restacked series.
Validation