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

@@ -25,16 +25,14 @@ UNMAP_AFTER_INIT ErrorOr<void> 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<void> 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);
+    TRY(setup_queues(2 + max_nr_ports > 0 ? 2 + 2 * max_nr_ports : 0));
-    if (!success)
-        return Error::from_errno(EIO);
     finish_init();
 
     if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) {
@@ -70,10 +66,10 @@ UNMAP_AFTER_INIT Console::Console(NonnullOwnPtr<TransportEntity> transport_entit
 {
 }
 
-bool Console::handle_device_config_change()
+ErrorOr<void> Console::handle_device_config_change()
 {
     dbgln("VirtIO::Console: Handle device config change");
-    return true;
+    return {};
 }
 
 void Console::handle_queue_update(u16 queue_index)

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<void> handle_device_config_change() override;
     virtual void handle_queue_update(u16 queue_index) override;
 
     Vector<LockRefPtr<ConsolePort>> m_ports;

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<void> 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<void> Device::setup_queue(u16 queue_index)
 {
-    auto queue_or_error = m_transport_entity->setup_queue({}, queue_index);
+    auto queue = TRY(m_transport_entity->setup_queue({}, queue_index));
-    if (queue_or_error.is_error())
-        return false;
-
-    auto queue = queue_or_error.release_value();
     dbgln_if(VIRTIO_DEBUG, "{}: Queue[{}] configured with size: {}", m_class_name, queue_index, queue->size());
 
-    m_queues.append(move(queue));
+    TRY(m_queues.try_append(move(queue)));
-    return true;
+    return {};
 }
 
-bool Device::setup_queues(u16 requested_queue_count)
+ErrorOr<void> 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++) {
+    for (u16 i = 0; i < m_queue_count; i++)
-        if (!setup_queue(i))
+        TRY(setup_queue(i));
-            return false;
+
-    }
+    // NOTE: Queues can only be activated *after* all others queues were also configured
-    for (u16 i = 0; i < m_queue_count; i++) { // Queues can only be activated *after* all others queues were also configured
+    for (u16 i = 0; i < m_queue_count; i++)
-        if (!m_transport_entity->activate_queue({}, i))
+        TRY(m_transport_entity->activate_queue({}, i));
-            return false;
+    return {};
-    }
-    return true;
 }
 
 void Device::finish_init()
@@ -143,7 +137,7 @@ bool Device::handle_irq(Badge<TransportInterruptHandler>)
     }
     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());
         }

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<void> setup_queues(u16 requested_queue_count = 0);
     void finish_init();
 
     Queue& get_queue(u16 queue_index)
@@ -51,7 +51,7 @@ protected:
     }
 
     template<typename F>
-    bool negotiate_features(F f)
+    ErrorOr<void> 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<void> 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<void> accept_device_features(u64 device_features, u64 accepted_features);
 
-    bool setup_queue(u16 queue_index);
+    ErrorOr<void> setup_queue(u16 queue_index);
-    bool activate_queue(u16 queue_index);
+    ErrorOr<void> activate_queue(u16 queue_index);
     void notify_queue(u16 queue_index);
 
     Vector<NonnullOwnPtr<Queue>> m_queues;

Kernel/Bus/VirtIO/RNG.cpp

@@ -19,14 +19,10 @@ UNMAP_AFTER_INIT NonnullLockRefPtr<RNG> RNG::must_create_for_pci_instance(PCI::D
 UNMAP_AFTER_INIT ErrorOr<void> RNG::initialize_virtio_resources()
 {
     TRY(Device::initialize_virtio_resources());
-    bool success = negotiate_features([&](auto) {
+    TRY(negotiate_features([&](auto) {
         return 0;
-    });
+    }));
-    if (!success)
+    TRY(setup_queues(1));
-        return Error::from_errno(EIO);
-    success = setup_queues(1);
-    if (!success)
-        return Error::from_errno(EIO);
     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<TransportEntity> transport_entity)
 {
 }
 
-bool RNG::handle_device_config_change()
+ErrorOr<void> 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)

