qemu kvm: correct gsi bitmap

PATCH In review Jason Baron 3 participants 8 messages Mar 27, 2012 KVM

lore

Share & tools

lore

Jason Baron

author

2012-03-27 21:00 UTC #0 lore raw

cover qemu kvm: correct gsi bitmap

Hi,

While testing pci passthrough, I ran out of gsi bits, b/c the bitmap is not initialized correctly. Fix the initialization of the bitmap, and add extra error reporting for this condition.

Thanks,

-Jason

Jason Baron (2):
qemu kvm: Set up gsi bitmap correctly
qemu kvm: add better error reporting when kvm_get_irq_route_gsi() fails

 hw/device-assignment.c |    4 +++-
 kvm-all.c              |    4 ++--
 qemu-kvm.c             |    2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

Jason Baron

author

2012-03-27 21:00 UTC #1 lore raw

PATCH 1/2 qemu kvm: Set up gsi bitmap correctly

The current 'kvm_init_irq_routing()' doesn't set up the gsi bitmap correctly, and as a consequence pins max_gsi to 32 when it really should be 1024. I ran into this limitation while testing pci passthrough, where I consistently would get -ENOSPACE return from kvm_get_irq_route_gsi() in assigned_dev_update_msix_mmio().

Signed-off-by Jason Baron <[email protected]>

---
 kvm-all.c  |    4 ++--
 qemu-kvm.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

kvm-all.c +2 -2

index ab88c7c..7d602af 100644
@@ -873,9 +873,9 @@ static void kvm_init_irq_routing(KVMState *s)
         unsigned int gsi_bits, i;
 
         /* Round up so we can search ints using ffs */
-        gsi_bits = (gsi_count + 31) / 32;
+        gsi_bits = ALIGN(gsi_count, 32);
         s->used_gsi_bitmap = g_malloc0(gsi_bits / 8);
