Kernel: New PCI driver subsystem design #23448

supercomputer7 · 2024-03-03T16:23:19Z

This is the PR I talked about in the discord server. It puts the PCI subsystem inline with the USB subsystem, and I intend to expand on this to make it possible to compile some drivers as kernel modules (and maybe even loadable ones).

Still, it's incomplete as I don't have userspace interfaces to ensure we can bring back lspci, so it's a draft, but we will need to discuss many aspects of the design probably, to get the best shape out of this.

cc @Hendiadyoin1 @spholz

Hendiadyoin1

Two general questions:

Can we get the __matches arrays in .rodata, as they should not be written to afaict
I think it's unfortunate to loose a lot of UNMAP_AFTER_INIT, can we instead just delay that until we've finished PCI enumeration?

Kernel/Bus/PCI/Drivers/RNG/VirtIO.cpp

Hendiadyoin1 · 2024-03-03T16:39:07Z

Kernel/Arch/init.cpp

@@ -34,6 +34,7 @@
 #include <Kernel/Devices/SerialDevice.h>
 #include <Kernel/Devices/Storage/StorageManagement.h>
 #include <Kernel/Devices/TTY/ConsoleManagement.h>
+#include <Kernel/Devices/TTY/PCI/Serial8250Device.h>


Adding this include here seems unrelated

Hendiadyoin1 · 2024-03-03T16:52:25Z

Kernel/Bus/PCI/Device.h

+    SpinlockProtected<RawPtr<PCI::Driver>, LockRank::None>& driver(Badge<Access>) { return m_driver; }
+
+    template<typename T>
+    inline ErrorOr<Memory::TypedMapping<T>> map_resource(HeaderType0BaseRegister bar, size_t size = sizeof(T), Memory::Region::Access access = IsConst<T> ? Memory::Region::Access::Read : Memory::Region::Access::ReadWrite)


Not (yet) sure if this changes in a later commit,
But as it turns out fully relying on the size of the Object seems to not be correct in all cases, as some passed in objects end in flexible arrays, which don't contribute to the size

Hendiadyoin1 · 2024-03-03T16:53:30Z

Kernel/Bus/PCI/Device.h

+    RawPtr<PCI::HostController> m_host_controller {};
+    RawPtr<Bus> m_parent_bus {};


Any reason why we need RawPtr here?

Hendiadyoin1 · 2024-03-03T17:00:11Z

Kernel/Devices/GPU/Bochs/GraphicsAdapter.cpp

+        auto registers_mapping = TRY(Memory::map_typed_writable<BochsDisplayMMIORegisters volatile>(PhysicalAddress(m_pci_device->resources()[2].physical_memory_address())));
        VERIFY(registers_mapping.region);
-        m_display_connector = QEMUDisplayConnector::must_create(PhysicalAddress(TRY(PCI::get_bar_address(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0))), bar0_space_size, move(registers_mapping));
+        m_display_connector = QEMUDisplayConnector::must_create(PhysicalAddress(m_pci_device->resources()[0].physical_memory_address()), bar0_space_size, move(registers_mapping));
    }
 #else
-    auto registers_mapping = TRY(PCI::map_bar<BochsDisplayMMIORegisters volatile>(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR2));
+    auto registers_mapping = TRY(Memory::map_typed_writable<BochsDisplayMMIORegisters volatile>(PhysicalAddress(m_pci_device->resources()[2].physical_memory_address())));
    VERIFY(registers_mapping.region);
-    m_display_connector = QEMUDisplayConnector::must_create(PhysicalAddress(TRY(PCI::get_bar_address(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0))), bar0_space_size, move(registers_mapping));
+    m_display_connector = QEMUDisplayConnector::must_create(PhysicalAddress(m_pci_device->resources()[0].physical_memory_address()), bar0_space_size, move(registers_mapping));


Any reason why m_pci_device->map_resource would not work here?

Hendiadyoin1 · 2024-03-03T17:03:29Z

Kernel/Boot/CommandLine.cpp

Best to update the kernel command line man page

Hendiadyoin1 · 2024-03-03T17:11:46Z

