Some AHCI devices support only MSI-x, which made them inoperatible with FreeBSD. Add MSI-x interrupts to generic driver to fix it.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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?
Comment Actions
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.
Comment Actions
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.
Comment Actions
Yes, you're absolutely right. Fixed the logic to avoid allocation for both msix and msi.
| sys/dev/ahci/ahci_pci.c | ||
|---|---|---|
| 417–419 ↗ | (On Diff #6752) | Please use usual block comment style here. |
| 420–422 ↗ | (On Diff #6752) | You aren't supposed to touch the private bits of pci's devinfo like this. |
| sys/dev/ahci/ahci_pci.c | ||
|---|---|---|
| 420–422 ↗ | (On Diff #6752) | 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. |
Comment Actions
Added MSI-x table and PBA BAR number resolving in generic way, as per PCIe specification.
Comment Actions
Please give any feedback. We'd like to submit it next week if no other objections are reported.
Comment Actions
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 | ||
|---|---|---|
| 498 ↗ | (On Diff #6774) | 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;
} |
| 502 ↗ | (On Diff #6774) | Maybe use ENXIO instead of -1 for readability since the values here are errno values? |
| 522 ↗ | (On Diff #6774) | 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. |
Comment Actions
Forgot to include new BAR allocation, fixing it now. It should work even if all BARs (mem+msix+pba) are the same.
Comment Actions
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 | ||
|---|---|---|
| 511 ↗ | (On Diff #7123) | Small style nit of having 'else if' on previous line with '}' |