Kernel/VirtIO: Improve error handling during device initialization

Rename the initialize method to initialize_virtio_resources so it's
clear what this method is intended for.

To ensure healthier device initialization, we could also return the type
of ErrorOr<void> from this method, so in all overriden instances and in
the original method code, we could leverage TRY() pattern which also
does simplify the code a bit.

Kernel/Bus/VirtIO/Console.cpp

@@ -19,47 +19,49 @@ UNMAP_AFTER_INIT NonnullLockRefPtr<Console> Console::must_create(PCI::DeviceIden
     return adopt_lock_ref_if_nonnull(new Console(pci_device_identifier)).release_nonnull();
 }
 
-UNMAP_AFTER_INIT void Console::initialize()
+UNMAP_AFTER_INIT ErrorOr<void> Console::initialize_virtio_resources()
 {
-    Device::initialize();
+    TRY(Device::initialize_virtio_resources());
-    if (auto const* cfg = get_config(ConfigurationType::Device)) {
+    auto const* cfg = get_config(VirtIO::ConfigurationType::Device);
-        bool success = negotiate_features([&](u64 supported_features) {
+    if (!cfg)
-            u64 negotiated = 0;
+        return Error::from_errno(ENODEV);
-            if (is_feature_set(supported_features, VIRTIO_CONSOLE_F_SIZE))
+    bool success = negotiate_features([&](u64 supported_features) {
-                dbgln("VirtIO::Console: Console size is not yet supported!");
+        u64 negotiated = 0;
-            if (is_feature_set(supported_features, VIRTIO_CONSOLE_F_MULTIPORT))
+        if (is_feature_set(supported_features, VIRTIO_CONSOLE_F_SIZE))
-                negotiated |= VIRTIO_CONSOLE_F_MULTIPORT;
+            dbgln("VirtIO::Console: Console size is not yet supported!");
-            return negotiated;
+        if (is_feature_set(supported_features, VIRTIO_CONSOLE_F_MULTIPORT))
-        });
+            negotiated |= VIRTIO_CONSOLE_F_MULTIPORT;
-        if (success) {
+        return negotiated;
-            u32 max_nr_ports = 0;
+    });
-            u16 cols = 0, rows = 0;
+    if (!success)
-            read_config_atomic([&]() {
+        return Error::from_errno(EIO);
-                if (is_feature_accepted(VIRTIO_CONSOLE_F_SIZE)) {
+    u32 max_nr_ports = 0;
-                    cols = config_read16(*cfg, 0x0);
+    u16 cols = 0, rows = 0;
-                    rows = config_read16(*cfg, 0x2);
+    read_config_atomic([&]() {
-                }
+        if (is_feature_accepted(VIRTIO_CONSOLE_F_SIZE)) {
-                if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) {
+            cols = config_read16(*cfg, 0x0);
-                    max_nr_ports = config_read32(*cfg, 0x4);
+            rows = config_read16(*cfg, 0x2);
-                    m_ports.resize(max_nr_ports);
-                }
-            });
-            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) {
+        if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) {
-            finish_init();
+            max_nr_ports = config_read32(*cfg, 0x4);
+            m_ports.resize(max_nr_ports);
+        }
+    });
+    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);
+    finish_init();
 
-            if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) {
+    if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) {
-                setup_multiport();
+        setup_multiport();
-            } else {
+    } else {
-                auto port = MUST(DeviceManagement::the().try_create_device<VirtIO::ConsolePort>(0u, *this));
+        auto port = TRY(DeviceManagement::the().try_create_device<VirtIO::ConsolePort>(0u, *this));
-                port->init_receive_buffer({});
+        port->init_receive_buffer({});
-                m_ports.append(port);
+        m_ports.append(port);
-            }
-        }
     }
+    return {};
 }
 
 UNMAP_AFTER_INIT Console::Console(PCI::DeviceIdentifier const& pci_device_identifier)

Kernel/Bus/VirtIO/Console.h

@@ -29,7 +29,7 @@ public:
         return m_device_id;
     }
 
-    virtual void initialize() override;
+    virtual ErrorOr<void> initialize_virtio_resources() override;
 
 private:
     virtual StringView class_name() const override { return "VirtIOConsole"sv; }

Kernel/Bus/VirtIO/Device.cpp

