plic: fix context calculation

[mhorne]

Summary

The PLIC registers are divided up by "context" which is purposefully
left ambiguous in the PLIC spec. Currently we assume each CPU number
corresponds 1-to-1 with a context number, but that is not correct. Most
existing PLIC implementations (such as SiFive's) have multiple contexts
per-cpu. For example, a single CPU might have a context for machine mode
interrupts and a context for supervisor mode interrupts. To complicate
things further, FreeBSD renumbers the CPUs during boot, but the PLIC
driver still assumes that CPU ID equals the RISC-V hart number, meaning
interrupt enables/claims might be performed for the wrong context
registers.

To fix this, we must calculate each CPU's context number during
attachment. This is done by reading the interrupt properties from the
device tree, from which a mapping from context to RISC-V hart to CPU
number can be created

Relevant PLIC specs:
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

https://sifive.cdn.prismic.io/sifive%2Fdc4980ff-17db-448b-b521-4c7ab26b7488_sifive+u54-mc+manual+v19.08.pdf (Chapter 10)

[br]

I was looking at Linux driver before writing this one. Why don't they do the same ?

https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-sifive-plic.c

[mhorne]

In D21927#479243, @br wrote:

I was looking at Linux driver before writing this one. Why don't they do the same ?

https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-sifive-plic.c

I found this confusing too, but I believe the same behavior is achieved by the "skip context holes" check on line 254. In this case, the holes are the machine-mode contexts. Checking their dts file seems to show why they use -1 for the check.

https://github.com/torvalds/linux/blob/master/arch/riscv/boot/dts/sifive/fu540-c000.dtsi

[br]

https://github.com/torvalds/linux/blob/master/arch/riscv/boot/dts/sifive/fu540-c000.dtsi

Are these example of PLIC contexts?

interrupts-extended = <
				&cpu0_intc 0xffffffff
				&cpu1_intc 0xffffffff &cpu1_intc 9
				&cpu2_intc 0xffffffff &cpu2_intc 9
				&cpu3_intc 0xffffffff &cpu3_intc 9
				&cpu4_intc 0xffffffff &cpu4_intc 9>;

In that case cpu1 supervisor context is number 2, but macro calculates 3?

[mhorne]

In D21927#479338, @br wrote:

https://github.com/torvalds/linux/blob/master/arch/riscv/boot/dts/sifive/fu540-c000.dtsi

Are these example of PLIC contexts?

Yes these are them.

interrupts-extended = <
				&cpu0_intc 0xffffffff
				&cpu1_intc 0xffffffff &cpu1_intc 9
				&cpu2_intc 0xffffffff &cpu2_intc 9
				&cpu3_intc 0xffffffff &cpu3_intc 9
				&cpu4_intc 0xffffffff &cpu4_intc 9>;

In that case cpu1 supervisor context is number 2, but macro calculates 3?

I think you're correct here. The Hifive Unleashed appears to be a special case since it does not have a supervisor context for hart 0. To handle this case we might have to read these device tree indices. I will rework this patch and test it further, but I might need you to test it on your board if qemu's sifive_u doesn't simulate the extra hart 0.

[mhorne]

Rework to calculate contexts from the device tree.

Now, we read the interrupts-extended property to make the
mapping from context -> hart -> cpuid. I think this solution
might still be a little brittle, but the approach is more
correct now.

[mhorne]

sys/riscv/riscv/plic.c

100 ↗

(On Diff #63612)

I don't really think this belongs here, but I was wondering where it should be declare. riscv/include/md_var.h maybe?

338 ↗

(On Diff #63612)

This is assuming that interrupts-extended will follow a format of [phandle, value, phandle, value, ....], but this isn't guaranteed by the device tree spec. As far as I can tell this is the format that is followed in today's RISC-V device trees. Ideally we would examine interrupt-cells for each CPU, but to me that seems a little beyond this change.

[br]

looks good!

sys/riscv/riscv/plic.c

338 ↗

(On Diff #63612)

It seems we need some generic code in dev/ofw that handles "interrupts-extended" property ?
grep(1) shows there is some code already for that. I wonder if it could be used or easily converted to what we need here?

[mhorne]

sys/riscv/riscv/plic.c

338 ↗

(On Diff #63612)

Yes my hope originally was to use those one of those generic functions, but they don't seem to really fit this use case. I will take another look to see if it's possible to make them work for us or add some new generic helper function.

[br]

Looks good

[mhorne]

Rebase.

[br]

Looks good.

sys/riscv/riscv/plic.c

293 ↗

(On Diff #64172)

/* Register the interrupt sources. */

[mhorne]

sys/riscv/riscv/plic.c

90 ↗

(On Diff #64172)

It just occurred to me that using DPCPU might be more appropriate here. Is there any reason I might not want to do that?

293 ↗

(On Diff #64172)

Thanks, will fix before I commit.

[markj]

sys/riscv/riscv/plic.c

90 ↗

(On Diff #64172)

In this case where your per-CPU fields are associated with a device context it would be a bit strange to use DPCPU, at least in my opinion. I don't think it would hurt anything though, unless there is some possibility of having multiple PLICs in a system.