From 08d7250882dcd73b95dc86892db04c6393fb43db Mon Sep 17 00:00:00 2001 From: Utkarsh Adhran Date: Tue, 7 Jul 2026 10:05:33 +0530 Subject: [PATCH 1/2] hw: use enum class for PCI config registers Replace PCI config register #defines with typed enum classes (config_reg, command, cap_id). Updates in pci_device.cpp and pci_msi.cpp. Related: #2333 --- api/hw/pci_device.hpp | 67 ++++++++++++++++++++++++++----------------- src/hw/pci_device.cpp | 20 +++++++------ src/hw/pci_msi.cpp | 14 +++------ 3 files changed, 55 insertions(+), 46 deletions(-) diff --git a/api/hw/pci_device.hpp b/api/hw/pci_device.hpp index 40921708c1..c0cdaf2682 100644 --- a/api/hw/pci_device.hpp +++ b/api/hw/pci_device.hpp @@ -22,34 +22,47 @@ #include #include -/* PCI Register Config Space */ -#define PCI_DEV_VEND_REG 0x00 /* for the 32 bit read of dev/vend */ -#define PCI_VENDID_REG 0x00 -#define PCI_DEVID_REG 0x02 -#define PCI_CMD_REG 0x04 -#define PCI_STATUS_REG 0x06 -#define PCI_REVID_REG 0x08 -#define PCI_PROGIF_REG 0x09 -#define PCI_SUBCLASS_REG 0x0a -#define PCI_CLASS_REG 0x0b -#define PCI_CLSZ_REG 0x0c -#define PCI_LATTIM_REG 0x0d -#define PCI_HEADER_REG 0x0e -#define PCI_BIST_REG 0x0f -#define PCI_CAPABILITY_REG 0x34 - -#define PCI_COMMAND_IO 0x01 -#define PCI_COMMAND_MEM 0x02 -#define PCI_COMMAND_MASTER 0x04 - -#define PCI_CAP_ID_VNDR 0x09 -#define PCI_CAP_ID_AF 0x13 /* PCI Advanced Features */ -#define PCI_CAP_ID_MAX PCI_CAP_ID_AF -#define PCI_EXT_CAP_ID_PASID 0x1B /* Process Address Space ID */ -#define PCI_EXT_CAP_ID_MAX PCI_EXT_CAP_ID_PASID - namespace PCI { + /** PCI configuration space register offsets */ + enum class config_reg : uint8_t { + DEV_VEND = 0x00, /**< 32-bit device/vendor ID */ + DEVID = 0x02, + CMD = 0x04, + STATUS = 0x06, + REVID = 0x08, + PROGIF = 0x09, + SUBCLASS = 0x0a, + CLASS = 0x0b, + CLSZ = 0x0c, + LATTIM = 0x0d, + HEADER = 0x0e, + BIST = 0x0f, + CAPABILITY = 0x34 + }; + + /** PCI command register flags */ + enum class command : uint16_t { + IO = 0x01, + MEM = 0x02, + MASTER = 0x04 + }; + + /** Standard PCI capability IDs */ + enum class cap_id : uint8_t { + MSI = 0x05, + VNDR = 0x09, + MSIX = 0x11, + AF = 0x13, /**< PCI Advanced Features */ + MAX = AF + }; + + /** Extended PCI capability IDs */ + enum class ext_cap_id : uint8_t { + PASID = 0x1B, /**< Process Address Space ID */ + MAX = PASID + }; + static const uint16_t CONFIG_ADDR {0xCF8U}; static const uint16_t CONFIG_DATA {0xCFCU}; static const uint8_t CONFIG_INTR {0x3CU}; @@ -308,7 +321,7 @@ struct msix_t; int m_iobase = -1; std::array m_resources; - std::array caps; + std::array(PCI::cap_id::MAX)+1> caps; // has msix support if not null msix_t* msix = nullptr; diff --git a/src/hw/pci_device.cpp b/src/hw/pci_device.cpp index 508ffea81d..1dab1bb736 100644 --- a/src/hw/pci_device.cpp +++ b/src/hw/pci_device.cpp @@ -90,9 +90,11 @@ namespace hw { : pci_addr_{pci_addr}, device_id_{device_id} { // set master, mem and io flags - uint32_t cmd = read32(PCI_CMD_REG); - cmd |= PCI_COMMAND_MASTER | PCI_COMMAND_MEM | PCI_COMMAND_IO; - write_dword(PCI_CMD_REG, cmd); + uint32_t cmd = read32(static_cast(PCI::config_reg::CMD)); + cmd |= static_cast(PCI::command::MASTER) + | static_cast(PCI::command::MEM) + | static_cast(PCI::command::IO); + write_dword(static_cast(PCI::config_reg::CMD), cmd); // device class info is coming from pci manager to save a PCI read this->devtype_.reg = devclass; @@ -167,11 +169,11 @@ namespace hw { caps = {}; // the capability list is only available if bit 4 // in the status register is set - uint16_t status = read16(PCI_STATUS_REG); + uint16_t status = read16(static_cast(PCI::config_reg::STATUS)); //printf("read16 %#x status %#x\n", PCI_STATUS_REG, status); if ((status & 0x10) == 0) return; // this offset works for non-cardbus bridges - uint32_t offset = PCI_CAPABILITY_REG; + uint32_t offset = static_cast(PCI::config_reg::CAPABILITY); // read first capability offset = read16(offset) & 0xff; offset &= ~0x3; // lower 2 bits reserved @@ -189,19 +191,19 @@ namespace hw { void PCI_Device::deactivate() { // disables device (except for configuration) - write_dword(PCI_CMD_REG, 0); + write_dword(static_cast(PCI::config_reg::CMD), 0); } void PCI_Device::intx_enable() { - auto cmd = read16(PCI_CMD_REG); - write16(PCI_CMD_REG, cmd & ~(1 << 10)); + auto cmd = read16(static_cast(PCI::config_reg::CMD)); + write16(static_cast(PCI::config_reg::CMD), cmd & ~(1 << 10)); // delete msi-x if (this->msix) delete this->msix; } bool PCI_Device::intx_status() { - auto stat = read16(PCI_STATUS_REG); + auto stat = read16(static_cast(PCI::config_reg::STATUS)); return stat & (1 << 3); } diff --git a/src/hw/pci_msi.cpp b/src/hw/pci_msi.cpp index 358aa3e253..de7c34b2ee 100644 --- a/src/hw/pci_msi.cpp +++ b/src/hw/pci_msi.cpp @@ -2,12 +2,6 @@ #include #include -#define PCI_CMD_REG 0x04 - -// MSI and MSI-X capability registers -#define PCI_CAP_ID_MSI 0x05 /* Message Signalled Interrupts */ -#define PCI_CAP_ID_MSIX 0x11 /* MSI-X */ - // Message Signalled Interrupts registers #define PCI_MSI_FLAGS_ENABLE 0x0001 /* MSI feature enabled */ #define PCI_MSI_FLAGS_QMASK 0x000e /* Maximum queue size available */ @@ -30,20 +24,20 @@ namespace hw { int PCI_Device::msi_cap() { - return caps[PCI_CAP_ID_MSI]; + return caps[static_cast(PCI::cap_id::MSI)]; } int PCI_Device::msix_cap() { - return caps[PCI_CAP_ID_MSIX]; + return caps[static_cast(PCI::cap_id::MSIX)]; } void PCI_Device::init_msix() { assert(this->msix == nullptr); // disable intx - auto cmd = read16(PCI_CMD_REG); - write16(PCI_CMD_REG, cmd | (1 << 10)); + auto cmd = read16(static_cast(PCI::config_reg::CMD)); + write16(static_cast(PCI::config_reg::CMD), cmd | (1 << 10)); // enable MSI-X this->msix = new msix_t(*this, msix_cap()); // deallocate if it failed From e47ca607188ffefe8c78814196ddba276b106352 Mon Sep 17 00:00:00 2001 From: Utkarsh Adhran Date: Wed, 8 Jul 2026 15:45:28 +0530 Subject: [PATCH 2/2] hw: typed PCI config accessors and command bitmask ops - Add read/write overloads taking PCI::config_reg; uint8_t overloads delegate - Enable enable_bitmask_ops for IO | MEM | MASTER - Document enum names against Linux pci_regs.h / PCI Local Bus Spec - Use command::INTX_DISABLE instead of raw bit 10 in intx/msix paths --- api/hw/pci_device.hpp | 70 +++++++++++++++++++++++++++++-------------- src/hw/pci_device.cpp | 45 +++++++++++++++++++++------- src/hw/pci_msi.cpp | 5 ++-- 3 files changed, 84 insertions(+), 36 deletions(-) diff --git a/api/hw/pci_device.hpp b/api/hw/pci_device.hpp index c0cdaf2682..65819f9dd7 100644 --- a/api/hw/pci_device.hpp +++ b/api/hw/pci_device.hpp @@ -21,39 +21,47 @@ #include #include #include +#include namespace PCI { - /** PCI configuration space register offsets */ + /** + * PCI configuration space register offsets. + * + * Names follow IncludeOS PCI_* macros; see Linux include/linux/pci_regs.h + * (PCI_COMMAND, PCI_CACHE_LINE_SIZE, PCI_CAPABILITY_LIST, etc.) and the + * PCI Local Bus Specification: https://www.pcisig.com/specifications + */ enum class config_reg : uint8_t { - DEV_VEND = 0x00, /**< 32-bit device/vendor ID */ - DEVID = 0x02, - CMD = 0x04, - STATUS = 0x06, - REVID = 0x08, - PROGIF = 0x09, - SUBCLASS = 0x0a, - CLASS = 0x0b, - CLSZ = 0x0c, - LATTIM = 0x0d, - HEADER = 0x0e, - BIST = 0x0f, - CAPABILITY = 0x34 + DEV_VEND = 0x00, /**< PCI_VENDOR_ID — 32-bit device/vendor ID */ + DEVID = 0x02, /**< PCI_DEVICE_ID */ + CMD = 0x04, /**< PCI_COMMAND */ + STATUS = 0x06, /**< PCI_STATUS */ + REVID = 0x08, /**< PCI_REVISION_ID */ + PROGIF = 0x09, /**< PCI_CLASS_PROG */ + SUBCLASS = 0x0a, /**< PCI_CLASS_DEVICE */ + CLASS = 0x0b, /**< Class code byte */ + CLSZ = 0x0c, /**< PCI_CACHE_LINE_SIZE */ + LATTIM = 0x0d, /**< PCI_LATENCY_TIMER */ + HEADER = 0x0e, /**< PCI_HEADER_TYPE */ + BIST = 0x0f, /**< PCI_BIST */ + CAPABILITY = 0x34 /**< PCI_CAPABILITY_LIST */ }; - /** PCI command register flags */ + /** PCI command register flags (PCI_COMMAND_*) */ enum class command : uint16_t { - IO = 0x01, - MEM = 0x02, - MASTER = 0x04 + IO = 0x01, /**< PCI_COMMAND_IO */ + MEM = 0x02, /**< PCI_COMMAND_MEMORY */ + MASTER = 0x04, /**< PCI_COMMAND_MASTER */ + INTX_DISABLE = 0x400, /**< PCI_COMMAND_INTX_DISABLE */ }; - /** Standard PCI capability IDs */ + /** Standard PCI capability IDs (PCI_CAP_ID_*) */ enum class cap_id : uint8_t { - MSI = 0x05, - VNDR = 0x09, - MSIX = 0x11, - AF = 0x13, /**< PCI Advanced Features */ + MSI = 0x05, /**< PCI_CAP_ID_MSI — Message Signalled Interrupts */ + VNDR = 0x09, /**< PCI_CAP_ID_VNDR — Vendor-specific */ + MSIX = 0x11, /**< PCI_CAP_ID_MSIX — MSI-X */ + AF = 0x13, /**< PCI_CAP_ID_AF — PCI Advanced Features */ MAX = AF }; @@ -183,15 +191,20 @@ struct msix_t; explicit PCI_Device(const uint16_t pci_addr, const uint32_t, const uint32_t); //! @brief Read from device with implicit pci_address (e.g. used by Nic) + uint32_t read32(PCI::config_reg reg) noexcept; uint32_t read32(const uint8_t reg) noexcept; //! @brief Read from device with explicit pci_addr + static uint32_t read_dword(const uint16_t pci_addr, PCI::config_reg reg) noexcept; static uint32_t read_dword(const uint16_t pci_addr, const uint8_t reg) noexcept; //! @brief Write to device with implicit pci_address (e.g. used by Nic) + void write_dword(PCI::config_reg reg, const uint32_t value) noexcept; void write_dword(const uint8_t reg, const uint32_t value) noexcept; + uint16_t read16(PCI::config_reg reg) noexcept; uint16_t read16(const uint8_t reg) noexcept; + void write16(PCI::config_reg reg, const uint16_t value) noexcept; void write16(const uint8_t reg, const uint16_t value) noexcept; /** A descriptive name */ @@ -374,4 +387,15 @@ static const char* PCI::vendor_str(uint16_t code){ return it == classcodes.end() ? "Unknown vendor" : it->second; } +/** Enable bitmask operators for PCI command register flags */ +namespace util { +inline namespace bitops { +template<> +struct enable_bitmask_ops { + using type = std::underlying_type::type; + static constexpr bool enable = true; +}; +} +} + #endif //< HW_PCI_DEVICE_HPP diff --git a/src/hw/pci_device.cpp b/src/hw/pci_device.cpp index 1dab1bb736..09cf922ced 100644 --- a/src/hw/pci_device.cpp +++ b/src/hw/pci_device.cpp @@ -20,9 +20,12 @@ #include #include #include +#include namespace hw { + using namespace util::bitops; + static constexpr std::array bridge_subclasses { "Host", "ISA", @@ -90,16 +93,36 @@ namespace hw { : pci_addr_{pci_addr}, device_id_{device_id} { // set master, mem and io flags - uint32_t cmd = read32(static_cast(PCI::config_reg::CMD)); - cmd |= static_cast(PCI::command::MASTER) - | static_cast(PCI::command::MEM) - | static_cast(PCI::command::IO); - write_dword(static_cast(PCI::config_reg::CMD), cmd); + uint32_t cmd = read32(PCI::config_reg::CMD); + cmd |= static_cast(PCI::command::MASTER + | PCI::command::MEM + | PCI::command::IO); + write_dword(PCI::config_reg::CMD, cmd); // device class info is coming from pci manager to save a PCI read this->devtype_.reg = devclass; } + uint32_t PCI_Device::read32(PCI::config_reg reg) noexcept { + return read32(static_cast(reg)); + } + + uint32_t PCI_Device::read_dword(const uint16_t pci_addr, PCI::config_reg reg) noexcept { + return read_dword(pci_addr, static_cast(reg)); + } + + void PCI_Device::write_dword(PCI::config_reg reg, const uint32_t value) noexcept { + write_dword(static_cast(reg), value); + } + + uint16_t PCI_Device::read16(PCI::config_reg reg) noexcept { + return read16(static_cast(reg)); + } + + void PCI_Device::write16(PCI::config_reg reg, const uint16_t value) noexcept { + write16(static_cast(reg), value); + } + uint32_t PCI_Device::read_dword(const uint16_t pci_addr, const uint8_t reg) noexcept { PCI::msg req; @@ -169,8 +192,7 @@ namespace hw { caps = {}; // the capability list is only available if bit 4 // in the status register is set - uint16_t status = read16(static_cast(PCI::config_reg::STATUS)); - //printf("read16 %#x status %#x\n", PCI_STATUS_REG, status); + uint16_t status = read16(PCI::config_reg::STATUS); if ((status & 0x10) == 0) return; // this offset works for non-cardbus bridges uint32_t offset = static_cast(PCI::config_reg::CAPABILITY); @@ -191,19 +213,20 @@ namespace hw { void PCI_Device::deactivate() { // disables device (except for configuration) - write_dword(static_cast(PCI::config_reg::CMD), 0); + write_dword(PCI::config_reg::CMD, 0); } void PCI_Device::intx_enable() { - auto cmd = read16(static_cast(PCI::config_reg::CMD)); - write16(static_cast(PCI::config_reg::CMD), cmd & ~(1 << 10)); + auto cmd = read16(PCI::config_reg::CMD); + write16(PCI::config_reg::CMD, + cmd & ~static_cast(PCI::command::INTX_DISABLE)); // delete msi-x if (this->msix) delete this->msix; } bool PCI_Device::intx_status() { - auto stat = read16(static_cast(PCI::config_reg::STATUS)); + auto stat = read16(PCI::config_reg::STATUS); return stat & (1 << 3); } diff --git a/src/hw/pci_msi.cpp b/src/hw/pci_msi.cpp index de7c34b2ee..45be321589 100644 --- a/src/hw/pci_msi.cpp +++ b/src/hw/pci_msi.cpp @@ -36,8 +36,9 @@ namespace hw { assert(this->msix == nullptr); // disable intx - auto cmd = read16(static_cast(PCI::config_reg::CMD)); - write16(static_cast(PCI::config_reg::CMD), cmd | (1 << 10)); + auto cmd = read16(PCI::config_reg::CMD); + write16(PCI::config_reg::CMD, + cmd | static_cast(PCI::command::INTX_DISABLE)); // enable MSI-X this->msix = new msix_t(*this, msix_cap()); // deallocate if it failed