[PATCH 15/15] HiFive1: disable/enable interrupts during context switch

[Hesham Almatary]

On Sat, Aug 19, 2017 at 10:56 PM, Denis Obrezkov
<denisobrezkov at gmail.com> wrote:
> 2017-08-19 14:33 GMT+02:00 Gedare Bloom <gedare at rtems.org>:
>>
>> On Fri, Aug 18, 2017 at 11:30 AM, Denis Obrezkov
>> <denisobrezkov at gmail.com> wrote:
>> > 2017-08-17 23:58 GMT+02:00 Gedare Bloom <gedare at rtems.org>:
>> >>
>> >> On Thu, Aug 17, 2017 at 4:17 PM, Denis Obrezkov
>> >> <denisobrezkov at gmail.com>
>> >> wrote:
>> >> > 2017-08-17 22:07 GMT+02:00 Gedare Bloom <gedare at rtems.org>:
>> >> >>
>> >> >> On Thu, Aug 17, 2017 at 1:48 PM, Denis Obrezkov
>> >> >> <denisobrezkov at gmail.com>
>> >> >> wrote:
>> >> >> > 2017-08-17 17:25 GMT+02:00 Gedare Bloom <gedare at rtems.org>:
>> >> >> >>
>> >> >> >> On Wed, Aug 16, 2017 at 11:13 AM, Denis Obrezkov
>> >> >> >> <denisobrezkov at gmail.com> wrote:
>> >> >> >> > ---
>> >> >> >> >  cpukit/score/cpu/riscv32/riscv-context-switch.S | 12
>> >> >> >> > ++++++++++--
>> >> >> >> >  1 file changed, 10 insertions(+), 2 deletions(-)
>> >> >> >> >
>> >> >> >> > diff --git a/cpukit/score/cpu/riscv32/riscv-context-switch.S
>> >> >> >> > b/cpukit/score/cpu/riscv32/riscv-context-switch.S
>> >> >> >> > index a199596..bcdfe0e 100644
>> >> >> >> > --- a/cpukit/score/cpu/riscv32/riscv-context-switch.S
>> >> >> >> > +++ b/cpukit/score/cpu/riscv32/riscv-context-switch.S
>> >> >> >> > @@ -46,6 +46,7 @@ PUBLIC(restore)
>> >> >> >> >
>> >> >> >> >  SYM(_CPU_Context_switch):
>> >> >> >> >    /* Disable interrupts and store all registers */
>> >> >> >> > +  csrci mstatus, 0x8
>> >> >> >> Why is this necessary?
>> >> >> >>
>> >> >> >> >    SREG x1, 4(a0)
>> >> >> >> >    SREG x2, 8(a0)
>> >> >> >> >    SREG x3, 12(a0)
>> >> >> >> > @@ -78,8 +79,9 @@ SYM(_CPU_Context_switch):
>> >> >> >> >    SREG x30, 120(a0)
>> >> >> >> >    SREG x31, 124(a0)
>> >> >> >> >
>> >> >> >> > -SYM(restore):
>> >> >> >> >
>> >> >> >> > +SYM(restore):
>> >> >> >> > +
>> >> >> >> >    LREG x1, 4(a1)
>> >> >> >> >    LREG x2, 8(a1)
>> >> >> >> >    LREG x3, 12(a1)
>> >> >> >> > @@ -111,9 +113,15 @@ SYM(restore):
>> >> >> >> >    LREG x29, 116(a1)
>> >> >> >> >    LREG x30, 120(a1)
>> >> >> >> >    LREG x31, 124(a1)
>> >> >> >> > -       ret
>> >> >> >> > +
>> >> >> >> > +
>> >> >> >> > +  csrsi mstatus, 0x8
>> >> >> >> > +  nop
>> >> >> >> > +  nop
>> >> >> >> Why the nops?
>> >> >> >>
>> >> >> >> > +  ret
>> >> >> >> >
>> >> >> >> >  SYM(_CPU_Context_restore):
>> >> >> >> > +  csrci mstatus, 0x8
>> >> >> >> >    mv     a1, a0
>> >> >> >> >    j       restore
>> >> >> >> >    nop
>> >> >> >> > --
>> >> >> >> > 2.1.4
>> >> >> >> >
>> >> >> >> > _______________________________________________
>> >> >> >> > devel mailing list
>> >> >> >> > devel at rtems.org
>> >> >> >> > http://lists.rtems.org/mailman/listinfo/devel
>> >> >> >
>> >> >> > So, don't we turn off interrupts during the context switch?
>> >> >>
>> >> >> Nope, and turning them back on unconditionally is wrong too.
>> >> >>
>> >> >> > Yes, nops are unnecessary.
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> > Regards, Denis Obrezkov
>> >> >
>> >> > Ok, I was confused by this obsolete comment:
>> >> > /* Disable interrupts and store all registers */
>> >> > Will remove all that enabling/disabling.
>> >> >
>> >> > Is the same true for start.S file with interrupted task's stack
>> >> > saving?
>> >> >
>> >> I'm not sure exactly what you're referring to, but usually start.S
>> >> will, among other things, turn off and ack pending interrupts to
>> >> quiesce the hardware before handing off the rest of system init to
>> >> boot_card. Eventually RTEMS will enable interrupts again using the
>> >> cpukit's interrupt_enable layer.
>> >>
>> >> >
>> >> >
>> >> > --
>> >> > Regards, Denis Obrezkov
>> >
>> > I mean when an interrupt occurs, the execution flow jumps to the trap
>> > address.
>> > Then, in my case all process'es registers are saved in a common stack
>> > (I know that dedicated interrupt stack should be implemented, but I
>> > haven't
>> > time to do that)
>> > and then we jump to IRQ dispatching routine. After interrupt handling we
>> > restore saved
>> > registers. So, my question is - should we disable/enable interrupts
>> > during
>> > interrupt handling?
>> > Disabling:
>> >
>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L201
>> > Jump to dispatching routine:
>> >
>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L241
>> > Enabling interrupts back:
>> >
>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/c/src/lib/libbsp/riscv32/hifive1/start/start.S#L285
>> >
>> This interrupt handling is incomplete/broken I think. Keeping the
>> interrupts turned off for the duration of RTEMS + User ISR handling is
>> not "real-time". What other CPU did you look at to understand how to
>> deal with interrupts?
>
> I'v kept Hasham's RISC-V generic interrupt code. I thought that we don't
> want to have nested interrupts. I also tried to turn off interrupts only
> during
> register saving/restoring.
As I mentioned before, there is no interrupt (handling) at all in my
port/BSP (simulator-based); if there's any, they are just stubs. I'd
suggest you look at other BSPs and see how they deal with
enable/disable interrupts during traps, following Gedare's suggestion.
>>
>>
>> Usually, the BSP will support installing interrupt handlers that jump
>> to ISR_Handler(), which then sets up a safe "C" environment for the
>> user handler to use. The ISR_Handler() should enable interrupts before
>> invoking the user code. From what I can tell, your implementation of
>> ISR_Handler doesn't do anything.
>
> Ok, I am going to figure out how to implement it better.
>
> --
> Regards, Denis Obrezkov
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel



-- 
Hesham