If the kernel irqchip has been disabled, we don't want the
{add,release}_adapter_routes routines to call any kvm_irqchip_*
interfaces, as they may rely on an irqchip actually having been
created. Just take a quick exit in that case instead.

Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

Without this patch, QEMU with kernel_irqchip=off will crash in
kvm_irqchip_release_virq(), so alternatively, we could add a check
there. kvm_irqchip_add_adapter_route() is actually fine.

---
 hw/intc/s390_flic_kvm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index dddd33ea61c8..44b7960ebcc8 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -331,6 +331,10 @@ static int kvm_s390_add_adapter_routes(S390FLICState *fs,
     int ret, i;
     uint64_t ind_offset = routes->adapter.ind_offset;
 
+    if (!kvm_gsi_routing_enabled()) {
+        return -ENOSYS;
+    }
+
     for (i = 0; i < routes->num_routes; i++) {
         ret = kvm_irqchip_add_adapter_route(kvm_state, &routes->adapter);
         if (ret < 0) {
@@ -358,6 +362,10 @@ static void kvm_s390_release_adapter_routes(S390FLICState *fs,
 {
     int i;
 
+    if (!kvm_gsi_routing_enabled()) {
+        return;
+    }
+
     for (i = 0; i < routes->num_routes; i++) {
         if (routes->gsi[i] >= 0) {
             kvm_irqchip_release_virq(kvm_state, routes->gsi[i]);
-- 
2.21.1

On 16/01/2020 13.37, Cornelia Huck wrote:
> If the kernel irqchip has been disabled, we don't want the
> {add,release}_adapter_routes routines to call any kvm_irqchip_*
> interfaces, as they may rely on an irqchip actually having been
> created. Just take a quick exit in that case instead.
> 
> Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> Without this patch, QEMU with kernel_irqchip=off will crash in
> kvm_irqchip_release_virq(), so alternatively, we could add a check
> there. kvm_irqchip_add_adapter_route() is actually fine.
> 
> ---
>  hw/intc/s390_flic_kvm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index dddd33ea61c8..44b7960ebcc8 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -331,6 +331,10 @@ static int kvm_s390_add_adapter_routes(S390FLICState *fs,
>      int ret, i;
>      uint64_t ind_offset = routes->adapter.ind_offset;
>  
> +    if (!kvm_gsi_routing_enabled()) {
> +        return -ENOSYS;
> +    }

As you wrote, this check is not really necessary since it is already
done in  kvm_irqchip_add_adapter_route() ...

>      for (i = 0; i < routes->num_routes; i++) {
>          ret = kvm_irqchip_add_adapter_route(kvm_state, &routes->adapter);
>          if (ret < 0) {

... so I wonder if it would be simply best to set

               routes->gsi[i] = -1;

before the "goto" instead to make sure that
kvm_s390_release_adapter_routes() does not try to clean it up? That
would also fix a potential crash in case kvm_irqchip_add_adapter_route()
returned an error code in case of a different problem, I think.

 Thomas


> @@ -358,6 +362,10 @@ static void kvm_s390_release_adapter_routes(S390FLICState *fs,
>  {
>      int i;
>  
> +    if (!kvm_gsi_routing_enabled()) {
> +        return;
> +    }
> +
>      for (i = 0; i < routes->num_routes; i++) {
>          if (routes->gsi[i] >= 0) {
>              kvm_irqchip_release_virq(kvm_state, routes->gsi[i]);
> 

On Thu, 16 Jan 2020 13:52:21 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 16/01/2020 13.37, Cornelia Huck wrote:
> > If the kernel irqchip has been disabled, we don't want the
> > {add,release}_adapter_routes routines to call any kvm_irqchip_*
> > interfaces, as they may rely on an irqchip actually having been
> > created. Just take a quick exit in that case instead.
> > 
> > Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > 
> > Without this patch, QEMU with kernel_irqchip=off will crash in
> > kvm_irqchip_release_virq(), so alternatively, we could add a check
> > there. kvm_irqchip_add_adapter_route() is actually fine.
> > 
> > ---
> >  hw/intc/s390_flic_kvm.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> > index dddd33ea61c8..44b7960ebcc8 100644
> > --- a/hw/intc/s390_flic_kvm.c
> > +++ b/hw/intc/s390_flic_kvm.c
> > @@ -331,6 +331,10 @@ static int kvm_s390_add_adapter_routes(S390FLICState *fs,
> >      int ret, i;
> >      uint64_t ind_offset = routes->adapter.ind_offset;
> >  
> > +    if (!kvm_gsi_routing_enabled()) {
> > +        return -ENOSYS;
> > +    }  
> 
> As you wrote, this check is not really necessary since it is already
> done in  kvm_irqchip_add_adapter_route() ...

I do think it is cleaner, though.

> 
> >      for (i = 0; i < routes->num_routes; i++) {
> >          ret = kvm_irqchip_add_adapter_route(kvm_state, &routes->adapter);
> >          if (ret < 0) {  
> 
> ... so I wonder if it would be simply best to set
> 
>                routes->gsi[i] = -1;
> 
> before the "goto" instead to make sure that
> kvm_s390_release_adapter_routes() does not try to clean it up? That
> would also fix a potential crash in case kvm_irqchip_add_adapter_route()
> returned an error code in case of a different problem, I think.

I think we should pre-initialize gsi[] to -1 instead, just to be on the
safe side.