Kernel/Devices/Storage/ATA/GenericIDE/Channel.h

@@ -37,7 +37,7 @@ class AsyncBlockDeviceRequest;

 class IDEController;
 #if ARCH(X86_64)
-class PCIIDELegacyModeController;
+class PIIX4IDEController;


I have the feeling that this and the changes below belong to the rename commit

Kernel/Bus/PCI/Drivers/USB/EHCI.cpp

supercomputer7 · 2024-03-03T18:00:51Z

Two general questions:

* Can we get the `__matches` arrays in `.rodata`, as they should not be written to afaict

Sure. I will see how to do that cleanly.

* I think it's unfortunate to loose a lot of UNMAP_AFTER_INIT, can we instead just delay that until we've finished PCI enumeration?

It's part of how loading kernel modules work not only here, but in Linux as well. If we have UNMAP_AFTER_INIT, it means that you basically can never have PCI device hotplug capabilities. It also means that you can't insert a kernel module that needs these methods as it will fail, so loadable kernel modules are not an option as well. I rather stick to remove those UNMAP_AFTER_INIT tags now.

Also, trying to synchronize PCI enumeration with the main init process is just asking for problems - it eliminates the power of multi-threading to actually allow faster boot times - I intend that in the future we will spawn the init process even before of completing the PCI enumeration, so you could reach a functional system even sooner and all system services will spawn lazily and attach to new devices when they appear even after running the service :)

Kernel/Arch/x86_64/PCI/Initializer.cpp

Kernel/Bus/PCI/Controller/HostController.cpp

spholz · 2024-03-03T18:52:28Z

Kernel/Bus/PCI/Resource.h

