RISCV: Introduce support for APLIC interrupt controller

[himanshu_thechauhan.dev]

Summary

This patch introduces support for RISCV APLIC interrupt controller. Currently, it is only supports direct mode i.e. without an IMSIC. Work on IMSIC support is under progress.

Test Plan

The development and testing has been done on Qemu with the following command:

doas ${QEMU_CMD} \
         -machine virt,aia=aplic \
         -s \
         -m 4096M \
         -smp 8 \
         -d guest_errors -D ./qemu.log \
         -nographic \
         -monitor telnet:localhost:5555,server,nowait \
         -kernel ${KERN_IMG} \
         -append "vfs.root.mountfrom=ufs:/dev/vtbd0" \
         -bios ${FW_IMG} \
         -drive file=${ROOTFS},format=raw,id=hd0 \
         -device virtio-blk-device,drive=hd0 \
         -netdev tap,ifname=tap0,script=no,id=net0 \
         -device virtio-net-device,netdev=net0

The option aia=aplic enables the APLIC in the virt platform.

The following output of the "vmstat -i" command shows the block device and UART interrupts on APLIC.

root@qemu:~ # vmstat -i
interrupt                                             total       rate
intc,1:                                                7020        138
intc,5:                                              237194       4670
aplic0,8:-tio_mmio0                                    4231         83
aplic0,10: uart0                                        271          5
Total                                                248716       4897

[mhorne]

sys/riscv/riscv/aplic.c

66–74

This struct is not actually used except for the offsets calculated below. It could become a comment and hard-coded constants instead.

81–84

Is this unused?

113

If you define it this way, you can pass the IRQ index directly, and do not need to remember to add "- 1" for each caller.

Alternatively, it could be:

#define	APLIC_SRC_CFG(_idx)		(0x0004 + (((_idx) - 1) * 4))

116

Same comment as for APLIC_SRC_CFG.

163

170–181

This function is unused in the current revision.

204–205

It is better to be clear about hart/cpu terminology here, and we can save a step.

208–210

IMO it is best not to reuse local variables like this. There is no runtime cost to adding another.

212–219

Suggested simplification.

284–286

I think it makes more sense to check this property in the attach method. If present, we can print an error message and fail attach, and in the future take different action on it.

288

I suggest a different description, probably elaborate on the acronym:

"Advanced Platform-Level Interrupt Controller"

From what I know this description is only printed during device attach (dmesg(8)), so it is good to be verbose.

337–338

355–358

Here you need to write the hart index, not cpu, to the APLIC registers (see my top-level comment).

371

In the majority of cases this information is not of interest to the user; the established practice is to hide stuff like this behind the bootverbose flag.

452

Here is another instance of cpu/hart confusion. You should obtain the hart ID and pass this instead:

hart = pcpu_find(cpu)->pc_hart;

468

Check the whitespace on this line. They all should be aligned with tab characters.

[himanshu_thechauhan.dev]

@mhorne Thanks for the review! I have taken care of your comments. Will be posting new patch shortly.

sys/riscv/riscv/aplic.c

468

I checked but there are no whitespaces here. Alignment is with TAB only.

[himanshu_thechauhan.dev]

Changes in v5:

• Use hartid instead of logical cpu when programming APLIC registers
• Add defines for some hard coded numbers
• Check for "msi-parent" property during attach and fail if found
• Removed the IDC structure and added defines for the IDC register offsets and calculations
• Removed reuse of local variables
• Removed unused function and structures
• Removed "all rights reserved" from copyright comment
• Removed ambiguity of cpu vs hartid

Tested

• Boot with single core and upto 16 cores
• virtio block device working okay
• virtio net device working okay
• UART working okay

[mhorne]

This looks pretty good, with one remaining issue that I see.

I will test the patch locally in the next two days just to ensure that rebinding interrupts to different CPUs works correctly...

sys/riscv/riscv/aplic.c

69

Is this correctly sized? IRQ zero is invalid, but you still access this array with the exact IRQ index e.g. IRQ 1 -> isrcs[1] and IRQ 1023 -> isrcs[1023]. The latter is out-of-bounds.

Maybe it should contain APLIC_MAX_IRQS + 1 entries? (Meaning isrcs[0] is not used)

355

[himanshu_thechauhan.dev]

sys/riscv/riscv/aplic.c

69

True! My bad! Too bad! Thanks for catching it out. I will make it APLIC_MAX_IRQS+1

[himanshu_thechauhan.dev]

changes in v6:

• Fixed wrong sizing of isrcs
• Fixed indentation of goto label

[mhorne]

I am happy with it, and did some testing locally. Rebinding interrupts seems to work as expected.

I will wait a couple days for other feedback, otherwise I'll merge this at the end of the week. Thanks for your effort and responsiveness.

