Feat driver testing enhancement

[torgeiru]

Major stuff: Makes it so that you can write a PCI driver outside source (using PCI code in your unikernel).

Minor stuff: Moving definitions from source to header file for PCI device.

Can rebase on request.

[mazunki]

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.

[torgeiru]

For next PR. Keeping this PR short.

[torgeiru]

I don't want conflicts as I integrate Virtio.

[mazunki]

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?

[torgeiru]

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.

[alfreb]

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.

[alfreb]

Please make a task to address conversion to enum class.

[mazunki]

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.

[MagnusS]

Please make a task to address conversion to enum class.

I added a tracking issue in #2333

[mazunki]

Is this different to PCI::vendor_product_t?

[torgeiru]

Answer later at Deichman RN.

[torgeiru]

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.

[mazunki]

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.

[alfreb]

Lgtm - do all the tests pass?

[alfreb]

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.

[alfreb]

Please make a task to address conversion to enum class.

[torgeiru]

I ran the test before the bump and it worked. Now we have bumped llvm and have to recompile. Can we have some kind of window for these sort of build things so that we don't have to recompile too often? We can have a cache, but I like having the ability to compile it myself for security and not fry my system more than needed. Alternatively, I will use testctrl for all my development. We can bump llvm, nixpkgs and musl at the same time. I can update musl now since it has a 0-day and I have to recompile anyway. Also, why not just bump to llvmpackages20???

[mazunki]

The update cycle for both llvm and nixpkgs is roughly every 6 months. The update cycle for the c++ standard itself is roughly every 3-5 years. All three of these will require recompiling quite a bit of the toolchain, but to different degrees. It's hard to say exactly which one would be worse.

Since we're compiling static builds, updating nixpkgs shouldn't require causing a complete rebuild, but I suspect nix will do so anyway, even for packages that are on the exact same version. Enabling ccache here will most likely help. I haven't actually looked into this.

Updating llvm itself, though, often comes with performance gains, deprecation warnings and bugfixes so there is a good reason to rebuild as soon as llvm gets updated. Bumping musl, well, good luck waiting for an update. Closer to 2 years with that one. We are already on the latest tag, so unless we pin to an untagged release...

---

I just ran your tests on llvmPackages_19, and it works fine. In case you're unaware, my cache server should be up to date with origin/main. In theory identical to the testctrl server, modulo reproducibility bugs. Hopefully we can get a cache server on NREC soon-ish.

I'm currently trying to get llvm20 to work (see #2307), and I'm also trying to get c++23 to work (can't wait for std::println and unreachable()! 🙏🏽 ). It might be easier to bump from llvm19 to llvm20 in one go, and c++20 to c++23 in another bulk. This gives people an opportunity to detach from deprecation warnings and the sort too (albeit llvm19 is fairly new, so it shouldn't be a major issue: although it has already helped me with a few warnings!).

[torgeiru]

Testctrl it is and cache as backing for development and saving the planet and more importantly my computer.

[mazunki]

All tests are passing on my end.

[mazunki]

Testctrl it is and cache as backing for development and saving the planet and more importantly my computer.

This is exactly why I'm offering my personal nix cache (#2304). Feel free to use it :)

[torgeiru]

I can bump musl to untagged or add the 0-day patch? Must ask for testctrl access (forgot old ssh key). Will follow up with patches for thread local storage and multicore.
We can turn off locks for musl and use own locks for now. We can also discuss the possibility of patching away all locks with custom spinlocks (most viable solution at the moment because we don't have futex and we don't have threads). Join tmp discord (see issue about communication) for this.

Can then investigate if we hack futex to bring everything C++std for println instead.

[torgeiru]

I was asked to give the intention for this PR: I want to create a Virtio console driver that is not in src/drivers for testing my Virtio control and data plane. It is not easy to do in units so it is an integration test. The console is a simple device that only uses queue buffer enqeue/dequeue logic for sending data and MSIX interrupts/callbacks. I implemented the FUSE protocol above this so I needed to make sure the layer below (Virtio) works. I follow up with more modern Virtio and test PRs.

After that VirtioFS (broken up in multiple PRs).

And finally VirtioPMEM (single PR, very simple).

Future work: Discussed with Magnus and agreed something needs to be done about the vfs situation. It is not flexible for a clean musl integration (seems to assume block devices and driver should implement file mmap). I suggest using Linux struct file_operations that filesystem drivers interface with. I think delegates for methods: struct file_operations.

Join the tmp discord to discuss filesystem implenentation.

[torgeiru]

All tests passing according to Mazunki.

[torgeiru]

All tests are passing on my end.

Here. Please so that I can send a modern Virtio PR. Trying to keep the momentum going xD. Also, join OSLAB-IFI discord to discuss VFS and other PR stuff.