* [Qemu-devel] [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
@ 2012-03-27 21:00 ` Jason Baron
0 siblings, 0 replies; 16+ messages in thread
From: Jason Baron @ 2012-03-27 21:00 UTC (permalink / raw)
To: kvm; +Cc: aliguori, mst, jan.kiszka, qemu-devel, alex.williamson, avi
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
2012-03-27 21:00 ` [Qemu-devel] " Jason Baron
@ 2012-03-27 21:31 ` Alex Williamson
-1 siblings, 0 replies; 16+ messages in thread
From: Alex Williamson @ 2012-03-27 21:31 UTC (permalink / raw)
To: Jason Baron; +Cc: aliguori, kvm, mst, jan.kiszka, qemu-devel, avi
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
@ 2012-03-27 21:31 ` Alex Williamson
0 siblings, 0 replies; 16+ messages in thread
From: Alex Williamson @ 2012-03-27 21:31 UTC (permalink / raw)
To: Jason Baron; +Cc: aliguori, kvm, mst, jan.kiszka, qemu-devel, avi
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
2012-03-27 21:31 ` [Qemu-devel] " Alex Williamson
@ 2012-03-27 22:01 ` Jan Kiszka
-1 siblings, 0 replies; 16+ messages in thread
From: Jan Kiszka @ 2012-03-27 22:01 UTC (permalink / raw)
To: Alex Williamson; +Cc: Jason Baron, aliguori, kvm, mst, qemu-devel, avi
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
@ 2012-03-27 22:01 ` Jan Kiszka
0 siblings, 0 replies; 16+ messages in thread
From: Jan Kiszka @ 2012-03-27 22:01 UTC (permalink / raw)
To: Alex Williamson; +Cc: aliguori, kvm, mst, Jason Baron, qemu-devel, avi
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
2012-03-27 22:01 ` [Qemu-devel] " Jan Kiszka
@ 2012-03-27 22:25 ` Alex Williamson
-1 siblings, 0 replies; 16+ messages in thread
From: Alex Williamson @ 2012-03-27 22:25 UTC (permalink / raw)
To: Jan Kiszka; +Cc: aliguori, kvm, mst, Jason Baron, qemu-devel, avi
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
@ 2012-03-27 22:25 ` Alex Williamson
0 siblings, 0 replies; 16+ messages in thread
From: Alex Williamson @ 2012-03-27 22:25 UTC (permalink / raw)
To: Jan Kiszka; +Cc: aliguori, kvm, mst, Jason Baron, qemu-devel, avi
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
2012-03-27 22:25 ` [Qemu-devel] " Alex Williamson
@ 2012-03-27 22:32 ` Jan Kiszka
-1 siblings, 0 replies; 16+ messages in thread
From: Jan Kiszka @ 2012-03-27 22:32 UTC (permalink / raw)
To: Alex Williamson; +Cc: aliguori, kvm, mst, Jason Baron, qemu-devel, avi
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
@ 2012-03-27 22:32 ` Jan Kiszka
0 siblings, 0 replies; 16+ messages in thread
From: Jan Kiszka @ 2012-03-27 22:32 UTC (permalink / raw)
To: Alex Williamson; +Cc: aliguori, kvm, mst, Jason Baron, qemu-devel, avi
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
2012-03-27 21:00 ` [Qemu-devel] " Jason Baron
@ 2012-03-27 21:52 ` Jan Kiszka
-1 siblings, 0 replies; 16+ messages in thread
From: Jan Kiszka @ 2012-03-27 21:52 UTC (permalink / raw)
To: Jason Baron; +Cc: kvm, qemu-devel, mst, alex.williamson, avi, aliguori
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] qemu kvm: Set up gsi bitmap correctly
@ 2012-03-27 21:52 ` Jan Kiszka
0 siblings, 0 replies; 16+ messages in thread
From: Jan Kiszka @ 2012-03-27 21:52 UTC (permalink / raw)
To: Jason Baron; +Cc: aliguori, kvm, mst, qemu-devel, alex.williamson, avi
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread