Kernel/PCI: Don't hold spinlocks when doing fast device enumeration

Instead, hold the lock while we copy the contents to a stack-based
Vector then iterate on it without any locking.

Because we rely on heap allocations, we need to propagate errors back
in case of OOM condition, therefore, both PCI::enumerate API function
and PCI::Access::add_host_controller_and_enumerate_attached_devices use
now a ErrorOr<void> return value to propagate errors. OOM Error can only
occur when enumerating the m_device_identifiers vector under a spinlock
and trying to expand the temporary Vector which will be used locklessly
to actually iterate over the PCI::DeviceIdentifiers objects.

14 files changed, 69 insertions, 42 deletions

diff --git a/Kernel/Bus/PCI/API.cpp b/Kernel/Bus/PCI/API.cpp
index 9b26d4c049..f2c45bcb6d 100644
--- a/Kernel/Bus/PCI/API.cpp
+++ b/Kernel/Bus/PCI/API.cpp
@@ -17,9 +17,9 @@ u8 read8(Address address, PCI::RegisterOffset field) { return Access::the().read
 u16 read16(Address address, PCI::RegisterOffset field) { return Access::the().read16_field(address, to_underlying(field)); }
 u32 read32(Address address, PCI::RegisterOffset field) { return Access::the().read32_field(address, to_underlying(field)); }
 
-void enumerate(Function<void(DeviceIdentifier const&)> callback)
+ErrorOr<void> enumerate(Function<void(DeviceIdentifier const&)> callback)
 {
-    Access::the().fast_enumerate(callback);
+    return Access::the().fast_enumerate(callback);
 }
 
 DeviceIdentifier get_device_identifier(Address address)
diff --git a/Kernel/Bus/PCI/API.h b/Kernel/Bus/PCI/API.h
index 198e55e1d9..6c95db9aa8 100644
--- a/Kernel/Bus/PCI/API.h
+++ b/Kernel/Bus/PCI/API.h
@@ -6,6 +6,8 @@
 
 #pragma once
 
+#include <AK/Error.h>
+#include <AK/Try.h>
 #include <Kernel/Bus/PCI/Definitions.h>
 
 namespace Kernel::PCI {
@@ -19,7 +21,7 @@ u32 read32(Address address, PCI::RegisterOffset field);
 
 HardwareID get_hardware_id(PCI::Address);
 bool is_io_space_enabled(Address);
-void enumerate(Function<void(DeviceIdentifier const&)> callback);
+ErrorOr<void> enumerate(Function<void(DeviceIdentifier const&)> callback);
 void enable_interrupt_line(Address);
 void disable_interrupt_line(Address);
 void raw_access(Address, u32, size_t, u32);
diff --git a/Kernel/Bus/PCI/Access.cpp b/Kernel/Bus/PCI/Access.cpp
index 67c3cea1f2..12d1c42d1e 100644
--- a/Kernel/Bus/PCI/Access.cpp
+++ b/Kernel/Bus/PCI/Access.cpp
@@ -117,20 +117,36 @@ UNMAP_AFTER_INIT bool Access::initialize_for_one_pci_domain()
     return true;
 }
 
-void Access::add_host_controller_and_enumerate_attached_devices(NonnullOwnPtr<HostController> controller, Function<void(DeviceIdentifier const&)> callback)
+ErrorOr<void> Access::add_host_controller_and_enumerate_attached_devices(NonnullOwnPtr<HostController> controller, Function<void(DeviceIdentifier const&)> callback)
 {
-    SpinlockLocker locker(m_access_lock);
-    SpinlockLocker scan_locker(m_scan_lock);
-    auto domain_number = controller->domain_number();
+    // Note: We hold the spinlocks for a moment just to ensure we append the
+    // device identifiers safely. Afterwards, enumeration goes lockless to allow
+    // IRQs to be fired if necessary.
+    Vector<DeviceIdentifier> device_identifiers_behind_host_controller;
+    {
+        SpinlockLocker locker(m_access_lock);
+        SpinlockLocker scan_locker(m_scan_lock);
+        auto domain_number = controller->domain_number();
 
-    VERIFY(!m_host_controllers.contains(domain_number));
-    // Note: We need to register the new controller as soon as possible, and
-    // definitely before enumerating devices behing that.
-    m_host_controllers.set(domain_number, move(controller));
-    m_host_controllers.get(domain_number).value()->enumerate_attached_devices([&](DeviceIdentifier const& device_identifier) -> void {
-        m_device_identifiers.append(device_identifier);
+        VERIFY(!m_host_controllers.contains(domain_number));
+        // Note: We need to register the new controller as soon as possible, and
+        // definitely before enumerating devices behing that.
+        m_host_controllers.set(domain_number, move(controller));
+        ErrorOr<void> expansion_result;
+        m_host_controllers.get(domain_number).value()->enumerate_attached_devices([&](DeviceIdentifier const& device_identifier) -> void {
+            m_device_identifiers.append(device_identifier);
+            auto result = device_identifiers_behind_host_controller.try_append(device_identifier);
+            if (result.is_error())
+                expansion_result = result;
+        });
+        if (expansion_result.is_error())
+            return expansion_result;
+    }
+
+    for (auto const& device_identifier : device_identifiers_behind_host_controller) {
         callback(device_identifier);
-    });
+    }
+    return {};
 }
 
 UNMAP_AFTER_INIT void Access::add_host_controller(NonnullOwnPtr<HostController> controller)
@@ -156,13 +172,20 @@ UNMAP_AFTER_INIT void Access::rescan_hardware()
     }
 }
 
