Re: [PATCH 3/3] target/openrisc: Move pic_cpu code into CPU object proper

[Stafford Horne]

On Fri, Nov 27, 2020 at 10:51:27PM +0000, Peter Maydell wrote:
> The openrisc code uses an old style of interrupt handling, where a
> separate standalone set of qemu_irqs invoke a function
> openrisc_pic_cpu_handler() which signals the interrupt to the CPU
> proper by directly calling cpu_interrupt() and cpu_reset_interrupt().
> Because CPU objects now inherit (indirectly) from TYPE_DEVICE, they
> can have GPIO input lines themselves, and the neater modern way to
> implement this is to simply have the CPU object itself provide the
> input IRQ lines.
> 
> Create GPIO inputs to the OpenRISC CPU object, and make the only user
> of cpu_openrisc_pic_init() wire up directly to those instead.
> 
> This allows us to delete the hw/openrisc/pic_cpu.c file entirely.
> 
> This fixes a trivial memory leak reported by Coverity of the IRQs
> allocated in cpu_openrisc_pic_init().
> 
> Fixes: Coverity CID 1421934
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/openrisc/cpu.h      |  1 -
>  hw/openrisc/openrisc_sim.c |  3 +-
>  hw/openrisc/pic_cpu.c      | 61 --------------------------------------
>  target/openrisc/cpu.c      | 32 ++++++++++++++++++++
>  hw/openrisc/meson.build    |  2 +-
>  5 files changed, 34 insertions(+), 65 deletions(-)
>  delete mode 100644 hw/openrisc/pic_cpu.c
> 
> diff --git a/target/openrisc/cpu.h b/target/openrisc/cpu.h
> index bd42faf144f..82cbaeb4f84 100644
> --- a/target/openrisc/cpu.h
> +++ b/target/openrisc/cpu.h
> @@ -293,7 +293,6 @@ typedef struct CPUOpenRISCState {
>      uint32_t picmr;         /* Interrupt mask register */
>      uint32_t picsr;         /* Interrupt contrl register*/
>  #endif
> -    void *irq[32];          /* Interrupt irq input */
>  } CPUOpenRISCState;
>  
>  /**
> diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> index 75ba0f47444..39f1d344ae9 100644
> --- a/hw/openrisc/openrisc_sim.c
> +++ b/hw/openrisc/openrisc_sim.c
> @@ -54,7 +54,7 @@ static void main_cpu_reset(void *opaque)
>  
>  static qemu_irq get_cpu_irq(OpenRISCCPU *cpus[], int cpunum, int irq_pin)
>  {
> -    return cpus[cpunum]->env.irq[irq_pin];
> +    return qdev_get_gpio_in_named(DEVICE(cpus[cpunum]), "IRQ", irq_pin);
>  }
>  
>  static void openrisc_sim_net_init(hwaddr base, hwaddr descriptors,
> @@ -154,7 +154,6 @@ static void openrisc_sim_init(MachineState *machine)
>              fprintf(stderr, "Unable to find CPU definition!\n");
>              exit(1);
>          }
> -        cpu_openrisc_pic_init(cpus[n]);
>  
>          cpu_openrisc_clock_init(cpus[n]);
>  
> diff --git a/hw/openrisc/pic_cpu.c b/hw/openrisc/pic_cpu.c
> deleted file mode 100644
> index 36f93508309..00000000000
> --- a/hw/openrisc/pic_cpu.c
> +++ /dev/null
> @@ -1,61 +0,0 @@
> -/*
> - * OpenRISC Programmable Interrupt Controller support.
> - *
> - * Copyright (c) 2011-2012 Jia Liu <proljc@gmail.com>
> - *                         Feng Gao <gf91597@gmail.com>
> - *
> - * This library is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2.1 of the License, or (at your option) any later version.
> - *
> - * This library is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> - */
> -
> -#include "qemu/osdep.h"
> -#include "hw/irq.h"
> -#include "cpu.h"
> -
> -/* OpenRISC pic handler */
> -static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
> -{
> -    OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;
> -    CPUState *cs = CPU(cpu);
> -    uint32_t irq_bit;
> -
> -    if (irq > 31 || irq < 0) {
> -        return;
> -    }
> -
> -    irq_bit = 1U << irq;
> -
> -    if (level) {
> -        cpu->env.picsr |= irq_bit;
> -    } else {
> -        cpu->env.picsr &= ~irq_bit;
> -    }
> -
> -    if (cpu->env.picsr & cpu->env.picmr) {
> -        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> -    } else {
> -        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> -        cpu->env.picsr = 0;
> -    }
> -}
> -
> -void cpu_openrisc_pic_init(OpenRISCCPU *cpu)
> -{
> -    int i;
> -    qemu_irq *qi;
> -    qi = qemu_allocate_irqs(openrisc_pic_cpu_handler, cpu, NR_IRQS);
> -
> -    for (i = 0; i < NR_IRQS; i++) {
> -        cpu->env.irq[i] = qi[i];
> -    }
> -}
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index 5528c0918f4..b0bdfbe4fe2 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -65,6 +65,34 @@ static void openrisc_cpu_reset(DeviceState *dev)
>  #endif
>  }
>  
> +#ifndef CONFIG_USER_ONLY
> +static void openrisc_cpu_set_irq(void *opaque, int irq, int level)
> +{
> +    OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;
> +    CPUState *cs = CPU(cpu);
> +    uint32_t irq_bit;
> +
> +    if (irq > 31 || irq < 0) {
> +        return;
> +    }
> +
> +    irq_bit = 1U << irq;
> +
> +    if (level) {
> +        cpu->env.picsr |= irq_bit;
> +    } else {
> +        cpu->env.picsr &= ~irq_bit;
> +    }
> +
> +    if (cpu->env.picsr & cpu->env.picmr) {
> +        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> +    } else {
> +        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> +        cpu->env.picsr = 0;
> +    }
> +}
> +#endif
> +
>  static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -88,6 +116,10 @@ static void openrisc_cpu_initfn(Object *obj)
>      OpenRISCCPU *cpu = OPENRISC_CPU(obj);
>  
>      cpu_set_cpustate_pointers(cpu);
> +
> +#ifndef CONFIG_USER_ONLY
> +    qdev_init_gpio_in_named(DEVICE(cpu), openrisc_cpu_set_irq, "IRQ", 
> NR_IRQS);
> +#endif
>  }
>  
>  /* CPU models */
> diff --git a/hw/openrisc/meson.build b/hw/openrisc/meson.build
> index 57c42558e18..947f63ee087 100644
> --- a/hw/openrisc/meson.build
> +++ b/hw/openrisc/meson.build
> @@ -1,5 +1,5 @@
>  openrisc_ss = ss.source_set()
> -openrisc_ss.add(files('pic_cpu.c', 'cputimer.c'))
> +openrisc_ss.add(files('cputimer.c'))
>  openrisc_ss.add(when: 'CONFIG_OR1K_SIM', if_true: files('openrisc_sim.c'))
>  
>  hw_arch += {'openrisc': openrisc_ss}
> -- 
> 2.20.1

This is nice, thanks for the patch.

Reviewed-by: Stafford Horne <shorne@gmail.com>

Please feel free to merge.