KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Michael S. Tsirkin]

The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
been merged for 2.6.30. However, I note that PCI spec allows devices to
support multiple vectors with MSI as well (support will be in linux
2.6.30).

Even though qemu for now only uses a single vector with MSI, it would
seem that it's better to make the kernel/user interface generic straight
away rather than add more ioctls later. What do you think? It might not
be too late to fix this for 2.6.30.

-- 
MST

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Marcelo Tosatti]

On Tue, May 05, 2009 at 01:30:28PM +0300, Michael S. Tsirkin wrote:
> The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
> been merged for 2.6.30. However, I note that PCI spec allows devices to
> support multiple vectors with MSI as well (support will be in linux
> 2.6.30).
> 
> Even though qemu for now only uses a single vector with MSI, it would
> seem that it's better to make the kernel/user interface generic straight
> away rather than add more ioctls later. What do you think? It might not
> be too late to fix this for 2.6.30.

Can't you use more than one KVM_ASSIGN_SET_MSIX_ENTRY call per assigned
device?

If you can't, it would be better to change the ioctls before 2.6.30 is
release IMO.

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Michael S. Tsirkin]

On Tue, May 05, 2009 at 08:04:15AM -0300, Marcelo Tosatti wrote:
> On Tue, May 05, 2009 at 01:30:28PM +0300, Michael S. Tsirkin wrote:
> > The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
> > been merged for 2.6.30. However, I note that PCI spec allows devices to
> > support multiple vectors with MSI as well (support will be in linux
> > 2.6.30).
> > 
> > Even though qemu for now only uses a single vector with MSI, it would
> > seem that it's better to make the kernel/user interface generic straight
> > away rather than add more ioctls later. What do you think? It might not
> > be too late to fix this for 2.6.30.
> 
> Can't you use more than one KVM_ASSIGN_SET_MSIX_ENTRY call per assigned
> device?

Sure, but only one KVM_ASSIGN_SET_MSIX_NR.

> If you can't, it would be better to change the ioctls before 2.6.30 is
> release IMO.
> 

-- 
MST

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Gregory Haskins]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

Michael S. Tsirkin wrote:
> The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
> been merged for 2.6.30. However, I note that PCI spec allows devices to
> support multiple vectors with MSI as well (support will be in linux
> 2.6.30).
>
> Even though qemu for now only uses a single vector with MSI, it would
> seem that it's better to make the kernel/user interface generic straight
> away rather than add more ioctls later. What do you think? It might not
> be too late to fix this for 2.6.30.
>   

+1



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Avi Kivity]

Michael S. Tsirkin wrote:
> On Tue, May 05, 2009 at 08:04:15AM -0300, Marcelo Tosatti wrote:
>   
>> On Tue, May 05, 2009 at 01:30:28PM +0300, Michael S. Tsirkin wrote:
>>     
>>> The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
>>> been merged for 2.6.30. However, I note that PCI spec allows devices to
>>> support multiple vectors with MSI as well (support will be in linux
>>> 2.6.30).
>>>
>>> Even though qemu for now only uses a single vector with MSI, it would
>>> seem that it's better to make the kernel/user interface generic straight
>>> away rather than add more ioctls later. What do you think? It might not
>>> be too late to fix this for 2.6.30.
>>>       
>> Can't you use more than one KVM_ASSIGN_SET_MSIX_ENTRY call per assigned
>> device?
>>     
>
> Sure, but only one KVM_ASSIGN_SET_MSIX_NR.
>
>   

MSIX_NR is the size of the table, while MSIX_ENTRY updates a single 
entry, if I read the code correctly.


-- 
error compiling committee.c: too many arguments to function

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Michael S. Tsirkin]

