From 009feefee0dc45af2a38b0c6c9f6963ddda5468d Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 14 Aug 2021 10:13:27 +0300 Subject: [PATCH] Kernel/Devices: Ensure appropriate locking on the Device map singleton Devices might be removed and inserted at anytime, so let's ensure we always do these kind of operations with a good known state of the HashMap. The VirtIO code was modified to create devices outside the IRQ handler, so now it works with the new locking of the devices singleton, but a better approach might be needed later on. --- Kernel/Bus/VirtIO/Console.cpp | 41 +++++++++++++++++------------- Kernel/Bus/VirtIO/ConsolePort.cpp | 3 +-- Kernel/Bus/VirtIO/ConsolePort.h | 5 ++-- Kernel/Devices/Device.cpp | 42 ++++++++++++++++++++----------- Kernel/Devices/Device.h | 3 ++- 5 files changed, 57 insertions(+), 37 deletions(-) diff --git a/Kernel/Bus/VirtIO/Console.cpp b/Kernel/Bus/VirtIO/Console.cpp index 38e695a07d..a636a3d37c 100644 --- a/Kernel/Bus/VirtIO/Console.cpp +++ b/Kernel/Bus/VirtIO/Console.cpp @@ -7,6 +7,7 @@ #include #include +#include namespace Kernel::VirtIO { @@ -51,8 +52,11 @@ UNMAP_AFTER_INIT void Console::initialize() if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) setup_multiport(); - else - m_ports.append(make_ref_counted(0u, *this)); + else { + auto port = make_ref_counted(0u, *this); + port->init_receive_buffer({}); + m_ports.append(port); + } } } } @@ -146,23 +150,26 @@ void Console::process_control_message(ControlMessage message) { switch (message.event) { case (u16)ControlEvent::DeviceAdd: { - u32 id = message.id; - if (id >= m_ports.size()) { - dbgln("Device provided an invalid port number {}. max_nr_ports: {}", id, m_ports.size()); - return; - } else if (!m_ports.at(id).is_null()) { - dbgln("Device tried to add port {} which was already added!", id); - return; - } + g_io_work->queue([message, this]() -> void { + u32 id = message.id; + if (id >= m_ports.size()) { + dbgln("Device provided an invalid port number {}. max_nr_ports: {}", id, m_ports.size()); + return; + } else if (!m_ports.at(id).is_null()) { + dbgln("Device tried to add port {} which was already added!", id); + return; + } - m_ports.at(id) = make_ref_counted(id, *this); - ControlMessage ready_event { - .id = static_cast(id), - .event = (u16)ControlEvent::PortReady, - .value = (u16)ControlMessage::Status::Success - }; + m_ports.at(id) = make_ref_counted(id, *this); + ControlMessage ready_event { + .id = static_cast(id), + .event = (u16)ControlEvent::PortReady, + .value = (u16)ControlMessage::Status::Success + }; + + write_control_message(ready_event); + }); - write_control_message(ready_event); break; } case (u16)ControlEvent::ConsolePort: diff --git a/Kernel/Bus/VirtIO/ConsolePort.cpp b/Kernel/Bus/VirtIO/ConsolePort.cpp index 8dd8840769..7121006062 100644 --- a/Kernel/Bus/VirtIO/ConsolePort.cpp +++ b/Kernel/Bus/VirtIO/ConsolePort.cpp @@ -21,10 +21,9 @@ ConsolePort::ConsolePort(unsigned port, VirtIO::Console& console) m_transmit_buffer = make("VirtIO::ConsolePort Transmit", RINGBUFFER_SIZE); m_receive_queue = m_port == 0 ? 0 : m_port * 2 + 2; m_transmit_queue = m_port == 0 ? 1 : m_port * 2 + 3; - init_receive_buffer(); } -void ConsolePort::init_receive_buffer() +void ConsolePort::init_receive_buffer(Badge) { auto& queue = m_console.get_queue(m_receive_queue); SpinlockLocker queue_lock(queue.lock()); diff --git a/Kernel/Bus/VirtIO/ConsolePort.h b/Kernel/Bus/VirtIO/ConsolePort.h index 7b511087ac..0282d95a3a 100644 --- a/Kernel/Bus/VirtIO/ConsolePort.h +++ b/Kernel/Bus/VirtIO/ConsolePort.h @@ -7,6 +7,7 @@ #pragma once +#include #include #include #include @@ -29,6 +30,8 @@ public: void set_open(Badge, bool state) { m_open = state; } bool is_open() const { return m_open; } + void init_receive_buffer(Badge); + private: constexpr static size_t RINGBUFFER_SIZE = 2 * PAGE_SIZE; @@ -40,8 +43,6 @@ private: virtual KResultOr write(OpenFileDescription&, u64, const UserOrKernelBuffer&, size_t) override; virtual KResultOr> open(int options) override; - void init_receive_buffer(); - static unsigned next_device_id; u16 m_receive_queue {}; u16 m_transmit_queue {}; diff --git a/Kernel/Devices/Device.cpp b/Kernel/Devices/Device.cpp index ac418c3abc..42684fa75c 100644 --- a/Kernel/Devices/Device.cpp +++ b/Kernel/Devices/Device.cpp @@ -7,28 +7,34 @@ #include #include #include +#include +#include namespace Kernel { -static Singleton> s_all_devices; +static Singleton>> s_all_devices; -HashMap& Device::all_devices() +MutexProtected>& Device::all_devices() { return *s_all_devices; } void Device::for_each(Function callback) { - for (auto& entry : all_devices()) - callback(*entry.value); + all_devices().with_exclusive([&](auto& map) -> void { + for (auto& entry : map) + callback(*entry.value); + }); } Device* Device::get_device(unsigned major, unsigned minor) { - auto it = all_devices().find(encoded_device(major, minor)); - if (it == all_devices().end()) - return nullptr; - return it->value; + return all_devices().with_exclusive([&](auto& map) -> Device* { + auto it = map.find(encoded_device(major, minor)); + if (it == map.end()) + return nullptr; + return it->value; + }); } Device::Device(unsigned major, unsigned minor) @@ -36,17 +42,23 @@ Device::Device(unsigned major, unsigned minor) , m_minor(minor) { u32 device_id = encoded_device(major, minor); - auto it = all_devices().find(device_id); - if (it != all_devices().end()) { - dbgln("Already registered {},{}: {}", major, minor, it->value->class_name()); - } - VERIFY(!all_devices().contains(device_id)); - all_devices().set(device_id, this); + all_devices().with_exclusive([&](auto& map) -> void { + auto it = map.find(device_id); + if (it != map.end()) { + dbgln("Already registered {},{}: {}", major, minor, it->value->class_name()); + } + VERIFY(!map.contains(device_id)); + map.set(device_id, this); + }); } Device::~Device() { - all_devices().remove(encoded_device(m_major, m_minor)); + u32 device_id = encoded_device(m_major, m_minor); + all_devices().with_exclusive([&](auto& map) -> void { + VERIFY(map.contains(device_id)); + map.remove(encoded_device(m_major, m_minor)); + }); } String Device::absolute_path() const diff --git a/Kernel/Devices/Device.h b/Kernel/Devices/Device.h index 4be7c40ac6..fcd503e1f6 100644 --- a/Kernel/Devices/Device.h +++ b/Kernel/Devices/Device.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -61,7 +62,7 @@ protected: void set_uid(UserID uid) { m_uid = uid; } void set_gid(GroupID gid) { m_gid = gid; } - static HashMap& all_devices(); + static MutexProtected>& all_devices(); private: unsigned m_major { 0 };