[himanshu_thechauhan.dev]

@mhorne That's great! Thanks for all your time and guidance! I really appreciate it!

[jrtc27]

With apologies for the delay:

Please be consistent about (_)hart_id vs (_)hartid; I think the latter is what we tend to use.

Also this doesn't properly deal with the interrupts-extended property. I believe that, like the PLIC, you need to iterate over its cells to figure out the mapping between architectural hart ID and APLIC hart index, since they are two independent schemes.

sys/riscv/riscv/aplic.c

52

69

Please just call this mem_res or similar (not just res, as D35901 will require an irq_res here too); intc risks confusion with riscv,cpu-intc nodes.

83

93

(repeated down to line 126)

94–95

136

and repeated a few times below. Though maybe factor out (APLIC_IDC_BASE + (_hart_id) * APLIC_IDC_SZ) as some APLIC_IDC(_hart_id)?

151

Though not sure why << isn't parenthesised but & is? AFAIK they both have higher precedence than |?

194

We don't do this for the plain PLIC, we just silently ignore it. Which is right? Note this will result in intr_machdep.c printing a message to the console even for non-verbose boots.

261

This got renamed to riscv,delegation in the latest dt-bindings patch, but OpenSBI should be marking such devices as disabled anyway, and the Linux patch series doesn't do anything special to check for them.

464–469

Shorter form of the same thing

[mhorne]

In D43293#991694, @jrtc27 wrote:

Also this doesn't properly deal with the interrupts-extended property. I believe that, like the PLIC, you need to iterate over its cells to figure out the mapping between architectural hart ID and APLIC hart index, since they are two independent schemes.

I was doubtful of this, but indeed you are right, per section 4.3 of the AIA spec:

Within a given interrupt domain, each of the domain’s harts has a unique index number in the
range 0 to 2^14 − 1 (= 16,383). The index number a domain associates with a hart may or may
not have any relationship to the unique hart identifier (“hart ID”) that the RISC-V Privileged
Architecture assigns to the hart. Two different interrupt domains may employ entirely different
index numbers for the same set of harts."

I guess this becomes important when >1 domains are involved, I won't pretend to grasp the potential edge-cases here. It is a shame that the obtuseness of the interrupts-extended layout was not improved with the new spec.

[himanshu_thechauhan.dev]

@jrtc27 Thanks for your review! I have taken care of your comments. Patch will follow shortly.

sys/riscv/riscv/aplic.c

194

Interrupt 0 is invalid interrupt. If the irq is 0, its a bug. Then it's probably good that it prints even in non-verbose boots.

261

I have removed this check.

[himanshu_thechauhan.dev]

Changes in v6:

• Added verification of harts that can take interrupt using "interrupts-extended" property of FDT.
• Added target_cpu cpuset which contains the bit map of logical cpus that can take interrupts.
• During binding of the irqs, the cpus from target_cpu cpuset is used.
• Handled failure case and deregister ircs.
• Took care of comments regarding the brackets.
• Added a macro APLIC_IDC to give offset of a given hart.
• Removed return of FILTER_STRAY when IRQ is 0, added a KASSERT instead.
• Use of DEFINE_CLASS_0

[jrtc27]

In D43293#991986, @himanshu_thechauhan.dev wrote:

@jrtc27 Thanks for your review! I have taken care of your comments. Patch will follow shortly.

Unless I'm mistaken you still use the hartid to index the APLIC, not the APLIC's hart indexes. You need to actually record, and later use, the index <-> cpu mapping when iterating over interrupts-extended.

Also, FYI, please make sure to upload patches with full context (see https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface). It's not so important here as it's almost entirely the new file, but for your IMSIC revision it is quite annoying not having the context in Phabricator.

sys/conf/files.riscv

56

Please keep it sorted alphabetically

sys/riscv/riscv/aplic.c

391

I don't think we'd be able to handle this not being all CPUs?

[himanshu_thechauhan.dev]

In D43293#993109, @jrtc27 wrote:

In D43293#991986, @himanshu_thechauhan.dev wrote:

@jrtc27 Thanks for your review! I have taken care of your comments. Patch will follow shortly.

Unless I'm mistaken you still use the hartid to index the APLIC, not the APLIC's hart indexes. You need to actually record, and later use, the index <-> cpu mapping when iterating over interrupts-extended.

Whatever hartid is read from the FDT, its logical CPU is kept in target_cpu cpuset. We already, in a way, keep that mapping. When the interrupts are bound, the logical CPU from target_cpu points to the required hart. There is no need to keep separate mapping.

Also, FYI, please make sure to upload patches with full context (see https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface). It's not so important here as it's almost entirely the new file, but for your IMSIC revision it is quite annoying not having the context in Phabricator.

