On Thu, Aug 04 2022, Scott Cheloha <scottchel...@gmail.com> wrote: > On Thu, Aug 04, 2022 at 09:39:13AM +0200, Jeremie Courreges-Anglas wrote: >> On Mon, Aug 01 2022, Scott Cheloha <scottchel...@gmail.com> wrote: >> > On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote: >> >> On Sun, Jul 31 2022, Scott Cheloha <scottchel...@gmail.com> wrote: >> >> > Hi, >> >> > >> >> > I am unsure how to properly mask RISC-V timer interrupts in hardware >> >> > at or above IPL_CLOCK. I think that would be cleaner than doing >> >> > another timer interrupt deferral thing. >> >> > >> >> > But, just to get the ball rolling, here a first attempt at the timer >> >> > interrupt deferral thing for riscv64. The motivation, as with every >> >> > other platform, is to eventually make it unnecessary for the machine >> >> > dependent code to know anything about the clock interrupt schedule. >> >> > >> >> > The thing I'm most unsure about is where to retrigger the timer in the >> >> > PLIC code. It seems right to do it from plic_setipl() because I want >> >> > to retrigger it before doing soft interrupt work, but I'm not sure. >> >> You're adding the timer reset to plic_setipl() but the latter is called >> after softintr processing in plic_splx(). >> >> /* Pending software intr is handled here */ >> if (ci->ci_ipending & riscv_smask[new]) >> riscv_do_pending_intr(new); >> >> plic_setipl(new); > > Yes, but plic_setipl() is also called from the softintr loop in > riscv_do_pending_intr() (riscv64/intr.c) right *before* we dispatch > any pending soft interrupts: > > 594 void > 595 riscv_do_pending_intr(int pcpl) > 596 { > 597 struct cpu_info *ci = curcpu(); > 598 u_long sie; > 599 > 600 sie = intr_disable(); > 601 > 602 #define DO_SOFTINT(si, ipl) \ > 603 if ((ci->ci_ipending & riscv_smask[pcpl]) & \ > 604 SI_TO_IRQBIT(si)) { \ > 605 ci->ci_ipending &= ~SI_TO_IRQBIT(si); \ > * 606 riscv_intr_func.setipl(ipl); \ > 607 intr_restore(sie); \ > 608 softintr_dispatch(si); \ > 609 sie = intr_disable(); \ > 610 } > 611 > 612 do { > 613 DO_SOFTINT(SIR_TTY, IPL_SOFTTTY); > 614 DO_SOFTINT(SIR_NET, IPL_SOFTNET); > 615 DO_SOFTINT(SIR_CLOCK, IPL_SOFTCLOCK); > 616 DO_SOFTINT(SIR_SOFT, IPL_SOFT); > 617 } while (ci->ci_ipending & riscv_smask[pcpl]); > > We might be fine doing it just once in plic_splx() before we do any > soft interrupt stuff. That's closer to what we're doing on other > platforms. > > I just figured it'd be safer to do it in plic_setipl() because we're > already disabling interrupts there. It seems I guessed correctly > because the patch didn't hang your machine.
Ugh, I had missed that setipl call, thanks for pointing it out. Since I don't wander into this code on a casual basis I won't object, but this looks very unobvious to me. :) -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE