Re: x86 MD MSI/MSI-X implementation

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

Hi,

On 2015/04/10 16:15, Matt Thomas wrote:
>> On Apr 9, 2015, at 7:13 PM, Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost> wrote:
>> On 2015/04/10 1:59, Matt Thomas wrote:
>>
>>> void *vih;
>>>
>>> That should be pci_msix_handle_t or pci_msi_handle_t unless you want
>>> still use pci_intr_handle_t.
>>>
>>> +		mutex_enter(&cpu_lock);
>>> +		error = intr_distribute(vih, affinity, NULL);
>>> +		mutex_exit(&cpu_lock);
>>>
>>>
>>> Why the cpu_lock?  shouldn't intr_distribute handle that itself?
>>
>> intr_distribute() update multiple cpu_info, so it require cpu_lock.
>> Furthermore, getting cpu_lock in the out of intr_distribute() is
>> required to unify the same style as intrctl(8).
>> # intrctl(8) is near future work
> 
> I found the previous incarnations of intrctl to be too x86 centric.
> Remember this is supposed to be MI and it isn't.  It should be noted
> that the intr_xxx interface is implicitly MD.  Only the <bus>_intr_<foo>
> are really MI.  So relying of MD interfaces is not appropriate.

I think your suggestion intr_distribute() is MI, but it is a
misunderstanding.
http://mail-index.netbsd.org/tech-kern/2014/08/27/msg017578.html

OK, I will change to pci_intr_distribute().

>> intrctl(8) needs to get the handle for intr_distribute() before doing
>> intr_distribute(), since userland processes know the interrupt name
>> only. To prevent race conditions among multiple intrctl(8) processes,
>> it required cpu_lock like below code:
>> ====================
>> intr_set_affinity_sysctl()
>> {
>>    mutex_enter(&cpu_lock);
>>    ich = intr_get_handler("interrupt name from userland");
>>    error = intr_distribute(ich, newcpuset, oldcpuset);
>>    mutex_exit(&cpu_lock);
>> }
>> ====================
>> To unify with above code, it get cpu_lock in the out of intr_distribute().
> 
> It just doesn't seem right to expose that locking to the user of API.
> You could have a intr_distribute_handler("name", ...." and avoid exposing it.

OK, I move cpu_lock in pci_intr_distribute(). And I implement
intr_distribute_handler("name",...) which also get cpu_lock in the function.

>>> This should a pci_intr_distribute and be done before the establish, not after.
>>
>> This reason is similar to above description, that is, to unify the same
>> as intrctl(8) which run after device established.
> 
> Again, intrctl is too x86 specific.  That doesn't help powerpc, mips, or arm.

Well..., my co-worker port intrctl(8) to powerpc/booke dual core board.
Furthermore, it works well and helps MP tests. So, I think intrctl(8) helps
other than x86.

>>> Why do you assume you will only allocate one slot per msi/msix call?
>>> Some device will use multiple contiguous slots so you have to alllocate
>>> them as a set.
>>
>> Well..., could you tell me what do you mean "slot"? MSI-X table entry?
>> If it means MSI-X table entry, pci_msix_alloc() and pci_msix_alloc_exact()
>> allocate contiguous MSI-X table entries. In contrast, pci_msix_alloc_map()
>> can allocate sparse MSI-X table entries.
>> MSI always use contiguous vector number.
> 
> The call doesn't contain the count of MSIX/MSI entries desired.  Nor does
> it return the number of entries allocated.

Here is the function specifications copied from man of my patch.
====================
     int
     pci_msi_alloc(struct pci_attach_args *pa, pci_intr_handle_t **ihps, int
     *count);

     int
     pci_msi_alloc_exect(struct pci_attach_args *pa, pci_intr_handle_t
     **ihps, int count);

     int
     pci_msix_alloc(struct pci_attach_args *pa, pci_intr_handle_t **ihps, int
     *count);

     int
     pci_msix_alloc_exect(struct pci_attach_args *pa, pci_intr_handle_t
     **ihps, int count);

     int
     pci_msix_alloc_map(struct pci_attach_args *pa, pci_intr_handle_t
     **ihps, u_int *table_indexes, int count);
====================
All of these functions include "count" parameter, which is the number of
MSI/MSI-X vectors. pci_msi_alloc() and pci_msix_alloc() can change "count"
parameter to the number which the system really allocated.
# These specification of pci_msi_alloc() and pci_msix_alloc() are
# similar to FreeBSD's pci_alloc_msi() and pci_alloc_msix()

In contrast, pci_msi_alloc_exect(), pci_msix_alloc_exect() and 
pci_msix_alloc_map() simply fail (return non-zero value) if they cannot
allocate "count" number of vectors. Because these APIs should be used
when device drivers need exactly MSI/MSI-X vector numbers, the APIs
must not allocate MSI/MSI-X vectors less than requested "count".
# Many device drivers give up to use MSI-X, when the API cannot
# allocate requested counts. So, I implement *exact() APIs.


Thanks,

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

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

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