diff mbox

[v5] kvm: Use a bitmap for tracking used GSIs

Message ID 20090513150815.8221.49221.stgit@dl380g6-3.ned.telco.ned.telco (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Williamson May 13, 2009, 3:13 p.m. UTC
We're currently using a counter to track the most recent GSI we've
handed out.  This quickly hits KVM_MAX_IRQ_ROUTES when using device
assignment with a driver that regularly toggles the MSI enable bit.
This can mean only a few minutes of usable run time.  Instead, track
used GSIs in a bitmap.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 v2: Added mutex to protect gsi bitmap
 v3: Updated for comments from Michael Tsirkin
     No longer depends on "[PATCH] kvm: device-assignment: Catch GSI overflow"
 v4: Fix gsi_bytes calculation noted by Sheng Yang
 v5: Remove mutex per Avi
     Fix negative gsi_count path per Michael
     Remove KVM_CAP_IRQ_ROUTING per Michael, ppc should still be protected
         by the KVM_IOAPIC_NUM_PINS check

 hw/device-assignment.c  |    4 ++-
 kvm/libkvm/kvm-common.h |    3 +-
 kvm/libkvm/libkvm.c     |   74 ++++++++++++++++++++++++++++++++++++++---------
 kvm/libkvm/libkvm.h     |   10 ++++++
 4 files changed, 75 insertions(+), 16 deletions(-)


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

Comments

Michael S. Tsirkin May 13, 2009, 4:05 p.m. UTC | #1
On Wed, May 13, 2009 at 09:13:38AM -0600, Alex Williamson wrote:
> @@ -323,6 +326,28 @@ kvm_context_t kvm_init(struct kvm_callbacks *callbacks,
>  	kvm->no_irqchip_creation = 0;
>  	kvm->no_pit_creation = 0;
>  
> +	gsi_count = kvm_get_gsi_count(kvm);
> +	if (gsi_count > 0) {
> +		int gsi_bytes, i;
> +
> +		/* Round up so we can search ints using ffs */
> +		gsi_bytes = ((gsi_count + 31) / 32) * 4;

Let's take ALIGN macro from linux/kernel.h?

> +		kvm->used_gsi_bitmap = malloc(gsi_bytes);
> +		if (!kvm->used_gsi_bitmap)
> +			goto out_close;
> +		memset(kvm->used_gsi_bitmap, 0, gsi_bytes);
> +		kvm->max_gsi = gsi_bytes * 8;
> +
> +		/* Mark all the IOAPIC pin GSIs and any over-allocated
> +	 	* GSIs as already in use. */

Align '*'s please.

> +#ifdef KVM_IOAPIC_NUM_PINS

I think we should just export
#define KVM_IOAPIC_NUM_PINS 0
for ppc in kernel headers (or in libkvm),
and get rid of this ifdef completely.

Avi, agree?

> +		for (i = 0; i < min(KVM_IOAPIC_NUM_PINS, gsi_count); i++)
> +			set_bit(kvm->used_gsi_bitmap, i);
> +#endif
> +		for (i = gsi_count; i < kvm->max_gsi; i++)
> +			set_bit(kvm->used_gsi_bitmap, i);
> +	}
> +
>  	return kvm;
>   out_close:
>  	close(fd);
Alex Williamson May 13, 2009, 5:13 p.m. UTC | #2
On Wed, 2009-05-13 at 19:05 +0300, Michael S. Tsirkin wrote:
> On Wed, May 13, 2009 at 09:13:38AM -0600, Alex Williamson wrote:
> > @@ -323,6 +326,28 @@ kvm_context_t kvm_init(struct kvm_callbacks *callbacks,
> >  	kvm->no_irqchip_creation = 0;
> >  	kvm->no_pit_creation = 0;
> >  
> > +	gsi_count = kvm_get_gsi_count(kvm);
> > +	if (gsi_count > 0) {
> > +		int gsi_bytes, i;
> > +
> > +		/* Round up so we can search ints using ffs */
> > +		gsi_bytes = ((gsi_count + 31) / 32) * 4;
> 
> Let's take ALIGN macro from linux/kernel.h?

It's already defined in libkvm.c, I'll just move it up in the file.
There's also a BITMAP_SIZE macro by it that looks like it can be nuked.

> > +		kvm->used_gsi_bitmap = malloc(gsi_bytes);
> > +		if (!kvm->used_gsi_bitmap)
> > +			goto out_close;
> > +		memset(kvm->used_gsi_bitmap, 0, gsi_bytes);
> > +		kvm->max_gsi = gsi_bytes * 8;
> > +
> > +		/* Mark all the IOAPIC pin GSIs and any over-allocated
> > +	 	* GSIs as already in use. */
> 
> Align '*'s please.

Argh, fixed.

> > +#ifdef KVM_IOAPIC_NUM_PINS
> 
> I think we should just export
> #define KVM_IOAPIC_NUM_PINS 0
> for ppc in kernel headers (or in libkvm),
> and get rid of this ifdef completely.

Ok, I'll add an #ifndef and make it zero in libkvm.c.  It can be cleaned
out further from there.  Thanks,

Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity May 17, 2009, 8:51 p.m. UTC | #3
Michael S. Tsirkin wrote:
>
>> +#ifdef KVM_IOAPIC_NUM_PINS
>>     
>
> I think we should just export
> #define KVM_IOAPIC_NUM_PINS 0
> for ppc in kernel headers (or in libkvm),
> and get rid of this ifdef completely.
>
> Avi, agree?
>
>   

ppc doesn't have an ioapic, so it shouldn't have this define.
diff mbox

Patch

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index a7365c8..a6cc9b9 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -561,8 +561,10 @@  static void free_dev_irq_entries(AssignedDevice *dev)
 {
     int i;
 
-    for (i = 0; i < dev->irq_entries_nr; i++)
+    for (i = 0; i < dev->irq_entries_nr; i++) {
         kvm_del_routing_entry(kvm_context, &dev->entry[i]);
+        kvm_free_irq_route_gsi(kvm_context, dev->entry[i].gsi);
+    }
     free(dev->entry);
     dev->entry = NULL;
     dev->irq_entries_nr = 0;
diff --git a/kvm/libkvm/kvm-common.h b/kvm/libkvm/kvm-common.h
index 591fb53..c95c591 100644
--- a/kvm/libkvm/kvm-common.h
+++ b/kvm/libkvm/kvm-common.h
@@ -67,7 +67,8 @@  struct kvm_context {
 	struct kvm_irq_routing *irq_routes;
 	int nr_allocated_irq_routes;
 #endif
-	int max_used_gsi;
+	void *used_gsi_bitmap;
+	int max_gsi;
 };
 
 int kvm_alloc_kernel_memory(kvm_context_t kvm, unsigned long memory,
diff --git a/kvm/libkvm/libkvm.c b/kvm/libkvm/libkvm.c
index ba0a5d1..74fb59b 100644
--- a/kvm/libkvm/libkvm.c
+++ b/kvm/libkvm/libkvm.c
@@ -61,10 +61,13 @@ 
 #define DPRINTF(fmt, args...) do {} while (0)
 #endif
 
+#define min(x,y) ((x) < (y) ? (x) : (y))
 
 int kvm_abi = EXPECTED_KVM_API_VERSION;
 int kvm_page_size;
 
+static inline void set_bit(uint32_t *buf, unsigned int bit);
+
 struct slot_info {
 	unsigned long phys_addr;
 	unsigned long len;
@@ -285,7 +288,7 @@  kvm_context_t kvm_init(struct kvm_callbacks *callbacks,
 {
 	int fd;
 	kvm_context_t kvm;
-	int r;
+	int r, gsi_count;
 
 	fd = open("/dev/kvm", O_RDWR);
 	if (fd == -1) {
@@ -323,6 +326,28 @@  kvm_context_t kvm_init(struct kvm_callbacks *callbacks,
 	kvm->no_irqchip_creation = 0;
 	kvm->no_pit_creation = 0;
 
+	gsi_count = kvm_get_gsi_count(kvm);
+	if (gsi_count > 0) {
+		int gsi_bytes, i;
+
+		/* Round up so we can search ints using ffs */
+		gsi_bytes = ((gsi_count + 31) / 32) * 4;
+		kvm->used_gsi_bitmap = malloc(gsi_bytes);
+		if (!kvm->used_gsi_bitmap)
+			goto out_close;
+		memset(kvm->used_gsi_bitmap, 0, gsi_bytes);
+		kvm->max_gsi = gsi_bytes * 8;
+
+		/* Mark all the IOAPIC pin GSIs and any over-allocated
+	 	* GSIs as already in use. */
+#ifdef KVM_IOAPIC_NUM_PINS
+		for (i = 0; i < min(KVM_IOAPIC_NUM_PINS, gsi_count); i++)
+			set_bit(kvm->used_gsi_bitmap, i);
+#endif
+		for (i = gsi_count; i < kvm->max_gsi; i++)
+			set_bit(kvm->used_gsi_bitmap, i);
+	}
+
 	return kvm;
  out_close:
 	close(fd);
@@ -1298,8 +1323,6 @@  int kvm_add_routing_entry(kvm_context_t kvm,
 	new->flags = entry->flags;
 	new->u = entry->u;
 
-	if (entry->gsi > kvm->max_used_gsi)
-		kvm->max_used_gsi = entry->gsi;
 	return 0;
 #else
 	return -ENOSYS;
@@ -1404,19 +1427,42 @@  int kvm_commit_irq_routes(kvm_context_t kvm)
 #endif
 }
 
+static inline void set_bit(uint32_t *buf, unsigned int bit)
+{
+	buf[bit / 32] |= 1U << (bit % 32);
+}
+
+static inline void clear_bit(uint32_t *buf, unsigned int bit)
+{
+	buf[bit / 32] &= ~(1U << (bit % 32));
+}
+
+static int kvm_find_free_gsi(kvm_context_t kvm)
+{
+	int i, bit, gsi;
+	uint32_t *buf = kvm->used_gsi_bitmap;
+
+	for (i = 0; i < kvm->max_gsi / 32; i++) {
+		bit = ffs(~buf[i]);
+		if (!bit)
+			continue;
+
+		gsi = bit - 1 + i * 32;
+		set_bit(buf, gsi);
+		return gsi;
+	}
+
+	return -ENOSPC;
+}
+
 int kvm_get_irq_route_gsi(kvm_context_t kvm)
 {
-#ifdef KVM_CAP_IRQ_ROUTING
-	if (kvm->max_used_gsi >= KVM_IOAPIC_NUM_PINS)  {
-	    if (kvm->max_used_gsi <= kvm_get_gsi_count(kvm))
-                return kvm->max_used_gsi + 1;
-            else
-                return -ENOSPC;
-        } else
-            return KVM_IOAPIC_NUM_PINS;
-#else
-	return -ENOSYS;
-#endif
+	return kvm_find_free_gsi(kvm);
+}
+ 
+void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi)
+{
+	clear_bit(kvm->used_gsi_bitmap, gsi);
 }
 
 #ifdef KVM_CAP_DEVICE_MSIX
diff --git a/kvm/libkvm/libkvm.h b/kvm/libkvm/libkvm.h
index 4821a1e..845bb8a 100644
--- a/kvm/libkvm/libkvm.h
+++ b/kvm/libkvm/libkvm.h
@@ -856,6 +856,16 @@  int kvm_commit_irq_routes(kvm_context_t kvm);
  */
 int kvm_get_irq_route_gsi(kvm_context_t kvm);
 
+/*!
+ * \brief Free used GSI number
+ *
+ * Free used GSI number acquired from kvm_get_irq_route_gsi()
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \param gsi GSI number to free
+ */
+void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi);
+
 #ifdef KVM_CAP_DEVICE_MSIX
 int kvm_assign_set_msix_nr(kvm_context_t kvm,
 			   struct kvm_assigned_msix_nr *msix_nr);