Re: [PATCH 09/10] KVM: Enable MSI for device assignment

[Avi Kivity]

Sheng Yang wrote:

We enable guest MSI and host MSI support in this patch. The userspace want to
enable MSI should set KVM_DEV_IRQ_ASSIGN_ENABLE_MSI in the assigned_irq's
flag. Function would return -ENOTTY if can't enable MSI, userspace shouldn't
set MSI Enable bit when KVM_ASSIGN_IRQ return -ENOTTY with
KVM_DEV_IRQ_ASSIGN_ENABLE_MSI.
  


Need a KVM_CAP_MSI so that userspace can tell before trying.


+   if (assigned_dev-irq_requested_type  KVM_ASSIGNED_DEV_GUEST_INTX)
+   kvm_set_irq(assigned_dev-kvm,
+   assigned_dev-irq_source_id,
+   assigned_dev-guest_irq, 1);
+   else if (assigned_dev-irq_requested_type 
+   KVM_ASSIGNED_DEV_GUEST_MSI) {
+   assigned_device_msi_dispatch(assigned_dev);
+   enable_irq(assigned_dev-host_irq);
+   }
  


Hmm.  This means the only path that can trigger msi is device 
assignment.  But we also want virtio-pci to use msi.


How about modifying kvm_set_irq() to dispatch either msi or normal 
irqs?  We'd need a 'struct kvm_guest_irq' if we can't define the msi 
using the bits available in gsi (and also define a new KVM_IRQ_LINE... I 
really hope we can fit the msi info in gsi).


The device-assignment-independent parts should be in a separate patch.

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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] KVM: Enable MSI for device assignment

[Sheng Yang]

On Tue, Nov 04, 2008 at 04:35:19PM +0200, Avi Kivity wrote:
 Sheng Yang wrote:
 In fact, it's not just for pci.  We could msi-enable e1000 and get   
 improved performance there as well.
 

 E1000? Don't understand... Sounds like INTx-MSI...
   

 There are msi-capable e1000 cards.  If we upgrade qemu's e1000 to  
 support msi, and if the guest supports msi, it will be msi all the way.

Oh, forgot we got e1000 emulation in qemu...

 But I guess what you means is only set
 gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
 be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
 list, for gsi = IOAPIC_PINS,
   
 Let's say, (gsi  24) == 1.  That gives us 16M potential MSIs.
 

 Confused... GSI become a bitmap?
   

 A structure...

 struct gsi {
u32 n : 24;
u32 type : 8; // 0 - classic gsi, 1 - msi
 };

I just don't understand why we need 24 bits for n? GSI can be that wide?

--
regards
Yang, Sheng


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

 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to [EMAIL PROTECTED]
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] KVM: Enable MSI for device assignment

[Avi Kivity]

Sheng Yang wrote:

One question here: did the architectures other than X86 and IA64 need
dispatch MSI support?  


s390 doesn't.  ppc may, one day in the future.  Not now I think.


If so, I am afraid reuse assigned_device_msi_dispatch
(or similar solution) won't be elegant (though I already lean toward a arch
specific dispatch function).

I am not familiar with virtio-pci and how it works, it would be good if
it can be illustrated a little more. 


For this context, it's just a pci device.  qemu calls KVM_IRQ_LINE to 
inject an irq, and the pci layer in the guest delivers the irq to the 
virtio-pci driver.


In fact, it's not just for pci.  We could msi-enable e1000 and get 
improved performance there as well.



But I guess what you means is only set
gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
list, for gsi = IOAPIC_PINS,

Let's say, (gsi  24) == 1.  That gives us 16M potential MSIs.


 kvm_set_irq() checked the list and deliver the
interrupt. Of course, some lock should be taken to maintain the list, maybe
kvm-lock in first step as we used for kvm_set_irq (kvm-lock become more
and more bigger...).
  


I think kvm_set_irq() already takes kvm-lock.

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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] KVM: Enable MSI for device assignment

[Sheng Yang]

On Tue, Nov 04, 2008 at 03:32:49PM +0200, Avi Kivity wrote:
 Sheng Yang wrote:
 One question here: did the architectures other than X86 and IA64 need
 dispatch MSI support?  

 s390 doesn't.  ppc may, one day in the future.  Not now I think.

OK, so reuse is enough for today.

 If so, I am afraid reuse assigned_device_msi_dispatch
 (or similar solution) won't be elegant (though I already lean toward a arch
 specific dispatch function).

 I am not familiar with virtio-pci and how it works, it would be good if
 it can be illustrated a little more. 

 For this context, it's just a pci device.  qemu calls KVM_IRQ_LINE to  
 inject an irq, and the pci layer in the guest delivers the irq to the  
 virtio-pci driver.

