search

riscv / riscv-fast-interrupt

Proposal for a RISC-V Core-Local Interrupt Controller (CLIC)
https://jira.riscv.org/browse/RVG-63
Creative Commons Attribution 4.0 International
247 stars 49 forks source link

Many (93) comments in attached PDF of latest CLIC spec #401

Closed james-ball-qualcomm closed 1 month ago

james-ball-qualcomm commented 3 months ago

I read through the CLIC spec in detail and found it easiest to just add comments to the PDF (attached). RISC-V CLIC (v0.9, 2024-06-28) - With Comments.pdf

None of the comments suggest any functional changes. Many of the comments suggest minor changes to improve clarity in the spec so I didn't think it prudent to attempt to create a GitHub issue for each comment. My hope is that the comments will be read by a CLIC author and improvements to the spec made to improve clarity. If needed, I can make the edits to the spec and submit a pull request to have them incorporated in the spec (after appropriate review).

Here's some highlights in the comments:

  1. The use of the terms "vectored" and "non-vectored" is problematic since the first sentence of the spec states (edited) "The CLIC provides low-latency, vectored, pre-emptive interrupts ..." implying all interrupts are vectored. But then the shv bit is used to select between vectored and non-vectored interrupts. I suggest we adopt a different term for the shv-controlled behavior such as Zephyr's terminology of "direct" and "indirect" or something else like "shared" and "non-shared".
  2. I'm recommending moving some top-level chapters into other top-level chapters where they seem to belong anyways. This also makes it easier to navigate the spec since there are many top-level chapters now.
  3. Always use the term "mode" to refer to U/S/M modes and always use the term "level" to refer to interrupt level.
  4. Make it clearer in several places that the vector table pointed to by the xtvt CSR is used for all interrupts (not just "vectored").
  5. I recommend we make smclicshv mandatory (this isn't actually in a comment in the PDF). The spec would be simpler if we made this assumption since this extension makes changes in many places when present. If an implementation doesn't want to support the smclicshv functionality, it can just tie the clicintattr.shv bit to 0 (non-vectored).
  6. The xnxti CSR has very strange behaviors relative to other CSRs. Can we consider adding a new instruction for this instead of this very strange CSR?
james-ball-qualcomm commented 3 months ago

One of my PDF comments was why does xnxti need to filter out vectored interrupts. The answer is in https://github.com/riscv/riscv-fast-interrupt/issues/381. In summary, vectored interrupts return with xret whereas as non-vectored interrupts return with ret and then the interrupt funnel restores lots of registers and then does an xret. So, this requirement on xnxti is a symptom of the CLIC's support for 2 different interrupt handler ABIs (vectored and non-vectored).

christian-herber-nxp commented 3 months ago

btw, https://github.com/riscv/riscv-fast-interrupt/issues/400 would solve this issue of nxti non-interoperability

james-ball-qualcomm commented 2 months ago

I'd like to suggestion #6 above (replace xnxti CSR with an instruction) since that will probably be rejected by Krste and means adding a new instruction which would delay ratification.