From 657bc71247681d1e7cdd73591f87ab03163113a1 Mon Sep 17 00:00:00 2001
From: Liav A <liavalb@gmail.com>
Date: Fri, 10 Mar 2023 09:13:05 +0200
Subject: [PATCH] Kernel/VirtIO: Ignore the VIRTIO_PCI_CAP_PCI_CFG
 configuration type

This configuration exposes a suboptimal mechanism to access other
VirtIO device configurations. It is also the only configuration to use a
zero length for a configuration structure, and specify a valid BAR which
triggered a kernel panic when attaching a virtio-gpu-pci device before
95b15e49010299902abd2aa65bcc71e4158b7326 was applied.

The real solution for that problem is to ignore this configuration type
because we never actually use it. It means that we can VERIFY that all
other configuration types have a valid length, as being expected.
---
 Kernel/Bus/VirtIO/Device.cpp | 26 +++++++++++++++++++++++---
 Kernel/Bus/VirtIO/Device.h   |  2 +-
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/Kernel/Bus/VirtIO/Device.cpp b/Kernel/Bus/VirtIO/Device.cpp
index 81cf8f57d2..64634b8258 100644
--- a/Kernel/Bus/VirtIO/Device.cpp
+++ b/Kernel/Bus/VirtIO/Device.cpp
@@ -96,7 +96,21 @@ UNMAP_AFTER_INIT void Device::initialize()
             // We have a virtio_pci_cap
             Configuration config {};
             auto raw_config_type = capability.read8(0x3);
-            if (raw_config_type < static_cast<u8>(ConfigurationType::Common) || raw_config_type > static_cast<u8>(ConfigurationType::PCI)) {
+            // NOTE: The VirtIO specification allows iteration of configurations
+            // through a special PCI capbility structure with the VIRTIO_PCI_CAP_PCI_CFG tag:
+            //
+            // "Each structure can be mapped by a Base Address register (BAR) belonging to the function, or accessed via
+            // the special VIRTIO_PCI_CAP_PCI_CFG field in the PCI configuration space"
+            //
+            // "The VIRTIO_PCI_CAP_PCI_CFG capability creates an alternative (and likely suboptimal) access method
+            // to the common configuration, notification, ISR and device-specific configuration regions."
+            //
+            // Also, it is *very* likely to see this PCI capability as the first vendor-specific capbility of a certain PCI function,
+            // but this is not guaranteed by the VirtIO specification.
+            // Therefore, ignore this type of configuration as this is not needed by our implementation currently.
+            if (raw_config_type == static_cast<u8>(ConfigurationType::PCICapabilitiesAccess))
+                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;
             }
@@ -113,9 +127,15 @@ UNMAP_AFTER_INIT void Device::initialize()
             }
             config.offset = capability.read32(0x8);
             config.length = capability.read32(0xc);
-            // NOTE: Configuration length of zero is an invalid configuration that should be ignored.
-            if (config.length == 0)
+            // NOTE: Configuration length of zero is an invalid configuration, or at the very least a configuration
+            // type we don't know how to handle correctly...
+            // The VIRTIO_PCI_CAP_PCI_CFG configuration structure has length of 0
+            // but because we ignore that type and all other types should have a length
+            // greater than 0, we should ignore any other configuration in case this condition is not met.
+            if (config.length == 0) {
+                dbgln("{}: Found configuration {}, with invalid length of 0", m_class_name, (u32)config.cfg_type);
                 continue;
+            }
             dbgln_if(VIRTIO_DEBUG, "{}: Found configuration {}, bar: {}, offset: {}, length: {}", m_class_name, (u32)config.cfg_type, config.bar, config.offset, config.length);
             if (config.cfg_type == ConfigurationType::Common)
                 m_use_mmio = true;
diff --git a/Kernel/Bus/VirtIO/Device.h b/Kernel/Bus/VirtIO/Device.h
index d62c17ec8b..e389eabff9 100644
--- a/Kernel/Bus/VirtIO/Device.h
+++ b/Kernel/Bus/VirtIO/Device.h
@@ -70,7 +70,7 @@ enum class ConfigurationType : u8 {
     Notify = 2,
     ISR = 3,
     Device = 4,
-    PCI = 5
+    PCICapabilitiesAccess = 5
 };
 
 struct Configuration {