RISC-V interrupts in HiFive1

[Hesham Almatary]

On Wed, Aug 16, 2017 at 10:57 AM, Joel Sherrill <joel at rtems.org> wrote:
>
>
> On Tue, Aug 15, 2017 at 7:50 PM, Denis Obrezkov <denisobrezkov at gmail.com>
> wrote:
>>
>> 2017-08-16 2:06 GMT+02:00 Hesham Almatary <heshamelmatary at gmail.com>:
>>>
>>> /
>>>
>>> On Wed, Aug 16, 2017 at 3:03 AM, Denis Obrezkov <denisobrezkov at gmail.com>
>>> wrote:
>>> > 2017-08-15 14:57 GMT+02:00 Joel Sherrill <joel at rtems.org>:
>>> >>
>>> >>
>>> >>
>>> >> On Aug 15, 2017 4:32 AM, "Denis Obrezkov" <denisobrezkov at gmail.com>
>>> >> wrote:
>>> >>
>>> >> 2017-08-15 5:44 GMT+02:00 Hesham Almatary <heshamelmatary at gmail.com>:
>>> >>>
>>> >>> Hi Denis,
>>> >>>
>>> >>> You just need to modify riscv_interrupt_disable(). Read the priv-spec
>>> >>> manual for your RISC-V version, and determine which bit should be
>>> >>> cleared (it's called MIE in priv-1.10, but you mentioned you work
>>> >>> with
>>> >>> priv-1.9).
>>> >>
>>> >>
>>> >> Is the bit set correctly for the idle thread?
>>> >>
>>> >> I assume this is with a real clock driver so is it maintained properly
>>> >> across an ISR?
>>> >>
>>> >> It is either screwed up by the thread context initialization, context
>>> >> switch, or ISR code. ISR code has two exit paths. It could be the
>>> >> preemption
>>> >> path. That would occur about 5 seconds into the execution of ticker
>>> >> and TA1
>>> >> will preemption idle.
>>> >>>
>>> >>>
>>> >>
>>> > I understand this but still can find a mistake.
>>> > This is what I get:
>>> >
>>> >
>>> > *** LOW MEMORY CLOCK TICK TEST ***
>>> >
>>> > TA1 - rtems_clock_get_tod - 09:00:00 12/31/1988
>>> >
>>> > TA2 - rtems_clock_get_tod - 09:00:00 12/31/1988
>>> >
>>> > TA3 - rtems_clock_get_tod - 09:00:00 12/31/1988
>>> >
>>> > TA1 - rtems_clock_get_tod - 09:00:04 12/31/1988
>>> >
>>> > TA2 - rtems_clock_get_tod - 09:00:09 12/31/1988
>>> >
>>> > TA1 - rtems_clock_get_tod - 09:00:09 12/31/1988
>>> >
>>> >
>>> > After that I can examine: p Clock_driver_ticks
>>> >
>>> > $8 = 1001
>>> >
>>> >
>>> > mstatus value is 0x0001800 (interrupts disabled)
>>> >
>>> > Instruction for enabling global interrupts is:
>>> > csrsi mstatus, 0x8
>>> > for disabling:
>>> > csrci mstatus, 0x8
>>> > it atomically changes the fourth bit of mstatus (ie bit).
>>> >
>>> > I think that something wrong happens in context switches between TA1,
>>> > TA2
>>> > and Idle.
>>> >
>>> > This is my context switch file:
>>> >
>>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpukit/score/cpu/riscv32/riscv-context-switch.S
>>> You added new fields (e.g. mepc, mstatus) to the _CPU_Context_switch
>>> function without reserving an area for them in the corresponding
>>> riscv32 Context_Control structure.
>>
>> Yes, I found this mistake already, and decided not to save mstatus,
>> because there is no point
>> in saving it at the current stage of development.
>>>
>>>
>>> > Interrupt disabling:
>>> >
>>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpukit/score/cpu/riscv32/rtems/score/cpu.h#L542
>>> That's not right [1]. You should return the previous mstatus content
>>> (with interrupts enabled i.e. before disabling interrupts). Also, if I
>>> understand correctly, this [2] is also wrong. You should simply set
>>> mstatus with the level variable (as my code does). That is supposed to
>>> get this level value by a previous call to riscv_interrupt_disable(),
>>> assuming it's implemented correctly. This way you're enabling
>>> interrupts back.
>>
>> As I said above, there is no need to save mstatus content since we don't
>> change it in other parts of the program. mstatus is used only for
>> interrupt
>> enabling/disabling, thus I simple enable and disable interrupts using
>> appropriate instructions.
>>>
>>>
>>> [1]
>>> https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpukit/score/cpu/riscv32/rtems/score/cpu.h#L554.
>>> [2]
>>> https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpukit/score/cpu/riscv32/rtems/score/cpu.h#L574
>>> > Context initialize:
>>> >
>>> > https://github.com/embeddedden/rtems-riscv/blob/hifive1/cpukit/score/cpu/riscv32/riscv-context-initialize.c
>>> Now that you use interrupts, it makes sense to add mstatus to
>>> Context_Control structure for riscv32, and initialize mstatus context
>>> in this function to enable interrupts (after addressing the above
>>> comment).
>>
>> I agree that it should be done, but I think I will have no time for this
>> before evaluation.
>
>
> As Hesham pointed out, interrupt disable needs to return a status value.
> It is usually just the contents of the status register with the interrupt
> enable/disable bit(s).
>
+1 mstatus encodes some other read only bits that might affect how
your program behaves of they changed. For example, writing 0s to some
RO  (or hardwired ones) fields might trigger an underfined behaviour,
depending on the micoarchitecure implementation.

> The context should include the status register. It may no longer be
> technically necessary but most every other port does it so I wouldn't
> bet on not needing this. For sure, as long as setting the interrupt
> disable level in rtems_task_mode works, you have to support this.
>
+1

> Not having enough memory in the context area would cause weirdness.
>
> I would expect interrupts to be manually disabled after you return from the
> C ISR handler and then **restored** at the end of the ISR. I know this
> is redundant and I am talking in the abstract but there are two paths
> out of the ISR and it is easy to mess one up.
>
> It is also easy to blow a stack in an ISR handler.
>
> --joel
>
>>>
>>>
>>
>> I also think that after GSoC we should refresh our toolchain.
>> --
>>
>> Regards, Denis Obrezkov
>
>



-- 
Hesham