Paths

Table of Contentst

Differential D21927

plic: fix context calculation
ClosedPublic
Actions

Authored by mhorne on Oct 7 2019, 10:58 PM.

Details

Reviewers

markj
br
kp

Commits

rS354717: plic: fix context calculation

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)

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 27443
Build 25681: arc lint + arc unit

Event Timeline

mhorne created this revision.Oct 7 2019, 10:58 PM

Harbormaster completed remote builds in B26918: Diff 63011.Oct 7 2019, 10:58 PM

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

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

mhorne edited the summary of this revision. (Show Details)Oct 8 2019, 5:16 PM

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?

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 added subscribers: philip, nick.Oct 16 2019, 7:57 PM

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.

Harbormaster completed remote builds in B27184: Diff 63612.Oct 23 2019, 11:06 PM

mhorne mentioned this in D21928: plic: support irq distribution.Oct 23 2019, 11:08 PM

mhorne mentioned this in D21940: plic: handle renumbered harts.Oct 23 2019, 11:09 PM

mhorne added inline comments.Oct 23 2019, 11:15 PM

sys/riscv/riscv/plic.c
100	I don't really think this belongs here, but I was wondering where it should be declare. `riscv/include/md_var.h` maybe?
339	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.

mhorne edited the summary of this revision. (Show Details)Oct 23 2019, 11:25 PM

looks good!

sys/riscv/riscv/plic.c
339	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 added inline comments.Oct 25 2019, 1:03 PM

sys/riscv/riscv/plic.c
339	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.

Looks good

This revision is now accepted and ready to land.Nov 4 2019, 1:30 PM

Rebase.

This revision now requires review to proceed.Nov 11 2019, 1:45 PM

Harbormaster completed remote builds in B27443: Diff 64172.Nov 11 2019, 1:45 PM

Looks good.

sys/riscv/riscv/plic.c
293	/* Register the interrupt sources. */

This revision is now accepted and ready to land.Nov 11 2019, 2:22 PM

mhorne added inline comments.Nov 11 2019, 11:42 PM

sys/riscv/riscv/plic.c
90	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	Thanks, will fix before I commit.

markj added inline comments.Nov 12 2019, 3:17 PM

sys/riscv/riscv/plic.c
90	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.