-void Access::fast_enumerate(Function<void(DeviceIdentifier const&)>& callback) const
+ErrorOr<void> Access::fast_enumerate(Function<void(DeviceIdentifier const&)>& callback) const
 {
-    SpinlockLocker locker(m_access_lock);
-    VERIFY(!m_device_identifiers.is_empty());
-    for (auto const& device_identifier : m_device_identifiers) {
+    // Note: We hold the m_access_lock for a brief moment just to ensure we get
+    // a complete Vector in case someone wants to mutate it.
+    Vector<DeviceIdentifier> device_identifiers;
+    {
+        SpinlockLocker locker(m_access_lock);
+        VERIFY(!m_device_identifiers.is_empty());
+        TRY(device_identifiers.try_extend(m_device_identifiers));
+    }
+    for (auto const& device_identifier : device_identifiers) {
         callback(device_identifier);
     }
+    return {};
 }
 
 DeviceIdentifier Access::get_device_identifier(Address address) const
diff --git a/Kernel/Bus/PCI/Access.h b/Kernel/Bus/PCI/Access.h
index 8444b80c18..fa1dc16fb2 100644
--- a/Kernel/Bus/PCI/Access.h
+++ b/Kernel/Bus/PCI/Access.h
@@ -7,6 +7,7 @@
 #pragma once
 
 #include <AK/Bitmap.h>
+#include <AK/Try.h>
 #include <AK/Vector.h>
 #include <Kernel/Bus/PCI/Controller/HostController.h>
 #include <Kernel/Bus/PCI/Definitions.h>
@@ -21,7 +22,7 @@ public:
     static bool initialize_for_multiple_pci_domains(PhysicalAddress mcfg_table);
     static bool initialize_for_one_pci_domain();
 
-    void fast_enumerate(Function<void(DeviceIdentifier const&)>&) const;
+    ErrorOr<void> fast_enumerate(Function<void(DeviceIdentifier const&)>&) const;
     void rescan_hardware();
 
     static Access& the();
@@ -39,7 +40,7 @@ public:
     Spinlock const& scan_lock() const { return m_scan_lock; }
     RecursiveSpinlock const& access_lock() const { return m_access_lock; }
 
-    void add_host_controller_and_enumerate_attached_devices(NonnullOwnPtr<HostController>, Function<void(DeviceIdentifier const&)> callback);
+    ErrorOr<void> add_host_controller_and_enumerate_attached_devices(NonnullOwnPtr<HostController>, Function<void(DeviceIdentifier const&)> callback);
 
 private:
     u8 read8_field(Address address, RegisterOffset field);
diff --git a/Kernel/Bus/PCI/Initializer.cpp b/Kernel/Bus/PCI/Initializer.cpp
index 792e3fd8ad..0357b09689 100644
--- a/Kernel/Bus/PCI/Initializer.cpp
+++ b/Kernel/Bus/PCI/Initializer.cpp
@@ -62,9 +62,9 @@ UNMAP_AFTER_INIT void initialize()
 
     PCI::PCIBusSysFSDirectory::initialize();
 
-    PCI::enumerate([&](DeviceIdentifier const& device_identifier) {
+    MUST(PCI::enumerate([&](DeviceIdentifier const& device_identifier) {
         dmesgln("{} {}", device_identifier.address(), device_identifier.hardware_id());
-    });
+    }));
 }
 
 UNMAP_AFTER_INIT bool test_pci_io()
