| Message ID | 20200116123703.14624-1-cohuck@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/flic: adapter routes handling if !kernel_irqchip | expand |
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.
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]);
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(+)