Skip to content

Feat driver testing enhancement #2328

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

Closed
wants to merge 2 commits into from
Closed

Feat driver testing enhancement #2328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6267fb
Driver testing enhancement and minor refactoring
torgeiru Sep 26, 2025
fa1eed3
Merge branch 'includeos:main' into feat_driver_testing_enhancement
torgeiru Oct 21, 2025
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
29 changes: 25 additions & 4 deletions api/hw/pci_device.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,31 @@
#include <vector>
#include <unordered_map>

#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
/* PCI Register Config Space */
Copy link
Contributor

@mazunki mazunki Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Could we change these #define values to proper types? I assume these are addresses, in which case I would place them as static const uint8_t values under PCI::Registry::. Alternatively under an enumeration if it makes sense to switch-case over them (could also make for cleaner code, but that's subjective).

I see PCI::msg has a uint8_t reg field. I think it would be nice to make this "pointer" a proper type, for instance PCI::registry_p maybe (opinionated names), which you could use for all these _REG values, instead of uint8_t mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For next PR. Keeping this PR short.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want conflicts as I integrate Virtio.

Copy link
Contributor

@mazunki mazunki Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that's fair. I generally dislike #define values, which is why I bring it up. They are inherently not typesafe: changing the order of parameters in functions makes for very easy bugs that are annoying to debug (this guy brings up plenty of examples in this regard: https://youtu.be/gV7jhTMYkKc).

We currently have 403 files with a total of 1483 #defines across the code: migrating all of them in a single PR would be a huge task. Instead, converting them over while already touching related code makes this burden better over time. That's just my opinion though, I think this can be merged as-is too.

That said: does it makes sense to make a specific type for registry addresses?

Copy link
Contributor Author

@torgeiru torgeiru Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I dunno. I think it may have made sense to have a specific registry type when it was made, but converting entire codebase to proper types is risky in terms of breaking something (mistakes and low level sensitivity). I would be careful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you both. We probably do want these defines to be strongly typed. Because:

Macros do not obey scope and type rules. Also, macro names are removed during preprocessing and so usually don’t appear in tools like debuggers.

From here:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-macro

But also - we can do a separate pass at this. One problem will be that enum class doesn't support bitwise ops by default. We did add useful helpers for that here:
https://github.com/includeos/IncludeOS/blob/main/api/util/bitops.hpp

So it's not hard to add, but it it should be done consistently. Wrt risky; we might also find some bugs when we do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please make a task to address conversion to enum class.

Copy link
Contributor

Choose a reason for hiding this comment

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

One problem will be that enum class doesn't support bitwise ops by default.

That's a good thing!

We did add useful helpers for that here:
https://github.com/includeos/IncludeOS/blob/main/api/util/bitops.hpp

Can confirm, they work great.

Copy link
Member

Choose a reason for hiding this comment

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

Please make a task to address conversion to enum class.

I added a tracking issue in #2333

#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 {

Expand Down
13 changes: 12 additions & 1 deletion api/hw/pci_manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,31 @@

namespace hw {

struct pcidev_info {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this different to PCI::vendor_product_t?

Copy link
Contributor Author

@torgeiru torgeiru Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Answer later at Deichman RN.

Copy link
Contributor Author

@torgeiru torgeiru Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is a superset of it.

struct pcidev_info {
  const uintptr_t pci_addr;
  uint32_t vendor;
  hw::PCI_Device::class_revision_t dev_class;
};

The uint32_t is equivalent to it:

    union vendor_product_t {
      uint32_t both;
      struct __attribute__((packed)) {
        uint16_t vendor;
        uint16_t product;
      };
    };

It is an union right. It can further be broken down to vendor and product thus its name vendor_product.

Vendor can be e.g. redhat or Virtio (1af4) and product can be VirtioFS 105a (if I didn't forget).

struct pcidev_info {
  const uintptr_t pci_addr;
  vendor_product_t product;
  hw::PCI_Device::class_revision_t dev_class;
};

Something like the above would be better since we have a type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, of course. Maybe it's overkill, but would we benefit from providing an interface to the pci.ids file (https://admin.pci-ids.ucw.cz/read/PC/). I believe this is what tools like hwdb and similar uses.

const uintptr_t pci_addr;
uint32_t vendor;
hw::PCI_Device::class_revision_t dev_class;
};

class PCI_manager {
public:
// a <...> driver is constructed from a PCI device,
// and returns a unique_ptr to itself
using NIC_driver = delegate< std::unique_ptr<hw::Nic> (PCI_Device&, uint16_t) >;
using Device_vector = std::vector<const hw::PCI_Device*>;
using Devinfo_vector = std::vector<pcidev_info>;
static void register_nic(uint16_t, uint16_t, NIC_driver);

using BLK_driver = delegate< std::unique_ptr<hw::Block_device> (PCI_Device&) >;
static void register_blk(uint16_t, uint16_t, BLK_driver);

static void init();
static void init_devices(uint8_t classcode);
static Device_vector devices();
/* Returns devices that were attempted to be initialized */
static Device_vector devices();
/* Returns all PCI device information except PCI bridges.
Useful for testing driver code outside of src. */
static const Devinfo_vector &devinfos();

private:
static void scan_bus(int bus);
Expand Down
21 changes: 1 addition & 20 deletions src/hw/pci_device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,6 @@
#include <hw/pci_device.hpp>
#include <hw/msi.hpp>

/* 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_COMMAND_IO 0x01
#define PCI_COMMAND_MEM 0x02
#define PCI_COMMAND_MASTER 0x04

namespace hw {

static constexpr std::array<const char*,3> bridge_subclasses {
Expand Down Expand Up @@ -190,7 +171,7 @@ namespace hw {
//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 = 0x34;
uint32_t offset = PCI_CAPABILITY_REG;
// read first capability
offset = read16(offset) & 0xff;
offset &= ~0x3; // lower 2 bits reserved
Expand Down
10 changes: 4 additions & 6 deletions src/hw/pci_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,7 @@ using Driver_entry = std::pair<uint32_t, Driver>;
template <typename Driver>
using fixed_factory_t = std::vector<Driver_entry<Driver>>;

struct pcidev_info {
const uintptr_t pci_addr;
uint32_t vendor;
hw::PCI_Device::class_revision_t dev_class;
};
static std::vector<pcidev_info> devinfos_;

static std::vector<hw::PCI_Device> devices_;
static std::vector<Driver_entry<PCI_manager::NIC_driver>> nic_fact;
static std::vector<Driver_entry<PCI_manager::BLK_driver>> blk_fact;
Expand Down Expand Up @@ -75,6 +69,10 @@ PCI_manager::Device_vector PCI_manager::devices () {
return device_vec;
}

const PCI_manager::Devinfo_vector& PCI_manager::devinfos() {
return devinfos_;
}

void PCI_manager::scan_bus(const int bus)
{
INFO2("|");
Expand Down