[PATCH v2 0/4] target/nios2: Shadow register set, EIC and VIC

[PATCH v2 0/4] target/nios2: Shadow register set, EIC and VIC

[Amir Gonnen]

Implement nios2 Shadow register set, EIC and VIC.

Currently nios2 on QEMU contains an internal Interrupt Controller.
The nios2 architecture can support a more powerful External Interrupt
Controller (EIC) instead of the internal, and implements special cpu
features to support it: Shadow register set and External Interrupt
Controller Interface.

This patch series introduces the necessary changes to the nios2 cpu to
support an External Interrupt Controller, and includes a Vectored
Interrupt Controller (VIC) device that can be attach to the EIC.

Following Peter's suggestion in the previous version, I've splitted this
into several independant patches that rely on each other incrementally
and added a board that wires up the VIC:

1. Shadow Register Set support on the nios2 core
2. External Interrupt Controller interface on the nios2 core
3. Vectored Interrupt Controller
4. A board that uses the VIC instead of the default internal interrupt
   controller

Signed-off-by: Amir Gonnen <amir.gonnen@neuroblade.ai>

Amir Gonnen (4):
  target/nios2: Shadow register set
  target/nios2: Exteral Interrupt Controller (EIC)
  hw/intc: Vectored Interrupt Controller (VIC)
  hw/nios2: Machine with a Vectored Interrupt Controller

 hw/intc/Kconfig           |   4 +
 hw/intc/meson.build       |   1 +
 hw/intc/nios2_vic.c       | 327 ++++++++++++++++++++++++++++++++++++++
 hw/nios2/10m50_devboard.c |  64 +++++++-
 target/nios2/cpu.c        |  59 +++++--
 target/nios2/cpu.h        |  69 +++++++-
 target/nios2/helper.c     |  33 +++-
 target/nios2/helper.h     |   3 +
 target/nios2/op_helper.c  |  31 +++-
 target/nios2/translate.c  |  32 +++-
 10 files changed, 597 insertions(+), 26 deletions(-)
 create mode 100644 hw/intc/nios2_vic.c

--
2.25.1


The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.

[PATCH v2 1/4] target/nios2: Shadow register set

[Amir Gonnen]

Implement shadow register set and related instructions
rdprs, wrprs. Fix eret to update either status or sstatus
according to current register set.
eret also changes register set when needed.

