| Message ID | 20201127191233.11200-2-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/nios2: Roll cpu_pic code into CPU itself | expand |
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); > } ...
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
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
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 }
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(-)