hw/riscv: fix interrupts being lost or delayed when MIE=0 on the ESP32-C3

hw/riscv/esp32c3_intmatrix.c

@@ -30,12 +30,6 @@
 #define SET_BIT(reg, bit)   do { (reg) |= BIT(bit); } while(0)
 
 
-static void esp32c3_do_int(ESP32C3IntMatrixState *s, int line)
-{
-    qemu_irq_pulse(s->out_irqs[line]);
-}
-
-
 static int esp32c3_get_output_line_level(ESP32C3IntMatrixState *s, int line)
 {
     int level_shared = 0;
@@ -51,27 +45,37 @@ static int esp32c3_get_output_line_level(ESP32C3IntMatrixState *s, int line)
     return level_shared;
 }
 
-/**
+static bool esp32c3_intmatrix_line_should_assert(ESP32C3IntMatrixState *s, int line)
- * Try to clear the given interrupt from the pending bitmap.
- * If the signal is shared with other interrupt sources, make sure all of them are low (0)
- * before clearing the pending IRQ from the bitmap.
- */
-static void esp32c3_intmatrix_clear_pending(ESP32C3IntMatrixState *s, int line)
 {
-    /* Check if another GPIO IRQ is sharing the same output line. They must all be low before
+    if (line == 0) {
-     * clearing the pending bit. This is due to the fact that output lines
+        return false;
-     * can be shared on the ESP32-C3 */
+    }
-    const int output_level = esp32c3_get_output_line_level(s, line);
+
-    if (!output_level) {
+    return BIT_SET(s->irq_enabled, line) &&
-        /* The output interrupt line is 0, so we can clear the pending flag */
+           esp32c3_get_output_line_level(s, line) != 0 &&
-        CLEAR_BIT(s->irq_pending, line);
+           (s->irq_prio[line] >= s->irq_thres);
+}
+
+static void esp32c3_intmatrix_update_line(ESP32C3IntMatrixState *s, int line)
+{
+    if (line == 0) {
+        return;
+    }
+
+    const bool assert_line = esp32c3_intmatrix_line_should_assert(s, line);
+
+    if (assert_line) {
+        qemu_irq_raise(s->out_irqs[line]);
+    } else {
+        qemu_irq_lower(s->out_irqs[line]);
     }
 }
 
-
+static void esp32c3_intmatrix_refresh_all(ESP32C3IntMatrixState *s)
-static inline bool esp32c3_intmatrix_can_trigger(ESP32C3IntMatrixState *s)
 {
-    return esp_cpu_accept_interrupts(s->cpu);
+    for (uint32_t i = 1; i <= ESP32C3_CPU_INT_COUNT; i++) {
+        esp32c3_intmatrix_update_line(s, i);
+    }
 }
 
 
@@ -80,7 +84,7 @@ static void esp32c3_intmatrix_irq_handler(void *opaque, int n, int level)
     ESP32C3IntMatrixState *s = ESP32C3_INTMATRIX(opaque);
 
     /* Update the level mirror */
-    assert(n <= ESP32C3_INT_MATRIX_INPUTS);
+    assert(n < ESP32C3_INT_MATRIX_INPUTS);
 
     /* Make sure level is 0 or 1 */
     level = level ? 1 : 0;
@@ -94,128 +98,13 @@ static void esp32c3_intmatrix_irq_handler(void *opaque, int n, int level)
         CLEAR_BIT(s->irq_levels, n);
     }
 
-    const int line = s->irq_map[n];
+    /* Nothing to do if the level is unchanged. */
-
+    if (former_level != level) {
-    /* If the line is not enable, don't do anything special, the level has been recorded already.
+        const int line = s->irq_map[n];
-     * Don't do anything if the line is at the same level as before */
+        esp32c3_intmatrix_update_line(s, line);
-    if ((s->irq_enabled & BIT(line)) == 0 || former_level == level) {
-        return;
-    }
-
-    /* If the new level is high, check that the priority is equal or bigger than the threshold.
-     * If that's the case, we can execute the interrupt, else, mark it as pending. */
-    if (level == 1) {
-#if INTMATRIX_DEBUG
-        info_report("\x1b[31m[INTMATRIX] IRQ %d priority set to %d, CPU threshold %d \x1b[0m\n",
-                    line, s->irq_prio[line], s->irq_thres);
-#endif
-
-        if (s->irq_prio[line] >= s->irq_thres && esp32c3_intmatrix_can_trigger(s)) {
-            esp32c3_do_int(s, line);
-        } else {
-            SET_BIT(s->irq_pending, line);
-        }
-    } else if (BIT_SET(s->irq_pending, line)) {
-        esp32c3_intmatrix_clear_pending(s, line);
     }
 }
 