On Tue, May 05, 2009 at 02:57:10PM +0300, Avi Kivity wrote:
> Michael S. Tsirkin wrote:
>> On Tue, May 05, 2009 at 08:04:15AM -0300, Marcelo Tosatti wrote:
>>   
>>> On Tue, May 05, 2009 at 01:30:28PM +0300, Michael S. Tsirkin wrote:
>>>     
>>>> The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
>>>> been merged for 2.6.30. However, I note that PCI spec allows devices to
>>>> support multiple vectors with MSI as well (support will be in linux
>>>> 2.6.30).
>>>>
>>>> Even though qemu for now only uses a single vector with MSI, it would
>>>> seem that it's better to make the kernel/user interface generic straight
>>>> away rather than add more ioctls later. What do you think? It might not
>>>> be too late to fix this for 2.6.30.
>>>>       
>>> Can't you use more than one KVM_ASSIGN_SET_MSIX_ENTRY call per assigned
>>> device?
>>>     
>>
>> Sure, but only one KVM_ASSIGN_SET_MSIX_NR.
>>
>>   
>
> MSIX_NR is the size of the table, while MSIX_ENTRY updates a single  
> entry, if I read the code correctly.

Right. So we'll need something like this for MSI as well.
Actually maybe MSIX_NR MSIX_ENTRY should be renamed to MSI_NR / MSI_ENTRY
and changed to do the right thing depending on the IRQ type?

-- 
MST

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Avi Kivity]

Michael S. Tsirkin wrote:
> On Tue, May 05, 2009 at 02:57:10PM +0300, Avi Kivity wrote:
>   
>> Michael S. Tsirkin wrote:
>>     
>>> On Tue, May 05, 2009 at 08:04:15AM -0300, Marcelo Tosatti wrote:
>>>   
>>>       
>>>> On Tue, May 05, 2009 at 01:30:28PM +0300, Michael S. Tsirkin wrote:
>>>>     
>>>>         
>>>>> The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
>>>>> been merged for 2.6.30. However, I note that PCI spec allows devices to
>>>>> support multiple vectors with MSI as well (support will be in linux
>>>>> 2.6.30).
>>>>>
>>>>> Even though qemu for now only uses a single vector with MSI, it would
>>>>> seem that it's better to make the kernel/user interface generic straight
>>>>> away rather than add more ioctls later. What do you think? It might not
>>>>> be too late to fix this for 2.6.30.
>>>>>       
>>>>>           
>>>> Can't you use more than one KVM_ASSIGN_SET_MSIX_ENTRY call per assigned
>>>> device?
>>>>     
>>>>         
>>> Sure, but only one KVM_ASSIGN_SET_MSIX_NR.
>>>
>>>   
>>>       
>> MSIX_NR is the size of the table, while MSIX_ENTRY updates a single  
>> entry, if I read the code correctly.
>>     
>
> Right. So we'll need something like this for MSI as well.
> Actually maybe MSIX_NR MSIX_ENTRY should be renamed to MSI_NR / MSI_ENTRY
> and changed to do the right thing depending on the IRQ type?
>   

Works for me.  Sheng, is there a reason why it wasn't done like this?

btw, it could be further simplified by using irqfd.  Instead of the host 
device tying directly into kvm, it could just trigger an eventfd; and we 
could terminate the eventfd either in kvm (irqfd) or in qemu.

-- 
error compiling committee.c: too many arguments to function

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Michael S. Tsirkin]

