| Message ID | 025b834272f9972f97873779be6523527dad42a8.1332881354.git.jbaron@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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;
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(-)