Point taken. I will try to use arc for the next IMSIC revision.

sys/riscv/riscv/aplic.c

391

I am not sure what you mean here. If the interrupts are not being extended to all CPUs, they shouldn't take part in handling the interrupts. Otherwise, the interrupts-extended property is of no use.

[jrtc27]

In D43293#993161, @himanshu_thechauhan.dev wrote:

In D43293#993109, @jrtc27 wrote:

In D43293#991986, @himanshu_thechauhan.dev wrote:

@jrtc27 Thanks for your review! I have taken care of your comments. Patch will follow shortly.

Unless I'm mistaken you still use the hartid to index the APLIC, not the APLIC's hart indexes. You need to actually record, and later use, the index <-> cpu mapping when iterating over interrupts-extended.

Whatever hartid is read from the FDT, its logical CPU is kept in target_cpu cpuset. We already, in a way, keep that mapping. When the interrupts are bound, the logical CPU from target_cpu points to the required hart. There is no need to keep separate mapping.

But the APLIC does not deal with mhartid values. It deals with logical hart indices that have no relation to mhartid values. Then interrupts-extended tells you how it's been wired up. Please go look at the PLIC driver more closely to see what I mean; you will see there is no use of mhartid outside of the interrupts-extended loop (in order to map it to a FreeBSD cpuid), with everything else dealing with FreeBSD's cpuid and mapping that to the corresponding context determined during the aforementioned loop. The APLIC driver needs to be keeping a similar table of cpuid-to-APLIC-hart-index mappings and use those everywhere, not hart IDs.

[jrtc27]

sys/riscv/riscv/aplic.c

391

Indeed they shouldn't, but I do not think the rest of the system would work in such a case, so it's a bit pointless to support this one tiny part of it.

And no, the property is not of no use. See the comment I just made about hart IDs vs indices; that is why this property exists.

[himanshu_thechauhan.dev]

Changes in v7:

• Keep per-cpu IDC offsets after reading interrupts-extended property
• Accessing IDC is via the stored per-CPU IDC offsets
• Remove cleanup after failure in *_attach function (like other drivers)
• Added aplic.c in files.riscv alphabetically

[himanshu_thechauhan.dev]

After Convert local interrupt controller to a newbus PIC, commit rebase is required.

Changes in v8:

• Moved to newbus architecture

[himanshu_thechauhan.dev]

ping.

[mhorne]

LGTM!

[jrtc27]

sys/riscv/riscv/aplic.c

156

This is a hart index again not a hart ID. The APLIC has no knowledge of hart IDs.

[jrtc27]

sys/riscv/riscv/aplic.c

138

Why the double underscore?

[himanshu_thechauhan.dev]

Changes in v10:

• Save hart indices from FDT and use them to create target value

[jrtc27]

sys/riscv/riscv/aplic.c

73

hart_indices?

74

Don't need to store both this and the indices?

139–154

A bit less repetitive this way (may also want to consider dropping the wrappers, renaming the _OFFS to not have the suffix and just using APLIC_IDC_REG directly like I did in fu740_pci_dw, but that gets more subjective perhaps)

157

This << is UB if you have >= 2^23 harts (legal in the APLIC spec, if a bit silly), since you shift into the sign bit. Best to make the array elements unsigned.

[himanshu_thechauhan.dev]

Sorry for the mess up in versions in comments. Last patch was v9, as per my local branches. This one is v10.

Changes in v10:

• Revmoed idc_offs which had pre-calculated IDC offsets
• renamed hindices to hart_indices and changed its type to unsigned int
• Added a macro to calculate the IDC offsets based on hart_indices

[jrtc27]

sys/riscv/riscv/aplic.c

140

That's not what I suggested, and why are the double underscores back with a vengeance?

[himanshu_thechauhan.dev]

sys/riscv/riscv/aplic.c

140

You seem to have a trouble with double underscores. Also, can you please enunciate what exactly you want. What exactly do you find wrong with this?

[jrtc27]

sys/riscv/riscv/aplic.c

140

I dislike the use of double underscores for two main reasons:

1. It's inconsistent with every single other macro in the file where you have a single underscore.
2. Double underscore identifiers are reserved, not for general use. Yes, this is kernel code so reserved is less of a thing to care about, but it's still not great practice for this kind of code.

As for what my suggestion is, I gave a very exact code suggestion in https://reviews.freebsd.org/D43293#inline-264523, which you marked as done, but only did a tiny bit of, and thus still have a lot of boilerplate (and adopting the APLIC_IDC_REG name does not make sense unless you take the full suggestion on board, since in your current version of the patch it is not a register).

[himanshu_thechauhan.dev]

Reworked APLIC_IDC_REG as asked