Page MenuHomeFreeBSD

plic: support irq distribution
ClosedPublic

Authored by mhorne on Oct 7 2019, 11:01 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 26 2024, 7:32 AM
Unknown Object (File)
Feb 4 2024, 2:24 PM
Unknown Object (File)
Jan 23 2024, 9:14 AM
Unknown Object (File)
Dec 20 2023, 3:16 AM
Unknown Object (File)
Nov 21 2023, 5:19 AM
Unknown Object (File)
Nov 21 2023, 3:41 AM
Unknown Object (File)
Nov 19 2023, 1:40 PM
Unknown Object (File)
Nov 14 2023, 12:53 AM
Subscribers

Details

Summary

Our PLIC implementation only enables interrupts on the boot hart.
Implement plic_bind_intr() so that they can be redistributed near the
end of boot during intr_irq_shuffle().

This also slightly modifies how enable bits are handled in an attempt to
better fit the PIC interface. plic_enable_intr/plic_disable_intr are
converted to manage an interrupt source's threshold value, since this
value can be used as to globally enable/disable an irq. All handing of the
per-context enable bits is moved to the new methods plic_setup_intr()
and plic_bind_intr().

Test Plan

The system can boot successfully to multi-user mode with both BBL and OpenSBI with any number of CPUs.
PLIC interrupts can be bound using cpuset(1).

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Why do we need to enable interrupts for all CPUs and not only current cpu?
where an interrupt will be signaled in that case?

In D21928#484089, @br wrote:

Why do we need to enable interrupts for all CPUs and not only current cpu?
where an interrupt will be signaled in that case?

PIC_ENABLE_INTR is called very seldomly, often only once while setting up the PIC during intr_setup_irq by the BSP. Since it is not called per-cpu, this function should handle enabling the interrupt for all active CPUs.

BBL sets all PLIC enable bits to 1 by default, whereas OpenSBI sets them to 0, which is how I noticed that APs weren't having their interrupts enabled.

In D21928#484089, @br wrote:

Why do we need to enable interrupts for all CPUs and not only current cpu?
where an interrupt will be signaled in that case?

PIC_ENABLE_INTR is called very seldomly, often only once while setting up the PIC during intr_setup_irq by the BSP. Since it is not called per-cpu, this function should handle enabling the interrupt for all active CPUs.

BBL sets all PLIC enable bits to 1 by default, whereas OpenSBI sets them to 0, which is how I noticed that APs weren't having their interrupts enabled.

It looks like we are missing SMP method pic_bind_intr() which should configure which CPUs to send the interrupt to.
Look at gic_v3.c

I also have a comment from jhb@:
@br typically we only route device interrupts to a single core since interrupt handlers generally assume they are not run concurrently (interrupt filters, ithreads will be single-threaded even if the interrupt is broadcast)
as Andy noted, you want to handle interrupt binding requests, but you also need to do something for the “default” case.
Not sure what INTRNG does, but on x86 we round-robin interrupts among CPUs when they are first setup.

In D21928#484625, @br wrote:

I also have a comment from jhb@:
@br typically we only route device interrupts to a single core since interrupt handlers generally assume they are not run concurrently (interrupt filters, ithreads will be single-threaded even if the interrupt is broadcast)
as Andy noted, you want to handle interrupt binding requests, but you also need to do something for the “default” case.
Not sure what INTRNG does, but on x86 we round-robin interrupts among CPUs when they are first setup.

Okay, this makes a lot of sense actually, and clears up some of my misconceptions. I did a little more reading on how INTRNG uses the PIC interface, and how arm64 implements it for gicv3.
Below I have described my current understanding of the various PIC methods and how I propose our RISC-V PLIC should implement them. Let me know if there are gaps or errors in my current understanding.

PLIC Functions

The RISC-V PLIC has the following three types of registers for managing interrupts:

  1. PLIC_PRIORITY(irq): These registers control the priority level of interrupt events for irq (priority 0 disables the interrupt)
  2. PLIC_ENABLE(irq,cpu): These registers control whether an event for irq can trigger an interrupt on cpu
  3. PLIC_THRESHOLD(cpu): These registers control which priority levels can interrupt cpu (we just set these to zero during plic_attach(), allowing all sources)

PIC Functions

PIC_SETUP_INTR()

  • Purpose: perform any platform specific initialization for the designated irq
  • Called once during per-irq in intr_setup_irq()
  • Current behaviour: none
  • Proposed behaviour: We should disable the enable bits for all CPUs other than the boot CPU for the designated irq. This will ensure a consistent startup state.

PIC_ENABLE_INTR()

  • Purpose: globally enable/disable the interrupt source for the designated irq
  • Called once immediately following PIC_SETUP_INTR() in intr_setup_irq or during teardown for PIC_DISABLE_INTR()
  • Current behaviour: sets the PLIC_ENABLE bit for the current CPU (which is always the boot CPU)
  • Proposed behaviour: set the PLIC_PRIORITY of irq to at least 1, to ensure that the PLIC source will generate interrupts. Don't touch the PLIC_ENABLE bits, that's the job of PIC_SETUP_INTR() and PIC_BIND_INTR()

PIC_BIND_INTR()

  • Purpose: Set/choose the CPU for which the designated interrupt source should be enabled
  • Called once for each irq in intr_irq_shuffle() to assign interrupts at SI_SUB_SMP, and during intr_isrc_assign_cpu when explicitly binding an interrupt
  • Current behaviour: none
  • Proposed behaviour: This function should set/unset the PLIC_ENABLE bits depending on the selected CPU.

PIC_PRE_ITHREAD()

  • Purpose: globally disable the interrupt source while it is being serviced, but ensure that the CPU can receive interrupts from other sources (this is described explicitly in sys/interrupt.h)
  • Called from intr_isrc_pre_ithread()
  • Current PLIC behaviour: sets the irq threshold to zero, disabling it
  • Proposed behaviour: keep it the same, but perform the PLIC_CLAIM here instead of in plic_intr() I'm unsure about this part, it's hard to tell if a PLIC claim is analogous to an EOI on other architectures.

Rework the changes as described. This moves all PLIC_ENABLE bit handling to the new plic_bind_intr function.
Interrupts are enabled/disabled otherwise by adjusting their priority.

I think this version still has room for improvement but is closer to what we need to implement. One flaw with it currently is that it won't work properly with EARLY_AP_STARTUP, as intr_irq_shuffle will be called before the PLIC is initialized.

Whoops. Upload the correct commit.

sys/riscv/riscv/plic.c
108 ↗(On Diff #64173)

Is it static?

380 ↗(On Diff #64173)

from style(9):
/* Insert an empty line if the function has no local variables. */

mhorne added inline comments.
sys/riscv/riscv/plic.c
380 ↗(On Diff #64173)

This was made optional recently, but I do prefer this style.

mhorne retitled this revision from plic: improve enable bit handling to plic: support irq distribution.Nov 12 2019, 4:42 AM
mhorne edited the summary of this revision. (Show Details)
mhorne edited the test plan for this revision. (Show Details)
This revision is now accepted and ready to land.Nov 14 2019, 3:22 PM
This revision was automatically updated to reflect the committed changes.
mhorne marked an inline comment as done.

@br thanks for your help and patience in reviewing these PLIC changes. I learned a lot while writing them!