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.mayd...@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 <pro...@gmail.com>
> - * Feng Gao <gf91...@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 <sho...@gmail.com>
Please feel free to merge.