Signed-off-by: Amir Gonnen <amir.gonnen@neuroblade.ai>
---
 target/nios2/cpu.c       |  1 +
 target/nios2/cpu.h       | 47 ++++++++++++++++++++++++++++++++++++++--
 target/nios2/helper.h    |  3 +++
 target/nios2/op_helper.c | 24 ++++++++++++++++++++
 target/nios2/translate.c | 32 ++++++++++++++++++++++-----
 5 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index 4cade61e93..0886705818 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -54,6 +54,7 @@ static void nios2_cpu_reset(DeviceState *dev)
     ncc->parent_reset(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[R_PC] = cpu->reset_addr;

 #if defined(CONFIG_USER_ONLY)
diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
index d2ba0c5bbd..1d3503825b 100644
--- a/target/nios2/cpu.h
+++ b/target/nios2/cpu.h
@@ -57,9 +57,14 @@ struct Nios2CPUClass {
 #define EXCEPTION_ADDRESS     0x00000004
 #define FAST_TLB_MISS_ADDRESS 0x00000008

+#define NUM_GP_REGS 32
+#define NUM_CR_REGS 32

 /* GP regs + CR regs + PC */
-#define NUM_CORE_REGS (32 + 32 + 1)
+#define NUM_CORE_REGS (NUM_GP_REGS + NUM_CR_REGS + 1)
+
+/* 63 shadow register sets. 0 is the primary set */
+#define NUM_REG_SETS 64

 /* General purpose register aliases */
 #define R_ZERO   0
@@ -80,7 +85,7 @@ struct Nios2CPUClass {
 #define R_RA     31

 /* Control register aliases */
-#define CR_BASE  32
+#define CR_BASE  NUM_GP_REGS
 #define CR_STATUS    (CR_BASE + 0)
 #define   CR_STATUS_PIE  (1 << 0)
 #define   CR_STATUS_U    (1 << 1)
@@ -88,7 +93,9 @@ struct Nios2CPUClass {
 #define   CR_STATUS_IH   (1 << 3)
 #define   CR_STATUS_IL   (63 << 4)
 #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_ESTATUS   (CR_BASE + 1)
@@ -131,6 +138,7 @@ struct Nios2CPUClass {

 /* Other registers */
 #define R_PC         64
+#define R_SSTATUS    30

 /* Exceptions */
 #define EXCP_BREAK    0x1000
@@ -157,6 +165,7 @@ struct Nios2CPUClass {

 struct CPUNios2State {
     uint32_t regs[NUM_CORE_REGS];
+    uint32_t shadow_regs[NUM_REG_SETS][NUM_GP_REGS];

 #if !defined(CONFIG_USER_ONLY)
     Nios2MMU mmu;
@@ -246,4 +255,38 @@ static inline void cpu_get_tb_cpu_state(CPUNios2State *env, target_ulong *pc,
     *flags = (env->regs[CR_STATUS] & (CR_STATUS_EH | CR_STATUS_U));
 }

+static inline uint32_t cpu_get_crs(const CPUNios2State *env)
+{
+    return (env->regs[CR_STATUS] & CR_STATUS_CRS)
+                    >> CR_STATUS_CRS_OFFSET;
+}
+
+static inline uint32_t cpu_get_prs(const CPUNios2State *env)
+{
+    return (env->regs[CR_STATUS] & CR_STATUS_PRS)
+                    >> CR_STATUS_PRS_OFFSET;
+}
+
+static inline void cpu_change_reg_set(CPUNios2State *env, uint32_t prev_set,
+                                      uint32_t new_set)
+{
+    if (new_set == prev_set) {
+        return;
+    }
+    memcpy(env->shadow_regs[prev_set], env->regs,
+           sizeof(uint32_t) * NUM_GP_REGS);
+    memcpy(env->regs, env->shadow_regs[new_set],
+           sizeof(uint32_t) * NUM_GP_REGS);
+    env->regs[CR_STATUS] = (env->regs[CR_STATUS] & (~CR_STATUS_PRS))
+                       | ((prev_set << CR_STATUS_PRS_OFFSET) & CR_STATUS_PRS);
+    env->regs[CR_STATUS] = (env->regs[CR_STATUS] & (~CR_STATUS_CRS))
+                       | ((new_set << CR_STATUS_CRS_OFFSET) & CR_STATUS_CRS);
+}
+
+static inline void cpu_set_crs(CPUNios2State *env, uint32_t value)
+{
+    uint32_t crs = cpu_get_crs(env);
+    cpu_change_reg_set(env, crs, value);
+}
+
 #endif /* NIOS2_CPU_H */
diff --git a/target/nios2/helper.h b/target/nios2/helper.h
index 6c8f0b5b35..3e5c016e9c 100644
--- a/target/nios2/helper.h
+++ b/target/nios2/helper.h
@@ -18,6 +18,9 @@
  * <http://www.gnu.org/licenses/lgpl-2.1.html>
  */

+DEF_HELPER_2(eret, void, env, i32)
+DEF_HELPER_3(wrprs, void, env, i32, i32)
+DEF_HELPER_2(rdprs, i32, env, i32)
 DEF_HELPER_FLAGS_2(raise_exception, TCG_CALL_NO_WG, noreturn, env, i32)

 #if !defined(CONFIG_USER_ONLY)
diff --git a/target/nios2/op_helper.c b/target/nios2/op_helper.c
index a59003855a..5e4f7a47ae 100644
--- a/target/nios2/op_helper.c
+++ b/target/nios2/op_helper.c
@@ -59,3 +59,27 @@ void helper_raise_exception(CPUNios2State *env, uint32_t index)
     cs->exception_index = index;
     cpu_loop_exit(cs);
 }
+
+void helper_wrprs(CPUNios2State *env, uint32_t reg_index, uint32_t value)
+{
+    uint32_t prs = cpu_get_prs(env);
+    env->shadow_regs[prs][reg_index] = value;
+}
+
+uint32_t helper_rdprs(CPUNios2State *env, uint32_t reg_index)
+{
+    uint32_t prs = cpu_get_prs(env);
+    return env->shadow_regs[prs][reg_index];
+}
+
+void helper_eret(CPUNios2State *env, uint32_t new_pc)
+{
+    uint32_t crs = cpu_get_crs(env);
+    if (crs == 0) {
+        env->regs[CR_STATUS] = env->regs[CR_ESTATUS];
+    } else {
+        env->regs[CR_STATUS] = env->regs[R_SSTATUS];
+    }
+    cpu_change_reg_set(env, crs, cpu_get_crs(env));
+    env->regs[R_PC] = new_pc;
+}
diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index f9abc2fdd2..e22ab1996a 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -205,6 +205,19 @@ static void call(DisasContext *dc, uint32_t code, uint32_t flags)
 /*
  * I-Type instructions
  */
+
+/*
+ * rB <- prs.rA + sigma(IMM16)
+ */
+static void rdprs(DisasContext *dc, uint32_t code, uint32_t flags)
+{
+    I_TYPE(instr, code);
+    TCGv t = tcg_temp_new();
+    gen_helper_rdprs(t, cpu_env, tcg_const_i32(instr.a));
+    tcg_gen_addi_tl(cpu_R[instr.b], t, instr.imm16.s);
+    tcg_temp_free(t);
+}
+
 /* Load instructions */
 static void gen_ldx(DisasContext *dc, uint32_t code, uint32_t flags)
 {
@@ -365,7 +378,7 @@ static const Nios2Instruction i_type_instructions[] = {
     INSTRUCTION_FLG(gen_stx, MO_SL),                  /* stwio */
     INSTRUCTION_FLG(gen_bxx, TCG_COND_LTU),           /* bltu */
     INSTRUCTION_FLG(gen_ldx, MO_UL),                  /* ldwio */
-    INSTRUCTION_UNIMPLEMENTED(),                      /* rdprs */
+    INSTRUCTION(rdprs),                               /* rdprs */
     INSTRUCTION_ILLEGAL(),
     INSTRUCTION_FLG(handle_r_type_instr, 0),          /* R-Type */
     INSTRUCTION_NOP(),                                /* flushd */
@@ -378,14 +391,23 @@ static const Nios2Instruction i_type_instructions[] = {
 /*
  * R-Type instructions
  */
+
+/*
+ * prs.rC <- rA
+ */
+static void wrprs(DisasContext *dc, uint32_t code, uint32_t flags)
+{
+    R_TYPE(instr, code);
+    gen_helper_wrprs(cpu_env, tcg_const_i32(instr.c), cpu_R[instr.a]);
+}
+
 /*
- * status <- estatus
+ * status <- CRS == 0? estatus: sstatus
  * PC <- ea
  */
 static void eret(DisasContext *dc, uint32_t code, uint32_t flags)
 {
-    tcg_gen_mov_tl(cpu_R[CR_STATUS], cpu_R[CR_ESTATUS]);
-    tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_EA]);
+    gen_helper_eret(cpu_env, cpu_R[R_EA]);

     dc->base.is_jmp = DISAS_JUMP;
 }
@@ -672,7 +694,7 @@ static const Nios2Instruction r_type_instructions[] = {
     INSTRUCTION_ILLEGAL(),
     INSTRUCTION(slli),                                /* slli */
     INSTRUCTION(sll),                                 /* sll */
-    INSTRUCTION_UNIMPLEMENTED(),                      /* wrprs */
+    INSTRUCTION(wrprs),                               /* wrprs */
     INSTRUCTION_ILLEGAL(),
     INSTRUCTION(or),                                  /* or */
     INSTRUCTION(mulxsu),                              /* mulxsu */
--
2.25.1


The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.

[PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)

[Amir Gonnen]

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 <amir.gonnen@neuroblade.ai>
---
 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 tlb_num_entries;
@@ -197,10 +200,17 @@ struct Nios2CPU {
     uint32_t reset_addr;
     uint32_t exception_addr;
     uint32_t fast_tlb_miss_addr;
+
+    /* External Interrupt Controller Interface */
+    uint32_t rha; /* Requested handler address */
+    uint32_t ril; /* Requested interrupt level */
+    uint32_t rrs; /* Requested register set */
+    uint32_t rnmi; /* Requested nonmaskable interrupt */
 };


 void nios2_tcg_init(void);
+bool nios2_take_eic_irq(const Nios2CPU *cpu);
 void nios2_cpu_do_interrupt(CPUState *cs);
 void dump_mmu(CPUNios2State *env);
 void nios2_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
@@ -255,6 +265,18 @@ static inline void cpu_get_tb_cpu_state(CPUNios2State *env, target_ulong *pc,
     *flags = (env->regs[CR_STATUS] & (CR_STATUS_EH | CR_STATUS_U));
 }

+static inline uint32_t cpu_get_il(const CPUNios2State *env)
+{
+    return (env->regs[CR_STATUS] & CR_STATUS_IL)
+                    >> CR_STATUS_IL_OFFSET;
+}
+
+static inline void cpu_set_il(CPUNios2State *env, uint32_t value)
+{
+    env->regs[CR_STATUS] = (env->regs[CR_STATUS] & ~CR_STATUS_IL)
+                    | ((value << CR_STATUS_IL_OFFSET) & CR_STATUS_IL);
+}
+
 static inline uint32_t cpu_get_crs(const CPUNios2State *env)
 {
     return (env->regs[CR_STATUS] & CR_STATUS_CRS)
diff --git a/target/nios2/helper.c b/target/nios2/helper.c
index e5c98650e1..bc022e969d 100644
--- a/target/nios2/helper.c
+++ b/target/nios2/helper.c
@@ -54,21 +54,46 @@ void nios2_cpu_do_interrupt(CPUState *cs)
     Nios2CPU *cpu = NIOS2_CPU(cs);
     CPUNios2State *env = &cpu->env;

+    if (cs->exception_index != EXCP_IRQ) {
+        cpu_set_crs(env, 0);
+    }
+
     switch (cs->exception_index) {
     case EXCP_IRQ:
         assert(env->regs[CR_STATUS] & CR_STATUS_PIE);

         qemu_log_mask(CPU_LOG_INT, "interrupt at pc=%x\n", env->regs[R_PC]);

-        env->regs[CR_ESTATUS] = env->regs[CR_STATUS];
-        env->regs[CR_STATUS] |= CR_STATUS_IH;
+        uint32_t last_status = env->regs[CR_STATUS];
         env->regs[CR_STATUS] &= ~(CR_STATUS_PIE | CR_STATUS_U);

         env->regs[CR_EXCEPTION] &= ~(0x1F << 2);
         env->regs[CR_EXCEPTION] |= (cs->exception_index & 0x1F) << 2;

-        env->regs[R_EA] = env->regs[R_PC] + 4;
-        env->regs[R_PC] = cpu->exception_addr;
+        if (!cpu->intc_present) {
+            cpu_set_crs(env, cpu->rrs);
+            cpu_set_il(env, cpu->ril);
+            if (cpu->rnmi) {
+                env->regs[CR_STATUS] |= CR_STATUS_NMI;
+            } else {
+                env->regs[CR_STATUS] &= ~CR_STATUS_NMI;
+            }
+            if (cpu->rrs == 0) {
+                env->regs[CR_ESTATUS] = last_status;
+            } else {
+                env->regs[R_SSTATUS] = last_status;
+                env->regs[R_SSTATUS] |= CR_STATUS_SRS;
+            }
+            env->regs[CR_STATUS] |= CR_STATUS_IH;
+            env->regs[R_EA] = env->regs[R_PC] + 4;
+            env->regs[R_PC] = cpu->rha;
+
+        } else {
+            env->regs[CR_ESTATUS] = last_status;
+            env->regs[R_EA] = env->regs[R_PC] + 4;
+            env->regs[R_PC] = cpu->exception_addr;
+        }
+
         break;

     case EXCP_TLBD:
diff --git a/target/nios2/op_helper.c b/target/nios2/op_helper.c
index 5e4f7a47ae..1de823a067 100644
--- a/target/nios2/op_helper.c
+++ b/target/nios2/op_helper.c
@@ -40,8 +40,11 @@ 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);
+        Nios2CPU *cpu = (Nios2CPU *)env_cpu(env);
+        if (cpu->intc_present || nios2_take_eic_irq(cpu)) {
+            env->irq_pending = 0;
+            cpu_interrupt(env_cpu(env), CPU_INTERRUPT_HARD);
+        }
     }
 }

--
2.25.1


The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.

[PATCH v2 3/4] hw/intc: Vectored Interrupt Controller (VIC)

[Amir Gonnen]

Implement nios2 Vectored Interrupt Controller (VIC).
VIC is connected to EIC. It needs to update rha, ril, rrs and rnmi
fields on Nios2CPU before raising an IRQ.
For that purpose, VIC has a "cpu" property which should refer to the
nios2 cpu and set by the board that connects VIC.

Signed-off-by: Amir Gonnen <amir.gonnen@neuroblade.ai>
---
 hw/intc/Kconfig     |   4 +
 hw/intc/meson.build |   1 +
 hw/intc/nios2_vic.c | 327 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 332 insertions(+)
 create mode 100644 hw/intc/nios2_vic.c

diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index 528e77b4a6..8000241428 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -81,3 +81,7 @@ config GOLDFISH_PIC

 config M68K_IRQC
     bool
+
+config NIOS2_VIC
+    default y
+    depends on NIOS2 && TCG
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index 7466024402..547e16eb2d 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -61,3 +61,4 @@ specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
                if_true: files('spapr_xive_kvm.c'))
 specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
 specific_ss.add(when: 'CONFIG_M68K_IRQC', if_true: files('m68k_irqc.c'))
+specific_ss.add(when: 'CONFIG_NIOS2_VIC', if_true: files('nios2_vic.c'))
diff --git a/hw/intc/nios2_vic.c b/hw/intc/nios2_vic.c
new file mode 100644
index 0000000000..a9c9b3e7ac
--- /dev/null
+++ b/hw/intc/nios2_vic.c
@@ -0,0 +1,327 @@
+/*
+ * Vectored Interrupt Controller for nios2 processor
+ *
+ * Copyright (c) 2022 Neuroblade
+ *
+ * Interface:
+ * QOM property "cpu": link to the Nios2 CPU (must be set)
+ * Unnamed GPIO inputs 0..NIOS2_VIC_MAX_IRQ-1: input IRQ lines
+ * IRQ should be connected to nios2 IRQ0.
+ *
+ * Reference: "Embedded Peripherals IP User Guide
+ *             for Intel® Quartus® Prime Design Suite: 21.4"
+ * Chapter 38 "Vectored Interrupt Controller Core"
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/bitops.h"
+#include "qemu/log.h"
+#include "qom/object.h"
+#include "cpu.h"
+
+#define LOG_VIC(...) qemu_log_mask(CPU_LOG_INT, ##__VA_ARGS__)
+
+#define TYPE_NIOS2_VIC "nios2-vic"
+
+OBJECT_DECLARE_SIMPLE_TYPE(Nios2Vic, NIOS2_VIC)
+
+#define NIOS2_VIC_MAX_IRQ 32
+
+enum {
+    INT_CONFIG0 = 0,
+    INT_CONFIG31 = 31,
+    INT_ENABLE = 32,
+    INT_ENABLE_SET = 33,
+    INT_ENABLE_CLR = 34,
+    INT_PENDING = 35,
+    INT_RAW_STATUS = 36,
+    SW_INTERRUPT = 37,
+    SW_INTERRUPT_SET = 38,
+    SW_INTERRUPT_CLR = 39,
+    VIC_CONFIG = 40,
+    VIC_STATUS = 41,
+    VEC_TBL_BASE = 42,
+    VEC_TBL_ADDR = 43,
+    CSR_COUNT /* Last! */
+};
+
+struct Nios2Vic {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    qemu_irq output_int;
+
+    /* properties */
+    CPUState *cpu;
+    MemoryRegion csr;
+
+    uint32_t int_config[32];
+    uint32_t vic_config;
+    uint32_t int_raw_status;
+    uint32_t int_enable;
+    uint32_t sw_int;
+    uint32_t vic_status;
+    uint32_t vec_tbl_base;
+    uint32_t vec_tbl_addr;
+};
+
+/* Requested interrupt level (INT_CONFIG[0:5]) */
+static inline uint32_t vic_int_config_ril(const Nios2Vic *vic, int irq_num)
+{
+    return extract32(vic->int_config[irq_num], 0, 6);
+}
+
+/* Requested NMI (INT_CONFIG[6]) */
+static inline uint32_t vic_int_config_rnmi(const Nios2Vic *vic, int irq_num)
+{
+    return extract32(vic->int_config[irq_num], 6, 1);
+}
+
+/* Requested register set (INT_CONFIG[7:12]) */
+static inline uint32_t vic_int_config_rrs(const Nios2Vic *vic, int irq_num)
+{
+    return extract32(vic->int_config[irq_num], 7, 6);
+}
+
+static inline uint32_t vic_config_vec_size(const Nios2Vic *vic)
+{
+    return 1 << (2 + extract32(vic->vic_config, 0, 3));
+}
+
+static inline uint32_t vic_int_pending(const Nios2Vic *vic)
+{
+    return (vic->int_raw_status | vic->sw_int) & vic->int_enable;
+}
+
+static void vic_update_irq(Nios2Vic *vic)
+{
+    Nios2CPU *cpu = NIOS2_CPU(vic->cpu);
+    uint32_t pending = vic_int_pending(vic);
+    int irq = -1;
+    int max_ril = 0;
+
+    vic->vec_tbl_addr = 0;
+    vic->vic_status = 0;
+
+    if (pending == 0) {
+        qemu_irq_lower(vic->output_int);
+        return;
+    }
+
+    for (int i = 0; i < NIOS2_VIC_MAX_IRQ; i++) {
+        if (pending & BIT(i)) {
+            int ril = vic_int_config_ril(vic, i);
+            if (ril > max_ril) {
+                irq = i;
+                max_ril = ril;
+            }
+        }
+    }
+
+    if (irq < 0) {
+        qemu_irq_lower(vic->output_int);
+        return;
+    }
+
+    vic->vec_tbl_addr = irq * vic_config_vec_size(vic) + vic->vec_tbl_base;
+    vic->vic_status = deposit32(vic->vic_status, 0, 6, irq) | BIT(31);
+
+    cpu->rha = vic->vec_tbl_addr;
+    cpu->ril = max_ril;
+    cpu->rrs = vic_int_config_rrs(vic, irq);
+    cpu->rnmi = vic_int_config_rnmi(vic, irq);
+
+    qemu_irq_raise(vic->output_int);
+}
+
+static void vic_set_irq(void *opaque, int irq_num, int level)
+{
+    Nios2Vic *vic = opaque;
+    LOG_VIC("%s: irq %d level %d", __func__, irq_num, level);
+
+    if (level) {
+        vic->int_raw_status |= BIT(irq_num);
+    } else {
+        vic->int_raw_status &= ~BIT(irq_num);
+    }
+
+    vic_update_irq(vic);
+}
+
+static void nios2_vic_reset(DeviceState *dev)
+{
+    Nios2Vic *vic = NIOS2_VIC(dev);
+    memset(&vic->int_config, 0, sizeof(vic->int_config));
+    vic->vic_config = 0;
+    vic->int_raw_status = 0;
+    vic->int_enable = 0;
+    vic->sw_int = 0;
+    vic->vic_status = 0;
+    vic->vec_tbl_base = 0;
+    vic->vec_tbl_addr = 0;
+}
+
+static uint64_t nios2_vic_csr_read(void *opaque, hwaddr offset, unsigned size)
+{
+    Nios2Vic *vic = opaque;
+    int index = offset / 4;
+
+    switch (index) {
+    case INT_CONFIG0 ... INT_CONFIG31:
+        return vic->int_config[index - INT_CONFIG0];
+    case INT_ENABLE:
+        return vic->int_enable;
+    case INT_PENDING:
+        return vic_int_pending(vic);
+    case INT_RAW_STATUS:
+        return vic->int_raw_status;
+    case SW_INTERRUPT:
+        return vic->sw_int;
+    case VIC_CONFIG:
+        return vic->vic_config;
+    case VIC_STATUS:
+        return vic->vic_status;
+    case VEC_TBL_BASE:
+        return vic->vec_tbl_base;
+    case VEC_TBL_ADDR:
+        return vic->vec_tbl_addr;
+    default:
+        return 0;
+    }
+}
+
+static void nios2_vic_csr_write(void *opaque, hwaddr offset, uint64_t value,
+                                unsigned size)
+{
+    Nios2Vic *vic = opaque;
+    int index = offset / 4;
+
+    switch (index) {
+    case INT_CONFIG0 ... INT_CONFIG31:
+        vic->int_config[index - INT_CONFIG0] = value;
+        break;
+    case INT_ENABLE:
+        vic->int_enable = value;
+        break;
+    case INT_ENABLE_SET:
+        vic->int_enable |= value;
+        break;
+    case INT_ENABLE_CLR:
+        vic->int_enable &= ~value;
+        break;
+    case SW_INTERRUPT:
+        vic->sw_int = value;
+        break;
+    case SW_INTERRUPT_SET:
+        vic->sw_int |= value;
+        break;
+    case SW_INTERRUPT_CLR:
+        vic->sw_int &= ~value;
+        break;
+    case VIC_CONFIG:
+        vic->vic_config = value;
+        break;
+    case VEC_TBL_BASE:
+        vic->vec_tbl_base = value;
+        break;
+    default:
+        ;
+    }
+
+    vic_update_irq(vic);
+}
+
+static const MemoryRegionOps nios2_vic_csr_ops = {
+    .read = nios2_vic_csr_read,
+    .write = nios2_vic_csr_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = { .min_access_size = 4, .max_access_size = 4 }
+};
+
+static void nios2_vic_realize(DeviceState *dev, Error **errp)
+{
+    Nios2Vic *vic = NIOS2_VIC(dev);
+
+    if (!vic->cpu) {
+        /* This is a programming error in the code using this device */
+        error_setg(errp, "nios2-vic 'cpu' link property was not set");
+        return;
+    }
+
+    sysbus_init_irq(SYS_BUS_DEVICE(dev), &vic->output_int);
+    qdev_init_gpio_in(dev, vic_set_irq, NIOS2_VIC_MAX_IRQ);
+
+    memory_region_init_io(&vic->csr, OBJECT(dev), &nios2_vic_csr_ops, vic,
+                          "nios2.vic.csr", CSR_COUNT * sizeof(uint32_t));
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &vic->csr);
+}
+
+static Property nios2_vic_properties[] = {
+    DEFINE_PROP_LINK("cpu", Nios2Vic, cpu, TYPE_CPU, CPUState *),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static const VMStateDescription nios2_vic_vmstate = {
+    .name = "nios2-vic",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]){ VMSTATE_UINT32_ARRAY(int_config, Nios2Vic, 32),
+                                VMSTATE_UINT32(vic_config, Nios2Vic),
+                                VMSTATE_UINT32(int_raw_status, Nios2Vic),
+                                VMSTATE_UINT32(int_enable, Nios2Vic),
+                                VMSTATE_UINT32(sw_int, Nios2Vic),
+                                VMSTATE_UINT32(vic_status, Nios2Vic),
+                                VMSTATE_UINT32(vec_tbl_base, Nios2Vic),
+                                VMSTATE_UINT32(vec_tbl_addr, Nios2Vic),
+                                VMSTATE_END_OF_LIST() },
+};
+
+
+static void nios2_vic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = nios2_vic_reset;
+    dc->realize = nios2_vic_realize;
+    dc->vmsd = &nios2_vic_vmstate;
+    device_class_set_props(dc, nios2_vic_properties);
+}
+
+static const TypeInfo nios2_vic_info = {
+    .name = TYPE_NIOS2_VIC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(Nios2Vic),
+    .class_init = nios2_vic_class_init,
+};
+
+static void nios2_vic_register_types(void)
+{
+    type_register_static(&nios2_vic_info);
+}
+
+type_init(nios2_vic_register_types);
--
2.25.1


The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.

[PATCH v2 4/4] hw/nios2: Machine with a Vectored Interrupt Controller

[Amir Gonnen]

Demonstrate how to use nios2 VIC on a machine.
Introduce a new machine "10m50-ghrd-vic" which is based on "10m50-ghrd"
with a VIC attached and internal interrupt controller removed.

When VIC is present, irq0 connects the VIC to the cpu, intc_present
is set to false to disable the internal interrupt controller, and the
devices on the machine are attached to the VIC (and not directly to cpu).
To allow VIC update EIC fields, we set the "cpu" property of the VIC
with a reference to the nios2 cpu.

Signed-off-by: Amir Gonnen <amir.gonnen@neuroblade.ai>
---
 hw/nios2/10m50_devboard.c | 64 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)

diff --git a/hw/nios2/10m50_devboard.c b/hw/nios2/10m50_devboard.c
index 3d1205b8bd..9f62a2993f 100644
--- a/hw/nios2/10m50_devboard.c
+++ b/hw/nios2/10m50_devboard.c
@@ -36,10 +36,23 @@

 #include "boot.h"

+#define TYPE_NIOS2_MACHINE  MACHINE_TYPE_NAME("10m50-ghrd")
+typedef struct Nios2MachineClass Nios2MachineClass;
+DECLARE_OBJ_CHECKERS(MachineState, Nios2MachineClass,
+                     NIOS2_MACHINE, TYPE_NIOS2_MACHINE)
+
 #define BINARY_DEVICE_TREE_FILE    "10m50-devboard.dtb"

+struct Nios2MachineClass {
+    MachineClass parent_obj;
+
+    bool vic;
+};
+
 static void nios2_10m50_ghrd_init(MachineState *machine)
 {
+    Nios2MachineClass *nmc = NIOS2_MACHINE_GET_CLASS(machine);
+
     Nios2CPU *cpu;
     DeviceState *dev;
     MemoryRegion *address_space_mem = get_system_memory();
@@ -74,8 +87,24 @@ static void nios2_10m50_ghrd_init(MachineState *machine)

     /* Create CPU -- FIXME */
     cpu = NIOS2_CPU(cpu_create(TYPE_NIOS2_CPU));
-    for (i = 0; i < 32; i++) {
-        irq[i] = qdev_get_gpio_in_named(DEVICE(cpu), "IRQ", i);
+
+    if (nmc->vic) {
+        DeviceState *dev = qdev_new("nios2-vic");
+
+        object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_fatal);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        cpu->intc_present = false;
+        qemu_irq cpu_irq = qdev_get_gpio_in_named(DEVICE(cpu), "IRQ", 0);
+        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irq);
+        for (int i = 0; i < 32; i++) {
+            irq[i] = qdev_get_gpio_in(dev, i);
+        }
+        MemoryRegion *dev_mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
+        memory_region_add_subregion(address_space_mem, 0x18002000, dev_mr);
+    } else {
+        for (i = 0; i < 32; i++) {
+            irq[i] = qdev_get_gpio_in_named(DEVICE(cpu), "IRQ", i);
+        }
     }

     /* Register: Altera 16550 UART */
@@ -105,11 +134,38 @@ static void nios2_10m50_ghrd_init(MachineState *machine)
                       BINARY_DEVICE_TREE_FILE, NULL);
 }