Got it.

 In fact, it's not just for pci.  We could msi-enable e1000 and get  
 improved performance there as well.

E1000? Don't understand... Sounds like INTx-MSI...

 But I guess what you means is only set
 gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
 be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
 list, for gsi = IOAPIC_PINS,
 Let's say, (gsi  24) == 1.  That gives us 16M potential MSIs.

Confused... GSI become a bitmap?

  kvm_set_irq() checked the list and deliver the
 interrupt. Of course, some lock should be taken to maintain the list, maybe
 kvm-lock in first step as we used for kvm_set_irq (kvm-lock become more
 and more bigger...).
   

 I think kvm_set_irq() already takes kvm-lock.

Yes. So I means maybe we can improve the granularity of the lock in interrupt
handling part in the future.

--
regards
Yang, Sheng

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

 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to [EMAIL PROTECTED]
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] KVM: Enable MSI for device assignment

[Avi Kivity]

Sheng Yang wrote:
In fact, it's not just for pci.  We could msi-enable e1000 and get  
improved performance there as well.



E1000? Don't understand... Sounds like INTx-MSI...
  


There are msi-capable e1000 cards.  If we upgrade qemu's e1000 to 
support msi, and if the guest supports msi, it will be msi all the way.



But I guess what you means is only set
gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
list, for gsi = IOAPIC_PINS,
  

Let's say, (gsi  24) == 1.  That gives us 16M potential MSIs.



Confused... GSI become a bitmap?
  


A structure...

struct gsi {
   u32 n : 24;
   u32 type : 8; // 0 - classic gsi, 1 - msi
};


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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] KVM: Enable MSI for device assignment

[Sheng Yang]

On Tue, Nov 04, 2008 at 01:23:04PM +0200, Avi Kivity wrote:
 Sheng Yang wrote:
 We enable guest MSI and host MSI support in this patch. The userspace want to
 enable MSI should set KVM_DEV_IRQ_ASSIGN_ENABLE_MSI in the assigned_irq's
 flag. Function would return -ENOTTY if can't enable MSI, userspace shouldn't
 set MSI Enable bit when KVM_ASSIGN_IRQ return -ENOTTY with
 KVM_DEV_IRQ_ASSIGN_ENABLE_MSI.
   

 Need a KVM_CAP_MSI so that userspace can tell before trying.

 +if (assigned_dev-irq_requested_type  KVM_ASSIGNED_DEV_GUEST_INTX)
 +kvm_set_irq(assigned_dev-kvm,
 +assigned_dev-irq_source_id,
 +assigned_dev-guest_irq, 1);
 +else if (assigned_dev-irq_requested_type 
 +KVM_ASSIGNED_DEV_GUEST_MSI) {
 +assigned_device_msi_dispatch(assigned_dev);
 +enable_irq(assigned_dev-host_irq);
 +}
   

 Hmm.  This means the only path that can trigger msi is device  
 assignment.  But we also want virtio-pci to use msi.

 How about modifying kvm_set_irq() to dispatch either msi or normal irqs?  
 We'd need a 'struct kvm_guest_irq' if we can't define the msi using the 
 bits available in gsi (and also define a new KVM_IRQ_LINE... I really 
 hope we can fit the msi info in gsi).

 The device-assignment-independent parts should be in a separate patch.

One question here: did the architectures other than X86 and IA64 need
dispatch MSI support?  If so, I am afraid reuse assigned_device_msi_dispatch
(or similar solution) won't be elegant (though I already lean toward a arch
specific dispatch function).

I am not familiar with virtio-pci and how it works, it would be good if
it can be illustrated a little more. But I guess what you means is only set
gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
list, for gsi = IOAPIC_PINS, kvm_set_irq() checked the list and deliver the
interrupt. Of course, some lock should be taken to maintain the list, maybe
kvm-lock in first step as we used for kvm_set_irq (kvm-lock become more
and more bigger...).

--
regards
Yang, Sheng

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

 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to [EMAIL PROTECTED]
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/10] KVM: Enable MSI for device assignment

[Sheng Yang]

Signed-off-by: Sheng Yang [EMAIL PROTECTED]
---
 virt/kvm/kvm_main.c |   74 ++
 1 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index be0f943..0f851ef 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -149,9 +149,15 @@ static void kvm_assigned_dev_interrupt_work_handler(struct 
work_struct *work)
 * finer-grained lock, update this
 */
