| Message ID | 1402058765-48921-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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?
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
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>
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
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
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;
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(-)