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

[Jan Kiszka]

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

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

Jan

signature.asc
Description: OpenPGP digital signature