Kernel: Add MSIx interrupt mechanism for PCI devices #18580

Panky-codes · 2023-04-29T21:45:27Z

This adds support to MSI(x) based interrupt mechanism, an alternative to pin-based interrupts.

The first 10 commits add the necessary infrastructure to use MSIx interrupt mechanism. The next three commits add MSIx support to the NVMe device. This PR only adds MSIx support. TODOs have been added for the old MSI-based interrupts which could be extended later.

Test:

$ SERENITY_NVME_ENABLE=1 Meta/serenity.sh run

Trace from QEMU for the NVMe device:

...
pci_nvme_io_cmd cid 54 nsid 0x1 sqid 1 opc 0x2 opname 'NVME_NVM_CMD_READ'
<snip>
pci_nvme_irq_msix raising MSI-X IRQ vector 1
pci_nvme_mmio_write addr 0x100c data 0x36 size 4
pci_nvme_mmio_doorbell_cq cqid 1 new_head 54
...

kleinesfilmroellchen

Mostly style comments, exciting changes!

Kernel/Bus/PCI/Device.cpp

Kernel/Arch/PCIMSI.h

Kernel/Arch/x86_64/PCI/MSI.h

Kernel/Bus/PCI/Device.cpp

Kernel/Bus/PCI/Device.h

Kernel/Interrupts/PCIIRQHandler.cpp

Kernel/Storage/NVMe/NVMeQueue.h

supercomputer7 · 2023-04-30T03:42:42Z

I will look into this today or tomorrow.
Regardless, adding MSI-x support is a good thing, thank you for doing this!

Kernel/Interrupts/PCIIRQHandler.h

supercomputer7

Besides my comment on that explicit keyword, this looks wonderful!
I was not sure about the whole PCIIRQHandler thing, but it does seem correct to me, so let's try this out and if we happen to have something better we can always fix this :)

kleinesfilmroellchen

Just two more things

Kernel/Bus/PCI/Device.cpp

Kernel/Interrupts/GenericInterruptHandler.h

Kernel/Arch/x86_64/PCI/Initializer.cpp

Kernel/Arch/Interrupts.h

Kernel/Bus/PCI/Device.h

Kernel/Interrupts/PCIIRQHandler.cpp

Kernel/Storage/NVMe/NVMeController.cpp

Kernel/Interrupts/GenericInterruptHandler.h

Kernel/Interrupts/PCIIRQHandler.cpp

Kernel/Arch/x86_64/Interrupts.cpp

Kernel/Arch/aarch64/Interrupts.cpp

Kernel/Arch/PCIMSI.h

Kernel/Storage/NVMe/NVMeInterruptQueue.cpp

Pin-based PCI device are allocated an IRQ, and it could be shared with multiple devices. An interrupt handler with an IRQ for a PCI device will get registered only during the driver initialization. For MSI(x) interrupts, the driver has to allocate IRQs and this field can be used to skip IRQs that have already been reserved by pin-based interrupts so that we don't have to share IRQs, which generally will reduce the performance.

Set pin-based interrupt handler as reserved during PCI bus init. This is required so that MSI(x) based interrupts can avoid sharing the IRQ which has been marked as reserved.

MSI(x) interrupts need to reserve IRQs so that it can be programmed by the device. Add an API to reserve contiguous ranges of interrupt handlers so that it can used by PCI devices that use MSI(x) mechanism. This API needs to be implemented by aarch64 architecture.

MSI(x) mechanism requires the device to write to its Capability structure. Add write{8,16,32} similar to read{8,16,32}.

Add a struct named MSIxInfo that stores all the relevant MSIx information as a part of PCI DeviceIdentifier struct. Populate the MSIx struct during the PCI device init. As the DeviceIdentifier struct need to populate MSIx info, don't mark DeviceIdentifier as const in the PCI::Device class.

Instead of iterating through the capabilities, use the is_msix_capable() API from the PCIIdentifier class that belongs to the device.