@@ -27,12 +27,12 @@ UNMAP_AFTER_INIT void detect()
         switch (device_identifier.hardware_id().device_id) {
         case PCI::DeviceID::VirtIOConsole: {
             auto& console = Console::must_create(device_identifier).leak_ref();
-            console.initialize();
+            MUST(console.initialize_virtio_resources());
             break;
         }
         case PCI::DeviceID::VirtIOEntropy: {
             auto& rng = RNG::must_create(device_identifier).leak_ref();
-            rng.initialize();
+            MUST(rng.initialize_virtio_resources());
             break;
         }
         case PCI::DeviceID::VirtIOGPU: {
@@ -86,7 +86,7 @@ static StringView determine_device_class(PCI::DeviceIdentifier const& device_ide
     }
 }
 
-UNMAP_AFTER_INIT void Device::initialize()
+UNMAP_AFTER_INIT ErrorOr<void> Device::initialize_virtio_resources()
 {
     enable_bus_mastering(device_identifier());
 
@@ -112,7 +112,7 @@ UNMAP_AFTER_INIT void Device::initialize()
                 continue;
             if (raw_config_type < static_cast<u8>(ConfigurationType::Common) || raw_config_type > static_cast<u8>(ConfigurationType::PCICapabilitiesAccess)) {
                 dbgln("{}: Unknown capability configuration type: {}", m_class_name, raw_config_type);
-                return;
+                return Error::from_errno(ENXIO);
             }
             config.cfg_type = static_cast<ConfigurationType>(raw_config_type);
             auto cap_length = capability.read8(0x2);
@@ -148,14 +148,14 @@ UNMAP_AFTER_INIT void Device::initialize()
 
     if (m_use_mmio) {
         for (auto& cfg : m_configs) {
-            auto mapping_io_window = IOWindow::create_for_pci_device_bar(device_identifier(), static_cast<PCI::HeaderType0BaseRegister>(cfg.bar)).release_value_but_fixme_should_propagate_errors();
+            auto mapping_io_window = TRY(IOWindow::create_for_pci_device_bar(device_identifier(), static_cast<PCI::HeaderType0BaseRegister>(cfg.bar)));
             m_register_bases[cfg.bar] = move(mapping_io_window);
         }
         m_common_cfg = get_config(ConfigurationType::Common, 0);
         m_notify_cfg = get_config(ConfigurationType::Notify, 0);
         m_isr_cfg = get_config(ConfigurationType::ISR, 0);
     } else {
-        auto mapping_io_window = IOWindow::create_for_pci_device_bar(device_identifier(), PCI::HeaderType0BaseRegister::BAR0).release_value_but_fixme_should_propagate_errors();
+        auto mapping_io_window = TRY(IOWindow::create_for_pci_device_bar(device_identifier(), PCI::HeaderType0BaseRegister::BAR0));
         m_register_bases[0] = move(mapping_io_window);
     }
 
@@ -169,6 +169,7 @@ UNMAP_AFTER_INIT void Device::initialize()
     set_status_bit(DEVICE_STATUS_ACKNOWLEDGE);
 
     set_status_bit(DEVICE_STATUS_DRIVER);
+    return {};
 }
 
 UNMAP_AFTER_INIT VirtIO::Device::Device(PCI::DeviceIdentifier const& device_identifier)

Kernel/Bus/VirtIO/Device.h

@@ -24,7 +24,7 @@ class Device
 public:
     virtual ~Device() override = default;
 
-    virtual void initialize();
+    virtual ErrorOr<void> initialize_virtio_resources();
 
 protected:
     virtual StringView class_name() const { return "VirtIO::Device"sv; }

Kernel/Bus/VirtIO/RNG.cpp

@@ -14,23 +14,22 @@ UNMAP_AFTER_INIT NonnullLockRefPtr<RNG> RNG::must_create(PCI::DeviceIdentifier c
     return adopt_lock_ref_if_nonnull(new RNG(device_identifier)).release_nonnull();
 }
 
-UNMAP_AFTER_INIT void RNG::initialize()
+UNMAP_AFTER_INIT ErrorOr<void> RNG::initialize_virtio_resources()
 {
-    Device::initialize();
+    TRY(Device::initialize_virtio_resources());
     bool success = negotiate_features([&](auto) {
         return 0;
     });
-    if (success) {
+    if (!success)
-        success = setup_queues(1);
+        return Error::from_errno(EIO);
-    }
+    success = setup_queues(1);
-    if (success) {
+    if (!success)
-        finish_init();
+        return Error::from_errno(EIO);
-        m_entropy_buffer = MM.allocate_contiguous_kernel_region(PAGE_SIZE, "VirtIO::RNG"sv, Memory::Region::Access::ReadWrite).release_value();
+    finish_init();
-        if (m_entropy_buffer) {
+    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());
+    memset(m_entropy_buffer->vaddr().as_ptr(), 0, m_entropy_buffer->size());
-            request_entropy_from_host();
+    request_entropy_from_host();
-        }
+    return {};
-    }
 }
 
 UNMAP_AFTER_INIT RNG::RNG(PCI::DeviceIdentifier const& device_identifier)

