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 <coh...@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]);
>