-static void nios2_10m50_ghrd_machine_init(struct MachineClass *mc)
+static void nios2_10m50_ghrd_machine_class_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
+    Nios2MachineClass *nmc = NIOS2_MACHINE_CLASS(oc);
     mc->desc = "Altera 10M50 GHRD Nios II design";
     mc->init = nios2_10m50_ghrd_init;
     mc->is_default = true;
+    nmc->vic = false;
 }

-DEFINE_MACHINE("10m50-ghrd", nios2_10m50_ghrd_machine_init);
+static void nios2_10m50_ghrd_vic_machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    Nios2MachineClass *nmc = NIOS2_MACHINE_CLASS(oc);
+    mc->desc = "Altera 10M50 GHRD Nios II design with VIC";
+    mc->init = nios2_10m50_ghrd_init;
+    mc->is_default = false;
+    nmc->vic = true;
+}
+
+static const TypeInfo nios_machine_types[] = {
+    {
+        .name          = MACHINE_TYPE_NAME("10m50-ghrd"),
+        .parent        = TYPE_MACHINE,
+        .class_size    = sizeof(Nios2MachineClass),
+        .class_init    = nios2_10m50_ghrd_machine_class_init,
+    }, {
+        .name          = MACHINE_TYPE_NAME("10m50-ghrd-vic"),
+        .parent        = TYPE_MACHINE,
+        .class_size    = sizeof(Nios2MachineClass),
+        .class_init    = nios2_10m50_ghrd_vic_machine_class_init,
+    }
+};
+
+DEFINE_TYPES(nios_machine_types)
--
2.25.1


