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.
Details
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
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
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. |
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
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 | ||
---|---|---|
70 | 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) | |
356 |
sys/riscv/riscv/aplic.c | ||
---|---|---|
70 | True! My bad! Too bad! Thanks for catching it out. I will make it APLIC_MAX_IRQS+1 |
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.
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 |
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.
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
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? |
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. |
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.
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. |
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
After Convert local interrupt controller to a newbus PIC, commit rebase is required.
Changes in v8:
- Moved to newbus architecture
sys/riscv/riscv/aplic.c | ||
---|---|---|
156 | This is a hart index again not a hart ID. The APLIC has no knowledge of hart IDs. |
sys/riscv/riscv/aplic.c | ||
---|---|---|
138 | Why the double underscore? |
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. |
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
sys/riscv/riscv/aplic.c | ||
---|---|---|
140 | That's not what I suggested, and why are the double underscores back with a vengeance? |
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? |
sys/riscv/riscv/aplic.c | ||
---|---|---|
140 | I dislike the use of double underscores for two main reasons:
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). |