[PATCH 0/2] target/nios2: Roll cpu_pic code into CPU itself

[PATCH 0/2] target/nios2: Roll cpu_pic code into CPU itself

[Peter Maydell]

(As well as the listed nios2 maintainers, I've cc'd a couple
of the more recent contributors to this target in case they're
interested or wish to test the changes.)

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.

This allows us to roll all of the code in hw/nios/cpu_pic.c
into target/nios2 one way or another.

The motivation here is fixing a trivial Coverity-reported leak
(CID 1421916) of the IRQ array allocated in nios2_cpu_pic_init(),
and also part of the extremely slow refactoring of code away
from using qemu_allocate_irqs() at all.

Tested with 'make check' and 'make check-acceptance' (which does
include a boot test of a nios2 10m50 guest).

thanks
-- PMM

Peter Maydell (2):
  target/nios2: Move cpu_pic code into CPU object proper
  target/nios2: Move nios2_check_interrupts() into target/nios2

 target/nios2/cpu.h        |  3 --
 hw/nios2/10m50_devboard.c |  8 ++---
 hw/nios2/cpu_pic.c        | 67 ---------------------------------------
 target/nios2/cpu.c        | 34 ++++++++++++++++++++
 target/nios2/op_helper.c  |  9 ++++++
 hw/nios2/meson.build      |  2 +-
 6 files changed, 47 insertions(+), 76 deletions(-)
 delete mode 100644 hw/nios2/cpu_pic.c

-- 
2.20.1

[PATCH 1/2] target/nios2: Move cpu_pic code into CPU object proper

[Peter Maydell]

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);
 #endif
 }
 
-- 
2.20.1

[PATCH 2/2] target/nios2: Move nios2_check_interrupts() into target/nios2

[Peter Maydell]

The function nios2_check_interrupts)() looks only at CPU-internal
state; it belongs in target/nios2, not hw/nios2.  Move it into the
same file as its only caller, so it can just be local to that file.

This removes the only remaining code from cpu_pic.c, so we can delete
that file entirely.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/nios2/cpu.h       |  2 --
 hw/nios2/cpu_pic.c       | 36 ------------------------------------
 target/nios2/op_helper.c |  9 +++++++++
 hw/nios2/meson.build     |  2 +-
 4 files changed, 10 insertions(+), 39 deletions(-)
 delete mode 100644 hw/nios2/cpu_pic.c

diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
index b7efb54ba7e..2ab82fdc713 100644
--- a/target/nios2/cpu.h
+++ b/target/nios2/cpu.h
@@ -201,8 +201,6 @@ void nios2_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
                                    MMUAccessType access_type,
                                    int mmu_idx, uintptr_t retaddr);
 
-void nios2_check_interrupts(CPUNios2State *env);
-
 void do_nios2_semihosting(CPUNios2State *env);
 
 #define CPU_RESOLVING_TYPE TYPE_NIOS2_CPU
diff --git a/hw/nios2/cpu_pic.c b/hw/nios2/cpu_pic.c
deleted file mode 100644
index 3fb621c5c85..00000000000
--- a/hw/nios2/cpu_pic.c
+++ /dev/null
@@ -1,36 +0,0 @@
-/*
- * Altera Nios2 CPU PIC
- *
- * Copyright (c) 2016 Marek Vasut <marek.vasut@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/lgpl-2.1.html>
- */
-
-#include "qemu/osdep.h"
-#include "cpu.h"
-#include "hw/irq.h"
-
-#include "qemu/config-file.h"
-
-#include "boot.h"
-
-void nios2_check_interrupts(CPUNios2State *env)
-{
-    if (env->irq_pending &&
-        (env->regs[CR_STATUS] & CR_STATUS_PIE)) {
-        env->irq_pending = 0;
-        cpu_interrupt(env_cpu(env), CPU_INTERRUPT_HARD);
-    }
-}
diff --git a/target/nios2/op_helper.c b/target/nios2/op_helper.c
index a60730faac3..a59003855ab 100644
--- a/target/nios2/op_helper.c
+++ b/target/nios2/op_helper.c
@@ -36,6 +36,15 @@ void helper_mmu_write(CPUNios2State *env, uint32_t rn, uint32_t v)
     mmu_write(env, rn, v);
 }
 