The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.

Re: [PATCH v2 1/4] target/nios2: Shadow register set

[Richard Henderson]

On 2/24/22 03:48, Amir Gonnen wrote:
> @@ -88,7 +93,9 @@ struct Nios2CPUClass {
>   #define   CR_STATUS_IH   (1 << 3)
>   #define   CR_STATUS_IL   (63 << 4)
>   #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_ESTATUS   (CR_BASE + 1)

It would be preferable to use hw/registerfields.h:

FIELD(CR_STATUS, IL, 4, 6)
FIELD(CR_STATUS, CRS, 10, 6)
FIELD(CR_STATUS, PRS, 16, 6)

> +static inline uint32_t cpu_get_crs(const CPUNios2State *env)
> +{
> +    return (env->regs[CR_STATUS] & CR_STATUS_CRS)
> +                    >> CR_STATUS_CRS_OFFSET;
> +}

This becomes

     return FIELD_EX32(env->regs[CR_STATUS], CR_STATUS, CRS);

> +    env->regs[CR_STATUS] = (env->regs[CR_STATUS] & (~CR_STATUS_PRS))
> +                       | ((prev_set << CR_STATUS_PRS_OFFSET) & CR_STATUS_PRS);

This becomes

     env->regs[CR_STATUS] = FIELD_DP32(env->regs[CR_STATUS],
                                       CR_STATUS, PRS, prev_set);

etc.


r~

Re: [PATCH v2 1/4] target/nios2: Shadow register set

[Richard Henderson]

On 2/24/22 03:48, Amir Gonnen wrote:
> +void helper_eret(CPUNios2State *env, uint32_t new_pc)
> +{
> +    uint32_t crs = cpu_get_crs(env);
> +    if (crs == 0) {
> +        env->regs[CR_STATUS] = env->regs[CR_ESTATUS];
> +    } else {
> +        env->regs[CR_STATUS] = env->regs[R_SSTATUS];
> +    }
> +    cpu_change_reg_set(env, crs, cpu_get_crs(env));

Hmm.  This could probably use a comment that the second computation of cpu_get_crs is 
using the value just restored into CR_STATUS.


> +void helper_wrprs(CPUNios2State *env, uint32_t reg_index, uint32_t value)
> +{
> +    uint32_t prs = cpu_get_prs(env);
> +    env->shadow_regs[prs][reg_index] = value;
> +}
> +
> +uint32_t helper_rdprs(CPUNios2State *env, uint32_t reg_index)
> +{
> +    uint32_t prs = cpu_get_prs(env);
> +    return env->shadow_regs[prs][reg_index];
> +}

These are fairly easy to compute inline, e.g. for rdprs:

     TCGv_i32 t = tcg_temp_i32_new();
     TCGv_ptr p = tcg_temp_ptr_new();

     tcg_gen_extract_i32(t, cpu_R[CR_STATUS],
                         R_CR_STATUS_CRS_SHIFT,
                         R_CR_STATUS_CRS_LENGTH);
     tcg_gen_muli_i32(t, t, sizeof(uint32_t) * NUM_GP_REGS);
     tcg_gen_ext_i32_ptr(p, t);

     tcg_gen_add_ptr(p, p, cpu_env);
     tcg_gen_ld_i32(t, p, offsetof(CPUNios2State, shadow_regs)
                     + sizeof(uint32_t) * instr.a);
     tcg_gen_addi_i32(cpu_R[instr.b], t, instr.imm16.s);

     tcg_temp_free_ptr(p);
     tcg_temp_free_i32(o);

> +static void rdprs(DisasContext *dc, uint32_t code, uint32_t flags)
> +{
> +    I_TYPE(instr, code);
> +    TCGv t = tcg_temp_new();
> +    gen_helper_rdprs(t, cpu_env, tcg_const_i32(instr.a));

You're missing a gen_check_supervisor here and in wrprs.

>  static void eret(DisasContext *dc, uint32_t code, uint32_t flags)
>  {
> -    tcg_gen_mov_tl(cpu_R[CR_STATUS], cpu_R[CR_ESTATUS]);
> -    tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_EA]);
> +    gen_helper_eret(cpu_env, cpu_R[R_EA]);

As an existing bug to be fixed by a separate patch, eret should also check for supervisor.

> 
> The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.
> 

You really need to suppress these footers when posting to a public mailing list.


r~

Re: [PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)

[Richard Henderson]

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~

Re: [PATCH v2 4/4] hw/nios2: Machine with a Vectored Interrupt Controller

[Peter Maydell]

On Thu, 24 Feb 2022 at 13:50, Amir Gonnen <amir.gonnen@neuroblade.ai> wrote:
>
> Demonstrate how to use nios2 VIC on a machine.
> Introduce a new machine "10m50-ghrd-vic" which is based on "10m50-ghrd"
> with a VIC attached and internal interrupt controller removed.
>
> When VIC is present, irq0 connects the VIC to the cpu, intc_present
> is set to false to disable the internal interrupt controller, and the
> devices on the machine are attached to the VIC (and not directly to cpu).
> To allow VIC update EIC fields, we set the "cpu" property of the VIC
> with a reference to the nios2 cpu.
>
> Signed-off-by: Amir Gonnen <amir.gonnen@neuroblade.ai>

Is a VIC a configurable option on the real hardware (well,
FPGA image, I guess) that this board is modelling ?
I couldn't find any docs on it with a quick google.

Also, I wonder if we should have a vic machine option to the
machine rather than creating a whole new machine type?

thanks
-- PMM

Re: [PATCH v2 3/4] hw/intc: Vectored Interrupt Controller (VIC)

[Peter Maydell]

On Thu, 24 Feb 2022 at 13:49, Amir Gonnen <amir.gonnen@neuroblade.ai> wrote:
>
> Implement nios2 Vectored Interrupt Controller (VIC).
> VIC is connected to EIC. It needs to update rha, ril, rrs and rnmi
> fields on Nios2CPU before raising an IRQ.
> For that purpose, VIC has a "cpu" property which should refer to the
> nios2 cpu and set by the board that connects VIC.

This device code looks pretty good to me. I have some comments
below, but they're fairly minor items.

> Signed-off-by: Amir Gonnen <amir.gonnen@neuroblade.ai>
> ---
>  hw/intc/Kconfig     |   4 +
>  hw/intc/meson.build |   1 +
>  hw/intc/nios2_vic.c | 327 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 332 insertions(+)
>  create mode 100644 hw/intc/nios2_vic.c
>
> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
> index 528e77b4a6..8000241428 100644
> --- a/hw/intc/Kconfig
> +++ b/hw/intc/Kconfig
> @@ -81,3 +81,7 @@ config GOLDFISH_PIC
>
>  config M68K_IRQC
>      bool
> +
> +config NIOS2_VIC
> +    default y
> +    depends on NIOS2 && TCG

You don't need these default and depends lines -- you want

config NIOS2_VIC
    bool

and then in patch 4 when you add the machine model support
you edit the "config NIOS2_10M50" section to add a line
    select NIOS2_VIC

> +/*
> + * Vectored Interrupt Controller for nios2 processor
> + *
> + * Copyright (c) 2022 Neuroblade
> + *
> + * Interface:
> + * QOM property "cpu": link to the Nios2 CPU (must be set)
> + * Unnamed GPIO inputs 0..NIOS2_VIC_MAX_IRQ-1: input IRQ lines
> + * IRQ should be connected to nios2 IRQ0.
> + *
> + * Reference: "Embedded Peripherals IP User Guide
> + *             for Intel® Quartus® Prime Design Suite: 21.4"
> + * Chapter 38 "Vectored Interrupt Controller Core"

Since Intel helpfully provide this document online we can give a URL
here, which will be useful for later readers:
  https://www.intel.com/content/www/us/en/docs/programmable/683130/

> +#define LOG_VIC(...) qemu_log_mask(CPU_LOG_INT, ##__VA_ARGS__)

You only use this macro once, so I think it hides more than it
helps. I would just drop it. In any case CPU_LOG_INT is really
intended for logging by the cpu proper, not for devices. Modern
QEMU device code would use tracepoints.

> +static void vic_update_irq(Nios2Vic *vic)
> +{
> +    Nios2CPU *cpu = NIOS2_CPU(vic->cpu);
> +    uint32_t pending = vic_int_pending(vic);
> +    int irq = -1;
> +    int max_ril = 0;

This initial value of max_ril correctly implements the behaviour
that setting RIL to 0 disables the interrupt, but it does so
without it being obvious that it's deliberate. A comment might help:

/* Note that if RIL is 0 for an interrupt it is effectively disabled */

> +
> +    vic->vec_tbl_addr = 0;
> +    vic->vic_status = 0;
> +
> +    if (pending == 0) {
> +        qemu_irq_lower(vic->output_int);
> +        return;
> +    }
> +
> +    for (int i = 0; i < NIOS2_VIC_MAX_IRQ; i++) {
> +        if (pending & BIT(i)) {
> +            int ril = vic_int_config_ril(vic, i);
> +            if (ril > max_ril) {
> +                irq = i;
> +                max_ril = ril;
> +            }
> +        }
> +    }
> +
> +    if (irq < 0) {
> +        qemu_irq_lower(vic->output_int);
> +        return;
> +    }
> +
> +    vic->vec_tbl_addr = irq * vic_config_vec_size(vic) + vic->vec_tbl_base;
> +    vic->vic_status = deposit32(vic->vic_status, 0, 6, irq) | BIT(31);

You might as well just write
   vic->vic_status = irq | BIT(31);
here I think.

> +
> +    cpu->rha = vic->vec_tbl_addr;
> +    cpu->ril = max_ril;
> +    cpu->rrs = vic_int_config_rrs(vic, irq);
> +    cpu->rnmi = vic_int_config_rnmi(vic, irq);
> +
> +    qemu_irq_raise(vic->output_int);

I think a comment here would be helpful since this is reaching
into the CPU object. Something like:

/*
 * In hardware, the interface between the VIC and the CPU is via the
 * External Interrupt Controller interface, where the interrupt controller
 * presents the CPU with a packet of data containing:
 *  - Requested Handler Address (RHA): 32 bits
 *  - Requested Register Set (RRS) : 6 bits
 *  - Requested Interrupt Level (RIL) : 6 bits
 *  - Requested NMI flag (RNMI) : 1 bit
 * In our emulation, we implement this by writing the data directly to
 * fields in the CPU object and then raising the IRQ line to tell
 * the CPU that we've done so.
 */

(There are more encapsulated ways one could write this communication
between the VIC device object and the CPU, but I think what you have
here is fine, as long as we have the comment to document that this is
a well-defined interaction and not just the device messing with
some other object's internal state in an arbitrary way.)

> +}
> +

> +static void nios2_vic_csr_write(void *opaque, hwaddr offset, uint64_t value,
> +                                unsigned size)
> +{
> +    Nios2Vic *vic = opaque;
> +    int index = offset / 4;
> +
> +    switch (index) {
> +    case INT_CONFIG0 ... INT_CONFIG31:
> +        vic->int_config[index - INT_CONFIG0] = value;
> +        break;
> +    case INT_ENABLE:
> +        vic->int_enable = value;
> +        break;
> +    case INT_ENABLE_SET:
> +        vic->int_enable |= value;
> +        break;
> +    case INT_ENABLE_CLR:
> +        vic->int_enable &= ~value;
> +        break;
> +    case SW_INTERRUPT:
> +        vic->sw_int = value;
> +        break;
> +    case SW_INTERRUPT_SET:
> +        vic->sw_int |= value;
> +        break;
> +    case SW_INTERRUPT_CLR:
> +        vic->sw_int &= ~value;
> +        break;
> +    case VIC_CONFIG:
> +        vic->vic_config = value;
> +        break;
> +    case VEC_TBL_BASE:
> +        vic->vec_tbl_base = value;
> +        break;
> +    default:
> +        ;

Write 'break;' rather than just a ';'.
Or use qemu_log_mask(LOG_GUEST_ERROR, ...) to report that
the guest did something wrong (wrote to a read-only or invalid
register offset, in this case), if you like.

> +    }
> +
> +    vic_update_irq(vic);
> +}

> +static const VMStateDescription nios2_vic_vmstate = {
> +    .name = "nios2-vic",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]){ VMSTATE_UINT32_ARRAY(int_config, Nios2Vic, 32),
> +                                VMSTATE_UINT32(vic_config, Nios2Vic),
> +                                VMSTATE_UINT32(int_raw_status, Nios2Vic),
> +                                VMSTATE_UINT32(int_enable, Nios2Vic),
> +                                VMSTATE_UINT32(sw_int, Nios2Vic),
> +                                VMSTATE_UINT32(vic_status, Nios2Vic),
> +                                VMSTATE_UINT32(vec_tbl_base, Nios2Vic),
> +                                VMSTATE_UINT32(vec_tbl_addr, Nios2Vic),
> +                                VMSTATE_END_OF_LIST() },

This is weirdly laid out; can you format it more like the way
other files do it, please?

    .fields = (VMStateField[]){
        VMSTATE_UINT32_ARRAY(int_config, Nios2Vic, 32),
        VMSTATE_UINT32(vic_config, Nios2Vic),
        [ etc ]
        VMSTATE_END_OF_LIST()
    },

> +};

