Re: change MSI/MSI-X APIs

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

On 2015/05/13 21:42, Christos Zoulas wrote:
> It was not obvious to me that pci_intr_release() free's all the handles.
> The API certainly does not feel symmetric. Calling something with 'alloc'
> int the name usually expects something to call 'free' later.

I design the APIs to be similar to FreeBSD's API. The FreeBSD's APIs
are below:
    https://www.freebsd.org/cgi/man.cgi?query=pci&sektion=9
that is
    + allocate APIs are
      - int pci_alloc_msi(device_t dev, int *count); => for MSI
      - int pci_alloc_msix(device_t dev, int *count); => for MSI-X
    + release(free) API is
      - int pci_release_msi(device_t dev); => for both MSI and MSI-X
 
> Still, there is room for sharing the code no matter what's the count
> of allocated interrupts per group. Again, the way I see it is that
> you want a function that allocates a vector of interrupts the best
> way it can (in general) and have a way to specify quirks for cards
> that need to do something special. How you split the interrupts in
> the driver between functions depends on the driver, but the driver
> does not really care in general what flavor the interrupts are (but
> it might want to ask if it needs to differentiate). Having 150+ lines
> of semi-duplicated code to do interrupt setup does not make a lot
> of sense to me.

Well...yes, but some device drivers need the allocating APIs, such as
"if_vioif". "if_vioif" setup below process:
    (1) allocate "virtqueue"s wanted interrupt numbers in if_vioif.c
    (2) allocate interrupts the same number of "virtqueue"s in virtio.c
    (3) bind each allocated interrupts to "virtqueue"s in virtio.c
So, it require the API which allocate particular MSI-X count.

However, as you indicated, that pci_msix_alloc(), pci_msi_alloc(),
and pci_intx_alloc() fallback code is certainly redundant...

So, I will design new wrapper function which call each allocating APIs,
in addition to existing each allocating APIs.
Currntly very rough design is below:
====================
typedef int pci_intr_type_t;
#define PCI_INTR_TYPE_INTX __BIT(0)
#define PCI_INTR_TYPE_MSI  __BIT(1)
#define PCI_INTR_TYPE_MSIX __BIT(2)

#define PCI_INTR_TYPE_ANY (PCI_INTR_TYPE_INTX | PCI_INTR_TYPE_MSI | PCI_INTR_TYPE_MSIX)

int
pci_intr_alloc(const struct pci_attach_args *, pci_intr_handle_t **ihps,
    int *counts, pci_intr_type_t which);

pci_intr_type_t
pci_intr_type(pci_intr_handle_t *ihp);
====================

The device driver usage pseudo code is below:
====================
xxxx_attach()
{
    int want_msix_count, want_msi_count;
    int intr_counts[2]; // MSI and MSI-X.
                        // The interrupt number of INTx is alway 1.

    // calculate wanted msix count and msi count. 
    intr_counts[0] = want_msi_count;
    intr_counts[1] = want_msix_count;

    // This driver want to use MSI-X, MSI, or INTx.
    // If the driver want to MSI or INTx "which" argument should be
    // "PCI_INTR_TYPE_INTX | PCI_INTR_TYPE_MSI".
    err = pci_intr_alloc(*pa, &sc->sc_intrs, intr_counts, PCI_INTR_TYPE_ANY);
    if (err)
        // cannot allocate any interrupt type, so error handling and exit.

    if (pci_intr_type(sc->sc_intrs[0]) == PCI_INTR_TYPE_MSIX)
        nintrs = intr_counts[1];
    else if (pci_intr_type(sc->sc_intrs[0]) == PCI_INTR_TYPE_MSI)
        nintrs = intr_count[0];
    else if (pci_intr_type(sc->sc_intrs[0]) == PCI_INTR_TYPE_INTX)
        nintrs = 1;
    else
        panic("unknown pci_intr_type");

    for (i = 0; i < nintrs; i++)
        sc->sc_ihs[i] = pci_intr_establish(pc, sc->sc_intrs[i], level, func, arg);

    // snip
}
====================

I believe above APIs satisfy the needs of a lot of device drivers.
# Again, there is some drivers which need pci_{intx,msi,msix}_alloc(),
# therefore other some drivers need pci_msix_alloc_map().

Could you comment about both whether above APIs match your suggestions
and any other pointing?


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>