diff --git a/Kernel/Bus/PCI/SysFSPCI.cpp b/Kernel/Bus/PCI/SysFSPCI.cpp
index 2f9d28a44f..7fe347ef0b 100644
--- a/Kernel/Bus/PCI/SysFSPCI.cpp
+++ b/Kernel/Bus/PCI/SysFSPCI.cpp
@@ -50,10 +50,10 @@ UNMAP_AFTER_INIT void PCIBusSysFSDirectory::initialize()
 UNMAP_AFTER_INIT PCIBusSysFSDirectory::PCIBusSysFSDirectory()
     : SysFSDirectory(SysFSComponentRegistry::the().buses_directory())
 {
-    PCI::enumerate([&](DeviceIdentifier const& device_identifier) {
+    MUST(PCI::enumerate([&](DeviceIdentifier const& device_identifier) {
         auto pci_device = PCI::PCIDeviceSysFSDirectory::create(*this, device_identifier.address());
         m_components.append(pci_device);
-    });
+    }));
 }
 
 StringView PCIDeviceAttributeSysFSComponent::name() const
diff --git a/Kernel/Bus/USB/USBManagement.cpp b/Kernel/Bus/USB/USBManagement.cpp
index 5b815eab9e..8b59a1a4e3 100644
--- a/Kernel/Bus/USB/USBManagement.cpp
+++ b/Kernel/Bus/USB/USBManagement.cpp
@@ -27,7 +27,7 @@ UNMAP_AFTER_INIT void USBManagement::enumerate_controllers()
     if (kernel_command_line().disable_usb())
         return;
 
-    PCI::enumerate([this](PCI::DeviceIdentifier const& device_identifier) {
+    MUST(PCI::enumerate([this](PCI::DeviceIdentifier const& device_identifier) {
         if (!(device_identifier.class_code().value() == 0xc && device_identifier.subclass_code().value() == 0x3))
             return;
         if (device_identifier.prog_if().value() == 0x0) {
@@ -56,7 +56,7 @@ UNMAP_AFTER_INIT void USBManagement::enumerate_controllers()
         }
 
         dmesgln("USBManagement: Unknown/unsupported controller at {} with programming interface 0x{:02x}", device_identifier.address(), device_identifier.prog_if().value());
-    });
+    }));
 }
 
 bool USBManagement::initialized()
diff --git a/Kernel/Bus/VirtIO/Device.cpp b/Kernel/Bus/VirtIO/Device.cpp
index 67fbd7a098..06f5850bcd 100644
--- a/Kernel/Bus/VirtIO/Device.cpp
+++ b/Kernel/Bus/VirtIO/Device.cpp
@@ -18,7 +18,7 @@ UNMAP_AFTER_INIT void detect()
 {
     if (kernel_command_line().disable_virtio())
         return;
-    PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
+    MUST(PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
         if (device_identifier.hardware_id().is_null())
             return;
         // TODO: We should also be checking that the device_id is in between 0x1000 - 0x107F inclusive
@@ -43,7 +43,7 @@ UNMAP_AFTER_INIT void detect()
             dbgln_if(VIRTIO_DEBUG, "VirtIO: Unknown VirtIO device with ID: {}", device_identifier.hardware_id().device_id);
             break;
         }
-    });
+    }));
 }
 
 static StringView determine_device_class(PCI::DeviceIdentifier const& device_identifier)
