Kernel/riscv: Use new DeviceTree helpers in PCI initializations

This also changes the PCI interface slightly to be a bit nicer to work
with.

Kernel/Arch/riscv64/PCI/Initializer.cpp

@@ -28,9 +28,9 @@ void initialize()
 
     new Access();
 
-    // https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf
-    // https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus-common.yaml
-    // https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-host-bridge.yaml
+    // [1]: https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf
+    // [2]: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus-common.yaml
+    // [3]: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-host-bridge.yaml
 
     // The pci controllers are usually in /soc/pcie?@XXXXXXXX
     // FIXME: They can also appear in the root node, or any simple-bus other than soc
@@ -58,23 +58,22 @@ void initialize()
     u32 pci_32bit_mmio_size = 0;
     FlatPtr pci_64bit_mmio_base = 0;
     u64 pci_64bit_mmio_size = 0;
-    HashMap<u32, u64> masked_interrupt_mapping;
-    u32 interrupt_mask = 0;
-    for (auto const& entry : soc.children()) {
-        if (!entry.key.starts_with("pci"sv))
+    HashMap<PCIInterruptSpecifier, u64> masked_interrupt_mapping;
+    PCIInterruptSpecifier interrupt_mask;
+    for (auto const& [name, node] : soc.children()) {
+        if (!name.starts_with("pci"sv))
             continue;
-        auto const& node = entry.value;
 
         if (auto device_type = node.get_property("device_type"sv); !device_type.has_value() || device_type.value().as_string() != "pci"sv) {
             // Technically, the device_type property is deprecated, but if it is present,
             // no harm's done in checking it anyway
-            dmesgln("PCI: PCI named devicetree entry {} not a PCI type device, got device type '{}' instead", entry.key, device_type.has_value() ? device_type.value().as_string() : "<None>"sv);
+            dmesgln("PCI: PCI named devicetree entry {} not a PCI type device, got device type '{}' instead", name, device_type.has_value() ? device_type.value().as_string() : "<None>"sv);
             continue;
         }
 
         auto maybe_compatible = node.get_property("compatible"sv);
         if (!maybe_compatible.has_value()) {
-            dmesgln("PCI: Devicetree node for {} does not have a 'compatible' string, rejecting", entry.key);
+            dmesgln("PCI: Devicetree node for {} does not have a 'compatible' string, rejecting", name);
             continue;
         }
         auto compatible = maybe_compatible.value();
@@ -92,14 +91,14 @@ void initialize()
             return IterationDecision::Continue;
         });
         if (controller_compatibility == ControllerCompatible::Unknown) {
-            dmesgln("PCI: Devicetree node for {} does not have a known 'compatible' string, rejecting", entry.key);
+            dmesgln("PCI: Devicetree node for {} does not have a known 'compatible' string, rejecting", name);
             dmesgln("PCI: Compatible strings provided: {}", compatible.as_strings());
             continue;
         }
 
         auto maybe_reg = node.get_property("reg"sv);
         if (!maybe_reg.has_value()) {
-            dmesgln("PCI: Devicetree node for {} does not have a physical address assigned to it, rejecting", entry.key);
+            dmesgln("PCI: Devicetree node for {} does not have a physical address assigned to it, rejecting", name);
             continue;
         }
         auto reg = maybe_reg.value();