thanks
-- PMM

Re: [PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)

[Peter Maydell]

On Thu, 24 Feb 2022 at 23:56, Richard Henderson
<richard.henderson@linaro.org> 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-914ad804f543@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 4/4] hw/nios2: Machine with a Vectored Interrupt Controller

[Amir Gonnen]

Hi Peter,

> Is a VIC a configurable option on the real hardware (well, FPGA image, I guess) that this board is modelling ?
> I couldn't find any docs on it with a quick google.

This specific example-board from Intel does not provide a VIC option, as far as I know.  (https://fpgacloud.intel.com/devstore/platform/15.1.0/Standard/max10-10m50-development-kit-ghrd-with-nios-iiddr3qspi-flashethernetmsgdmauartadc-with-linux/)
Unfortunately, I couldn't find a publicly available nios2 board with a VIC. I've added "10m50-ghrd-vic" as an example to demonstrate how to wire VIC.

In practice, we use Intel tooling (Quartus Prime) to generate both the hardware (nios2 + vic + other devices) and the software BSP that works with it.
That is probably the regular workflow. Since nios2 is a "soft" cpu on an FPGA, each one generates their own custom "board" wired with the devices they need, memories etc.
In the future I may be able to share Neuroblade's QEMU nios2 board because it is quite generic - it consumes a device tree, parses it, and wires devices according to it, so it can automatically match the generated HW.

> Also, I wonder if we should have a vic machine option to the machine rather than creating a whole new machine type?

Sure, if you think it makes more sense.
How do you suggest doing that? A class property for the nios2 machine class? Or is there some other standard way for adding a machine specific option?

Thanks,
Amir


The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.

RE: [PATCH v2 1/4] target/nios2: Shadow register set

[Amir Gonnen]

Hi Richard,

Thank you for your review and comments!

> You're missing a gen_check_supervisor here and in wrprs.

There's something I don't understand about gen_check_supervisor - it looks like it checks CR_STATUS_U when generating code instead of generating code that checks CR_STATUS_U.
Is that correct to assume that CR_STATUS_U would remain the same between generation and runtime?

> As an existing bug to be fixed by a separate patch, eret should also check for supervisor.

Do you suggest I shouldn't fix this here? Why not fix this anyway?

> You really need to suppress these footers when posting to a public mailing list.

I'm sorry about that.
I've worked with IT team to disable this automatic footer when sending to the mailing list so it shouldn't appear any more in the list, but it may still appear for individual recipients.
So, if this still annoys anyone please let me know and I'll contact them again for a different solution.

Re: [PATCH v2 1/4] target/nios2: Shadow register set

[Peter Maydell]

On Sun, 27 Feb 2022 at 16:16, Amir Gonnen <amir.gonnen@neuroblade.ai> wrote:
> There's something I don't understand about gen_check_supervisor -
> it looks like it checks CR_STATUS_U when generating code instead
> of generating code that checks CR_STATUS_U.

This is OK because it is checking the value of CR_STATUS_U in
the tbflags, not the one in the CPU state. Basically, when QEMU
looks for a pre-existing TB it does so by looking up in a hash
table where the key is (program counter, flags, cs_base). (cs_base
here is named what it is for historical reasons, but it can be
used for any data the target likes.) So the target code can
put some parts of its CPU state into the TB flags word, and then
at code-generation time it can generate code that assumes the
value of that state. If we ever run the same bit of code both in
supervisor and non-supervisor mode, we generate two separate
TBs for it. (You can see what nios2 is putting in the flags if
you look at cpu_get_tb_cpu_state() in cpu.h -- currently it
just keeps the U and EH status bits there.)

> > As an existing bug to be fixed by a separate patch, eret should also check for supervisor.
>
> Do you suggest I shouldn't fix this here? Why not fix this anyway?

It should go in a separate patch (but you can put that patch in
a v3 of this series), because it's a separate bug
fix -- it is not related to support of shadow registers.
We like to keep distinct changes in distinct patches (and thus
git commits, because it makes things easier to code review and
also means we have more information if we need to track down
the reason for a change or diagnose a regression in future.

thanks
-- PMM

RE: [PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)

[Amir Gonnen]

> 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)

[Peter Maydell]

On Thu, 3 Mar 2022 at 09:54, Amir Gonnen <amir.gonnen@neuroblade.ai> 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.henderson@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