Re: [PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)
On Thu, 3 Mar 2022 at 09:54, Amir Gonnen wrote:
>
> > This looks wrong. Of course, so does nios2_cpu_set_irq, from which you've
> > cribbed this.
>
> > For our purposes, I think simply re-using env->regs[CR_IPENDING] as the
> > external hw
> > request word is the right thing to do. But we need to update RDCTL to
> > compute the
> > correct value from CR_IPENDING & CR_IENABLE, and update WRCTL to ignore
> > writes.
>
> Since you've already fixed that on " target/nios2: Rewrite interrupt
> handling" patchset, I guess I'll need to rebase on it once it's merged.
> Until then for my next version I plan to just keep that with a "TODO" comment.
If you like you could rebase on those patches already; then when you
send your next version of the series include in the cover letter
the lines
Based-on: 20220227182125.21809-1-richard.hender...@linaro.org
("target/nios2: Rewrite interrupt handling")
The first of those tells the automated tooling like patchew.org that
it should apply the referenced patchseries first before trying to
apply and test your series; the second is for humans to read.
That might save you having to respin twice.
thanks
-- PMM
RE: [PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)
> This looks wrong. Of course, so does nios2_cpu_set_irq, from which you've
> cribbed this.
> For our purposes, I think simply re-using env->regs[CR_IPENDING] as the
> external hw
> request word is the right thing to do. But we need to update RDCTL to
> compute the
> correct value from CR_IPENDING & CR_IENABLE, and update WRCTL to ignore
> writes.
Since you've already fixed that on " target/nios2: Rewrite interrupt handling"
patchset, I guess I'll need to rebase on it once it's merged.
Until then for my next version I plan to just keep that with a "TODO" comment.
> > +if (cpu->rnmi) {
> > +return !(env->regs[CR_STATUS] & CR_STATUS_NMI);
> > +}
> I think this should be a separate
> #define CPU_INTERRUPT_NMI CPU_INTERRUPT_TGT_EXT_0
The NMI is not a separate interrupt line. It's part of the interrupt sideband
and is set just like the Requested-Handler-Address or any other EIC field.
Could you explain why NMI should be separate? What makes it different from
other EIC fields?
Amir
Re: [PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)
On Thu, 24 Feb 2022 at 23:56, Richard Henderson
wrote:
>
> On 2/24/22 03:48, Amir Gonnen wrote:
> > +static void nios2_cpu_set_eic_irq(Nios2CPU *cpu, int level)
> > +{
> > +CPUNios2State *env = &cpu->env;
> > +CPUState *cs = CPU(cpu);
> > +
> > +env->irq_pending = level;
> > +
> > +if (env->irq_pending && nios2_take_eic_irq(cpu)) {
> > +env->irq_pending = 0;
> > +cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> > +} else if (!env->irq_pending) {
> > +cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> > +}
> > +}
>
> This looks wrong. Of course, so does nios2_cpu_set_irq, from which you've
> cribbed this.
Bit of a thread from 2020 on nios2's odd interrupt handling;
https://lore.kernel.org/qemu-devel/3260735e-05ab-2d42-f7e4-914ad804f...@linaro.org/
though it's mostly just you saying the same things you're saying now :-)
xtensa's also weird in a similar way.
-- PMM
Re: [PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)
On 2/24/22 03:48, Amir Gonnen wrote:
+static void nios2_cpu_set_eic_irq(Nios2CPU *cpu, int level)
+{
+CPUNios2State *env = &cpu->env;
+CPUState *cs = CPU(cpu);
+
+env->irq_pending = level;
+
+if (env->irq_pending && nios2_take_eic_irq(cpu)) {
+env->irq_pending = 0;
+cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+} else if (!env->irq_pending) {
+cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
+}
+}
This looks wrong. Of course, so does nios2_cpu_set_irq, from which you've
cribbed this.
(1) The unset of irq_pending looks wrong, though come to think of it, it's existence also
looks wrong. I think that it's completely redundant with cpu->interrupt_request, and that
you should only use cpu_interrupt/cpu_reset_interrupt from this level.
Which also means that nios2_check_interrupts and callers all the way back up are also
incorrect. All that should be required from wrctl status is the return to the main loop
that you get from DISAS_UPDATE and tcg_gen_exit_tb.
Which also means that ipending is implemented incorrectly. The current manipulation of
CR_IPENDING in nios2_cpu_set_irc should instead be manipulating an internal "external hw
request", per Figure 3-2 in NII51003.
For our purposes, I think simply re-using env->regs[CR_IPENDING] as the external hw
request word is the right thing to do. But we need to update RDCTL to compute the
correct value from CR_IPENDING & CR_IENABLE, and update WRCTL to ignore writes.
(2) Checking nios2_take_eic_irq here, or CR_STATUS_PIE in nios2_cpu_set_irq is incorrect.
If you check this here, then you'll miss the interrupt when the interrupt enable bit is
reset. These checks belong in nios2_cpu_exec_interrupt.
+if (cpu->rnmi) {
+return !(env->regs[CR_STATUS] & CR_STATUS_NMI);
+}
I think this should be a separate
#define CPU_INTERRUPT_NMI CPU_INTERRUPT_TGT_EXT_0
r~
[PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)
Implement Exteral Interrupt Controller interface (EIC).
Added intc_present property, true by default. When set to false, nios2
uses the EIC interface when handling IRQ. When set to true (default)
it uses the internal interrupt controller.
When nios2 recieves irq, it first checks intc_present to decide whether
to use the internal interrupt controller or the EIC.
The EIC is triggered by IRQ gpio but also recieves additional data from
the external interrupt controller (such as VIC): rha, ril, rrs and rnmi.
The interrupt controller is expected to raise IRQ after setting these
fields on Nios2CPU.
rha, ril, rrs and rnmi are used when EIC handles external interrupt, in
order to decide if to take the interrupt now, which shadow register set
to use, which PC to jump to, whether to set NMI flag, etc.
Signed-off-by: Amir Gonnen
---
target/nios2/cpu.c | 58 +---
target/nios2/cpu.h | 22 +++
target/nios2/helper.c| 33 ---
target/nios2/op_helper.c | 7 +++--
4 files changed, 105 insertions(+), 15 deletions(-)
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index 0886705818..9bd8a6301a 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -55,6 +55,7 @@ static void nios2_cpu_reset(DeviceState *dev)
memset(env->regs, 0, sizeof(uint32_t) * NUM_CORE_REGS);
memset(env->shadow_regs, 0, sizeof(uint32_t) * NUM_REG_SETS * NUM_GP_REGS);
+env->regs[CR_STATUS] |= CR_STATUS_RSIE;
env->regs[R_PC] = cpu->reset_addr;
#if defined(CONFIG_USER_ONLY)
@@ -65,10 +66,28 @@ static void nios2_cpu_reset(DeviceState *dev)
#endif
}
+bool nios2_take_eic_irq(const Nios2CPU *cpu)
+{
+const CPUNios2State *env = &cpu->env;
+
+if (cpu->rnmi) {
+return !(env->regs[CR_STATUS] & CR_STATUS_NMI);
+}
+
+if (((env->regs[CR_STATUS] & CR_STATUS_PIE) == 0) ||
+(cpu->ril <= cpu_get_il(env)) ||
+(cpu->rrs == cpu_get_crs(env) &&
+ !(env->regs[CR_STATUS] & CR_STATUS_RSIE))) {
+
+return false;
+}
+
+return true;
+}
+
#ifndef CONFIG_USER_ONLY
-static void nios2_cpu_set_irq(void *opaque, int irq, int level)
+static void nios2_cpu_set_intc_irq(Nios2CPU *cpu, int irq, int level)
{
-Nios2CPU *cpu = opaque;
CPUNios2State *env = &cpu->env;
CPUState *cs = CPU(cpu);
@@ -83,6 +102,32 @@ static void nios2_cpu_set_irq(void *opaque, int irq, int
level)
cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
}
}
+
+static void nios2_cpu_set_eic_irq(Nios2CPU *cpu, int level)
+{
+CPUNios2State *env = &cpu->env;
+CPUState *cs = CPU(cpu);
+
+env->irq_pending = level;
+
+if (env->irq_pending && nios2_take_eic_irq(cpu)) {
+env->irq_pending = 0;
+cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+} else if (!env->irq_pending) {
+cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
+}
+}
+
+static void nios2_cpu_set_irq(void *opaque, int irq, int level)
+{
+Nios2CPU *cpu = opaque;
+
+if (cpu->intc_present) {
+nios2_cpu_set_intc_irq(cpu, irq, level);
+} else {
+nios2_cpu_set_eic_irq(cpu, level);
+}
+}
#endif
static void nios2_cpu_initfn(Object *obj)
@@ -94,13 +139,6 @@ static void nios2_cpu_initfn(Object *obj)
#if !defined(CONFIG_USER_ONLY)
mmu_init(&cpu->env);
-/*
- * These interrupt lines model the IIC (internal interrupt
- * controller). QEMU does not currently support the EIC
- * (external interrupt controller) -- if we did it would be
- * a separate device in hw/intc with a custom interface to
- * the CPU, and boards using it would not wire up these IRQ lines.
- */
qdev_init_gpio_in_named(DEVICE(cpu), nios2_cpu_set_irq, "IRQ", 32);
#endif
}
@@ -202,6 +240,8 @@ static Property nios2_properties[] = {
DEFINE_PROP_UINT32("mmu_tlb_num_ways", Nios2CPU, tlb_num_ways, 16),
/* ALTR,tlb-num-entries */
DEFINE_PROP_UINT32("mmu_pid_num_entries", Nios2CPU, tlb_num_entries, 256),
+/* interrupt-controller (internal) */
+DEFINE_PROP_BOOL("intc_present", Nios2CPU, intc_present, true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
index 1d3503825b..1b3d0ed25e 100644
--- a/target/nios2/cpu.h
+++ b/target/nios2/cpu.h
@@ -92,12 +92,14 @@ struct Nios2CPUClass {
#define CR_STATUS_EH (1 << 2)
#define CR_STATUS_IH (1 << 3)
#define CR_STATUS_IL (63 << 4)
+#define CR_STATUS_IL_OFFSET 6
#define CR_STATUS_CRS (63 << 10)
#define CR_STATUS_CRS_OFFSET 10
#define CR_STATUS_PRS (63 << 16)
#define CR_STATUS_PRS_OFFSET 16
#define CR_STATUS_NMI (1 << 22)
#define CR_STATUS_RSIE (1 << 23)
+#define CR_STATUS_SRS (1 << 31)
#define CR_ESTATUS (CR_BASE + 1)
#define CR_BSTATUS (CR_BASE + 2)
#define CR_IENABLE (CR_BASE + 3)
@@ -189,6 +191,7 @@ struct Nios2CPU {
CPUNios2State env;
bool mmu_present;
+bool intc_present;
uint32_t pid_num_bits;
uint32_t tlb_num_ways;
uint32_t t
