Re: [PATCH v2 3/4] hw/intc: Vectored Interrupt Controller (VIC)
On Thu, 24 Feb 2022 at 13:49, Amir Gonnen 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
> ---
> 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:
> +
[PATCH v2 3/4] hw/intc: Vectored Interrupt Controller (VIC)
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
---
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 00..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
