[Qemu-devel] [PATCH 0/2] qemu kvm: correct gsi bitmap

[Qemu-devel] [PATCH 0/2] qemu kvm: correct gsi bitmap

[Jason Baron]

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(-)

[Qemu-devel] [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly

[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 <jbaron@redhat.com>
---
 kvm-all.c  |    4 ++--
 qemu-kvm.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index ab88c7c..7d602af 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -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++) {
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 2047ebb..b17cae0 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -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;
-- 
1.7.1

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

[Alex Williamson]

On Tue, 2012-03-27 at 17:00 -0400, Jason Baron wrote:
> 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 <jbaron@redhat.com>
> ---
>  kvm-all.c  |    4 ++--
>  qemu-kvm.c |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index ab88c7c..7d602af 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -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:

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

>  
>          /* Mark any over-allocated bits as already in use */
>          for (i = gsi_count; i < gsi_bits; i++) {
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 2047ebb..b17cae0 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -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.

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

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

[Jan Kiszka]

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

On 2012-03-27 23:31, Alex Williamson wrote:
> On Tue, 2012-03-27 at 17:00 -0400, Jason Baron wrote:
>> 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 <jbaron@redhat.com>
>> ---
>>  kvm-all.c  |    4 ++--
>>  qemu-kvm.c |    2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index ab88c7c..7d602af 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -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:
> 
>> -        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.

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


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

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

[Alex Williamson]

On Wed, 2012-03-28 at 00:01 +0200, Jan Kiszka wrote:
> On 2012-03-27 23:31, Alex Williamson wrote:
> > On Tue, 2012-03-27 at 17:00 -0400, Jason Baron wrote:
> >> 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 <jbaron@redhat.com>
> >> ---
> >>  kvm-all.c  |    4 ++--
> >>  qemu-kvm.c |    2 +-
> >>  2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/kvm-all.c b/kvm-all.c
> >> index ab88c7c..7d602af 100644
> >> --- a/kvm-all.c
> >> +++ b/kvm-all.c
> >> @@ -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:
> > 
> >> -        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.
> 
> 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

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

[Jan Kiszka]

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

On 2012-03-28 00:25, Alex Williamson wrote:
> On Wed, 2012-03-28 at 00:01 +0200, Jan Kiszka wrote:
>> On 2012-03-27 23:31, Alex Williamson wrote:
>>> On Tue, 2012-03-27 at 17:00 -0400, Jason Baron wrote:
>>>> 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 <jbaron@redhat.com>
>>>> ---
>>>>  kvm-all.c  |    4 ++--
>>>>  qemu-kvm.c |    2 +-
>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>> index ab88c7c..7d602af 100644
>>>> --- a/kvm-all.c
>>>> +++ b/kvm-all.c
>>>> @@ -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:
>>>
>>>> -        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.
>>
>> 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.

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

Jan


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

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

[Jan Kiszka]

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

On 2012-03-27 23:00, Jason Baron wrote:
> 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 <jbaron@redhat.com>
> ---
>  kvm-all.c  |    4 ++--
>  qemu-kvm.c |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index ab88c7c..7d602af 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -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.

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

> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 2047ebb..b17cae0 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -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


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

[Qemu-devel] [PATCH 2/2] qemu kvm: add better error reporting when kvm_get_irq_route_gsi() fails

[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 <jbaron@redhat.com>
---
 hw/device-assignment.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 89823f1..d017537 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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;
-- 
1.7.1