Add MSI-x support to AHCI driver

[wma_semihalf.com]

Summary

Some AHCI devices support only MSI-x, which made them inoperatible with FreeBSD. Add MSI-x interrupts to generic driver to fix it.

[mav]

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?

[wma_semihalf.com]

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.

[mav]

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.

[wma_semihalf.com]

Yes, you're absolutely right. Fixed the logic to avoid allocation for both msix and msi.

[mav]

I can't review that resource allocation stuff, but otherwise have no objections.

[imp]

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.
Isn't there an approved set of accessor functions to go through to get this info?
If other drivers do this, they should also be fixed.

[imp]

I'd also suggest getting John Baldwin to review the MSIx parts.

[wma_semihalf.com]

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.
Nevertheless, I'll look for any function-accessor, but I'm affraid there is none. Since this file is PCI-specific, maybe reading BAR-id from ext_cap structure will be more generic. I will replace an access to pci_devinfo with that if fail to find any bettr idea.

[wma_semihalf.com]

Added MSI-x table and PBA BAR number resolving in generic way, as per PCIe specification.

[wma_semihalf.com]

Any comments?

[wma_semihalf.com]

Please give any feedback. We'd like to submit it next week if no other objections are reported.

[mav]

It looks better to me, though John's comment would be good to have.

[jhb]

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.

[wma_semihalf.com]

Forgot to include new BAR allocation, fixing it now. It should work even if all BARs (mem+msix+pba) are the same.

[jhb]

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 '}'