@@ -123,7 +122,7 @@ void initialize()
             domain_counter = domain_counter.value() + 1;
         } else {
             if (domain_counter.has_value()) {
-                dmesgln("PCI: Devicetree node for {} has a PCI-domain assigned, but a previous controller did not have one assigned", entry.key);
+                dmesgln("PCI: Devicetree node for {} has a PCI-domain assigned, but a previous controller did not have one assigned", name);
                 dmesgln("PCI: This could lead to domain collisions if handled improperly");
                 dmesgln("PCI: We will for now reject this device for now, further investigation is advised");
                 continue;
@@ -136,11 +135,7 @@ void initialize()
             // FIXME: Make this use a nice helper function
             // FIXME: Use the provided size field
             auto stream = reg.as_stream();
-            FlatPtr paddr;
-            if (soc_address_cells == 1)
-                paddr = MUST(stream.read_value<BigEndian<u32>>());
-            else
-                paddr = MUST(stream.read_value<BigEndian<u64>>());
+            FlatPtr paddr = MUST(stream.read_cells(soc_address_cells));
 
             Access::the().add_host_controller(
                 MemoryBackedHostBridge::must_create(
@@ -161,68 +156,107 @@ void initialize()
             auto address_cells = node.get_property("#address-cells"sv).value().as<u32>();
             VERIFY(address_cells == 3); // Additional cell for OpenFirmware PCI address metadata
             auto size_cells = node.get_property("#size-cells"sv).value().as<u32>();
+
             auto stream = maybe_ranges.value().as_stream();
             while (!stream.is_eof()) {
-                u32 pci_address_metadata = MUST(stream.read_value<BigEndian<u32>>());
-                FlatPtr pci_address = MUST(stream.read_value<BigEndian<u64>>());
-                FlatPtr mmio_address;
-                if (soc_address_cells == 1)
-                    mmio_address = MUST(stream.read_value<BigEndian<u32>>());
-                else
-                    mmio_address = MUST(stream.read_value<BigEndian<u64>>());
-                u64 mmio_size;
-                if (size_cells == 1)
-                    mmio_size = MUST(stream.read_value<BigEndian<u32>>());
-                else
-                    mmio_size = MUST(stream.read_value<BigEndian<u64>>());
-                auto space_type = (pci_address_metadata >> OpenFirmwareAddress::space_type_offset) & OpenFirmwareAddress::space_type_mask;
-                if (space_type != OpenFirmwareAddress::SpaceType::Memory32BitSpace && space_type != OpenFirmwareAddress::SpaceType::Memory64BitSpace)
+                auto pci_address_metadata = bit_cast<OpenFirmwareAddress>(MUST(stream.read_cell()));
+                FlatPtr pci_address = MUST(stream.read_cells(2));
+
+                FlatPtr mmio_address = MUST(stream.read_cells(soc_address_cells));
+                u64 mmio_size = MUST(stream.read_cells(size_cells));
+
+                if (pci_address_metadata.space_type != OpenFirmwareAddress::SpaceType::Memory32BitSpace
+                    && pci_address_metadata.space_type != OpenFirmwareAddress::SpaceType::Memory64BitSpace)
                     continue; // We currently only support memory-mapped PCI on RISC-V
+
                 // TODO: Support mapped PCI addresses
                 VERIFY(pci_address == mmio_address);
-                if (space_type == OpenFirmwareAddress::SpaceType::Memory32BitSpace) {
-                    auto prefetchable = (pci_address_metadata >> OpenFirmwareAddress::prefetchable_offset) & OpenFirmwareAddress::prefetchable_mask;
-                    if (prefetchable)
+                if (pci_address_metadata.space_type == OpenFirmwareAddress::SpaceType::Memory32BitSpace) {
+                    if (pci_address_metadata.prefetchable)
                         continue; // We currently only use non-prefetchable 32-bit regions, since 64-bit regions are always prefetchable - TODO: Use 32-bit prefetchable regions if only they are available
                     if (pci_32bit_mmio_size >= mmio_size)
                         continue; // We currently only use the single largest region - TODO: Use all available regions if needed
+
                     pci_32bit_mmio_base = mmio_address;
                     pci_32bit_mmio_size = mmio_size;
                 } else {
                     if (pci_64bit_mmio_size >= mmio_size)
                         continue; // We currently only use the single largest region - TODO: Use all available regions if needed
+
                     pci_64bit_mmio_base = mmio_address;
                     pci_64bit_mmio_size = mmio_size;
                 }
             }
         }
 
+        // 2.4.3 Interrupt Nexus Properties
+        // #interrupt-cells: [2] `1` for pci busses
+        // interrupt-map:
+        //  [{
+        //     child-unit-address(bus-node/#address-cells|3),
+        //     child-interrupt-specifier(#interrupt-cells|1),
+        //     interrupt-parent(phandle),
+        //     parent-unit-address(interrupt-parent/#address-cells),
+        //     parent-interrupt-specifier(interrupt-parent/#interrupt-cells)
+        //  }]
+        //   Note: The bus-node may be any other bus the child is connected to
+        //   FIXME?: Let's just hope this is always this/a PCI bus
+        // interrupt-map-mask:
+        // > This property specifies a  mask that is ANDed with the incoming
+        // > unit interrupt specifier being looked up in the table specified in the
+        // > interrupt-map property.
+        // Hence this should be of size:
+        // pci/#address-cells(3) + #interrupt-cells(1) = 4
         auto maybe_interrupt_map = node.get_property("interrupt-map"sv);
         auto maybe_interrupt_map_mask = node.get_property("interrupt-map-mask"sv);
         if (maybe_interrupt_map.has_value() && maybe_interrupt_map_mask.has_value()) {
+            VERIFY(node.get_property("#interrupt-cells"sv)->as<u32>() == 1);
+            VERIFY(maybe_interrupt_map_mask.value().size() == 4 * sizeof(u32));
+
             auto mask_stream = maybe_interrupt_map_mask.value().as_stream();
-            u32 metadata_mask = MUST(mask_stream.read_value<BigEndian<u32>>());
-            MUST(mask_stream.discard(sizeof(u32) * 2));
-            VERIFY(node.get_property("#interrupt-cells"sv)->as<u32>() == 1); // PCI interrupt pin should always fit in one word
-            u32 pin_mask = MUST(mask_stream.read_value<BigEndian<u32>>());
-            interrupt_mask = ((metadata_mask >> 8) << 8) | pin_mask;
+            auto metadata_mask = bit_cast<OpenFirmwareAddress>(MUST(mask_stream.read_cell()));
+            u64 phyical_address_mask = MUST(mask_stream.read_cells(2));
+            // [2]: phys.mid and phys.lo mask should be 0 -> physical-address-mask = 0
+            //      0 < metadata_mask < 0xff00
+            VERIFY(phyical_address_mask == 0);
+            VERIFY(metadata_mask.raw <= 0xff00);
+            // Additionally it would be ludicrous/impossible to differentiate interrupts on registers
+            VERIFY(metadata_mask.register_ == 0);
+
+            u32 pin_mask = MUST(mask_stream.read_cell());
+            // [2]: The interrupt specifier mask should be between 0 and 7
+            VERIFY(pin_mask <= 7);
+
+            interrupt_mask = PCIInterruptSpecifier {
+                .interrupt_pin = static_cast<u8>(pin_mask),
+                .function = metadata_mask.function,
+                .device = metadata_mask.device,
+                .bus = metadata_mask.bus,
+            };
             auto map_stream = maybe_interrupt_map.value().as_stream();
             while (!map_stream.is_eof()) {
-                u32 pci_address_metadata = MUST(map_stream.read_value<BigEndian<u32>>());
-                MUST(map_stream.discard(sizeof(u32) * 2));
-                u32 pin = MUST(map_stream.read_value<BigEndian<u32>>());
-                u32 interrupt_controller_phandle = MUST(map_stream.read_value<BigEndian<u32>>());
-                auto* interrupt_controller = device_tree.phandle(interrupt_controller_phandle);
+                auto pci_address_metadata = bit_cast<OpenFirmwareAddress>(MUST(map_stream.read_cell()));
+                MUST(map_stream.discard(sizeof(u32) * 2)); // Physical Address, the mask for those is guaranteed to be 0
+                u32 pin = MUST(map_stream.read_cell());
+
+                u32 interrupt_controller_phandle = MUST(map_stream.read_cell());
+                auto const* interrupt_controller = device_tree.phandle(interrupt_controller_phandle);
                 VERIFY(interrupt_controller);
+
                 auto interrupt_cells = interrupt_controller->get_property("#interrupt-cells"sv)->as<u32>();
                 VERIFY(interrupt_cells == 1 || interrupt_cells == 2);
-                u64 interrupt;
-                if (interrupt_cells == 1)
-                    interrupt = MUST(map_stream.read_value<BigEndian<u32>>());
-                else
-                    interrupt = MUST(map_stream.read_value<BigEndian<u64>>());
-                auto masked_specifier = (((pci_address_metadata >> 8) << 8) | pin) & interrupt_mask;
-                masked_interrupt_mapping.set(masked_specifier, interrupt);
+                u64 interrupt = MUST(map_stream.read_cells(interrupt_cells));
+
+                pin &= pin_mask;
+                pci_address_metadata.raw &= metadata_mask.raw;
+                masked_interrupt_mapping.set(
+                    PCIInterruptSpecifier {
+                        .interrupt_pin = static_cast<u8>(pin),
+                        .function = pci_address_metadata.function,
+                        .device = pci_address_metadata.device,
+                        .bus = pci_address_metadata.bus,
+                    },
+                    interrupt);
             }
         }
     }

Kernel/Bus/PCI/Controller/HostController.cpp

@@ -245,7 +245,13 @@ void HostController::configure_attached_devices(PCIConfiguration& config)
         write16_field(device_identifier.address().bus(), device_identifier.address().device(), device_identifier.address().function(), PCI::RegisterOffset::COMMAND, command_value);
         // assign interrupt number
         auto interrupt_pin = read8_field(device_identifier.address().bus(), device_identifier.address().device(), device_identifier.address().function(), PCI::RegisterOffset::INTERRUPT_PIN);
-        auto masked_identifier = (((u32)device_identifier.address().bus() << 16) | ((u32)device_identifier.address().device() << 11) | ((u32)device_identifier.address().function() << 8) | interrupt_pin) & config.interrupt_mask;
+        auto masked_identifier = PCIInterruptSpecifier{
+            .interrupt_pin = interrupt_pin,
+            .function = device_identifier.address().function(),
+            .device = device_identifier.address().device(),
+            .bus = device_identifier.address().bus()
+        };
+        masked_identifier &= config.interrupt_mask;
         auto interrupt_number = config.masked_interrupt_mapping.get(masked_identifier);
         if (interrupt_number.has_value())
             write8_field(device_identifier.address().bus(), device_identifier.address().device(), device_identifier.address().function(), PCI::RegisterOffset::INTERRUPT_LINE, interrupt_number.value());

Kernel/Bus/PCI/Controller/HostController.h

@@ -18,6 +18,46 @@ AK_TYPEDEF_DISTINCT_ORDERED_ID(u8, BusNumber);
 AK_TYPEDEF_DISTINCT_ORDERED_ID(u8, DeviceNumber);
 AK_TYPEDEF_DISTINCT_ORDERED_ID(u8, FunctionNumber);
 
+struct PCIInterruptSpecifier {
+    u8 interrupt_pin { 0 };
+    FunctionNumber function { 0 };
+    DeviceNumber device { 0 };
+    BusNumber bus { 0 };
+
+    bool operator==(PCIInterruptSpecifier const& other) const
+    {
+        return bus == other.bus && device == other.device && function == other.function && interrupt_pin == other.interrupt_pin;
+    }
+    PCIInterruptSpecifier operator&(PCIInterruptSpecifier other) const
+    {
+        return PCIInterruptSpecifier {
+            .interrupt_pin = static_cast<u8>(interrupt_pin & other.interrupt_pin),
+            .function = function.value() & other.function.value(),
+            .device = device.value() & other.device.value(),
+            .bus = bus.value() & other.bus.value(),
+        };
+    }
+
+    PCIInterruptSpecifier& operator&=(PCIInterruptSpecifier const& other)
+    {
+        *this = *this & other;
+        return *this;
+    }
+};
+
+}
+namespace AK {
+template<>
+struct Traits<Kernel::PCI::PCIInterruptSpecifier> : public DefaultTraits<Kernel::PCI::PCIInterruptSpecifier> {
+    static unsigned hash(Kernel::PCI::PCIInterruptSpecifier value)
+    {
+        return int_hash(value.bus.value() << 24 | value.device.value() << 16 | value.function.value() << 8 | value.interrupt_pin);
+    }
+};
+
+}
+namespace Kernel::PCI {
+
 struct PCIConfiguration {
     FlatPtr mmio_32bit_base { 0 };
     FlatPtr mmio_32bit_end { 0 };
@@ -25,8 +65,8 @@ struct PCIConfiguration {
     FlatPtr mmio_64bit_end { 0 };
     // The keys contains the bus, device & function at the same offsets as OpenFirmware PCI addresses,
     // with the least significant 8 bits being the interrupt pin.
-    HashMap<u32, u64> masked_interrupt_mapping;
-    u32 interrupt_mask { 0 };
+    HashMap<PCIInterruptSpecifier, u64> masked_interrupt_mapping;
+    PCIInterruptSpecifier interrupt_mask;
 };
 
 class HostController {

Kernel/Bus/PCI/Definitions.h

@@ -10,6 +10,7 @@
 #include <AK/DistinctNumeric.h>
 #include <AK/Function.h>
 #include <AK/RefCounted.h>
+#include <AK/Traits.h>
 #include <AK/Types.h>
 #include <AK/Vector.h>
 #include <Kernel/Debug.h>
@@ -99,21 +100,30 @@ static constexpr u8 msix_table_bir_mask = 0x7;
 static constexpr u16 msix_table_offset_mask = 0xfff8;
 static constexpr u16 msix_control_enable = 0x8000;
 
-namespace OpenFirmwareAddress {
-
-static constexpr u8 space_type_offset = 24;
-static constexpr u8 space_type_mask = 0x3;
-static constexpr u8 prefetchable_offset = 30;
-static constexpr u8 prefetchable_mask = 0x1;
-
-enum SpaceType {
-    ConfigurationSpace = 0,
-    IOSpace = 1,
-    Memory32BitSpace = 2,
-    Memory64BitSpace = 3,
+union OpenFirmwareAddress {
+    enum class SpaceType : u32 {
+        ConfigurationSpace = 0,
+        IOSpace = 1,
+        Memory32BitSpace = 2,
+        Memory64BitSpace = 3,
+    };
+    struct {
+        // https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
+        // Chapter: 2.2.1.1
+        // phys.hi cell
+        u32 register_ : 8;        // r
+        u32 function : 3;         // f
+        u32 device : 5;           // d
+        u32 bus : 8;              // b
+        SpaceType space_type : 2; // s
+        u32 : 3;                  // 0
+        u32 aliased : 1;          // t
+        u32 prefetchable : 1;     // p
+        u32 relocatable : 1;      // n
+    };
+    u32 raw;
 };
-
-}
+static_assert(AssertSize<OpenFirmwareAddress, 4>());
 
 // Taken from https://pcisig.com/sites/default/files/files/PCI_Code-ID_r_1_11__v24_Jan_2019.pdf
 enum class ClassID {
@@ -524,7 +534,6 @@ private:
 
 class Domain;
 class Device;
-
 }
 
 template<>

Userland/Libraries/LibDeviceTree/DeviceTree.h

@@ -23,6 +23,11 @@ struct DeviceTreeProperty {
     public:
         using AK::FixedMemoryStream::FixedMemoryStream;
 
+        ErrorOr<u32> read_cell()
+        {
+            return read_value<BigEndian<u32>>();
+        }
+
         ErrorOr<FlatPtr> read_cells(u32 cell_size)
         {
             // FIXME: There are rare cases of 3 cell size big values, even in addresses, especially in addresses
@@ -91,6 +96,9 @@ struct DeviceTreeProperty {
 };
 
 class DeviceTreeNodeView {
+    AK_MAKE_NONCOPYABLE(DeviceTreeNodeView);
+    AK_MAKE_DEFAULT_MOVABLE(DeviceTreeNodeView);
+
 public:
     bool has_property(StringView prop) const { return m_properties.contains(prop); }
     bool has_child(StringView child) const { return m_children.contains(child); }