Kernel/Bus/VirtIO/RNG.h

@@ -27,7 +27,7 @@ public:
 private:
     virtual StringView class_name() const override { return "VirtIORNG"sv; }
     explicit RNG(NonnullOwnPtr<TransportEntity>);
-    virtual bool handle_device_config_change() override;
+    virtual ErrorOr<void> handle_device_config_change() override;
     virtual void handle_queue_update(u16 queue_index) override;
     void request_entropy_from_host();
 

Kernel/Bus/VirtIO/Transport/Entity.cpp

@@ -135,16 +135,16 @@ void TransportEntity::notify_queue(Badge<VirtIO::Device>, NotifyQueueDescriptor
         config_write16(*m_notify_cfg, descriptor.possible_notify_offset * m_notify_multiplier, descriptor.queue_index);
 }
 
-bool TransportEntity::activate_queue(Badge<VirtIO::Device>, u16 queue_index)
+ErrorOr<void> TransportEntity::activate_queue(Badge<VirtIO::Device>, 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()

Kernel/Bus/VirtIO/Transport/Entity.h

@@ -30,7 +30,7 @@ public:
         u16 possible_notify_offset;
     };
     void notify_queue(Badge<VirtIO::Device>, NotifyQueueDescriptor);
-    bool activate_queue(Badge<VirtIO::Device>, u16 queue_index);
+    ErrorOr<void> activate_queue(Badge<VirtIO::Device>, u16 queue_index);
     ErrorOr<NonnullOwnPtr<Queue>> setup_queue(Badge<VirtIO::Device>, u16 queue_index);
     void set_status_bits(Badge<VirtIO::Device>, u8 status_bits);
     void reset_device(Badge<VirtIO::Device>);

Kernel/Devices/GPU/VirtIO/GraphicsAdapter.cpp

@@ -150,7 +150,7 @@ ErrorOr<void> 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<void> 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) {
+    dbgln_if(VIRTIO_DEBUG, "VirtIO::GraphicsAdapter: num_scanouts: {}", m_num_scanouts);
-        transport_entity().read_config_atomic([&]() {
+    TRY(setup_queues(2)); // CONTROLQ + CURSORQ
-            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);
     finish_init();
     return {};
 }
 
-bool VirtIOGraphicsAdapter::handle_device_config_change()
+ErrorOr<void> 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)

Kernel/Devices/GPU/VirtIO/GraphicsAdapter.h

@@ -69,7 +69,7 @@ private:
 
     ErrorOr<void> initialize_adapter();
 
-    virtual bool handle_device_config_change() override;
+    virtual ErrorOr<void> 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);

Kernel/Net/VirtIO/VirtIONetworkAdapter.cpp

@@ -127,7 +127,7 @@ UNMAP_AFTER_INIT ErrorOr<void> 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<void> 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();
+    TRY(handle_device_config_change());
-    if (!success)
+    TRY(setup_queues(2)); // receive & transmit
-        return Error::from_errno(EIO);
-
-    success = setup_queues(2); // receive & transmit
-    if (!success)
-        return Error::from_errno(EIO);
 
     finish_init();
 
@@ -168,7 +161,7 @@ UNMAP_AFTER_INIT ErrorOr<void> VirtIONetworkAdapter::initialize_virtio_resources
     return {};
 }
 
-bool VirtIONetworkAdapter::handle_device_config_change()
+ErrorOr<void> 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)

Kernel/Net/VirtIO/VirtIONetworkAdapter.h

@@ -37,7 +37,7 @@ private:
     explicit VirtIONetworkAdapter(StringView interface_name, NonnullOwnPtr<VirtIO::TransportEntity>);
 
     // VirtIO::Device
-    virtual bool handle_device_config_change() override;
+    virtual ErrorOr<void> handle_device_config_change() override;
     virtual void handle_queue_update(u16 queue_index) override;
 
     // NetworkAdapter