On Tue, May 05, 2009 at 03:08:40PM +0300, Avi Kivity wrote:
> Michael S. Tsirkin wrote:
>> On Tue, May 05, 2009 at 02:57:10PM +0300, Avi Kivity wrote:
>>   
>>> Michael S. Tsirkin wrote:
>>>     
>>>> On Tue, May 05, 2009 at 08:04:15AM -0300, Marcelo Tosatti wrote:
>>>>         
>>>>> On Tue, May 05, 2009 at 01:30:28PM +0300, Michael S. Tsirkin wrote:
>>>>>             
>>>>>> The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
>>>>>> been merged for 2.6.30. However, I note that PCI spec allows devices to
>>>>>> support multiple vectors with MSI as well (support will be in linux
>>>>>> 2.6.30).
>>>>>>
>>>>>> Even though qemu for now only uses a single vector with MSI, it would
>>>>>> seem that it's better to make the kernel/user interface generic straight
>>>>>> away rather than add more ioctls later. What do you think? It might not
>>>>>> be too late to fix this for 2.6.30.
>>>>>>                 
>>>>> Can't you use more than one KVM_ASSIGN_SET_MSIX_ENTRY call per assigned
>>>>> device?
>>>>>             
>>>> Sure, but only one KVM_ASSIGN_SET_MSIX_NR.
>>>>
>>>>         
>>> MSIX_NR is the size of the table, while MSIX_ENTRY updates a single   
>>> entry, if I read the code correctly.
>>>     
>>
>> Right. So we'll need something like this for MSI as well.
>> Actually maybe MSIX_NR MSIX_ENTRY should be renamed to MSI_NR / MSI_ENTRY
>> and changed to do the right thing depending on the IRQ type?
>>   
>
> Works for me.  Sheng, is there a reason why it wasn't done like this?
>
> btw, it could be further simplified by using irqfd.  Instead of the host  
> device tying directly into kvm, it could just trigger an eventfd; and we  
> could terminate the eventfd either in kvm (irqfd) or in qemu.

This probably is outside the scope for 2.6.30 :)

-- 
MST

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Michael S. Tsirkin]

On Tue, May 05, 2009 at 03:08:40PM +0300, Avi Kivity wrote:
> Michael S. Tsirkin wrote:
>> On Tue, May 05, 2009 at 02:57:10PM +0300, Avi Kivity wrote:
>>   
>>> Michael S. Tsirkin wrote:
>>>     
>>>> On Tue, May 05, 2009 at 08:04:15AM -0300, Marcelo Tosatti wrote:
>>>>         
>>>>> On Tue, May 05, 2009 at 01:30:28PM +0300, Michael S. Tsirkin wrote:
>>>>>             
>>>>>> The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
>>>>>> been merged for 2.6.30. However, I note that PCI spec allows devices to
>>>>>> support multiple vectors with MSI as well (support will be in linux
>>>>>> 2.6.30).
>>>>>>
>>>>>> Even though qemu for now only uses a single vector with MSI, it would
>>>>>> seem that it's better to make the kernel/user interface generic straight
>>>>>> away rather than add more ioctls later. What do you think? It might not
>>>>>> be too late to fix this for 2.6.30.
>>>>>>                 
>>>>> Can't you use more than one KVM_ASSIGN_SET_MSIX_ENTRY call per assigned
>>>>> device?
>>>>>             
>>>> Sure, but only one KVM_ASSIGN_SET_MSIX_NR.
>>>>
>>>>         
>>> MSIX_NR is the size of the table, while MSIX_ENTRY updates a single   
>>> entry, if I read the code correctly.
>>>     
>>
>> Right. So we'll need something like this for MSI as well.
>> Actually maybe MSIX_NR MSIX_ENTRY should be renamed to MSI_NR / MSI_ENTRY
>> and changed to do the right thing depending on the IRQ type?
>>   
>
> Works for me.  Sheng, is there a reason why it wasn't done like this?
>
> btw, it could be further simplified by using irqfd.  Instead of the host  
> device tying directly into kvm, it could just trigger an eventfd; and we  
> could terminate the eventfd either in kvm (irqfd) or in qemu.

If you are going wild, you could then split this code out from kvm
into something like a UIO driver. E.g. qemu could then in theory
support assigned devices even without VT-d hardware support in CPU.

-- 
MST

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Avi Kivity]

Michael S. Tsirkin wrote:
>> Works for me.  Sheng, is there a reason why it wasn't done like this?
>>
>> btw, it could be further simplified by using irqfd.  Instead of the host  
>> device tying directly into kvm, it could just trigger an eventfd; and we  
>> could terminate the eventfd either in kvm (irqfd) or in qemu.
>>     
>
> If you are going wild, you could then split this code out from kvm
> into something like a UIO driver. E.g. qemu could then in theory
> support assigned devices even without VT-d hardware support in CPU.
>   

