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 <address@hidden>
> >> ---
> >>  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