riscv: incoming MSI controller (IMSIC) support

[br]

Summary

RISC-V AIA IMSIC support

This is based on early work of Himanshu Chauhan on IMSIC support for IPI delivery. This, however, does not include IPI delivery support, instead it provides MSI support for *peripheral devices*.

IMSIC is a core-local interrupt controller with non-global interrupts, which means that interrupt identity is effectively (hart, interrupt ID).

When IMSIC present on hart, APLIC does not trap the CPU directly, instead it delivers the interrupt to the IMSIC instance associated with the target hart. Upon reception of interrupt, IMSIC on hart invokes APLIC for handling.
(Note that PCIe devices deliver interrupt directly to IMSIC).

Since IMSIC interrupt IDs are local to each hart, pre-allocating an intr_irqsrc for every possible (hart, interrupt ID) pair would result in static-space explosion.
Maintain a pool of pre-registered intr_irqsrc structures in the IMSIC driver and assign them to harts and interrupt IDs if required (interrupt sources handled by APLIC do not need an IMSIC intr_irqsrc).

Apart from IMSIC support, this includes APLIC changes:

• Obtain the interrupt trigger type from FDT and configure the interrupt accordingly (e.g. level or edge-triggered).
• Per the AIA requirement, level-triggered MSI interrupts must be retriggered manually after being handled. See the comment in aplic_post_ithread().
• Reorganize APLIC register definitions to have some structure.

Test Plan

• Tested on Spacemit K3 with PCIe and non-PCIe devices.
• Tested on QEMU in various configurations (imsic present and not present, MSI, MSI-X)

[mhorne]

Glad to see this moving forward. I only skimmed the overall structure, and in particular I am not confident about MSI code.

My main concern is about this 'pool' mechanism. Why is it important to preallocate these irqsrc structures, and is there any way to be smarter about it (upper bound, or runtime expansion of the pool as-needed?). Unless I am mistaken there is not a high reuse rate.

sys/riscv/include/intr.h

40

This will become very big with any bump of MAXCPU!

sys/riscv/riscv/aplic.c

70

possibly type is a better name for this field.

199

524

sys/riscv/riscv/imsic.c

119

This could be a mp_maxid-sized allocation.

124

Can this be made more dynamic in some way? This is a massive allocation. Certainly we should not bloat the softc like this.

152

162

There seem to be a few violations of this type in this file. Please double check the return statements.

555

I think it was noted in the original review that this is an abuse. IMSIC is not a root controller; entry occurs through intc driver handler, hence interrupt resource allocation and handler setup above.

I understand that it is for good reason, that the code in imsic_pic_init_secondary() needs to run on every hart.

It forces the question of whether INTRNG should execute init_secondary() method for all PICs; not just the root. Personally I do not see the harm in such a change.

An alternative would be the combination of config_intrhook and smp_rendezvous_cpus() to run these needed CSR writes.

I would prefer to find a better solution here.

[br]

• don't claim to be root PIC
• switch to intc bus
• NIRQ: lower down the value to 8192
• allocate memory for pcpu_data and imsic_irq_pool

[br]

remove irq pool to simplify operation. Allocate and register IRQs on demand

[br]

In D59695#1370984, @mhorne wrote:

Glad to see this moving forward. I only skimmed the overall structure, and in particular I am not confident about MSI code.

My main concern is about this 'pool' mechanism. Why is it important to preallocate these irqsrc structures, and is there any way to be smarter about it (upper bound, or runtime expansion of the pool as-needed?). Unless I am mistaken there is not a high reuse rate.

I was not able to register new irqsrcs during the PIC_BIND_INTR method of APLIC (due to isrc_table_lock) so come up with a pool.
But later discovered that irqsrcs are not needed on IMSIC side in that case. The pool now removed.

sys/riscv/include/intr.h

40

Agree, we should lower this value down. A single nvme controller could request 8-32 MSI vectors, so on a high end server it could be lot, but still not that much. Lets start with 8192?

sys/riscv/riscv/imsic.c

162

still working on that

555

Looks like we can't call pic_init_secondary on non-root PICs by design.
Instead, we have to coordinate this with the parent PIC and follow the PIC hierarchy:
See https://reviews.freebsd.org/D59813.

[br]

include the full patch

[br]

fix hartid type (should be uint32_t)

[mhorne]

Looking pretty good!

Missing update to sys/conf/files.riscv.

sys/riscv/include/intr.h

40

Seems better. It is tunable with machdep.nirq.

sys/riscv/riscv/aplic.c

79–81

While here?

83

546

sys/riscv/riscv/imsic.c

76–79

It seems like these values can be regular u_int sized.

80

?

232–250

Maybe these also belong inside #ifdef SMP?

I suggest a test compile with nooptions SMP to find out.

412

Is this right? It does not correspond to my reading of riscv,imsics.yaml.

414

rc is never non-zero.

475

Otherwise, it panics:

imsic0: <Incoming MSI Controller> on intc0
panic: vm_fault_lookup: fault on nofault entry, addr: 0xffffffc081c39000
cpuid = 0
time = 1
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_trace_self_wrapper() at db_trace_self_wrapper+0x48
kdb_backtrace() at kdb_backtrace+0x3e
vpanic() at vpanic+0x120
panic() at panic+0x26
vm_fault() at vm_fault+0x18a6
vm_fault_trap() at vm_fault_trap+0x4a
page_fault_handler() at page_fault_handler+0x1ee
do_trap_supervisor() at do_trap_supervisor+0x6e
cpu_exception_handler_supervisor() at cpu_exception_handler_supervisor+0x74
--- exception 13, tval = 0xffffffc081c395aa
lock_init() at lock_init+0x12
_mtx_init() at _mtx_init+0x5a
imsic_attach() at imsic_attach+0x500
device_attach() at device_attach+0x418
bus_generic_new_pass() at bus_generic_new_pass+0xf8
bus_generic_new_pass() at bus_generic_new_pass+0x9a
bus_generic_new_pass() at bus_generic_new_pass+0x9a
bus_generic_new_pass() at bus_generic_new_pass+0x9a
root_bus_configure() at root_bus_configure+0x32
configure() at configure+0xc
mi_startup() at mi_startup+0x1da
va() at va+0x5e
KDB: enter: panic
[ thread pid 0 tid 100000 ]
Stopped at      kdb_enter+0x36: sd      zero,40(s1)
db>

556

559

[br]

Address @mhorne comments.

[br]

sys/riscv/riscv/imsic.c

412

that was not correct as it depends on #address/size-cells properties of the parent node. Reuse fdt_regsize()

[mhorne]

LGTM!