[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH 1/2] target/nios2: Move cpu_pic code into CPU object proper
|
From: |
Wu, Wentong |
|
Subject: |
RE: [PATCH 1/2] target/nios2: Move cpu_pic code into CPU object proper |
|
Date: |
Sat, 28 Nov 2020 05:50:46 +0000 |
On 11/28/20 3:13 AM, Peter Maydell wrote:
> The nios2 code uses an old style of interrupt handling, where a separate
> standalone set of qemu_irqs invoke a function
> nios2_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 named "NMI" and "IRQ" GPIO inputs to the Nios2 CPU object, and make
> the only user of nios2_cpu_pic_init() wire up directly to those instead.
>
> This fixes a Coverity-reported trivial memory leak of the IRQ array allocated
> in nios2_cpu_pic_init().
>
> Fixes: Coverity CID 1421916
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> target/nios2/cpu.h | 1 -
> hw/nios2/10m50_devboard.c | 8 +++-----
> hw/nios2/cpu_pic.c | 31 -------------------------------
> target/nios2/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> 4 files changed, 37 insertions(+), 37 deletions(-)
>
> diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h index
> 86bbe1d8670..b7efb54ba7e 100644
> --- a/target/nios2/cpu.h
> +++ b/target/nios2/cpu.h
> @@ -201,7 +201,6 @@ void nios2_cpu_do_unaligned_access(CPUState *cpu, vaddr
> addr,
> MMUAccessType access_type,
> int mmu_idx, uintptr_t retaddr);
>
> -qemu_irq *nios2_cpu_pic_init(Nios2CPU *cpu); void
> nios2_check_interrupts(CPUNios2State *env);
>
> void do_nios2_semihosting(CPUNios2State *env); diff --git
> a/hw/nios2/10m50_devboard.c b/hw/nios2/10m50_devboard.c index
> 5c13b74306f..ac1993e8c08 100644
> --- a/hw/nios2/10m50_devboard.c
> +++ b/hw/nios2/10m50_devboard.c
> @@ -52,7 +52,7 @@ static void nios2_10m50_ghrd_init(MachineState *machine)
> ram_addr_t tcm_size = 0x1000; /* 1 kiB, but QEMU limit is 4 kiB */
> ram_addr_t ram_base = 0x08000000;
> ram_addr_t ram_size = 0x08000000;
> - qemu_irq *cpu_irq, irq[32];
> + qemu_irq irq[32];
> int i;
>
> /* Physical TCM (tb_ram_1k) with alias at 0xc0000000 */ @@ -76,14 +76,12
> @@ static void nios2_10m50_ghrd_init(MachineState *machine)
> /* Create CPU -- FIXME */
> cpu = NIOS2_CPU(cpu_create(TYPE_NIOS2_CPU));
>
> - /* Register: CPU interrupt controller (PIC) */
> - cpu_irq = nios2_cpu_pic_init(cpu);
> -
> /* Register: Internal Interrupt Controller (IIC) */
> dev = qdev_new("altera,iic");
> object_property_add_const_link(OBJECT(dev), "cpu", OBJECT(cpu));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> - sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irq[0]);
> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
> + qdev_get_gpio_in_named(DEVICE(cpu), "IRQ", 0));
> for (i = 0; i < 32; i++) {
> irq[i] = qdev_get_gpio_in(dev, i);
> }
> diff --git a/hw/nios2/cpu_pic.c b/hw/nios2/cpu_pic.c index
> 5ea7e52ab83..3fb621c5c85 100644
> --- a/hw/nios2/cpu_pic.c
> +++ b/hw/nios2/cpu_pic.c
> @@ -26,32 +26,6 @@
>
> #include "boot.h"
>
> -static void nios2_pic_cpu_handler(void *opaque, int irq, int level) -{
> - Nios2CPU *cpu = opaque;
> - CPUNios2State *env = &cpu->env;
> - CPUState *cs = CPU(cpu);
> - int type = irq ? CPU_INTERRUPT_NMI : CPU_INTERRUPT_HARD;
> -
> - if (type == CPU_INTERRUPT_HARD) {
> - env->irq_pending = level;
> -
> - if (level && (env->regs[CR_STATUS] & CR_STATUS_PIE)) {
> - env->irq_pending = 0;
> - cpu_interrupt(cs, type);
> - } else if (!level) {
> - env->irq_pending = 0;
> - cpu_reset_interrupt(cs, type);
> - }
> - } else {
> - if (level) {
> - cpu_interrupt(cs, type);
> - } else {
> - cpu_reset_interrupt(cs, type);
> - }
> - }
> -}
> -
> void nios2_check_interrupts(CPUNios2State *env) {
> if (env->irq_pending &&
> @@ -60,8 +34,3 @@ void nios2_check_interrupts(CPUNios2State *env)
> cpu_interrupt(env_cpu(env), CPU_INTERRUPT_HARD);
> }
> }
> -
> -qemu_irq *nios2_cpu_pic_init(Nios2CPU *cpu) -{
> - return qemu_allocate_irqs(nios2_pic_cpu_handler, cpu, 2);
> -}
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c index
> 8f7011fcb92..4b21e7c6d1c 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -64,6 +64,37 @@ static void nios2_cpu_reset(DeviceState *dev) #endif }
>
> +#ifndef CONFIG_USER_ONLY
> +static void nios2_cpu_set_nmi(void *opaque, int irq, int level) {
> + Nios2CPU *cpu = opaque;
> + CPUState *cs = CPU(cpu);
> +
> + if (level) {
> + cpu_interrupt(cs, CPU_INTERRUPT_NMI);
> + } else {
> + cpu_reset_interrupt(cs, CPU_INTERRUPT_NMI);
> + }
> +}
> +
> +static void nios2_cpu_set_irq(void *opaque, int irq, int level) {
> + Nios2CPU *cpu = opaque;
> + CPUNios2State *env = &cpu->env;
> + CPUState *cs = CPU(cpu);
+
+ env->irq_pending = level;
+
+ if (level && (env->regs[CR_STATUS] & CR_STATUS_PIE)) {
+ env->irq_pending = 0;
+ cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+ } else if (!level) {
+ env->irq_pending = 0;
+ cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
+ }
+}
+#endif
+
static void nios2_cpu_initfn(Object *obj) {
Nios2CPU *cpu = NIOS2_CPU(obj);
@@ -72,6 +103,9 @@ static void nios2_cpu_initfn(Object *obj)
#if !defined(CONFIG_USER_ONLY)
mmu_init(&cpu->env);
+
+ qdev_init_gpio_in_named(DEVICE(cpu), nios2_cpu_set_nmi, "NMI", 1);
+ qdev_init_gpio_in_named(DEVICE(cpu), nios2_cpu_set_irq, "IRQ", 1);
The code looks ok to me, and I tested the changes on Zephyr project, it works
well.
But, according
https://www.intel.com/content/dam/www/programmable/us/en/pdfs/literature/hb/nios2/n2sw_nii52006.pdf
,
The Nios II processor offers two distinct approaches to handling hardware
interrupts:
■ The internal interrupt controller (IIC)
■ The external interrupt controller (EIC) interface
We have already defined TypeInfo named "altera,iic" , and others can also
define EIC, so IMHO I don't think we should replace the internal interrupt
controller with GPIO.
> #endif
> }
>
> --
> 2.20.1