Re: x86 MD MSI/MSI-X implementation

[Matt Thomas <matt%3am-software.com@localhost>]

> On Apr 9, 2015, at 7:13 PM, Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost> wrote:
> 
> Hi,
> 
> 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.

> 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.

>> 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.

>> 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.