From 87a32ab869f306ce71820efde0f93154d6a5c4ba Mon Sep 17 00:00:00 2001 From: Liav A Date: Wed, 26 Apr 2023 17:01:04 +0300 Subject: [PATCH] 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 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 | 76 ++++++++++--------- Kernel/Bus/VirtIO/Console.h | 2 +- Kernel/Bus/VirtIO/Device.cpp | 13 ++-- Kernel/Bus/VirtIO/Device.h | 2 +- Kernel/Bus/VirtIO/RNG.cpp | 25 +++--- Kernel/Bus/VirtIO/RNG.h | 2 +- Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp | 53 ++++++------- Kernel/Graphics/VirtIOGPU/GraphicsAdapter.h | 2 +- 8 files changed, 89 insertions(+), 86 deletions(-) diff --git a/Kernel/Bus/VirtIO/Console.cpp b/Kernel/Bus/VirtIO/Console.cpp index 8b8818de76..08a67f040a 100644 --- a/Kernel/Bus/VirtIO/Console.cpp +++ b/Kernel/Bus/VirtIO/Console.cpp @@ -19,47 +19,49 @@ UNMAP_AFTER_INIT NonnullLockRefPtr 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 Console::initialize_virtio_resources() { - Device::initialize(); - if (auto const* cfg = get_config(ConfigurationType::Device)) { - bool success = 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) { - u32 max_nr_ports = 0; - u16 cols = 0, rows = 0; - read_config_atomic([&]() { - if (is_feature_accepted(VIRTIO_CONSOLE_F_SIZE)) { - cols = config_read16(*cfg, 0x0); - rows = config_read16(*cfg, 0x2); - } - if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) { - 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); + TRY(Device::initialize_virtio_resources()); + auto const* cfg = get_config(VirtIO::ConfigurationType::Device); + if (!cfg) + return Error::from_errno(ENODEV); + bool success = 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; + read_config_atomic([&]() { + if (is_feature_accepted(VIRTIO_CONSOLE_F_SIZE)) { + cols = config_read16(*cfg, 0x0); + rows = config_read16(*cfg, 0x2); } - if (success) { - finish_init(); + if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) { + 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)) { - setup_multiport(); - } else { - auto port = MUST(DeviceManagement::the().try_create_device(0u, *this)); - port->init_receive_buffer({}); - m_ports.append(port); - } - } + if (is_feature_accepted(VIRTIO_CONSOLE_F_MULTIPORT)) { + setup_multiport(); + } else { + auto port = TRY(DeviceManagement::the().try_create_device(0u, *this)); + port->init_receive_buffer({}); + m_ports.append(port); } + return {}; } UNMAP_AFTER_INIT Console::Console(PCI::DeviceIdentifier const& pci_device_identifier) diff --git a/Kernel/Bus/VirtIO/Console.h b/Kernel/Bus/VirtIO/Console.h index 3b8e58d302..204cdef14d 100644 --- a/Kernel/Bus/VirtIO/Console.h +++ b/Kernel/Bus/VirtIO/Console.h @@ -29,7 +29,7 @@ public: return m_device_id; } - virtual void initialize() override; + virtual ErrorOr initialize_virtio_resources() override; private: virtual StringView class_name() const override { return "VirtIOConsole"sv; } diff --git a/Kernel/Bus/VirtIO/Device.cpp b/Kernel/Bus/VirtIO/Device.cpp index 64634b8258..c1d8c08f32 100644 --- a/Kernel/Bus/VirtIO/Device.cpp +++ b/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 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(ConfigurationType::Common) || raw_config_type > static_cast(ConfigurationType::PCICapabilitiesAccess)) { dbgln("{}: Unknown capability configuration type: {}", m_class_name, raw_config_type); - return; + return Error::from_errno(ENXIO); } config.cfg_type = static_cast(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(cfg.bar)).release_value_but_fixme_should_propagate_errors(); + auto mapping_io_window = TRY(IOWindow::create_for_pci_device_bar(device_identifier(), static_cast(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) diff --git a/Kernel/Bus/VirtIO/Device.h b/Kernel/Bus/VirtIO/Device.h index 04fc889fff..cafcb5a38f 100644 --- a/Kernel/Bus/VirtIO/Device.h +++ b/Kernel/Bus/VirtIO/Device.h @@ -24,7 +24,7 @@ class Device public: virtual ~Device() override = default; - virtual void initialize(); + virtual ErrorOr initialize_virtio_resources(); protected: virtual StringView class_name() const { return "VirtIO::Device"sv; } diff --git a/Kernel/Bus/VirtIO/RNG.cpp b/Kernel/Bus/VirtIO/RNG.cpp index 0f19be3497..6996f7b5af 100644 --- a/Kernel/Bus/VirtIO/RNG.cpp +++ b/Kernel/Bus/VirtIO/RNG.cpp @@ -14,23 +14,22 @@ UNMAP_AFTER_INIT NonnullLockRefPtr 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 RNG::initialize_virtio_resources() { - Device::initialize(); + TRY(Device::initialize_virtio_resources()); bool success = negotiate_features([&](auto) { return 0; }); - if (success) { - success = setup_queues(1); - } - if (success) { - finish_init(); - m_entropy_buffer = MM.allocate_contiguous_kernel_region(PAGE_SIZE, "VirtIO::RNG"sv, Memory::Region::Access::ReadWrite).release_value(); - if (m_entropy_buffer) { - memset(m_entropy_buffer->vaddr().as_ptr(), 0, m_entropy_buffer->size()); - request_entropy_from_host(); - } - } + if (!success) + 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()); + request_entropy_from_host(); + return {}; } UNMAP_AFTER_INIT RNG::RNG(PCI::DeviceIdentifier const& device_identifier) diff --git a/Kernel/Bus/VirtIO/RNG.h b/Kernel/Bus/VirtIO/RNG.h index 5f7f8a1ee8..0070add908 100644 --- a/Kernel/Bus/VirtIO/RNG.h +++ b/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 initialize_virtio_resources() override; private: virtual StringView class_name() const override { return "VirtIORNG"sv; } diff --git a/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp b/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp index 279921cc92..b122c8bd3d 100644 --- a/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp +++ b/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp @@ -37,7 +37,7 @@ ErrorOr> 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 VirtIOGraphicsAdapter::initialize_virtio_resources() { - VirtIO::Device::initialize(); - if (auto* config = get_config(VirtIO::ConfigurationType::Device)) { - m_device_configuration = config; - bool success = 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"); - negotiated |= VIRTIO_GPU_F_VIRGL; - m_has_virgl_support = true; - } - if (is_feature_set(supported_features, VIRTIO_GPU_F_EDID)) - 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 + TRY(VirtIO::Device::initialize_virtio_resources()); + auto* config = get_config(VirtIO::ConfigurationType::Device); + if (!config) + return Error::from_errno(ENODEV); + m_device_configuration = config; + bool success = 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"); + negotiated |= VIRTIO_GPU_F_VIRGL; + m_has_virgl_support = true; } - VERIFY(success); - finish_init(); - } else { - VERIFY_NOT_REACHED(); + if (is_feature_set(supported_features, VIRTIO_GPU_F_EDID)) + 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 } + if (!success) + return Error::from_errno(EIO); + finish_init(); + return {}; } bool VirtIOGraphicsAdapter::handle_device_config_change() diff --git a/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.h b/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.h index ec01de1e76..1bd845e271 100644 --- a/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.h +++ b/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.h @@ -40,7 +40,7 @@ public: static ErrorOr probe(PCI::DeviceIdentifier const&); static ErrorOr> create(PCI::DeviceIdentifier const&); - virtual void initialize() override; + virtual ErrorOr initialize_virtio_resources() override; virtual StringView device_name() const override { return "VirtIOGraphicsAdapter"sv; }