Skip to content

hw: use enum class for PCI config registers #2369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
uadhran wants to merge 4 commits into
base: main
Choose a base branch
from
Open

hw: use enum class for PCI config registers #2369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08d7250
hw: use enum class for PCI config registers
uadhran Jul 7, 2026
e47ca60
hw: typed PCI config accessors and command bitmask ops
uadhran Jul 8, 2026
9d21426
hw: make config_reg the primary PCI config accessor path
uadhran Jul 9, 2026
a502cf9
hw: drop unused ext_cap_id and trim PCI enum docs
uadhran Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 40 additions & 27 deletions api/hw/pci_device.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,34 +22,47 @@
#include <vector>
#include <unordered_map>

/* 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};
Expand Down Expand Up @@ -308,7 +321,7 @@ struct msix_t;
int m_iobase = -1;
std::array<PCI::Resource, 6> m_resources;

std::array<pcicap_t, PCI_CAP_ID_MAX+1> caps;
std::array<pcicap_t, static_cast<size_t>(PCI::cap_id::MAX)+1> caps;

// has msix support if not null
msix_t* msix = nullptr;
Expand Down
20 changes: 11 additions & 9 deletions src/hw/pci_device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint8_t>(PCI::config_reg::CMD));
cmd |= static_cast<uint32_t>(PCI::command::MASTER)

@mazunki mazunki Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See api/mem/alloc/buddy.hpp for an example of api/util/bitops.hpp::enable_bitmask_ops. Casting this to uint32_t isn't necessary.

That said, can all combinations of enum fields be combined? I'd like for proper bitmask support to check for valid combinations.

@uadhran uadhran Jul 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also addressed in e47ca6071 — added enable_bitmask_ops<PCI::command> (same pattern as buddy.hpp) and the constructor now does:

cmd |= static_cast<uint32_t>(PCI::command::MASTER
                           | PCI::command::MEM
                           | PCI::command::IO);

Re combination validity: per PCI spec / Linux pci_regs.h, PCI_COMMAND_IO, PCI_COMMAND_MEMORY, and PCI_COMMAND_MASTER are independent enable bits (0x01, 0x02, 0x04), so OR'ing them is standard. I also added command::INTX_DISABLE (0x400) for the bit used in init_msix / intx_enable instead of a raw (1 << 10).

| static_cast<uint32_t>(PCI::command::MEM)
| static_cast<uint32_t>(PCI::command::IO);
write_dword(static_cast<uint8_t>(PCI::config_reg::CMD), cmd);

// device class info is coming from pci manager to save a PCI read
this->devtype_.reg = devclass;
Expand Down Expand Up @@ -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<uint8_t>(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<uint8_t>(PCI::config_reg::CAPABILITY);
// read first capability
offset = read16(offset) & 0xff;
offset &= ~0x3; // lower 2 bits reserved
Expand All @@ -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<uint8_t>(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<uint8_t>(PCI::config_reg::CMD));
write16(static_cast<uint8_t>(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<uint8_t>(PCI::config_reg::STATUS));
return stat & (1 << 3);
}

Expand Down
14 changes: 4 additions & 10 deletions src/hw/pci_msi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,6 @@
#include <hw/pci.hpp>
#include <hw/msi.hpp>

#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 */
Expand All @@ -30,20 +24,20 @@ namespace hw
{
int PCI_Device::msi_cap()
{
return caps[PCI_CAP_ID_MSI];
return caps[static_cast<size_t>(PCI::cap_id::MSI)];
}

int PCI_Device::msix_cap()
{
return caps[PCI_CAP_ID_MSIX];
return caps[static_cast<size_t>(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<uint8_t>(PCI::config_reg::CMD));
write16(static_cast<uint8_t>(PCI::config_reg::CMD), cmd | (1 << 10));
// enable MSI-X
this->msix = new msix_t(*this, msix_cap());
// deallocate if it failed
Expand Down