-
-static void esp32c3_intmatrix_irq_prio_changed(ESP32C3IntMatrixState* s, uint32_t line, uint8_t priority)
-{
-    const bool accept = esp32c3_intmatrix_can_trigger(s);
-
-    if (accept && priority >= s->irq_thres && BIT_SET(s->irq_pending, line)) {
-        /* No need to clear the pending bit here. As soon as the interrupt source will be ACK by the
-         * software, its level will be update, as well as its pending state. */
-        esp32c3_do_int(s, line);
-    }
-}
-
-
-static void esp32c3_intmatrix_core_prio_changed(ESP32C3IntMatrixState* s, uint64_t new_cpu_priority)
-{
-    uint64_t pending = s->irq_pending;
-    const bool accept = esp32c3_intmatrix_can_trigger(s);
-
-    if (pending && accept) {
-        int64_t priority = -1;
-        uint_fast32_t line = 0;
-
-        /* Clear all the interrupts that have a lower priority than the new CPU threshold */
-        for (uint_fast32_t i = 1; i <= ESP32C3_CPU_INT_COUNT; i++) {
-
-            const uint64_t line_prio = s->irq_prio[i];
-            if (line_prio < new_cpu_priority) {
-                CLEAR_BIT(pending, i);
-            }
-        }
-
-        /* No high level interrupt pending? */
-        if (pending == 0) {
-            return;
-        }
-
-        /* Look for the highest priority pending interrupt */
-        for (uint_fast32_t i = 1; i <= ESP32C3_CPU_INT_COUNT; i++) {
-            const int64_t line_prio = (int64_t) s->irq_prio[i];
-            if (BIT_SET(pending, i) && line_prio > priority) {
-                priority = line_prio;
-                line = i;
-            }
-        }
-
-        /* Make sure a line was selected with its new priority */
-        assert(line != 0);
-        assert(priority >= new_cpu_priority);
-        /* No need to clear the pending bit here. As soon as the interrupt source will be ACK by the
-         * software, its level will be update, as well as its pending state. */
-        esp32c3_do_int(s, line);
-    }
-}
-
-
-/**
- * This function is called when the status (enabled/disabled) of a line has just been changed.
- * It will update the pending IRQ map.
- */
-static void esp32c3_intmatrix_irq_status_changed(ESP32C3IntMatrixState* s, uint32_t line, int enabled)
-{
-    const bool accept = esp32c3_intmatrix_can_trigger(s);
-
-    if (!enabled) {
-
-        /* IRQ has just been disabled, if any interrupt is pending, clear it */
-        CLEAR_BIT(s->irq_pending, line);
-
-    } else if (esp32c3_get_output_line_level(s, line)) {
-
-        /* IRQ has just been re-enabled, we have to check if any interrupt source is mapped to it, and
-         * if that's the case, check if their level is high, as we would need to potentially trigger an
-         * interrupt. */
-        SET_BIT(s->irq_pending, line);
-
-        if (accept) {
-            /* If the CPU can accept interrupt, trigger an interrupt now */
-            esp32c3_do_int(s, line);
-        }
-    }
-}
-
-
-/**
- * Callback invoked by the CPU as soon as interrupts are re-enabled
- */
-static bool esp32c3_intmatrix_mie_enabled(void* opaque)
-{
-    ESP32C3IntMatrixState *s = ESP32C3_INTMATRIX(opaque);
-    /* We need to check if any interrupt is pending and trigger it. We have such function already, triggered when
-     * the core priority changes, let's reuse this function by giving the same core priority */
-    esp32c3_intmatrix_core_prio_changed(s, s->irq_thres);
-    return s->irq_pending != 0;
-}
-
-
 static uint64_t esp32c3_intmatrix_read(void* opaque, hwaddr addr, unsigned int size)
 {
     ESP32C3IntMatrixState *s = ESP32C3_INTMATRIX(opaque);
@@ -251,11 +140,16 @@ static void esp32c3_intmatrix_write(void* opaque, hwaddr addr, uint64_t value, u
     const uint32_t index = addr / sizeof(uint32_t);
 
     if (index < ESP32C3_INT_MATRIX_INPUTS) {
-
+        const uint8_t old_line = s->irq_map[index];
-        s->irq_map[index] = (value & 0x1f);
+        const uint8_t new_line = (value & 0x1f);
+        s->irq_map[index] = new_line;
 #if INTMATRIX_DEBUG
         info_report("\x1b[31m[INTMATRIX] Mapping interrupt %d to CPU line %d\x1b[0m\n", index, s->irq_map[index]);
 #endif
+        if (old_line != new_line) {
+            esp32c3_intmatrix_update_line(s, old_line);
+            esp32c3_intmatrix_update_line(s, new_line);
+        }
 
     } else if (index >= ESP32C3_INTMATRIX_IO_PRIO_START && index < ESP32C3_INTMATRIX_IO_PRIO_END) {
 
@@ -266,7 +160,7 @@ static void esp32c3_intmatrix_write(void* opaque, hwaddr addr, uint64_t value, u
         info_report("\x1b[31m[INTMATRIX] Priority of line %d set to %d\x1b[0m\n", line, priority);
 #endif
         /* Check if the new priority interrupts the CPU */
-        esp32c3_intmatrix_irq_prio_changed(s, line, priority);
+        esp32c3_intmatrix_update_line(s, line);
 
     } else if (index == ESP32C3_INTMATRIX_IO_THRESH_REG) {
 
@@ -289,7 +183,7 @@ static void esp32c3_intmatrix_write(void* opaque, hwaddr addr, uint64_t value, u
 #if INTMATRIX_DEBUG
             info_report("\x1b[31m[INTMATRIX] Setting CPU IRQ threshold to %d\x1b[0m", priority);
 #endif
-            esp32c3_intmatrix_core_prio_changed(s, priority);
+            esp32c3_intmatrix_refresh_all(s);
         }
 
     } else if (index == ESP32C3_INTMATRIX_IO_ENABLE_REG) {
@@ -302,7 +196,7 @@ static void esp32c3_intmatrix_write(void* opaque, hwaddr addr, uint64_t value, u
             const int new_st = value & BIT(i);
             const int old_st = prev  & BIT(i);
             if (new_st != old_st) {
-                esp32c3_intmatrix_irq_status_changed(s, i, new_st ? 1 : 0);
+                esp32c3_intmatrix_update_line(s, i);
             }
         }
     } else if (index == ESP32C3_INTMATRIX_IO_TYPE_REG) {
@@ -334,7 +228,6 @@ static void esp32c3_intmatrix_reset_hold(Object *obj, ResetType type)
     memset(s->irq_map, 0, sizeof(s->irq_map));
     memset(s->irq_prio, 0, sizeof(s->irq_prio));
     s->irq_thres = 0;
-    s->irq_pending = 0;
     s->irq_levels = 0;
     s->irq_trigger = 0;
     s->irq_enabled = 0;
@@ -349,15 +242,7 @@ static void esp32c3_intmatrix_reset_hold(Object *obj, ResetType type)
 
 static void esp32c3_intmatrix_realize(DeviceState *dev, Error **errp)
 {
-    ESP32C3IntMatrixState *s = ESP32C3_INTMATRIX(dev);
-    EspRISCVCPU *cpu = s->cpu;
-    EspRISCVCPUClass *cpu_klass = ESP_CPU_GET_CLASS(cpu);
-
     esp32c3_intmatrix_reset_hold(OBJECT(dev), RESET_TYPE_COLD);
-
-    /* Register MIE callback */
-    assert(cpu);
-    cpu_klass->esp_cpu_register_mie_callback(cpu, esp32c3_intmatrix_mie_enabled, s);
 }
 
 

include/hw/riscv/esp32c3_intmatrix.h

@@ -75,8 +75,6 @@ typedef struct ESP32C3IntMatrixState {
     uint8_t irq_prio[ESP32C3_CPU_INT_COUNT + 1];
     /* Current priority threshold of the CPU interrupts */
     uint8_t irq_thres;
-    /* Keep a bitmap of the pending interrupts */
-    uint64_t irq_pending;
     /* Bitmap that records the enabled/disabled interrupts */
     uint64_t irq_enabled;
     /* Bitmap that records the type of trigger for interrupts */

target/riscv/esp_cpu.c

@@ -123,13 +123,6 @@ static RISCVException esp_cpu_csr_write(CPURISCVState *env, int csrno, target_ul
 }
 
 
-static void esp_cpu_register_mie_callback(EspRISCVCPU *env, EspIntEnableCallback callback, void* opaque)
-{
-    assert(env != NULL);
-    env->mie_enabled_callback = callback;
-    env->mie_enabled_opaque = opaque;
-}
-
 riscv_csr_operations esp_cpu_csr_ops = {
     .predicate = esp_cpu_csr_predicate,
     .read = esp_cpu_csr_read,
@@ -137,19 +130,15 @@ riscv_csr_operations esp_cpu_csr_ops = {
 };
 
 
-/**
+static void esp_cpu_update_parent_irq(EspRISCVCPU *cpu)
- * Checks whether the CPU can accepts interrupts or not
- */
-bool esp_cpu_accept_interrupts(EspRISCVCPU *cpu)
 {
-    /* Get the MIE bit out of the MSTATUS register */
+    if (cpu->irq_lines != 0) {
-    CPURISCVState *env = &cpu->parent_obj.env;
+        qemu_irq_raise(cpu->parent_irq);
-    const bool mie = (riscv_csr_read(env, CSR_MSTATUS) & MSTATUS_MIE) != 0;
+    } else {
-
+        qemu_irq_lower(cpu->parent_irq);
-    return !cpu->irq_pending && mie;
+    }
 }
 
-
 /**
  * Function called when an interrupt is incoming.
  */
@@ -157,15 +146,34 @@ static void esp_cpu_irq_handler(void *opaque, int n, int level)
 {
     EspRISCVCPU *cpu = (EspRISCVCPU*) opaque;
 
-    /* Interrupt incoming if level is not 0, make sure we can receive interrupts */
+    /* Lines go from 1 to 31 included */
-    if (level && esp_cpu_accept_interrupts(cpu)) {
+    assert(n <= ESP_CPU_INT_LINES);
-        cpu->irq_pending = true;
+
-        cpu->irq_cause = n;
+    if (n == 0) {
-        qemu_irq_raise(cpu->parent_irq);
+        return;
     }
+
+    if (level != 0) {
+        SET_BIT(cpu->irq_lines, n);
+    } else {
+        CLEAR_BIT(cpu->irq_lines, n);
+    }
+
+    esp_cpu_update_parent_irq(cpu);
 }
 
 
+static uint32_t esp_cpu_select_irq_cause(EspRISCVCPU *cpu)
+{
+    for (uint32_t i = 1; i <= ESP_CPU_INT_LINES; i++) {
+        if (BIT_SET(cpu->irq_lines, i)) {
+            return i;
+        }
+    }
+
+    return 0;
+}
+
 /**
  * TCG operation called when the CPU has to actually jump to the interrupt handler.
  */
@@ -176,30 +184,10 @@ static bool esp_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
      * replace the most important part for us: the mcause. */
     EspRISCVCPU *cpu = ESP_CPU(cs);
     EspRISCVCPUClass *klass = ESP_CPU_GET_CLASS(cpu);
+    const uint32_t cause = esp_cpu_select_irq_cause(cpu);
 
-    if (!cpu->irq_pending) {
+    if (cause == 0) {
-        /* We arrive here after servicing an interrupt but haven't de-asserted the parent IRQ.
+        return false;
-         * If the CPU can now accept interrupts, invoke the callback that will check for the next
-         * interrupt. */
-        if (esp_cpu_accept_interrupts(cpu)) {
-            /* Mark whether we have interrupts pending or not */
-            bool pending = false;
-
-            if (cpu->mie_enabled_callback) {
-                /* If the callback schedules a new interrupt, `cpu->irq_pending` will be set after */
-                pending = cpu->mie_enabled_callback(cpu->mie_enabled_opaque);
-            }
-
-            /* If no further interrupt was scheduled OR no further interrupts are pending, lower the parent's IRQ */
-            if (!pending && !cpu->irq_pending) {
-                qemu_irq_lower(cpu->parent_irq);
-            }
-        }
-        /* If the CPU still doesn't accept interrupts or the callback invoked didn't schedule a new interrupt,
-         * return false to mark the absence of interrupt. */
-        if (!cpu->irq_pending) {
-            return false;
-        }
     }
 
     const bool accepted = klass->parent_exec_interrupt(cs, interrupt_request);
@@ -207,10 +195,6 @@ static bool esp_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     if (accepted) {
         CPURISCVState *env = &cpu->parent_obj.env;
         const bool vectored = (env->mtvec & 3) == 1;
-        const uint32_t cause = cpu->irq_cause;
-
-        /* IRQ has been acknowledged by the parent CPU, it is not pending anymore */
-        cpu->irq_pending = false;
 
         /* Update the mcause and the relevant PC */
         env->mcause = RISCV_EXCP_INT_FLAG | cause;
@@ -219,6 +203,7 @@ static bool esp_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         env->pc = (env->mtvec >> 2 << 2) + (vectored ? cause * 4 : 0);
     }
 
+    /* Similarly, make sure the parent IRQ reflects the current state */
     return accepted;
 }
 
@@ -234,7 +219,7 @@ static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
 static void esp_cpu_reset(void *opaque)
 {
     EspRISCVCPU *cpu = opaque;
-    cpu->irq_pending = 0;
+    cpu->irq_lines = 0;
     qemu_irq_lower(cpu->parent_irq);
     cpu_reset(CPU(cpu));
 }
@@ -343,9 +328,6 @@ static void esp_cpu_class_init(ObjectClass *klass, void *data)
     /* Save the parent realize function in order to be able to call it later */
     device_class_set_parent_realize(dc, esp_cpu_realize,
                                     &cpuclass->parent_realize);
-
-    /* Function to register MIE callback */
-    cpuclass->esp_cpu_register_mie_callback = esp_cpu_register_mie_callback;
 }
 
 static const TypeInfo esp_cpu_info = {

target/riscv/esp_cpu.h

@@ -41,15 +41,6 @@ typedef struct {
     uint64_t divider;
 } ESPCPUCycleCounter;
 
-/**
- * @brief Callback type called when MIE status bit is re-enabled
- *
- * @param Opaque context given when registering the callback
- *
- * @returns true if any interrupt is pending, false is no interrupt is pending
- */
-typedef bool (*EspIntEnableCallback)(void*);
-
 /**
  * Espressif's RISC-V core is different from standard RISC-V because of the way interrupts are handled.
  * Extend the standard RISC-V core implementation.
@@ -62,21 +53,13 @@ typedef struct EspRISCVCPU {
     ESPCPUCycleCounter cc_user;
     ESPCPUCycleCounter cc_machine;
 
-    /* Callback called when the interrupts are re-enabled */
-    EspIntEnableCallback mie_enabled_callback;
-    void* mie_enabled_opaque;
-
     /*< public >*/
     /* The parent object already has a reset vector property */
     uint32_t hartid_base;
     /* Parent IRQ_M line */
     qemu_irq parent_irq;
-    /* Number of the IRQ that triggered the interrupt */
+    /* Bitmap of interrupt matrix output lines currently asserted on CPU inputs. */
-    uint32_t irq_cause;
+    uint32_t irq_lines;
-    /* Interrupts are not always synchronous, so MIE may still be set to 1 while an
-     * interrupt is waiting to be handled. So, keep a mirrored MIE to mark whether
-     * we can receive interrupts or not. */
-    bool irq_pending;
 } EspRISCVCPU;
 
 
@@ -87,14 +70,4 @@ typedef struct EspRISCVCPUClass {
     DeviceReset parent_reset;
     bool (*parent_exec_interrupt)(CPUState *cpu, int interrupt_request);
 
-    /*< public >*/
-    void (*esp_cpu_register_mie_callback)(EspRISCVCPU *env, EspIntEnableCallback callback, void* opaque);
 } EspRISCVCPUClass;
-
-/**
- * @brief Check whether the current CPU state can accept interrupts or not.
- * If an interrupt is currently pending and the MEPC was not set to the reset vector yet,
- * this function returns false.
- * If MIE bit is not set in the MSTATUS register, it will also return false.
- */
-bool esp_cpu_accept_interrupts(EspRISCVCPU *cpu);