Implement enabling and disabling MSIx interrupts for a PCI device. Removes two TODO()s from PCI::Device.cpp :^)

MSIx table entry is used to program interrupt vectors and it is architecture specific. Add helper functions declaration in Arch/PCIMSI.h. The definition of the function is placed in the respective arch specific code.

Add reserve_irqs, allocate_irq, enable_interrupt and disable_interrupt API to a PCI device. reserve_irqs() can be used by a device driver that would like to reserve irqs for MSI(x) interrupts. The API returns the type of IRQ that was reserved by the PCI device. If the PCI device does not support MSI(x), then it is a noop. allocate_irq() API can be used to allocate an IRQ at an index. For MSIx the driver needs to map the vector table into the memory and add the corresponding IRQ at the given index. This API will return the actual IRQ that was used so that the driver can use it create interrupt handler for that IRQ. {enable, disable}_interrupt API is used to enable or disable a particular IRQ at the given index. It is a noop for pin-based interrupts. This could be used by IRQHandler to enable or disable an interrupt.

gmta · 2023-05-07T10:36:39Z

@Panky-codes result of running lsirq after SERENITY_NVME_ENABLE=1 Meta/serenity.sh run:

[lsirq(45:45)]: ASSERTION FAILED: !PtrTraits::is_null(bits)
[lsirq(45:45)]: ././Kernel/Library/LockRefPtr.h:447 in T* AK::LockRefPtr<T, PtrTraits>::as_nonnull_ptr(FlatPtr) const [with T = Kernel::IRQController; PtrTraits = AK::LockRefPtrTraits<Kernel::IRQController>; FlatPtr = long unsigned int]
[lsirq(45:45)]: KERNEL PANIC! :^(
[lsirq(45:45)]: Aborted
[lsirq(45:45)]: at ./Kernel/Arch/x86_64/CPU.cpp:35 in void abort()
[lsirq(45:45)]: Kernel + 0x00000000016e462f  Kernel::__panic(char const*, unsigned int, char const*) +0x16f
[lsirq(45:45)]: Kernel + 0x00000000016e5b41  abort.localalias +0x35a
[lsirq(45:45)]: Kernel + 0x00000000016e57e7  abort.localalias +0x0
[lsirq(45:45)]: Kernel + 0x00000000005c5206  Kernel::PCIIRQHandler::controller() const +0x166
[lsirq(45:45)]: Kernel + 0x0000000000b0e357  Kernel::SysFSInterrupts::try_generate(Kernel::KBufferBuilder&)::{lambda(Kernel::GenericInterruptHandler&)#1}::operator()(Kernel::GenericInterruptHandler&) const::{lambda()#1}::operator()() const +0x587
[lsirq(45:45)]: Kernel + 0x0000000000b1182d  AK::Function<void (Kernel::GenericInterruptHandler&)>::CallableWrapper<Kernel::SysFSInterrupts::try_generate(Kernel::KBufferBuilder&)::{lambda(Kernel::GenericInterruptHandler&)#1}>::call(Kernel::GenericInterruptHandler&) +0x12d
[lsirq(45:45)]: Kernel + 0x00000000016fa61d  Kernel::InterruptManagement::enumerate_interrupt_handlers(AK::Function<void (Kernel::GenericInterruptHandler&)>) +0x15d
[lsirq(45:45)]: Kernel + 0x0000000000b0c81a  Kernel::SysFSInterrupts::try_generate(Kernel::KBufferBuilder&) [clone .localalias] +0x21a
[lsirq(45:45)]: Kernel + 0x0000000000ba4119  Kernel::SysFSGlobalInformation::refresh_data(Kernel::OpenFileDescription&) const [clone .localalias] +0x689
[lsirq(45:45)]: Kernel + 0x0000000000a39d93  Kernel::SysFSInode::attach(Kernel::OpenFileDescription&) [clone .localalias] +0x283
[lsirq(45:45)]: Kernel + 0x00000000008ca620  Kernel::OpenFileDescription::attach() [clone .localalias] +0x2e0
[lsirq(45:45)]: Kernel + 0x00000000008dd480  Kernel::OpenFileDescription::try_create(Kernel::Custody&) +0x7f0
[lsirq(45:45)]: Kernel + 0x0000000000ca5f63  Kernel::VirtualFileSystem::open(Kernel::Process const&, Kernel::Credentials const&, AK::StringView, int, unsigned short, Kernel::Custody&, AK::Optional<Kernel::UidAndGid>) [clone .localalias] +0x633
[lsirq(45:45)]: Kernel + 0x0000000000caa3f3  Kernel::VirtualFileSystem::open(Kernel::Credentials const&, AK::StringView, int, unsigned short, Kernel::Custody&, AK::Optional<Kernel::UidAndGid>) +0x173
[lsirq(45:45)]: Kernel + 0x000000000133d18b  Kernel::Process::sys$open(AK::Userspace<Kernel::Syscall::SC_open_params const*>) +0x104b
[lsirq(45:45)]: Kernel + 0x000000000118c37f  Kernel::Syscall::handle(Kernel::RegisterState&, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long) [clone .localalias] +0x93f
[lsirq(45:45)]: Kernel + 0x000000000118e4e2  syscall_handler +0x902
[lsirq(45:45)]: Kernel + 0x0000000001749a81  syscall_entry +0x51

PCIIRQHandler is a generic IRQ handler that the device driver can inherit to use either Pin or MSI(x) based interrupt mechanism. The PCIIRQHandler can do what the existing IRQHandler can do for pin based interrupts but also deal with MSI based interrupts. We can hopefully convert all the PCI based devices to use this handler so that MSI(x) can be used.

Add an explicit QueueType enum which could be used to create a poll or an interrupt queue. This is better than passing an Optional<irq>. This refactoring is in preparation for adding MSIx support to NVMe.

This is in preparation for adding MSI(x) support to the NVMe device. NVMeInterruptQueue needs access to the PCI device to deal with MSI(x) interrupts. It is ok to pass the NVMeController as a reference to the NVMeQueue as NVMeController is the one that owns the NVMeQueue. This is very similar to how AHCIController passes its reference to its interrupt handler.

Add MSIx support to NVMe. Prefer MSIx over pin-based interrupts as they are more efficient and all modern hardware support them.

Now we support MSIx for NVMe. Retain the information about using nvme_poll until MSIx is tested on a Bare metal system.

Panky-codes · 2023-05-07T12:18:04Z

Thanks for the report @gmta . I missed one place to check for null ptr in PCIIRQHandler. Updated the code to return PCI-MSI similar to what Linux does.

gmta · 2023-05-07T12:36:06Z

Thanks @Panky-codes, that fixed it!

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 29, 2023

Panky-codes force-pushed the msix-v3 branch from 9fcb5f2 to 8b594c4 Compare April 29, 2023 21:51

kleinesfilmroellchen suggested changes Apr 29, 2023

View reviewed changes

Panky-codes force-pushed the msix-v3 branch from 8b594c4 to c147a60 Compare April 30, 2023 17:23

supercomputer7 reviewed Apr 30, 2023

View reviewed changes

Kernel/Interrupts/PCIIRQHandler.h Outdated Show resolved Hide resolved

supercomputer7 approved these changes Apr 30, 2023

View reviewed changes

Panky-codes force-pushed the msix-v3 branch 2 times, most recently from f9a53f6 to f65d4a6 Compare May 1, 2023 08:58

kleinesfilmroellchen suggested changes May 1, 2023

View reviewed changes

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

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

Panky-codes force-pushed the msix-v3 branch 2 times, most recently from c049cc3 to f9a1cb3 Compare May 4, 2023 10:40

gmta requested changes May 5, 2023

View reviewed changes

Panky-codes force-pushed the msix-v3 branch from f9a1cb3 to 2e25e18 Compare May 5, 2023 13:25

Panky-codes requested a review from gmta May 6, 2023 11:51

gmta reviewed May 6, 2023

View reviewed changes

Panky-codes added 9 commits May 6, 2023 23:22

Kernel/PCI: Set IRQ as reserved for pin-based interrupts

0eb0a3a

Set pin-based interrupt handler as reserved during PCI bus init. This is required so that MSI(x) based interrupts can avoid sharing the IRQ which has been marked as reserved.

Kernel: Add write{8,16,32} to the PCI Capability struct

e86b2ff

MSI(x) mechanism requires the device to write to its Capability structure. Add write{8,16,32} similar to read{8,16,32}.

Kernel: Use PCIIdentifier is_msix_capable API to retrieve MSIx status

5d73904

Instead of iterating through the capabilities, use the is_msix_capable() API from the PCIIdentifier class that belongs to the device.

Kernel: Implement {enable,disable}_msix interrupts in PCI Device

bae091d

Implement enabling and disabling MSIx interrupts for a PCI device. Removes two TODO()s from PCI::Device.cpp :^)

Kernel: Implement helpers to manipulate MSI(x) data structures

09e4c11

MSIx table entry is used to program interrupt vectors and it is architecture specific. Add helper functions declaration in Arch/PCIMSI.h. The definition of the function is placed in the respective arch specific code.

Panky-codes force-pushed the msix-v3 branch from 2e25e18 to 0987363 Compare May 6, 2023 21:42

gmta added ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels May 7, 2023

Panky-codes added 4 commits May 7, 2023 14:05

NVMe: Use an explicit Queue type instead of using an Optional irq

04e47f5

Add an explicit QueueType enum which could be used to create a poll or an interrupt queue. This is better than passing an Optional<irq>. This refactoring is in preparation for adding MSIx support to NVMe.

Kernel: Add MSIx support to NVMe

aa021b1

Add MSIx support to NVMe. Prefer MSIx over pin-based interrupts as they are more efficient and all modern hardware support them.

Documentation: Update information regarding MSIx for NVMe :^)

315b75a

Now we support MSIx for NVMe. Retain the information about using nvme_poll until MSIx is tested on a Bare metal system.

Panky-codes force-pushed the msix-v3 branch from 0987363 to 315b75a Compare May 7, 2023 12:16

github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author labels May 7, 2023

gmta approved these changes May 7, 2023

View reviewed changes

gmta added ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels May 7, 2023

gmta merged commit 53d47c6 into SerenityOS:master May 7, 2023
12 checks passed

github-actions bot removed the ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed label May 7, 2023

Panky-codes mentioned this pull request May 8, 2023

Kernel: Add MSI support for PCI devices #18732

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kernel: Add MSIx interrupt mechanism for PCI devices #18580

Kernel: Add MSIx interrupt mechanism for PCI devices #18580

Panky-codes commented Apr 29, 2023 •

edited

kleinesfilmroellchen left a comment

supercomputer7 commented Apr 30, 2023

supercomputer7 left a comment

kleinesfilmroellchen left a comment

gmta commented May 7, 2023

Panky-codes commented May 7, 2023 •

edited

gmta commented May 7, 2023

Kernel: Add MSIx interrupt mechanism for PCI devices #18580

Kernel: Add MSIx interrupt mechanism for PCI devices #18580

Conversation

Panky-codes commented Apr 29, 2023 • edited

kleinesfilmroellchen left a comment

Choose a reason for hiding this comment

supercomputer7 commented Apr 30, 2023

supercomputer7 left a comment

Choose a reason for hiding this comment

kleinesfilmroellchen left a comment

Choose a reason for hiding this comment

gmta commented May 7, 2023

Panky-codes commented May 7, 2023 • edited

gmta commented May 7, 2023

Panky-codes commented Apr 29, 2023 •

edited

Panky-codes commented May 7, 2023 •

edited