Re: x86 MD MSI/MSI-X implementation

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

Hi,

On 2015/04/10 4:30, Taylor R Campbell wrote:
>    Date: Thu, 09 Apr 2015 19:44:47 +0900
>    From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
> 
>    I implement x86 MD MSI/MSI-X support code. Here is the patch:
>        http://www.netbsd.org/~knakahara/md-msi-msix/x86-md-msi-msix.patch
> 
> The file pci_msi_machdep.c appears to be missing from this patch.
> Also, I don't see where intr_distribute is implemented -- is that part
> of a separate patch?

Mmm, it seems to include pci_msi_machdep.c at line#1939 in above patch...
I don't know why it miss the file, but I re-upload the patch. Could you
retry to download this patch? The file size must be 121,505byte.

intr_distribute() is implemented in sys/arch/x86/x86/intr.c. It seems
the file is not complete... Could you re-check after re-download?

>    Furthermore, here is the usage example of if_wm:
>        http://www.netbsd.org/~knakahara/md-msi-msix/if_wm-msi-msix.patch
> 
> It seems to me that it should not be the driver's job to distribute
> interrupts across CPUs.  Instead, intuitively, it feels like
> 
> (a) the driver should just establish handlers for all its interrupts,
> (b) the kernel should choose a default distribution across CPUs, and
> (c) the operator should be able to change it with intrctl(8).

To be honest, I cannot decide which is the best "default distribution",
Round-robin (like FreeBSD), assigning the same CPU as possible, or 
assigning different CPUs as possible. So, I leave it to the device driver
owner in this implementation. I think the best "default distribution" may
be decided after some driver implementations are done with this MD APIs.
So, (b) will be implemented later, probably the same time as implementing
MI APIs.

(c) is (near?) future work :)
I have the intrctl(8) implementation, however it is needs to refactor
a little more.

> Why is there a separate pci_intr_setattr function?  Why not pass
> PCI_INTR_MPSAFE to the relevant pci_*_establish function?

I think this is MI manner. MPSAFE flag must be passed by embedding
pci_intr_handle_t, furthermore pci_intr_handle_t may be not numerical
type such as Xen architecture.

> Minor nit: It looks like the MSI case of wm(4) uses pci_msix_establish
> instead of pci_msi_establish.  Is it important to use the same
> pci_intr_handle_t type for INTx, MSI, and MSI-X interrupt handles?
> Using a different type would catch that mistake.

Oh, it is a mistake. MSI should use pci_msi_establish().
# in x86 current implementation, pci_msi_establish() is the same as
# pci_msix_establish(), so I missed this mistake

It is not so important to use the same pci_intr_handle_t type. I use
the same type to simplify to write device driver code. So, I think
it is good to use different type for each interrupt type.
On the other hand, I also think INTx, MSI, and MSI-X should use the same
establish function such as pci_any_intr_establish() or simply modified
pci_intr_establish(). At least, x86 can implement this unified establish
function. By doing this, there is no need to prevent the boring bug like
I did :)


Thanks,

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

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

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