hw: use enum class for PCI config registers

[uadhran]

Replace PCI config register #defines with typed enum classes (config_reg, command, cap_id) for type safety.

Changes:

• api/hw/pci_device.hpp — enums with Doxygen mapping to Linux pci_regs.h / PCI Local Bus Spec; enable_bitmask_ops<PCI::command>
• Typed read16/write16/read32/write_dword overloads taking PCI::config_reg (cast to integer only inside HW accessors)
• uint8_t overloads retained for dynamic offsets (BARs, capability walk, drivers using CONFIG_INTR)
• src/hw/pci_device.cpp, src/hw/pci_msi.cpp — use typed APIs at standard config offsets; command::INTX_DISABLE for INTx disable bit

Tested: nix-build unittests.nix — 85/85 passed (Nix 2.34.7, Linux).

Split from #2367 per review feedback.

Closes: #2333

[mazunki]

All the integration checks seem to be working too :)

I wonder if it's reasonable to keep using shrtnd forms for the enum fields, or if we should use full words.

Are these acronyms standard in other places, or is it just to save a few keystrokes here? Personally I like when names are expressive on their own right, even if someone is unfamiliar with the context.

For instance, I have no idea what cap_id::MSIX means. At the very least, I would have hoped for its meaning to be expressed in the enum class definition, if not clear. Otherwise, if these are their standard names, I would include a reference url for where to find the information.

[mazunki]

You can add Closes: #2333 to this PR, and it will automatically close the related issue upon merge :)

[mazunki]

Casting things back to raw numbers for all operations defeats the purpose of using enum classes for type safety, and just adds noise with every cast.

Perhaps it'd be necessary to remove a generic write16 and read16, and make these methods for the enum-class instead for proper type safety?

If these functions are used by other parts of IncludeOS, I would approach it by first making one commit to refactor it internally, and exposing a temporary stub function, and a secondary commit to clean up legacy interface users (easy to find by just deleting the stub).

If these interfaces are required by an external protocol, I don't think there's too much we can do except hope that people prefer using the type-safe versions.

See also inline comments.

[mazunki]

Can we change the signature here? Is there a reason to keep this uint8_t?

  void PCI_Device::write16(const PCI_Device::cap_id reg, const uint16_t value) noexcept {
    PCI::msg req;
    req.data = 0x80000000;
    req.addr = pci_addr_;
    req.reg  = reg;

    outpd(PCI::CONFIG_ADDR, 0x80000000 | req.data);
    outpw(PCI::CONFIG_DATA + (reg & 2), value);
  }

  // idem for PCI_Device::config_reg etc

  // fallback if this needs to be exposed, otherwise for type safety better not
  void PCI_Device::write16(const uint8_t reg, const uint16_t value) noexcept {
    write16(static_cast<PCI_Device::cap_id>(reg), value);
  }

[uadhran]

Addressed in e47ca6071:

uint16_t PCI_Device::read16(PCI::config_reg reg) noexcept {
  return read16(static_cast<uint8_t>(reg));
}

void PCI_Device::write16(PCI::config_reg reg, const uint16_t value) noexcept {
  write16(static_cast<uint8_t>(reg), value);
}

Same pattern for read32, write_dword, and static read_dword. The uint8_t overloads remain for dynamic offsets (capability chain, BAR indices, etc.) and delegate inward where appropriate.

I used PCI::config_reg (not nested under PCI_Device) since the enums live in the PCI namespace — same as your sketch but with the actual type names from the header.

[mazunki]

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]

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).

[uadhran]

Good question. The short names are not arbitrary — they match what IncludeOS already used in pci_device.hpp macros (PCI_CMD_REG, PCI_CAP_ID_MSIX, etc.) and the values/names in Linux include/linux/pci_regs.h (which tracks the PCI Local Bus Specification).

For example:

• cap_id::MSIX = PCI_CAP_ID_MSIX (0x11) — MSI-X capability; the spec writes "MSI-X" in prose but C identifiers use MSIX
• config_reg::CMD = PCI_COMMAND (0x04)
• config_reg::CLSZ = PCI_CACHE_LINE_SIZE (0x0c)

I've added Doxygen on each enum member mapping to the Linux/spec name, plus a block comment linking https://www.pcisig.com/specifications and pci_regs.h. I kept the short identifiers rather than renaming to full words so we stay consistent with existing IncludeOS PCI code and kernel header conventions — happy to revisit if you'd prefer longer enum member names.

[uadhran]

Done — added Closes: #2333 to the PR description. Thanks!

[uadhran]

Agree on the casts — the latest commit (e47ca6071) adds typed read16/write16/read32/write_dword overloads taking PCI::config_reg, with static_cast<uint8_t> only inside the HW accessor implementations. Call sites in pci_device.cpp and pci_msi.cpp no longer cast enums at the use site.

I kept the uint8_t overloads as thin delegates for places that need raw/dynamic offsets (BAR probing, capability list walk, CONFIG_INTR in virtio/e1000). Those aren't standard fixed config headers, so a config_reg type wouldn't buy much there.

See inline replies for bitmask ops and naming/docs.