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