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