+static void nios2_check_interrupts(CPUNios2State *env)
+{
+    if (env->irq_pending &&
+        (env->regs[CR_STATUS] & CR_STATUS_PIE)) {
+        env->irq_pending = 0;
+        cpu_interrupt(env_cpu(env), CPU_INTERRUPT_HARD);
+    }
+}
+
 void helper_check_interrupts(CPUNios2State *env)
 {
     qemu_mutex_lock_iothread();
diff --git a/hw/nios2/meson.build b/hw/nios2/meson.build
index dd66ebb32f6..6c58e8082b4 100644
--- a/hw/nios2/meson.build
+++ b/hw/nios2/meson.build
@@ -1,5 +1,5 @@
 nios2_ss = ss.source_set()
-nios2_ss.add(files('boot.c', 'cpu_pic.c'))
+nios2_ss.add(files('boot.c'))
 nios2_ss.add(when: 'CONFIG_NIOS2_10M50', if_true: files('10m50_devboard.c'))
 nios2_ss.add(when: 'CONFIG_NIOS2_GENERIC_NOMMU', if_true: files('generic_nommu.c'))
 
-- 
2.20.1

Re: [PATCH 2/2] target/nios2: Move nios2_check_interrupts() into target/nios2

[Philippe Mathieu-Daudé]

On 11/27/20 8:12 PM, Peter Maydell wrote:
> The function nios2_check_interrupts)() looks only at CPU-internal
> state; it belongs in target/nios2, not hw/nios2.  Move it into the
> same file as its only caller, so it can just be local to that file.
> 
> This removes the only remaining code from cpu_pic.c, so we can delete
> that file entirely.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/nios2/cpu.h       |  2 --
>  hw/nios2/cpu_pic.c       | 36 ------------------------------------
>  target/nios2/op_helper.c |  9 +++++++++
>  hw/nios2/meson.build     |  2 +-
>  4 files changed, 10 insertions(+), 39 deletions(-)
>  delete mode 100644 hw/nios2/cpu_pic.c

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 1/2] target/nios2: Move cpu_pic code into CPU object proper

[Philippe Mathieu-Daudé]

On 11/27/20 8:12 PM, 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));

Ah, NMI is never used.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>      for (i = 0; i < 32; i++) {
>          irq[i] = qdev_get_gpio_in(dev, i);
>      }
...

RE: [PATCH 1/2] target/nios2: Move cpu_pic code into CPU object proper

[Wu, Wentong]

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

Re: [PATCH 1/2] target/nios2: Move cpu_pic code into CPU object proper

[Peter Maydell]

On Sat, 28 Nov 2020 at 05:50, Wu, Wentong <wentong.wu@intel.com> wrote:
> 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.

The "altera,iic" device is what connects to these GPIO lines -- the
single output line from the "altera,iic" device connects to the
"IRQ" GPIO input. The code currently in cpu_pic.c is in no way
an external-to-the-CPU device implementation:
 * it's not a device
 * it directly messes with CPUNios2State fields like irq_pending
   and env->regs[CR_STATUS]
It's been implemented as part of the CPU, it's just in the
wrong place in QEMU's source code and not very cleanly
connected to the rest of the system.

If we ever wanted to model an EIC, we'd need to also model the
EIC-to-CPU interface, which seems to be moderately complicated
(the EIC "presents one interrupt to the Nios II processor, with
interrupt handler address and register set selection information").
So we'd do that by modelling a suitable interface connection plus
the EIC device, and an board model with an EIC would wire that up
and simply not connect the NMI/IRQ GPIO lines to anything, which
would be the equivalent of "the IIC is disabled" (or if just not
connecting the inputs is insufficient, we'd have a QOM property
on the CPU object for "disable the IIC", which would be an exact
match for "to 'configure the h/w with the IIC not implemented").

You have, though, prompted me to look at hw/intc/nios2_iic.c,
which I had previously assumed was a real external interrupt controller,
and although that is coded as a separate device, it has no
internal state of its own -- it also is just looking directly
at the CPUNios2State fields and register state. It's part of the
CPU, it's just implemented in the wrong place in QEMU.

So I'll spin a v2 of this series that also folds that code properly
into the CPU object, so that the CPU object provides 32 input IRQ
lines. That will mean we're modelling the hardware much more closely:
 * the IIC is internal to the CPU itself
 * (hypothetical) board models using an EIC will provide an
   EIC model that connects to the CPU using an interface similar
   to what the real h/w does
 * if necessary, QOM property for the equivalent of "configure
   CPU with the IIC not implemented"

thanks
-- PMM