diff --git a/Kernel/Devices/Audio/Management.cpp b/Kernel/Devices/Audio/Management.cpp
index e444d3351f..abf57a4991 100644
--- a/Kernel/Devices/Audio/Management.cpp
+++ b/Kernel/Devices/Audio/Management.cpp
@@ -39,7 +39,7 @@ UNMAP_AFTER_INIT AudioManagement::AudioManagement()
 UNMAP_AFTER_INIT void AudioManagement::enumerate_hardware_controllers()
 {
     if (!PCI::Access::is_disabled()) {
-        PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
+        MUST(PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
             // Note: Only consider PCI audio controllers
             if (device_identifier.class_code().value() != to_underlying(PCI::ClassID::Multimedia)
                 || device_identifier.subclass_code().value() != to_underlying(PCI::Multimedia::SubclassID::AudioController))
@@ -53,7 +53,7 @@ UNMAP_AFTER_INIT void AudioManagement::enumerate_hardware_controllers()
                 return;
             }
             m_controllers_list.append(ac97_device.release_value());
-        });
+        }));
     }
 }
 
diff --git a/Kernel/Devices/PCISerialDevice.cpp b/Kernel/Devices/PCISerialDevice.cpp
index 7658088769..d7150efac7 100644
--- a/Kernel/Devices/PCISerialDevice.cpp
+++ b/Kernel/Devices/PCISerialDevice.cpp
@@ -15,7 +15,7 @@ static SerialDevice* s_the = nullptr;
 UNMAP_AFTER_INIT void PCISerialDevice::detect()
 {
     size_t current_device_minor = 68;
-    PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
+    MUST(PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
         for (auto& board_definition : board_definitions) {
             if (board_definition.device_id != device_identifier.hardware_id())
                 continue;
@@ -36,7 +36,7 @@ UNMAP_AFTER_INIT void PCISerialDevice::detect()
             dmesgln("PCISerialDevice: Found {} @ {}", board_definition.name, device_identifier.address());
             return;
         }
-    });
+    }));
 }
 
 SerialDevice& PCISerialDevice::the()
diff --git a/Kernel/GlobalProcessExposed.cpp b/Kernel/GlobalProcessExposed.cpp
index c216876ff9..a4c68ca489 100644
--- a/Kernel/GlobalProcessExposed.cpp
+++ b/Kernel/GlobalProcessExposed.cpp
@@ -5,6 +5,7 @@
  */
 
 #include <AK/JsonObjectSerializer.h>
+#include <AK/Try.h>
 #include <AK/UBSanitizer.h>
 #include <Kernel/Arch/x86/InterruptDisabler.h>
 #include <Kernel/Arch/x86/ProcessorInfo.h>
