Re: [Qemu-devel] [PATCH] KVM: Fix GSI number space limit

[Cornelia Huck]

On Fri, 06 Jun 2014 15:28:13 +0200
Alexander Graf <address@hidden> wrote:

> 
> On 06.06.14 15:23, Cornelia Huck wrote:
> > On Fri, 06 Jun 2014 15:15:54 +0200
> > Alexander Graf <address@hidden> wrote:
> >
> >> On 06.06.14 15:12, Cornelia Huck wrote:
> >>> On Fri,  6 Jun 2014 14:46:05 +0200
> >>> Alexander Graf <address@hidden> wrote:
> >>>
> >>>> KVM tells us the number of GSIs it can handle inside the kernel. That 
> >>>> value is
> >>>> basically KVM_MAX_IRQ_ROUTES. However when we try to set the GSI mapping 
> >>>> table,
> >>>> it checks for
> >>>>
> >>>>       r = -EINVAL;
> >>>>       if (routing.nr >= KVM_MAX_IRQ_ROUTES)
> >>>>           goto out;
> >>>>
> >>>> erroring out even when we're only using all of the GSIs. To make sure we 
> >>>> never
> >>>> hit that limit, let's reduce the number of GSIs we get from KVM by one.
> >>>>
> >>>> Signed-off-by: Alexander Graf <address@hidden>
> >>>> ---
> >>>>    kvm-all.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/kvm-all.c b/kvm-all.c
> >>>> index 4e19eff..56a251b 100644
> >>>> --- a/kvm-all.c
> >>>> +++ b/kvm-all.c
> >>>> @@ -938,7 +938,7 @@ void kvm_init_irq_routing(KVMState *s)
> >>>>    {
> >>>>        int gsi_count, i;
> >>>>
> >>>> -    gsi_count = kvm_check_extension(s, KVM_CAP_IRQ_ROUTING);
> >>>> +    gsi_count = kvm_check_extension(s, KVM_CAP_IRQ_ROUTING) - 1;
> >>>>        if (gsi_count > 0) {
> >>>>            unsigned int gsi_bits, i;
> >>>>
> >>> But gsi_count is already marked as used further down in this function,
> >>> isn't it? Confused.
> >>     gsi_bits = ALIGN(gsi_count, 32);
> >> [...]
> >>           for (i = gsi_count; i < gsi_bits; i++) {
> >>               set_gsi(s, i);
> >>           }
> >>
> >> So if you take gsi_count = 1024, what happens?
> >>
> >>     gsi_count = 1024;
> >>     gsi_bits = 1024;
> >>     for (i = 1024; i < 1024; i++) {
> >>               set_gsi(s, i);
> >>     }
> >>
> >> At least in my world of C that loop never runs, no?
> >>
> > But then kvm_irqchip_get_virq() should never return 1024, shouldn't it?
> 
> Right, because it returns the virq number which starts at 0. However, to 
> describe all virqs from [0..1023] we need 1024 entries which the kernel 
> errors out on.

Ah... that's kvm_irq_routing::nr and not kvm_irq_routing_entry::gsi, so
it's basically a kernel misfeature we need to work around.

> 
> >
> > And:
> >
> > void kvm_irqchip_add_irq_route(KVMState *s, int irq, int irqchip, int pin)
> > {
> > [...]
> >      assert(pin < s->gsi_count);
> >
> > would trigger too early with your change, wouldn't it?
> 
> Not really - with my change we only support 1023 virqs. So the biggest 
> virq number is 1022 which is < 1023 :).
> 
> 
> Sorry for describing this with actual numbers - I find it easier to 
> grasp when I think in concrete numbers here - this stuff is just really 
> spinning my head :).

And on top of that, it's Friday :)

But yes, makes sense now.

Acked-by: Cornelia Huck <address@hidden>