-        s->max_gsi = gsi_bits;
+        s->max_gsi = gsi_count;
 
         /* Mark any over-allocated bits as already in use */
         for (i = gsi_count; i < gsi_bits; i++) {

qemu-kvm.c +1 -1

index 2047ebb..b17cae0 100644
@@ -249,7 +249,7 @@ int kvm_get_irq_route_gsi(void)
     uint32_t *buf = s->used_gsi_bitmap;
 
     /* Return the lowest unused GSI in the bitmap */
-    for (i = 0; i < s->max_gsi / 32; i++) {
+    for (i = 0; i < (ALIGN(s->max_gsi, 32) / 32); i++) {
         bit = ffs(~buf[i]);
         if (!bit) {
             continue;

Signature · 1 line

1.7.1

Alex Williamson

2012-03-27 21:31 UTC #2 lore raw

Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly

Quoted 17 lines · Jason Baron

The current 'kvm_init_irq_routing()' doesn't set up the gsi bitmap correctly, and as a consequence pins max_gsi to 32 when it really should be 1024. I ran into this limitation while testing pci passthrough, where I consistently would get -ENOSPACE return from kvm_get_irq_route_gsi() in assigned_dev_update_msix_mmio().
Signed-off-by Jason Baron <[email protected]>
---
 kvm-all.c  |    4 ++--
 qemu-kvm.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
kvm-all.c +0 -0
index ab88c7c..7d602af 100644

kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 877

Quoted 6 lines

kvm-all.c +1 -1

@@ -873,9 +873,9 @@ static void kvm_init_irq_routing(KVMState *s)
         unsigned int gsi_bits, i;
 
         /* Round up so we can search ints using ffs */
-        gsi_bits = (gsi_count + 31) / 32;
+        gsi_bits = ALIGN(gsi_count, 32);
         s->used_gsi_bitmap = g_malloc0(gsi_bits / 8);

I think the above is all that's needed (it actually used to be this, then got broken in 84b058d). But if we do this:

kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 878

Quoted 2 lines

kvm-all.c +1 -1

-        s->max_gsi = gsi_bits;
+        s->max_gsi = gsi_count;

Then we'll hit this assert from the code immediately below where we're marking over-allocated bits as already used if we actually did a round-up:

static void set_gsi(KVMState *s, unsigned int gsi)

{
    assert(gsi < s->max_gsi);

Sorry, I had forgotten about this pre-allocation trick to avoid returning > gsi_count when we talked about this.

qemu-kvm.c int kvm_get_irq_route_gsi(void) · line 251

Quoted 11 lines

kvm-all.c +0 -0

         /* Mark any over-allocated bits as already in use */
         for (i = gsi_count; i < gsi_bits; i++) {

qemu-kvm.c +0 -0

index 2047ebb..b17cae0 100644
@@ -249,7 +249,7 @@ int kvm_get_irq_route_gsi(void)
     uint32_t *buf = s->used_gsi_bitmap;
 
     /* Return the lowest unused GSI in the bitmap */

And we get to avoid doing this ALIGN on every search.

qemu-kvm.c int kvm_get_irq_route_gsi(void) · line 255

Quoted 5 lines

qemu-kvm.c +1 -1

-    for (i = 0; i < s->max_gsi / 32; i++) {
+    for (i = 0; i < (ALIGN(s->max_gsi, 32) / 32); i++) {
         bit = ffs(~buf[i]);
         if (!bit) {
             continue;

Thanks,
Alex

Jan Kiszka

2012-03-27 22:01 UTC #3 lore raw

Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly

Quoted 35 lines · Alex Williamson

Quoted 17 lines · Jason Baron
The current 'kvm_init_irq_routing()' doesn't set up the gsi bitmap correctly, and as a consequence pins max_gsi to 32 when it really should be 1024. I ran into this limitation while testing pci passthrough, where I consistently would get -ENOSPACE return from kvm_get_irq_route_gsi() in assigned_dev_update_msix_mmio().
Signed-off-by Jason Baron <[email protected]>
---
 kvm-all.c  |    4 ++--
 qemu-kvm.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
kvm-all.c +0 -0
index ab88c7c..7d602af 100644
kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 877
Quoted 6 lines
kvm-all.c +1 -1
@@ -873,9 +873,9 @@ static void kvm_init_irq_routing(KVMState *s)
         unsigned int gsi_bits, i;
 
         /* Round up so we can search ints using ffs */
-        gsi_bits = (gsi_count + 31) / 32;
+        gsi_bits = ALIGN(gsi_count, 32);
         s->used_gsi_bitmap = g_malloc0(gsi_bits / 8);
I think the above is all that's needed (it actually used to be this, then got broken in 84b058d). But if we do this:
kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 878
Quoted 2 lines
kvm-all.c +1 -1
-        s->max_gsi = gsi_bits;
+        s->max_gsi = gsi_count;
Then we'll hit this assert from the code immediately below where we're marking over-allocated bits as already used if we actually did a round-up:

Quoted 6 lines

static void set_gsi(KVMState *s, unsigned int gsi)
{
    assert(gsi < s->max_gsi);
Sorry, I had forgotten about this pre-allocation trick to avoid returning > gsi_count when we talked about this.

Oh, indeed. That's slightly ugly, gsi_max remains misnamed.

Let's just drop the overeager asserts and keep the number of bitmap words in KVMState. That's what we really need for doing the work.

Jan

Alex Williamson

2012-03-27 22:25 UTC #4 lore raw

Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly

Quoted 40 lines · Jan Kiszka

Quoted 39 lines · Alex Williamson
Quoted 17 lines · Jason Baron
The current 'kvm_init_irq_routing()' doesn't set up the gsi bitmap correctly, and as a consequence pins max_gsi to 32 when it really should be 1024. I ran into this limitation while testing pci passthrough, where I consistently would get -ENOSPACE return from kvm_get_irq_route_gsi() in assigned_dev_update_msix_mmio().
Signed-off-by Jason Baron <[email protected]>
---
 kvm-all.c  |    4 ++--
 qemu-kvm.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
kvm-all.c +0 -0
index ab88c7c..7d602af 100644
kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 877
Quoted 6 lines
kvm-all.c +1 -1
@@ -873,9 +873,9 @@ static void kvm_init_irq_routing(KVMState *s)
         unsigned int gsi_bits, i;
 
         /* Round up so we can search ints using ffs */
-        gsi_bits = (gsi_count + 31) / 32;
+        gsi_bits = ALIGN(gsi_count, 32);
         s->used_gsi_bitmap = g_malloc0(gsi_bits / 8);
I think the above is all that's needed (it actually used to be this, then got broken in 84b058d). But if we do this:
kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 878
Quoted 2 lines
kvm-all.c +1 -1
-        s->max_gsi = gsi_bits;
+        s->max_gsi = gsi_count;
Then we'll hit this assert from the code immediately below where we're marking over-allocated bits as already used if we actually did a round-up:
static void set_gsi(KVMState *s, unsigned int gsi)
{
    assert(gsi < s->max_gsi);

Quoted 7 lines

Quoted 2 lines
Sorry, I had forgotten about this pre-allocation trick to avoid returning > gsi_count when we talked about this.
Oh, indeed. That's slightly ugly, gsi_max remains misnamed.
Let's just drop the overeager asserts and keep the number of bitmap words in KVMState. That's what we really need for doing the work.

We could solve that by just renaming it to s->gsi_count and keep the sanity test, but let's fix the bug first. I'm pretty sure the compiler is going to do something smart enough with the loop control that it doesn't matter if we save the number of words or the number of bits. Thanks,

Alex

Jan Kiszka

2012-03-27 22:32 UTC #5 lore raw

Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly

Quoted 46 lines · Alex Williamson

Quoted 45 lines · Jan Kiszka
Quoted 35 lines · Alex Williamson
Quoted 17 lines · Jason Baron
The current 'kvm_init_irq_routing()' doesn't set up the gsi bitmap correctly, and as a consequence pins max_gsi to 32 when it really should be 1024. I ran into this limitation while testing pci passthrough, where I consistently would get -ENOSPACE return from kvm_get_irq_route_gsi() in assigned_dev_update_msix_mmio().
Signed-off-by Jason Baron <[email protected]>
---
 kvm-all.c  |    4 ++--
 qemu-kvm.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
kvm-all.c +0 -0
index ab88c7c..7d602af 100644
kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 877
Quoted 6 lines
kvm-all.c +1 -1
@@ -873,9 +873,9 @@ static void kvm_init_irq_routing(KVMState *s)
         unsigned int gsi_bits, i;
 
         /* Round up so we can search ints using ffs */
-        gsi_bits = (gsi_count + 31) / 32;
+        gsi_bits = ALIGN(gsi_count, 32);
         s->used_gsi_bitmap = g_malloc0(gsi_bits / 8);
I think the above is all that's needed (it actually used to be this, then got broken in 84b058d). But if we do this:
kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 878
Quoted 2 lines
kvm-all.c +1 -1
-        s->max_gsi = gsi_bits;
+        s->max_gsi = gsi_count;
Then we'll hit this assert from the code immediately below where we're marking over-allocated bits as already used if we actually did a round-up:
Quoted 6 lines
static void set_gsi(KVMState *s, unsigned int gsi)
{
    assert(gsi < s->max_gsi);
Sorry, I had forgotten about this pre-allocation trick to avoid returning > gsi_count when we talked about this.
Oh, indeed. That's slightly ugly, gsi_max remains misnamed.

Quoted 7 lines

Quoted 2 lines
Let's just drop the overeager asserts and keep the number of bitmap words in KVMState. That's what we really need for doing the work.
We could solve that by just renaming it to s->gsi_count and keep the sanity test, but let's fix the bug first. I'm pretty sure the compiler is going to do something smart enough with the loop control that it doesn't matter if we save the number of words or the number of bits.

Well, then just fix that single line and do not rename. Can be refactored later while continuing to port the upstream.

Jan

Jan Kiszka

2012-03-27 21:52 UTC #6 lore raw

Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly

In reply to Jason Baron

Quoted 17 lines · Jason Baron

The current 'kvm_init_irq_routing()' doesn't set up the gsi bitmap correctly, and as a consequence pins max_gsi to 32 when it really should be 1024. I ran into this limitation while testing pci passthrough, where I consistently would get -ENOSPACE return from kvm_get_irq_route_gsi() in assigned_dev_update_msix_mmio().
Signed-off-by Jason Baron <[email protected]>
---
 kvm-all.c  |    4 ++--
 qemu-kvm.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
kvm-all.c +0 -0
index ab88c7c..7d602af 100644

kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 876

Quoted 5 lines

kvm-all.c +1 -1

@@ -873,9 +873,9 @@ static void kvm_init_irq_routing(KVMState *s)
         unsigned int gsi_bits, i;
 
         /* Round up so we can search ints using ffs */
-        gsi_bits = (gsi_count + 31) / 32;
+        gsi_bits = ALIGN(gsi_count, 32);

Oops.

kvm-all.c static void kvm_init_irq_routing(KVMState *s) · line 881

Quoted 6 lines

kvm-all.c +1 -1

         s->used_gsi_bitmap = g_malloc0(gsi_bits / 8);
-        s->max_gsi = gsi_bits;
+        s->max_gsi = gsi_count;
 
         /* Mark any over-allocated bits as already in use */
         for (i = gsi_count; i < gsi_bits; i++) {

When redefining its semantic anyway, ket's take the chance and rename gsi_max to gsi_count. gsi_max actually sounds to me like gsi_count - 1.

This change should then be a uq/master patch. The other bits for qemu-kvm can build on top.

qemu-kvm.c int kvm_get_irq_route_gsi(void) · line 255

Quoted 13 lines

qemu-kvm.c +1 -1

index 2047ebb..b17cae0 100644
@@ -249,7 +249,7 @@ int kvm_get_irq_route_gsi(void)
     uint32_t *buf = s->used_gsi_bitmap;
 
     /* Return the lowest unused GSI in the bitmap */
-    for (i = 0; i < s->max_gsi / 32; i++) {
+    for (i = 0; i < (ALIGN(s->max_gsi, 32) / 32); i++) {
         bit = ffs(~buf[i]);
         if (!bit) {
             continue;

Would be nicer to hold the loop limit in local variable.

Jan

Jason Baron

author

2012-03-27 21:00 UTC #7 lore raw

PATCH 2/2 qemu kvm: add better error reporting when kvm_get_irq_route_gsi() fails

In reply to Jason Baron

When kvm_get_irq_route_gsi() fails in assigned_dev_update_msix_mmio, it would be nice to have a better error message.

Signed-off-by Jason Baron <[email protected]>

---
 hw/device-assignment.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

hw/device-assignment.c +3 -1

index 89823f1..d017537 100644
@@ -1023,8 +1023,10 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
         }
 
         r = kvm_get_irq_route_gsi();
-        if (r < 0)
+        if (r < 0) {
+            perror("assigned_dev_update_msix_mmio: kvm_get_irq_route_gsi");
             return r;
+        }
 
         adev->entry[i].gsi = r;
         adev->entry[i].type = KVM_IRQ_ROUTING_MSI;

Signature · 1 line

1.7.1