Repository navigation
Groundwork for user drivers: thread-safe allocation and mapping, MSI-X teardown, DMA barriers - #3257
Closed
valentinbreiz wants to merge 9 commits into
Closed
valentinbreiz wants to merge 9 commits into
valentinbreiz wants to merge 9 commits into
Conversation
SmallHeap.Free called InternalCpu.EnableInterrupts on its common path and on its panic paths, and SmallHeap.Alloc did the same before its out-of-pages panic. Heap.Free already runs them in a scope that masks interrupts and restores the flag on exit, so each call unmasked interrupts in the middle of that scope and of any critical section the caller had nested it in. The calls are gone, along with the commented-out serial traces.
AllocPages scanned the RAT for an empty run and then marked it, with nothing to stop a preemption in between, so two threads could claim the same pages. The scan and the mark, Free(uint) and every FreePageCount update now run under an IRQ-safe spinlock. Pages are zeroed after the lock is released, so clearing a large run does not hold off interrupts. Free(uint) also read the byte past the end of the RAT when a run ended on the last page; the bound is now checked before the read. The serial line AllocPages wrote on every call is gone.
ReadyThread, BlockThread, CheckSleepingThreads and ConditionVariable wrote a line to the polled UART on every wake, block and signal. They run with interrupts masked, often from an ISR, so each byte stalled every IRQ. Those lines are gone. The timer tick is logged for the first 10 ticks only, to show at boot that it runs, instead of every 50 ticks for the whole uptime.
DeviceMapper.EnsureMapped walked and edited the page tables with no lock. On x64, two threads mapping under the same empty PML4 or PDPT slot could each link a new table, and the mappings made through the first one were lost. On ARM64, two splits could both fill the single spare L2 table. Both mappers now hold an IRQ-safe spinlock, which may take the PageAllocator lock beneath it but never the reverse. EnsureMapped and IPlatformInitializer.EnsureMmioMapped now return whether the block is mapped, so a caller can refuse to touch an alias that would fault. Existing drivers ignore the result for now; the Storage suite's high-BAR test checks it. The ARM64 mapper no longer logs every walk step and every already-mapped block, only new mappings and failures.
On x64 a config access is a CONFIG_ADDRESS write to 0xCF8 followed by a CONFIG_DATA access at 0xCFC. A thread preempted between the two resumed against whatever register another thread had selected meanwhile. Each pair now runs under one IRQ-safe spinlock. ECAM accesses are single loads or stores and stay unlocked.
DmaOrdering.ReadBarrier and WriteBarrier order CPU accesses to coherent DMA memory the way a device observes them: dmb oshld and dmb oshst on ARM64, in the Outer Shareable domain where DMA masters sit. x64 is TSO and needs no instruction, but each barrier stays an opaque native call so the compiler cannot move a DMA-memory access across it. Neither allocates, blocks or dispatches through an interface, so an ISR can use both. The NVMe completion handler now uses the read barrier instead of the interface call PlatformHAL.Initializer.DmaBarrier().
MSI-X could be enabled and programmed but never undone. A vector or LPI stayed allocated with its handler registered, and a second MsiX.Enable on the same function issued another MAPD over a mapped ITS DeviceID, orphaning the first ITT and every LPI translated through it. - MsiX.SetEntryMasked programs an entry and leaves it masked, so a driver still starting up receives nothing; the device holds the message in its Pending Bit Array until the entry is unmasked. Every entry is now masked before its address and data are rewritten. - MsiX.Disable sets Function Mask, masks every entry, clears MSI-X Enable and reads the register back, so the function has stopped signalling before any vector or LPI goes back to the allocator. - IMsiBinder gains UnbindEntry and ReleaseDevice. x64 records the vector each entry holds and frees it. ARM64 issues DISCARD and SYNC before freeing each LPI, then MAPD V=0 and SYNC before freeing the ITT. - PrepareDevice on a function whose earlier context was never released tears that context down first and logs it. Releasing the old context later does nothing, so it cannot free what the new owner holds.
Five new cells in the Interrupts suite (12 -> 17 per cell): - MsiX_SetEntryMasked_LeavesEntryMasked: on a scratch table, the entry is written but stays masked, and SetEntry still unmasks. - MsiRouting_UnbindEntry_ReturnsSlot: binds entries until the vector or LPI allocator runs dry, then checks that UnbindEntry frees a slot and that ReleaseDevice frees every slot the function still held. - MsiRouting_PrepareDevice_RemapDoesNotLeak: re-prepares one function repeatedly; no ITT page or routing slot leaks, and releasing a context that was taken over frees nothing twice. - MsiX_Disable_ClearsEnableAndMasksEntries: on an MSI-X function no driver has enabled, Disable clears MSI-X Enable, leaves Function Mask set and re-masks entries that were unmasked. - MsiX_EnableDisableEnable_SameFunction: after Disable the first routing context refuses to bind, and a second Enable on the same function programs its entries again. The first three skip without MSI routing (ARM64 GICv2). The last two also need an MSI-X function no driver owns, which ARM64 does not have (virtio-net owns the only one), so they run on x64 only (e1000e).
🧪 Graphic Tests
Skipped
📎 Artifacts
|
🧪 Interrupts Tests
Skipped
📎 Artifacts
|
🧪 Storage Tests
Skipped
📎 Artifacts
|
This was referenced Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First step towards letting a user kernel (such as
examples/DevKernel) register its own PCI and USB drivers. This PR adds no driver API. It fixes the Core and PCI paths that a driver would hit if it bound late, from a preemptible thread, and could be unbound. NoPublicAPI.*.txtfile changes.Problem
Today every driver binds during HAL init, one after another, and none ever unbinds. Several paths only work because of that:
PageAllocator.AllocPagesscanned the RAT for an empty run, then marked it, with no lock. A preemption in between let two threads claim the same pages.Free(uint)also read the byte past the end of the RAT when a run ended on the last page.SmallHeap.FreecalledInternalCpu.EnableInterrupts()on its common path. It runs insideHeap.Free's interrupt-masked scope, so every free of a small object unmasked interrupts in the middle of that scope and of any critical section around it.DeviceMapper.EnsureMappedwalked and edited the page tables with no lock.0xCF8followed by an access at0xCFC. A thread preempted between the two resumed against whatever register another thread had selected.MsiX.Enableon the same function issued anotherMAPDover a mapped ITS DeviceID, orphaning the first ITT and every LPI translated through it;SetEntryunmasks at once, so an entry can fire before its driver is ready;ReadyThread,BlockThread,CheckSleepingThreadsandConditionVariablewrote a line to the polled UART on every wake, block and signal. They run with interrupts masked, often from an ISR, so each byte stalled every IRQ. The timer ISR also logged every 50th tick for the whole uptime.PlatformHAL.Initializer?.DmaBarrier(). That is an interface call inside an ISR, and a full barrier where a load-to-load ordering is enough.Fix
Locks. Three IRQ-safe spinlocks (
Scheduler.SpinLock.AcquireIrqSafe):PageAllocator.s_ratLockAllocPages,Free(uint),FreePageCountAllocPageswith interrupts masked and spin on itself.DeviceMapper.s_lock(x64 and ARM64)PageAllocatorlock beneath it (table allocation), never the reverse. Not reentrant.PciDevice.s_configLock0xCF8write with its0xCFCaccesss_ratLockis released: the run is already marked, and clearing a large run with interrupts masked would hold off every IRQ.Free(void*)looks up the run's first page outside the lock. It only reads entries of the caller's own run, and it can throw.SmallHeapno longer touches the interrupt flag at all.Heap.Free's scope owns it.Mapping result.
DeviceMapper.EnsureMappedandIPlatformInitializer.EnsureMmioMappednow return whether the 2 MiB block is mapped on return (true when it already was). Existing drivers ignore the result for now. The ARM64 mapper no longer logs every walk step and every already-mapped block, only new mappings and failures.MSI-X teardown.
MsiX.SetEntryMaskedMsiX.DisableIMsiBinder.UnbindEntryDISCARD+SYNC, then frees the LPI.IMsiBinder.ReleaseDeviceMAPDV=0 +SYNCand frees the ITT.SetEntryandSetEntryMasked.Disablereads Message Control back before freeing anything: an ECAM store can retire before it reaches the function, and the read cannot complete ahead of it.Disablewrites the per-entry masks only when memory decode is on. Function Mask already covers every entry, and a table write the function does not decode is an unclaimed posted write: dropped on x64, an asynchronous abort on some ARM64 root complexes.Disableleaves the Command register alone, so INTx stays disabled asEnableleft it.Enabledoes not record what that bit held before, so restoring it is the owner's job.PrepareDeviceon a function whose earlier context was never released tears that context down first and logs[MSI] function 0x.. still had an unreleased context, releasing it first(x64) or[MSI-ITS] DeviceID 0x.. was still mapped, ...(ARM64).IMsiBinderandMsiRoutingnow document that every member runs in thread context only: the allocators take plain spinlocks.Logging. The per-wake, per-block and per-signal lines are gone, and the tick log stops after the first 10 ticks.
DMA barriers.
Core.Memory.DmaOrdering.ReadBarrierandWriteBarrier, backed by_native_dma_rmband_native_dma_wmb:dmb oshldanddmb oshst. DMB rather than DSB, because only the order of accesses matters, not their completion.DmaOrdering.ReadBarrier.Public surface. Every changed member is internal, except
DeviceMapper.EnsureMapped(void to bool) andGICv3Its.MapDevice(void to ulong, the ITT address). Those live in the arch Core assemblies, which have noPublicAPIfiles.Verification
Build.
make setup: 0 errors. The only warnings in the touched files are on lines this PR does not change.Test suites, QEMU (JUnit tests / failed / skipped):
New Interrupts cells (12 to 17 per cell):
MsiX_SetEntryMasked_LeavesEntryMaskedSetEntrystill unmasks.MsiRouting_UnbindEntry_ReturnsSlotUnbindEntryfrees a slot andReleaseDevicefrees all 172 again.MsiRouting_PrepareDevice_RemapDoesNotLeakMsiX_Disable_ClearsEnableAndMasksEntriesDisableclears MSI-X Enable, leaves Function Mask set and re-masks every entry.MsiX_EnableDisableEnable_SameFunctionDisable, the first routing context refuses to bind. A secondEnablesets MSI-X Enable, clears Function Mask and programs a zeroed entry again.Other suites.
nvmeandnvme+acpi-off:Mmio_HighBar_RemappedOnDemandnow also asserts thatEnsureMmioMappedreports the relocated BAR as mapped ([DeviceMapper] Mapping MMIO phys 0x110000000).Profile_NvmeInterruptModeMatchespasses with NVMe on MSI-X, on the new read barrier.Real hardware. The runs above are QEMU only.
Known gaps.
MsiX.Disablenever runs against a live function. virtio-net owns the only MSI-X function there, so the last two cells skip. A driver test suite in a later PR will cover it.EnsureMmioMapped's result yet (MsiX.Enable, NVMe, AHCI, xHCI, virtio). The driver model will turn a failed mapping into a declined bind.MsiX.Disableon its function. The binder side is safe, but the Message Control write would disable the new owner's MSI-X. This goes away once each function has a single owner, which is the next step.0xCF8write inPciDevice.SelectConfigAddressthrows, the lock stays held with interrupts masked. Only a nullPortIOcan do that, which is already fatal at boot.CachedInterfaceDispatchcallsSmallHeap.Allocwithout masking interrupts.