From 77188428297ac88cc7d1015bcdd2581319c5a030 Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 23 Sep 2023 00:44:33 +0300 Subject: [PATCH] Kernel/VirtIO: Ensure proper error propagation in core methods Simplify core methods in the VirtIO bus handling code by ensuring proper error propagation. This makes initialization of queues, handling changes in device configuration, and other core patterns more readable as well. It also allows us to remove the obnoxious pattern of checking for boolean "success" and if we get false answer then returning an actual errno code. --- Kernel/Bus/VirtIO/Console.cpp | 14 +++---- Kernel/Bus/VirtIO/Console.h | 2 +- Kernel/Bus/VirtIO/Device.cpp | 40 ++++++++----------- Kernel/Bus/VirtIO/Device.h | 12 +++--- Kernel/Bus/VirtIO/RNG.cpp | 14 +++---- Kernel/Bus/VirtIO/RNG.h | 2 +- Kernel/Bus/VirtIO/Transport/Entity.cpp | 6 +-- Kernel/Bus/VirtIO/Transport/Entity.h | 2 +- Kernel/Devices/GPU/VirtIO/GraphicsAdapter.cpp | 22 +++++----- Kernel/Devices/GPU/VirtIO/GraphicsAdapter.h | 2 +- Kernel/Net/VirtIO/VirtIONetworkAdapter.cpp | 19 +++------ Kernel/Net/VirtIO/VirtIONetworkAdapter.h | 2 +- 12 files changed, 56 insertions(+), 81 deletions(-) diff --git a/Kernel/Bus/VirtIO/Console.cpp b/Kernel/Bus/VirtIO/Console.cpp index 10643b8bb4..2ea0a33689 100644 --- a/Kernel/Bus/VirtIO/Console.cpp +++ b/Kernel/Bus/VirtIO/Console.cpp @@ -25,16 +25,14 @@ UNMAP_AFTER_INIT ErrorOr Console::initialize_virtio_resources() { TRY(Device::initialize_virtio_resources()); auto const* cfg = TRY(transport_entity().get_config(VirtIO::ConfigurationType::Device)); - bool success = negotiate_features([&](u64 supported_features) { + TRY(negotiate_features([&](u64 supported_features) { u64 negotiated = 0; if (is_feature_set(supported_features, VIRTIO_CONSOLE_F_SIZE)) dbgln("VirtIO::Console: Console size is not yet supported!"); if (is_feature_set(supported_features, VIRTIO_CONSOLE_F_MULTIPORT)) negotiated |= VIRTIO_CONSOLE_F_MULTIPORT; return negotiated; - }); - if (!success) - return Error::from_errno(EIO); + })); u32 max_nr_ports = 0; u16 cols = 0, rows = 0; transport_entity().read_config_atomic([&]() { @@ -49,9 +47,7 @@ UNMAP_AFTER_INIT ErrorOr Console::initialize_virtio_resources() }); dbgln("VirtIO::Console: cols: {}, rows: {}, max nr ports {}", cols, rows, max_nr_ports); // Base receiveq/transmitq for port0 + optional control queues and 2 per every additional port - success = setup_queues(2 + max_nr_ports > 0 ? 2 + 2 * max_nr_ports : 0); - if (!success) - return Error::from_errno(EIO); + TRY(setup_queues(2 + max_nr_ports > 0 ? 2 + 2 * max_nr_ports : 0)); finish_init(); if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) { @@ -70,10 +66,10 @@ UNMAP_AFTER_INIT Console::Console(NonnullOwnPtr transport_entit { } -bool Console::handle_device_config_change() +ErrorOr Console::handle_device_config_change() { dbgln("VirtIO::Console: Handle device config change"); - return true; + return {}; } void Console::handle_queue_update(u16 queue_index) diff --git a/Kernel/Bus/VirtIO/Console.h b/Kernel/Bus/VirtIO/Console.h index f98ee27297..3c7e598c14 100644 --- a/Kernel/Bus/VirtIO/Console.h +++ b/Kernel/Bus/VirtIO/Console.h @@ -59,7 +59,7 @@ private: constexpr static size_t CONTROL_MESSAGE_SIZE = sizeof(ControlMessage); constexpr static size_t CONTROL_BUFFER_SIZE = CONTROL_MESSAGE_SIZE * 32; - virtual bool handle_device_config_change() override; + virtual ErrorOr handle_device_config_change() override; virtual void handle_queue_update(u16 queue_index) override; Vector> m_ports; diff --git a/Kernel/Bus/VirtIO/Device.cpp b/Kernel/Bus/VirtIO/Device.cpp index e6c1b85310..4b75cce799 100644 --- a/Kernel/Bus/VirtIO/Device.cpp +++ b/Kernel/Bus/VirtIO/Device.cpp @@ -38,7 +38,7 @@ void Device::set_status_bit(u8 status_bit) m_transport_entity->set_status_bits({}, m_status); } -bool Device::accept_device_features(u64 device_features, u64 accepted_features) +ErrorOr Device::accept_device_features(u64 device_features, u64 accepted_features) { VERIFY(!m_did_accept_features); m_did_accept_features = true; @@ -70,28 +70,24 @@ bool Device::accept_device_features(u64 device_features, u64 accepted_features) if (!(m_status & DEVICE_STATUS_FEATURES_OK)) { set_status_bit(DEVICE_STATUS_FAILED); dbgln("{}: Features not accepted by host!", m_class_name); - return false; + return Error::from_errno(EIO); } m_accepted_features = accepted_features; dbgln_if(VIRTIO_DEBUG, "{}: Features accepted by host", m_class_name); - return true; + return {}; } -bool Device::setup_queue(u16 queue_index) +ErrorOr Device::setup_queue(u16 queue_index) { - auto queue_or_error = m_transport_entity->setup_queue({}, queue_index); - if (queue_or_error.is_error()) - return false; - - auto queue = queue_or_error.release_value(); + auto queue = TRY(m_transport_entity->setup_queue({}, queue_index)); dbgln_if(VIRTIO_DEBUG, "{}: Queue[{}] configured with size: {}", m_class_name, queue_index, queue->size()); - m_queues.append(move(queue)); - return true; + TRY(m_queues.try_append(move(queue))); + return {}; } -bool Device::setup_queues(u16 requested_queue_count) +ErrorOr Device::setup_queues(u16 requested_queue_count) { VERIFY(!m_did_setup_queues); m_did_setup_queues = true; @@ -103,7 +99,7 @@ bool Device::setup_queues(u16 requested_queue_count) m_queue_count = maximum_queue_count; } else if (requested_queue_count > maximum_queue_count) { dbgln("{}: {} queues requested but only {} available!", m_class_name, m_queue_count, maximum_queue_count); - return false; + return Error::from_errno(ENXIO); } else { m_queue_count = requested_queue_count; } @@ -113,15 +109,13 @@ bool Device::setup_queues(u16 requested_queue_count) } dbgln_if(VIRTIO_DEBUG, "{}: Setting up {} queues", m_class_name, m_queue_count); - for (u16 i = 0; i < m_queue_count; i++) { - if (!setup_queue(i)) - return false; - } - for (u16 i = 0; i < m_queue_count; i++) { // Queues can only be activated *after* all others queues were also configured - if (!m_transport_entity->activate_queue({}, i)) - return false; - } - return true; + for (u16 i = 0; i < m_queue_count; i++) + TRY(setup_queue(i)); + + // NOTE: Queues can only be activated *after* all others queues were also configured + for (u16 i = 0; i < m_queue_count; i++) + TRY(m_transport_entity->activate_queue({}, i)); + return {}; } void Device::finish_init() @@ -143,7 +137,7 @@ bool Device::handle_irq(Badge) } if (isr_type & DEVICE_CONFIG_INTERRUPT) { dbgln_if(VIRTIO_DEBUG, "{}: VirtIO Device config interrupt!", class_name()); - if (!handle_device_config_change()) { + if (handle_device_config_change().is_error()) { set_status_bit(DEVICE_STATUS_FAILED); dbgln("{}: Failed to handle device config change!", class_name()); } diff --git a/Kernel/Bus/VirtIO/Device.h b/Kernel/Bus/VirtIO/Device.h index 7d657f785c..b2138e56f8 100644 --- a/Kernel/Bus/VirtIO/Device.h +++ b/Kernel/Bus/VirtIO/Device.h @@ -35,7 +35,7 @@ protected: void mask_status_bits(u8 status_mask); void set_status_bit(u8); - bool setup_queues(u16 requested_queue_count = 0); + ErrorOr setup_queues(u16 requested_queue_count = 0); void finish_init(); Queue& get_queue(u16 queue_index) @@ -51,7 +51,7 @@ protected: } template - bool negotiate_features(F f) + ErrorOr negotiate_features(F f) { u64 device_features = m_transport_entity->get_device_features(); u64 accept_features = f(device_features); @@ -72,16 +72,16 @@ protected: void supply_chain_and_notify(u16 queue_index, QueueChain& chain); - virtual bool handle_device_config_change() = 0; + virtual ErrorOr handle_device_config_change() = 0; virtual void handle_queue_update(u16 queue_index) = 0; TransportEntity& transport_entity() { return *m_transport_entity; } private: - bool accept_device_features(u64 device_features, u64 accepted_features); + ErrorOr accept_device_features(u64 device_features, u64 accepted_features); - bool setup_queue(u16 queue_index); - bool activate_queue(u16 queue_index); + ErrorOr setup_queue(u16 queue_index); + ErrorOr activate_queue(u16 queue_index); void notify_queue(u16 queue_index); Vector> m_queues; diff --git a/Kernel/Bus/VirtIO/RNG.cpp b/Kernel/Bus/VirtIO/RNG.cpp index c2acf9dbb5..a028b0d278 100644 --- a/Kernel/Bus/VirtIO/RNG.cpp +++ b/Kernel/Bus/VirtIO/RNG.cpp @@ -19,14 +19,10 @@ UNMAP_AFTER_INIT NonnullLockRefPtr RNG::must_create_for_pci_instance(PCI::D UNMAP_AFTER_INIT ErrorOr RNG::initialize_virtio_resources() { TRY(Device::initialize_virtio_resources()); - bool success = negotiate_features([&](auto) { + TRY(negotiate_features([&](auto) { return 0; - }); - if (!success) - return Error::from_errno(EIO); - success = setup_queues(1); - if (!success) - return Error::from_errno(EIO); + })); + TRY(setup_queues(1)); finish_init(); m_entropy_buffer = TRY(MM.allocate_contiguous_kernel_region(PAGE_SIZE, "VirtIO::RNG"sv, Memory::Region::Access::ReadWrite)); memset(m_entropy_buffer->vaddr().as_ptr(), 0, m_entropy_buffer->size()); @@ -39,9 +35,9 @@ UNMAP_AFTER_INIT RNG::RNG(NonnullOwnPtr transport_entity) { } -bool RNG::handle_device_config_change() +ErrorOr RNG::handle_device_config_change() { - return false; // Device has no config + return Error::from_errno(EIO); // Device has no config } void RNG::handle_queue_update(u16 queue_index) diff --git a/Kernel/Bus/VirtIO/RNG.h b/Kernel/Bus/VirtIO/RNG.h index ccd2f75eea..55b26b1436 100644 --- a/Kernel/Bus/VirtIO/RNG.h +++ b/Kernel/Bus/VirtIO/RNG.h @@ -27,7 +27,7 @@ public: private: virtual StringView class_name() const override { return "VirtIORNG"sv; } explicit RNG(NonnullOwnPtr); - virtual bool handle_device_config_change() override; + virtual ErrorOr handle_device_config_change() override; virtual void handle_queue_update(u16 queue_index) override; void request_entropy_from_host(); diff --git a/Kernel/Bus/VirtIO/Transport/Entity.cpp b/Kernel/Bus/VirtIO/Transport/Entity.cpp index ae503c32fa..50097aa456 100644 --- a/Kernel/Bus/VirtIO/Transport/Entity.cpp +++ b/Kernel/Bus/VirtIO/Transport/Entity.cpp @@ -135,16 +135,16 @@ void TransportEntity::notify_queue(Badge, NotifyQueueDescriptor config_write16(*m_notify_cfg, descriptor.possible_notify_offset * m_notify_multiplier, descriptor.queue_index); } -bool TransportEntity::activate_queue(Badge, u16 queue_index) +ErrorOr TransportEntity::activate_queue(Badge, u16 queue_index) { if (!m_common_cfg) - return false; + return Error::from_errno(ENXIO); config_write16(*m_common_cfg, COMMON_CFG_QUEUE_SELECT, queue_index); config_write16(*m_common_cfg, COMMON_CFG_QUEUE_ENABLE, true); dbgln_if(VIRTIO_DEBUG, "Queue[{}] activated", queue_index); - return true; + return {}; } u64 TransportEntity::get_device_features() diff --git a/Kernel/Bus/VirtIO/Transport/Entity.h b/Kernel/Bus/VirtIO/Transport/Entity.h index 55a9d18da2..fb7fff7d04 100644 --- a/Kernel/Bus/VirtIO/Transport/Entity.h +++ b/Kernel/Bus/VirtIO/Transport/Entity.h @@ -30,7 +30,7 @@ public: u16 possible_notify_offset; }; void notify_queue(Badge, NotifyQueueDescriptor); - bool activate_queue(Badge, u16 queue_index); + ErrorOr activate_queue(Badge, u16 queue_index); ErrorOr> setup_queue(Badge, u16 queue_index); void set_status_bits(Badge, u8 status_bits); void reset_device(Badge); diff --git a/Kernel/Devices/GPU/VirtIO/GraphicsAdapter.cpp b/Kernel/Devices/GPU/VirtIO/GraphicsAdapter.cpp index 62cc40de8d..e9595aac2a 100644 --- a/Kernel/Devices/GPU/VirtIO/GraphicsAdapter.cpp +++ b/Kernel/Devices/GPU/VirtIO/GraphicsAdapter.cpp @@ -150,7 +150,7 @@ ErrorOr VirtIOGraphicsAdapter::initialize_virtio_resources() TRY(VirtIO::Device::initialize_virtio_resources()); auto* config = TRY(transport_entity().get_config(VirtIO::ConfigurationType::Device)); m_device_configuration = config; - bool success = negotiate_features([&](u64 supported_features) { + TRY(negotiate_features([&](u64 supported_features) { u64 negotiated = 0; if (is_feature_set(supported_features, VIRTIO_GPU_F_VIRGL)) { dbgln_if(VIRTIO_DEBUG, "VirtIO::GraphicsAdapter: VirGL is available, enabling"); @@ -160,21 +160,17 @@ ErrorOr VirtIOGraphicsAdapter::initialize_virtio_resources() if (is_feature_set(supported_features, VIRTIO_GPU_F_EDID)) negotiated |= VIRTIO_GPU_F_EDID; return negotiated; + })); + transport_entity().read_config_atomic([&]() { + m_num_scanouts = transport_entity().config_read32(*config, DEVICE_NUM_SCANOUTS); }); - if (success) { - transport_entity().read_config_atomic([&]() { - m_num_scanouts = transport_entity().config_read32(*config, DEVICE_NUM_SCANOUTS); - }); - dbgln_if(VIRTIO_DEBUG, "VirtIO::GraphicsAdapter: num_scanouts: {}", m_num_scanouts); - success = setup_queues(2); // CONTROLQ + CURSORQ - } - if (!success) - return Error::from_errno(EIO); + dbgln_if(VIRTIO_DEBUG, "VirtIO::GraphicsAdapter: num_scanouts: {}", m_num_scanouts); + TRY(setup_queues(2)); // CONTROLQ + CURSORQ finish_init(); return {}; } -bool VirtIOGraphicsAdapter::handle_device_config_change() +ErrorOr VirtIOGraphicsAdapter::handle_device_config_change() { auto events = get_pending_events(); if (events & VIRTIO_GPU_EVENT_DISPLAY) { @@ -184,9 +180,9 @@ bool VirtIOGraphicsAdapter::handle_device_config_change() } if (events & ~VIRTIO_GPU_EVENT_DISPLAY) { dbgln("VirtIO::GraphicsAdapter: Got unknown device config change event: {:#x}", events); - return false; + return Error::from_errno(EIO); } - return true; + return {}; } void VirtIOGraphicsAdapter::handle_queue_update(u16) diff --git a/Kernel/Devices/GPU/VirtIO/GraphicsAdapter.h b/Kernel/Devices/GPU/VirtIO/GraphicsAdapter.h index 2bb5d7bb83..bfcee9c56b 100644 --- a/Kernel/Devices/GPU/VirtIO/GraphicsAdapter.h +++ b/Kernel/Devices/GPU/VirtIO/GraphicsAdapter.h @@ -69,7 +69,7 @@ private: ErrorOr initialize_adapter(); - virtual bool handle_device_config_change() override; + virtual ErrorOr handle_device_config_change() override; virtual void handle_queue_update(u16 queue_index) override; u32 get_pending_events(); void clear_pending_events(u32 event_bitmask); diff --git a/Kernel/Net/VirtIO/VirtIONetworkAdapter.cpp b/Kernel/Net/VirtIO/VirtIONetworkAdapter.cpp index 326e6d7101..c42dd6d19d 100644 --- a/Kernel/Net/VirtIO/VirtIONetworkAdapter.cpp +++ b/Kernel/Net/VirtIO/VirtIONetworkAdapter.cpp @@ -127,7 +127,7 @@ UNMAP_AFTER_INIT ErrorOr VirtIONetworkAdapter::initialize_virtio_resources TRY(Device::initialize_virtio_resources()); m_device_config = TRY(transport_entity().get_config(VirtIO::ConfigurationType::Device)); - bool success = negotiate_features([&](u64 supported_features) { + TRY(negotiate_features([&](u64 supported_features) { u64 negotiated = 0; if (is_feature_set(supported_features, VIRTIO_NET_F_STATUS)) negotiated |= VIRTIO_NET_F_STATUS; @@ -138,17 +138,10 @@ UNMAP_AFTER_INIT ErrorOr VirtIONetworkAdapter::initialize_virtio_resources if (is_feature_set(supported_features, VIRTIO_NET_F_MTU)) negotiated |= VIRTIO_NET_F_MTU; return negotiated; - }); - if (!success) - return Error::from_errno(EIO); + })); - success = handle_device_config_change(); - if (!success) - return Error::from_errno(EIO); - - success = setup_queues(2); // receive & transmit - if (!success) - return Error::from_errno(EIO); + TRY(handle_device_config_change()); + TRY(setup_queues(2)); // receive & transmit finish_init(); @@ -168,7 +161,7 @@ UNMAP_AFTER_INIT ErrorOr VirtIONetworkAdapter::initialize_virtio_resources return {}; } -bool VirtIONetworkAdapter::handle_device_config_change() +ErrorOr VirtIONetworkAdapter::handle_device_config_change() { dbgln_if(VIRTIO_DEBUG, "VirtIONetworkAdapter: handle_device_config_change"); transport_entity().read_config_atomic([&]() { @@ -196,7 +189,7 @@ bool VirtIONetworkAdapter::handle_device_config_change() m_link_duplex = duplex == 0x01; } }); - return true; + return {}; } void VirtIONetworkAdapter::handle_queue_update(u16 queue_index) diff --git a/Kernel/Net/VirtIO/VirtIONetworkAdapter.h b/Kernel/Net/VirtIO/VirtIONetworkAdapter.h index bc56e134fd..e830c4c67b 100644 --- a/Kernel/Net/VirtIO/VirtIONetworkAdapter.h +++ b/Kernel/Net/VirtIO/VirtIONetworkAdapter.h @@ -37,7 +37,7 @@ private: explicit VirtIONetworkAdapter(StringView interface_name, NonnullOwnPtr); // VirtIO::Device - virtual bool handle_device_config_change() override; + virtual ErrorOr handle_device_config_change() override; virtual void handle_queue_update(u16 queue_index) override; // NetworkAdapter