On Fri, Aug 05, 2022 at 12:34:59AM +0200, Jeremie Courreges-Anglas wrote:
> >> [...]
> >>
> >> 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.
Np.
> Since I don't wander into this code on a casual basis I won't object,
> but this looks very unobvious to me. :)
I kind of agree.
I think it would be cleaner -- logically cleaner, not necessarily
cleaner in the code -- to mask timer interrupts when we raise the IPL
to or beyond IPL_CLOCK and unmask timer interrupts when we drop the
IPL below IPL_CLOCK.
... but doing it this way is a lot faster than taking the time to read
and understand the RISC-V privileged architecture spec and how the SBI
interacts with it.
At a glance I see that there are separate Interrupt-Enable bits for
External, Timer, and Software interrupts at the supervisor level. So
what I'm imagining might be possible. I just don't know how to get
the current code to do what I've described.