Kernel/Bus/VirtIO/RNG.h

@@ -24,7 +24,7 @@ public:
     virtual StringView device_name() const override { return class_name(); }
     virtual ~RNG() override = default;
 
-    virtual void initialize() override;
+    virtual ErrorOr<void> initialize_virtio_resources() override;
 
 private:
     virtual StringView class_name() const override { return "VirtIORNG"sv; }

Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp

@@ -37,7 +37,7 @@ ErrorOr<NonnullLockRefPtr<GenericGraphicsAdapter>> VirtIOGraphicsAdapter::create
 
     auto active_context_ids = TRY(Bitmap::create(VREND_MAX_CTX, false));
     auto adapter = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) VirtIOGraphicsAdapter(device_identifier, move(active_context_ids), move(scratch_space_region))));
-    adapter->initialize();
+    TRY(adapter->initialize_virtio_resources());
     TRY(adapter->initialize_adapter());
     return adapter;
 }
@@ -143,34 +143,35 @@ VirtIOGraphicsAdapter::VirtIOGraphicsAdapter(PCI::DeviceIdentifier const& device
     });
 }
 
-void VirtIOGraphicsAdapter::initialize()
+ErrorOr<void> VirtIOGraphicsAdapter::initialize_virtio_resources()
 {
-    VirtIO::Device::initialize();
+    TRY(VirtIO::Device::initialize_virtio_resources());
-    if (auto* config = get_config(VirtIO::ConfigurationType::Device)) {
+    auto* config = get_config(VirtIO::ConfigurationType::Device);
-        m_device_configuration = config;
+    if (!config)
-        bool success = negotiate_features([&](u64 supported_features) {
+        return Error::from_errno(ENODEV);
-            u64 negotiated = 0;
+    m_device_configuration = config;
-            if (is_feature_set(supported_features, VIRTIO_GPU_F_VIRGL)) {
+    bool success = negotiate_features([&](u64 supported_features) {
-                dbgln_if(VIRTIO_DEBUG, "VirtIO::GraphicsAdapter: VirGL is available, enabling");
+        u64 negotiated = 0;
-                negotiated |= VIRTIO_GPU_F_VIRGL;
+        if (is_feature_set(supported_features, VIRTIO_GPU_F_VIRGL)) {
-                m_has_virgl_support = true;
+            dbgln_if(VIRTIO_DEBUG, "VirtIO::GraphicsAdapter: VirGL is available, enabling");
-            }
+            negotiated |= VIRTIO_GPU_F_VIRGL;
-            if (is_feature_set(supported_features, VIRTIO_GPU_F_EDID))
+            m_has_virgl_support = true;
-                negotiated |= VIRTIO_GPU_F_EDID;
-            return negotiated;
-        });
-        if (success) {
-            read_config_atomic([&]() {
-                m_num_scanouts = config_read32(*config, DEVICE_NUM_SCANOUTS);
-            });
-            dbgln_if(VIRTIO_DEBUG, "VirtIO::GraphicsAdapter: num_scanouts: {}", m_num_scanouts);
-            success = setup_queues(2); // CONTROLQ + CURSORQ
         }
-        VERIFY(success);
+        if (is_feature_set(supported_features, VIRTIO_GPU_F_EDID))
-        finish_init();
+            negotiated |= VIRTIO_GPU_F_EDID;
-    } else {
+        return negotiated;
-        VERIFY_NOT_REACHED();
+    });
+    if (success) {
+        read_config_atomic([&]() {
+            m_num_scanouts = 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()

Kernel/Graphics/VirtIOGPU/GraphicsAdapter.h

@@ -40,7 +40,7 @@ public:
     static ErrorOr<bool> probe(PCI::DeviceIdentifier const&);
     static ErrorOr<NonnullLockRefPtr<GenericGraphicsAdapter>> create(PCI::DeviceIdentifier const&);
 
-    virtual void initialize() override;
+    virtual ErrorOr<void> initialize_virtio_resources() override;
 
     virtual StringView device_name() const override { return "VirtIOGraphicsAdapter"sv; }