Some AHCI devices support only MSI-x, which made them inoperatible with FreeBSD. Add MSI-x interrupts to generic driver to fix it.
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Up to this moment I haven't seen neither any specs nor hardware supporting MSI-X on AHCI. Could you please describe what hardware are we talking about and how does it use MSI-X? Is MSI-X operation functionally equivalent to multivector MSI defined in AHCI specifications?
I haven't actively used MSI-X myself before, but I have suspicions about those new resource allocations. I don't see driver using any of them after allocation. If they should just be allocated for some technical reason, shouldn't it be done by bus code or something else? Or they are prepared for some later use?
We are trying to upstream support for ThunderX armv8 system. The producer decided to support only MSI-x for all internal interfaces, including AHCI.
Regarding resource allocation, the MSI-x operates differently than MSI did. The new approach requires two additional BARs to be enabled (one is for configuring interrupt vector + data, the other is an array of pending interrupt bits). The only way to enable these BARs by the driver is to allocate them. It might seems unnecessary for the first glance, but allows the system to configure MSI-x table. This is exactly the same way how other drivers are handling this situation, like if_ix.c etc.
I think there may be a bug in case of some hypothetical device supporting both MSI and MSI-X -- your code will try to initialize both, and I am not sure what happen at the end, but suppose nothing good.
Yes, you're absolutely right. Fixed the logic to avoid allocation for both msix and msi.
sys/dev/ahci/ahci_pci.c | ||
---|---|---|
420–422 | Other drivers can use hardcoded values for this, because they know which BAR is for MSIx table and which for PBA. In general, the only way is to examine the configuration header, or, as in this case, read pci_devinfo. |
Added MSI-x table and PBA BAR number resolving in generic way, as per PCIe specification.
Please give any feedback. We'd like to submit it next week if no other objections are reported.
In general I think this looks fine. This is the first driver that doesn't "know" where it's PBA and table are. We could certainly add little accessor functions to return the BAR values cached in the dinfo.
Also, I think you need to explicitly handle the case that the BAR for the PBA and/or table might match the 'r_mem' BAR? Right now if that is true I think you fail to attach?
(Someday I will fix pci_alloc_msix() to reserve the PBA and table BARs, but that wasn't very feasible back when MSI was first added. It is somewhat less painful now that "reserved" resources exist as a real thing and not a hack of PCI bus attach.)
sys/dev/ahci/ahci_pci.c | ||
---|---|---|
470 | I would not set ctlr->numirqs here. You don't know which variant you are going to use. As it is you will now pass the MSI-X count to pci_alloc_msi() if pci_alloc_msix() fails which might cause it to fail (if it has more than 1 MSI-X message, but only 1 MSI message for example). Instead, I would do something like this: if (msi_count == 0 && msix_count == 0) ctlr->msi = 0; if (ctlr->msi < 0) ctlr->msi = 0; else if (ctlr->msi == 1) { /* Only use a single message if present. */ msi_count = min(1, msi_count); msix_count = min(1, msix_count); } else if (ctlr->msi > 1) { ctlr->msi = 2; /* Allocate MSI/MSI-X messages. */ if (ctlr->msi > 0) { error = ENXIO; if (msix_count > 0) { error = pci_alloc_msix(dev, &msix_count); if (error == 0) ctlr->numirqs = msix_count; } if ((error != 0) && (msi_count > 0)) { error = pci_alloc_msi(dev, &msi_count); if (error == 0) ctlr->numirqs = msi_count; } if (error != 0) ctlr->msi = 0; } | |
474 | Maybe use ENXIO instead of -1 for readability since the values here are errno values? | |
494 | I think just ctlr->msi > 0 is sufficient. If there are no messages present or they fail to allocate the code above always clears ctlr->msi to 0. |
Forgot to include new BAR allocation, fixing it now. It should work even if all BARs (mem+msix+pba) are the same.
One minor style nit, but lgtm. I'll try to remember to add accessors for the PBA and Table BARs so we can use those to replace ahci_pci_read_msix_bars() in the future.
sys/dev/ahci/ahci_pci.c | ||
---|---|---|
472 | Small style nit of having 'else if' on previous line with '}' |