On Thu, 24 Feb 2022 at 13:49, Amir Gonnen <amir.gon...@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.gon...@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