mutex_lock(assigned_dev-kvm-lock);
-   kvm_set_irq(assigned_dev-kvm,
-   assigned_dev-irq_source_id,
-   assigned_dev-guest_irq, 1);
+   if (assigned_dev-irq_requested_type  KVM_ASSIGNED_DEV_GUEST_INTX)
+   kvm_set_irq(assigned_dev-kvm,
+   assigned_dev-irq_source_id,
+   assigned_dev-guest_irq, 1);
+   else if (assigned_dev-irq_requested_type 
+   KVM_ASSIGNED_DEV_GUEST_MSI) {
+   assigned_device_msi_dispatch(assigned_dev);
+   enable_irq(assigned_dev-host_irq);
+   }
mutex_unlock(assigned_dev-kvm-lock);
kvm_put_kvm(assigned_dev-kvm);
 }
@@ -187,6 +193,8 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 {
if (irqchip_in_kernel(kvm)  assigned_dev-irq_requested_type)
free_irq(assigned_dev-host_irq, (void *)assigned_dev);
+   if (assigned_dev-irq_requested_type  KVM_ASSIGNED_DEV_HOST_MSI)
+   pci_disable_msi(assigned_dev-dev);
 
kvm_unregister_irq_ack_notifier(kvm, assigned_dev-ack_notifier);
kvm_free_irq_source_id(kvm, assigned_dev-irq_source_id);
@@ -232,6 +240,11 @@ static int assigned_device_update_intx(struct kvm *kvm,
return 0;
 
if (irqchip_in_kernel(kvm)) {
+   if (adev-irq_requested_type  KVM_ASSIGNED_DEV_HOST_MSI) {
+   free_irq(adev-host_irq, (void *)kvm);
+   pci_disable_msi(adev-dev);
+   }
+
if (!capable(CAP_SYS_RAWIO))
return -EPERM;
 
@@ -255,6 +268,38 @@ static int assigned_device_update_intx(struct kvm *kvm,
return 0;
 }
 
+static int assigned_device_update_msi(struct kvm *kvm,
+   struct kvm_assigned_dev_kernel *adev,
+   struct kvm_assigned_irq *airq)
+{
+   int r;
+
+   adev-guest_msi_addr = airq-guest_msi_addr;
+   adev-guest_msi_data = airq-guest_msi_data;
+   adev-ack_notifier.gsi = -1;
+
+   if (adev-irq_requested_type  KVM_ASSIGNED_DEV_HOST_MSI)
+   return 0;
+
+   if (irqchip_in_kernel(kvm)) {
+   if (adev-irq_requested_type  KVM_ASSIGNED_DEV_HOST_INTX)
+   free_irq(adev-host_irq, (void *)adev);
+
+   r = pci_enable_msi(adev-dev);
+   if (r)
+   return r;
+
+   adev-host_irq = adev-dev-irq;
+   if (request_irq(adev-host_irq, kvm_assigned_dev_intr, 0,
+   kvm_assigned_msi_device, (void *)adev))
+   return -EIO;
+   }
+
+   adev-irq_requested_type = KVM_ASSIGNED_DEV_GUEST_MSI |
+  KVM_ASSIGNED_DEV_HOST_MSI;
+   return 0;
+}
+
 static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
   struct kvm_assigned_irq
   *assigned_irq)
@@ -291,9 +336,26 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
}
}
 
-   r = assigned_device_update_intx(kvm, match, assigned_irq);
-   if (r)
-   goto out_release;
+   if (assigned_irq-flags  KVM_DEV_IRQ_ASSIGN_ENABLE_MSI) {
+   r = assigned_device_update_msi(kvm, match, assigned_irq);
+   if (r) {
+   printk(KERN_WARNING kvm: failed to enable 
+   MSI device!\n);
+   goto out_release;
+   }
+   } else if (assigned_irq-host_irq == 0  match-dev-irq == 0) {
+   /* Host device IRQ 0 means don't support INTx */
+   printk(KERN_WARNING kvm: wait device to enable MSI!\n);
+   r = 0;
+   } else {
+   /* Non-sharing INTx mode */
+   r = assigned_device_update_intx(kvm, match, assigned_irq);
+   if (r) {
+   printk(KERN_WARNING kvm: failed to enable 
+   INTx device!\n);
+   goto out_release;
+   }
+   }
 
mutex_unlock(kvm-lock);
return r;
-- 
1.5.4.5

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html