That's my thinking.  PCI interrupts don't work because we need to do 
some hacky stuff in there, but MSI should.  Oh, and we could improve UIO 
support for interrupts when using MSI, since there's no need to 
acknowledge the interrupt.

Support we can tell the kernel to signal an eventfd whenever an MSI 
fires.  We then ask kvm for an irqfd, and give that irqfd to the kernel 
for the MSI.

Voila, we assign an interrupt from userspace, without the device or kvm 
knowing anything about it.  Like you say, we can assign the device to 
pure qemu, or to a userspace driver.

Beautiful, I finally found something to replace my old Lego set.

-- 
error compiling committee.c: too many arguments to function

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Sheng Yang]

On Tuesday 05 May 2009 20:08:40 Avi Kivity wrote:
> Michael S. Tsirkin wrote:
> > On Tue, May 05, 2009 at 02:57:10PM +0300, Avi Kivity wrote:
> >> Michael S. Tsirkin wrote:
> >>> On Tue, May 05, 2009 at 08:04:15AM -0300, Marcelo Tosatti wrote:
> >>>> On Tue, May 05, 2009 at 01:30:28PM +0300, Michael S. Tsirkin wrote:
> >>>>> The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls
> >>>>> have been merged for 2.6.30. However, I note that PCI spec allows
> >>>>> devices to support multiple vectors with MSI as well (support will be
> >>>>> in linux 2.6.30).

Well, one question: when did them merged? IIRC, MSI-X related things are still 
pending for 2.6.31... :)

> >>>>>
> >>>>> Even though qemu for now only uses a single vector with MSI, it would
> >>>>> seem that it's better to make the kernel/user interface generic
> >>>>> straight away rather than add more ioctls later. What do you think?
> >>>>> It might not be too late to fix this for 2.6.30.
> >>>>
> >>>> Can't you use more than one KVM_ASSIGN_SET_MSIX_ENTRY call per
> >>>> assigned device?
> >>>
> >>> Sure, but only one KVM_ASSIGN_SET_MSIX_NR.
> >>
> >> MSIX_NR is the size of the table, while MSIX_ENTRY updates a single
> >> entry, if I read the code correctly.
> >
> > Right. So we'll need something like this for MSI as well.
> > Actually maybe MSIX_NR MSIX_ENTRY should be renamed to MSI_NR / MSI_ENTRY
> > and changed to do the right thing depending on the IRQ type?
>
> Works for me.  Sheng, is there a reason why it wasn't done like this?

No, I think it's fine. Also some related structure should be modified. And one 
flag field should be add to kvm_assigned_msix_nr and 
kvm_assigned_msix_entry(using padding ones) to indicate the interrupt type, 
for we can't determined the irq type by device's status at that time.

-- 
regards
Yang, Sheng

Re: KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY should support MSI?

[Sheng Yang]

On Tuesday 05 May 2009 19:29:14 Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > The new KVM_ASSIGN_SET_MSIX_NR and KVM_ASSIGN_SET_MSIX_ENTRY ioctls have
> > been merged for 2.6.30. However, I note that PCI spec allows devices to
> > support multiple vectors with MSI as well (support will be in linux
> > 2.6.30).
> >
> > Even though qemu for now only uses a single vector with MSI, it would
> > seem that it's better to make the kernel/user interface generic straight
> > away rather than add more ioctls later. What do you think? It might not
> > be too late to fix this for 2.6.30.
>
> +1

Hi

Sorry for I don't have time to work at it recently... And Avi's review 
patchset reminded me the merge for 2.6.31 would be soon. Anyone have 
interesting to get this done?

Thanks!

-- 
regards
Yang, Sheng