+    PhysicalAddress physical_memory_address() const
+    {
+        VERIFY(type != SpaceType::IOSpace);
+        return PhysicalAddress(address & PCI::bar_address_mask);


If address still contains the bottom type bits, maybe name it something like bar_value instead?

spholz

I didn't mean to finish my review yet, idk how I accidentally finished it.

spholz · 2024-03-03T19:14:29Z

Kernel/Bus/USB/UHCI/UHCIController.cpp

-    dmesgln_pci(*this, "Controller found {} @ {}", PCI::get_hardware_id(device_identifier()), device_identifier().address());
-    dmesgln_pci(*this, "I/O base {}", m_registers_io_window);
-    dmesgln_pci(*this, "Interrupt line: {}", interrupt_number());
+    dmesgln("UHCI I/O base {}", m_registers_io_window);


Why no dmesgln_pci here?

spholz · 2024-03-03T19:19:38Z

Kernel/Arch/x86_64/PCI/Initializer.cpp

-
-    // IRQ from pin-based interrupt should be set as reserved as soon as possible so that the PCI device
-    // that chooses to use MSI(x) based interrupt can avoid sharing the IRQ with other devices.
-    MUST(PCI::enumerate([&](DeviceIdentifier const& device_identifier) {


Can the FIXME at the bottom of riscv64/PCI/Initializer.cpp be removed now?

Ah no, this seems like you removed that temporarily.

spholz · 2024-03-03T19:20:06Z

Kernel/Arch/x86_64/PCI/Initializer.cpp

-        }
-    }));
-
-    MUST(PCI::enumerate([&](DeviceIdentifier const& device_identifier) {


You probably should also remove this printing loop from riscv64/PCI/Initializer.cpp as well.

Well RISC-V probably doesn't work at all right now, so I assume you will fix that up later.

Kernel/Bus/PCI/Device.h

We never used these virtual methods outside their own implementation, so let's stop pretending that we should be able to utilize this for unknown purpose.

We can just do the initialization sequence in the constructor.

This option was merely used by me in bare metal testing, but is useless for anyone else.

Prepare to remove biglock on PCI::Access in a future commit, so we can ensure we only lock a spinlock on a precise PCI HostController if needed instead of the entire subsystem.

GenericGraphicsAdapter is mouthful. Also, the idea is to move towards a more advanced subsystem that handles GPUs, not merely graphics adapters.

This code is actually for the old PIIX4 PCI host bridge, which requires to use legacy x86 IO instructions.

This file will be removed in a future commit, so let's get rid of what we can right now. Also, the ISAIDEController code as well as other places should have never included this file in the first place.

This WorkQueue will be used to probe devices from the PCI bus in asynchronous fashion.

Instead of waiting 2 seconds, wait 100 milliseconds. Do this 10000 so we wait 10 seconds for the boot device to appear. This will be used later on, as the boot device will always appear asynchronously.

It will change dramatically in the following commits, so let's remove the existence of the PCI bus from SysFS. As lspci needs /sys/bus/pci as well, we will remove it for now, as it will change as well.

The USB::Pipe is abstracted from the actual USB host controller implementation, so don't include the UHCIController.h file. Also, we missed an include to UserOrKernelBuffer.h, so this is added to ensure the code can still compile.

We do this by doing the following things: - Remove all drivers except e1000, because we need the e1000 driver to be able to connect the machine to the network. - Remove dependency on PCI::Device for removed (for now) drivers. - Remove the call to initialize from the NetworkingManagement singleton code.

We do this by doing the following things: - Remove all drivers except Intel HDA, because we need the Intel HDA driver to be able to perform audio operations. - Remove dependency on PCI::Device for removed (for now) drivers. - Remove the call to initialize from the AudioManagement singleton code.

This is a preparation before applying the new PCI subsystem design, as we will add these drivers properly afterwards.

We don't need all drivers except the NVMe driver, so remove everything until we add the drivers back after applying the new PCI subsystem design.

supercomputer7 · 2024-04-29T17:00:26Z

A friendly reminder to me to look on this:
#24152 (comment)
So I will do the removing the VirtIO namespace from the RNG and Console devices in this PR.

supercomputer7 · 2024-05-14T19:19:28Z

This PR also relies on #24210, to ensure that we can handle hotplug events, because down the road initialization for most things will happen in async fashion. :)

stale · 2024-06-05T02:58:48Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

supercomputer7 · 2024-06-07T18:05:24Z

Also, we should probably adapt userspace daemons and servers to recognize hotplug events somehow. For example, a GPU driver could initialize a display connector and a framebuffer way after the init process is done, and waiting on appearing of such device during boot negates the possible speedup we could achieve by making the boot process to happen in parallel. The main programs that need adaptions are:

* `WindowServer` for hotplug of display connectors and their framebuffer

* `AudioServer` for hotplug of audio stream devices

* `NetworkServer` for hotplug of network adapters

As for how to do this, I have 2 ideas in mind:

* Add a new filesystem called `devuserfs`, which is mounted with special arguments, to enable automatic spawn of device nodes immediately on the root directory of the filesystem, with the desired user and group IDs.
  For example, a user could mount `devuserfs` on `/dev/audio` with explicit user and group IDs of the `audio` assigned numbers and ask to filter events for hotplug of audio devices only, and the audio server could watch the root inode (and only that inode) to get notifications on new devices.
  The main disadvantage of this idea is lack of customization of file names. It also means that the user could mount this filesystem anywhere and we should track all instances and notify them. It will complicate the kernel code, so that's another unwanted side effect. Those filesystems will have no usage outside this one, so in overall I am not super excited about this idea.

* Add a mechanism in the `DeviceMapper` to map new device nodes in the corresponding directories. The actual service still needs to watch these RAMFS inode instances (`/dev/audio` for example), so no change from the previous option on this aspect. By far this is the most simplistic approach, and has no obvious disadvantages that I see right now.
  It still not obvious to me how such service could understand the properties of the new device, but we could probably take the major and minor numbers and use a `sysfs` link to a `sysfs` directory (`/sys/dev/{block,char}/{major:minor}`, which should be a link to the actual device on `sysfs`).

As far as I consider this, all programs should follow what I did in #24210, except that NetworkServer doesn't use a device file, so another way to detect hotplug of network devices should be used. Not sure what it is though, maybe a special sysfs node?

stale · 2024-06-29T06:02:47Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale · 2024-07-06T16:33:12Z

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

supercomputer7 · 2024-07-13T09:52:20Z

I'm working on rebasing this branch now. This relies on #24434, and to some extent on #22968 and #24698.
So even if I push brand new commits and things change, this will likely stay a draft until some prior work is done.

supercomputer7 · 2024-07-13T13:21:32Z

The rebase work is done on this branch:
https://github.com/supercomputer7/serenity/tree/pci-refactor-as-a-draft

Once we get it all OK there I will move the patches to this PR. #24699 should be merged as well for this to be ready.

stale · 2024-08-05T11:08:14Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale · 2024-08-29T00:25:59Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale · 2024-09-28T05:11:46Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

supercomputer7 · 2024-10-02T22:27:13Z

This is stale and sadly I don't have the time right now to keep it open or rebasing it over and over again.
I might re-open it sometime in the future :)

Hendiadyoin1 reviewed Mar 3, 2024

View reviewed changes

spholz reviewed Mar 3, 2024

View reviewed changes

Kernel/Bus/PCI/Device.h Outdated Show resolved Hide resolved

supercomputer7 added 25 commits March 15, 2024 18:23

Kernel: Remove UNMAP_AFTER_INIT from VGA text mode console

e180cb9

Kernel/VirtIO: Remove stale detect method

fe6c355

Kernel: Move PCI PIIX4 IDE controller code to an appropriate directory

0bfc7b0

Kernel/Devices: Remove UNMAP_AFTER_INIT from SerialDevice code

fbfa273

Kernel/Storage: Remove StorageController reset and shutdown methods

18e9506

We never used these virtual methods outside their own implementation, so let's stop pretending that we should be able to utilize this for unknown purpose.

Kernel/Devices: Remove SerialDevice::initialize method

0160141

We can just do the initialization sequence in the constructor.

Kernel: Move VirtIO console code to the Devices/Serial/VirtIO directory

ea21706

Kernel: Move VirtIO RNG implementation to the Security/Random directory

1fb9348

Kernel/Commandline: Remove unused disable_ide option

d4d1492

This option was merely used by me in bare metal testing, but is useless for anyone else.

Kernel/PCI: Add small access spinlock to each HostController

fe44874

Prepare to remove biglock on PCI::Access in a future commit, so we can ensure we only lock a spinlock on a precise PCI HostController if needed instead of the entire subsystem.

Kernel/GPU: Rename GenericGraphicsAdapter => GPUDevice

d4e3403

GenericGraphicsAdapter is mouthful. Also, the idea is to move towards a more advanced subsystem that handles GPUs, not merely graphics adapters.

Kernel/x86_64: Rename PCI HostBridge => PIIX4HostBridge

dd85a57

This code is actually for the old PIIX4 PCI host bridge, which requires to use legacy x86 IO instructions.

Kernel: Remove includes of PCI API.h file

d62f72f

This file will be removed in a future commit, so let's get rid of what we can right now. Also, the ISAIDEController code as well as other places should have never included this file in the first place.

Kernel/Tasks: Add a WorkQueue for PCI bus device probing & detection

f80d349

This WorkQueue will be used to probe devices from the PCI bus in asynchronous fashion.

Kernel/init: Change waiting duration for boot device to appear

c7071a9

Instead of waiting 2 seconds, wait 100 milliseconds. Do this 10000 so we wait 10 seconds for the boot device to appear. This will be used later on, as the boot device will always appear asynchronously.

Kernel+Userland: Remove the PCI subsystem from SysFS for now

9c6be26

It will change dramatically in the following commits, so let's remove the existence of the PCI bus from SysFS. As lspci needs /sys/bus/pci as well, we will remove it for now, as it will change as well.

Kernel: Remove old VirtIO PCI device detection method

6c3efb5

Kernel: Include actually correct files in USBPipe.h

3e83b65

The USB::Pipe is abstracted from the actual USB host controller implementation, so don't include the UHCIController.h file. Also, we missed an include to UserOrKernelBuffer.h, so this is added to ensure the code can still compile.

Kernel/Library: Add infrastructure for upcoming PCI::Driver class

1d9183a

Kernel/GPU: Prepare subsystem for the new PCI design

4db44a5

Kernel/USB: Remove all host controller drivers for now

9aaf6c7

This is a preparation before applying the new PCI subsystem design, as we will add these drivers properly afterwards.

Kernel/Storage: Remove all drivers except NVMe

d6e60d0

We don't need all drivers except the NVMe driver, so remove everything until we add the drivers back after applying the new PCI subsystem design.

Kernel/Storage: Find partitions in asynchronous fashion

cd04398

spholz mentioned this pull request May 8, 2024

Kernel/HID: Add support for virtio input devices (+ use them for RISC-V in run.py) #24254

Merged

This was referenced May 18, 2024

Kernel: Remove the ConsoleManagement singleton #24358

Merged

Kernel: Move VGATextModeConsole code to the Arch/x86_64/ISABus directory #24363

Merged

supercomputer7 mentioned this pull request May 25, 2024

Kernel+Meta: Remove VGA text mode functionality, remove QEMU ISA-PC machine type as a target #24434

Merged

stale bot added the stale label Jun 5, 2024

supercomputer7 removed the stale label Jun 7, 2024

stale bot added the stale label Jun 29, 2024

stale bot closed this Jul 6, 2024

supercomputer7 reopened this Jul 6, 2024

stale bot removed the stale label Jul 6, 2024

supercomputer7 mentioned this pull request Jul 12, 2024

Kernel: Remove the DeviceManagement singleton, improve construction paths for Device-based classes #24698

Merged

supercomputer7 mentioned this pull request Jul 13, 2024

Kernel: Another preparation before the big PCI subsystem refactor #24699

Open

stale bot added the stale label Aug 5, 2024

supercomputer7 removed the stale label Aug 6, 2024

stale bot added the stale label Aug 29, 2024

supercomputer7 removed the stale label Aug 29, 2024

stale bot added the stale label Sep 28, 2024

supercomputer7 closed this Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kernel: New PCI driver subsystem design #23448

Kernel: New PCI driver subsystem design #23448

supercomputer7 commented Mar 3, 2024

Hendiadyoin1 left a comment

Hendiadyoin1 Mar 3, 2024

Hendiadyoin1 Mar 3, 2024

Hendiadyoin1 Mar 3, 2024

Hendiadyoin1 Mar 3, 2024

Hendiadyoin1 Mar 3, 2024

Hendiadyoin1 Mar 3, 2024

supercomputer7 commented Mar 3, 2024

spholz Mar 3, 2024

spholz left a comment

spholz Mar 3, 2024

spholz Mar 3, 2024 •

edited

Loading

spholz Mar 3, 2024 •

edited

Loading

supercomputer7 commented Apr 29, 2024

supercomputer7 commented May 14, 2024

stale bot commented Jun 5, 2024

supercomputer7 commented Jun 7, 2024

stale bot commented Jun 29, 2024

stale bot commented Jul 6, 2024

supercomputer7 commented Jul 13, 2024

supercomputer7 commented Jul 13, 2024

stale bot commented Aug 5, 2024

stale bot commented Aug 29, 2024

stale bot commented Sep 28, 2024

supercomputer7 commented Oct 2, 2024

		RawPtr<PCI::HostController> m_host_controller {};
		RawPtr<Bus> m_parent_bus {};

Kernel: New PCI driver subsystem design #23448

Kernel: New PCI driver subsystem design #23448

Conversation

supercomputer7 commented Mar 3, 2024

Hendiadyoin1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supercomputer7 commented Mar 3, 2024

Choose a reason for hiding this comment

spholz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spholz Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

spholz Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

supercomputer7 commented Apr 29, 2024

supercomputer7 commented May 14, 2024

stale bot commented Jun 5, 2024

supercomputer7 commented Jun 7, 2024

stale bot commented Jun 29, 2024

stale bot commented Jul 6, 2024

supercomputer7 commented Jul 13, 2024

supercomputer7 commented Jul 13, 2024

stale bot commented Aug 5, 2024

stale bot commented Aug 29, 2024

stale bot commented Sep 28, 2024

supercomputer7 commented Oct 2, 2024

spholz Mar 3, 2024 •

edited

Loading

spholz Mar 3, 2024 •

edited

Loading