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

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

[Alexander Graf]

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 <agraf@suse.de>
---
 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;
 
-- 
1.7.12.4

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

[Cornelia Huck]

On Fri,  6 Jun 2014 14:46:05 +0200
Alexander Graf <agraf@suse.de> 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 <agraf@suse.de>
> ---
>  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.

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

[Alexander Graf]

On 06.06.14 15:12, Cornelia Huck wrote:
> On Fri,  6 Jun 2014 14:46:05 +0200
> Alexander Graf <agraf@suse.de> 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 <agraf@suse.de>
>> ---
>>   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?


Alex

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

[Cornelia Huck]

On Fri, 06 Jun 2014 15:15:54 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 06.06.14 15:12, Cornelia Huck wrote:
> > On Fri,  6 Jun 2014 14:46:05 +0200
> > Alexander Graf <agraf@suse.de> 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 <agraf@suse.de>
> >> ---
> >>   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?

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?

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

[Alexander Graf]

On 06.06.14 15:23, Cornelia Huck wrote:
> On Fri, 06 Jun 2014 15:15:54 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 06.06.14 15:12, Cornelia Huck wrote:
>>> On Fri,  6 Jun 2014 14:46:05 +0200
>>> Alexander Graf <agraf@suse.de> 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 <agraf@suse.de>
>>>> ---
>>>>    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.

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

Alex

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

[Cornelia Huck]

On Fri, 06 Jun 2014 15:28:13 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 06.06.14 15:23, Cornelia Huck wrote:
> > On Fri, 06 Jun 2014 15:15:54 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >
> >> On 06.06.14 15:12, Cornelia Huck wrote:
> >>> On Fri,  6 Jun 2014 14:46:05 +0200
> >>> Alexander Graf <agraf@suse.de> 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 <agraf@suse.de>
> >>>> ---
> >>>>    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 <cornelia.huck@de.ibm.com>

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

[Paolo Bonzini]

Il 06/06/2014 14:46, Alexander Graf ha scritto:
> 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 <agraf@suse.de>
> ---
>  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;
>
>

Applied, thanks!

Paolo

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

[Alexander Graf]

On 06.06.14 18:31, Paolo Bonzini wrote:
> Il 06/06/2014 14:46, Alexander Graf ha scritto:
>> 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 <agraf@suse.de>
>> ---
>>  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;
>>
>>
>
> Applied, thanks!

Please CC this to qemu-stable when you send it out :).


Alex