@@ -680,7 +681,7 @@ private:
     {
         auto array = TRY(JsonArraySerializer<>::try_create(builder));
         ErrorOr<void> result; // FIXME: Make this nicer
-        PCI::enumerate([&array, &result](PCI::DeviceIdentifier const& device_identifier) {
+        TRY(PCI::enumerate([&array, &result](PCI::DeviceIdentifier const& device_identifier) {
             if (result.is_error())
                 return;
             result = ([&]() -> ErrorOr<void> {
@@ -699,7 +700,7 @@ private:
                 TRY(obj.finish());
                 return {};
             })();
-        });
+        }));
         TRY(result);
         TRY(array.finish());
         return {};
diff --git a/Kernel/Graphics/GraphicsManagement.cpp b/Kernel/Graphics/GraphicsManagement.cpp
index b1f2d0570e..a3c6ec67f0 100644
--- a/Kernel/Graphics/GraphicsManagement.cpp
+++ b/Kernel/Graphics/GraphicsManagement.cpp
@@ -231,14 +231,14 @@ UNMAP_AFTER_INIT bool GraphicsManagement::initialize()
     else if (framebuffer_devices_use_bootloader_framebuffer())
         dbgln("Forcing use of framebuffer set up by the bootloader");
 
-    PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
+    MUST(PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
         // Note: Each graphics controller will try to set its native screen resolution
         // upon creation. Later on, if we don't want to have framebuffer devices, a
         // framebuffer console will take the control instead.
         if (!is_vga_compatible_pci_device(device_identifier) && !is_display_controller_pci_device(device_identifier))
             return;
         determine_and_initialize_graphics_device(device_identifier);
-    });
+    }));
 
     if (!m_console) {
         // If no graphics driver was instantiated and we had a bootloader provided
diff --git a/Kernel/Net/NetworkingManagement.cpp b/Kernel/Net/NetworkingManagement.cpp
index cf83c31dc3..510fa10492 100644
--- a/Kernel/Net/NetworkingManagement.cpp
+++ b/Kernel/Net/NetworkingManagement.cpp
@@ -112,13 +112,13 @@ UNMAP_AFTER_INIT RefPtr<NetworkAdapter> NetworkingManagement::determine_network_
 bool NetworkingManagement::initialize()
 {
     if (!kernel_command_line().is_physical_networking_disabled() && !PCI::Access::is_disabled()) {
-        PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
+        MUST(PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
             // Note: PCI class 2 is the class of Network devices
             if (device_identifier.class_code().value() != 0x02)
                 return;
             if (auto adapter = determine_network_device(device_identifier); !adapter.is_null())
                 m_adapters.with([&](auto& adapters) { adapters.append(adapter.release_nonnull()); });
-        });
+        }));
     }
     auto loopback = LoopbackAdapter::try_create();
     VERIFY(loopback);
diff --git a/Kernel/Storage/StorageManagement.cpp b/Kernel/Storage/StorageManagement.cpp
index cdf67309e0..3ca934d018 100644
--- a/Kernel/Storage/StorageManagement.cpp
+++ b/Kernel/Storage/StorageManagement.cpp
@@ -53,7 +53,7 @@ UNMAP_AFTER_INIT void StorageManagement::enumerate_pci_controllers(bool force_pi
     using SubclassID = PCI::MassStorage::SubclassID;
     if (!kernel_command_line().disable_physical_storage()) {
 
-        PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
+        MUST(PCI::enumerate([&](PCI::DeviceIdentifier const& device_identifier) {
             if (device_identifier.class_code().value() != to_underlying(PCI::ClassID::MassStorage)) {
                 return;
             }
@@ -62,7 +62,7 @@ UNMAP_AFTER_INIT void StorageManagement::enumerate_pci_controllers(bool force_pi
                 constexpr PCI::HardwareID vmd_device = { 0x8086, 0x9a0b };
                 if (device_identifier.hardware_id() == vmd_device) {
                     auto controller = PCI::VolumeManagementDevice::must_create(device_identifier);
-                    PCI::Access::the().add_host_controller_and_enumerate_attached_devices(move(controller), [this, nvme_poll](PCI::DeviceIdentifier const& device_identifier) -> void {
+                    MUST(PCI::Access::the().add_host_controller_and_enumerate_attached_devices(move(controller), [this, nvme_poll](PCI::DeviceIdentifier const& device_identifier) -> void {
                         auto subclass_code = static_cast<SubclassID>(device_identifier.subclass_code().value());
                         if (subclass_code == SubclassID::NVMeController) {
                             auto controller = NVMeController::try_initialize(device_identifier, nvme_poll);
@@ -72,7 +72,7 @@ UNMAP_AFTER_INIT void StorageManagement::enumerate_pci_controllers(bool force_pi
                                 m_controllers.append(controller.release_value());
                             }
                         }
-                    });
+                    }));
                 }
             }
 
@@ -93,7 +93,7 @@ UNMAP_AFTER_INIT void StorageManagement::enumerate_pci_controllers(bool force_pi
                     m_controllers.append(controller.release_value());
                 }
             }
-        });
+        }));
     }
 }