Using float when driver init in RISC-V arch might cause system to get stuck.

[ite-ch]

Describe the bug
I ran zephyr OS on a RISC-V based board, and had already enabled CONFIG_CPU_HAS_FPU, CONFIG_FPU, CONFIG_FPU_SHARING, CONFIG_FLOAT_HARD.
But unfortunately, our system was hanging when some device drivers using float in their driver init function.
After I traced code, I found that system will create bg_thread_main thread with K_ESSENTIAL but K_FP_REGS in init.c.
And RISC-V arch will not set FS filed in mstatus csr if we don't give K_FP_REGS option when creating thread.
After that, bg_thread_main will init device driver with FPU disabled, at the same time system might got stuck if some driver using float hw instruction.

To Reproduce
Enable CONFIG_CPU_HAS_FPU, CONFIG_FPU, CONFIG_FPU_SHARING, CONFIG_FLOAT_HARD.
Add float operation in hello_world sample.
Build hello_world sample and run on a RISC-V arch based board.

Expected behavior
System got stuck.

Impact
We have to limit drivers not to use float when initializing. And we must create another thread with float ability for using float operation.

Logs and console output
(z_swap_irqlock 161)mstatus:0x80007800
(bg_thread_main)mstatus:0x88
***** delaying boot 1ms (per build configuration) *****
*** Booting Zephyr OS version 2.3.0-rc1 (delayed boot 1ms) ***
[z_sched_lock] scheduler locked (0x800095a0:255)
[k_sched_unlock] scheduler unlocked (0x800095a0:0)

FS field didn't be set when entering main function but indeed be set when system init.

Environment (please complete the following information):

• OS: Zephyr 2.3.0
• Toolchain: Zephyr-sdk-0.11.3
• Commit SHA or Version used
  Zephyr 2.3.0

Additional context
Add any other context about the problem here.

[ite-ch]

@ioannisg Any updates on this issue?

[katsuster]

Hello @ite-ch,
PR #30455 has been merged. Does it solved your issue?

[ite-ch]

Hi @katsuster ,

Probably not.
I did the same fix on my local side, but our driver developer still got stuck when they use k_delayed_work_init.
I traced code and found sys work queue didn't give a float option when setting up a new thread in k_work_q_start function.
So, I think use float option to enable float when creating thread is not applicable to RISC-V arch.
In the end, I changed my mind, I modify thread.c in RISC-V arch directly.
Float will be enabled by default when developer config CONFIG_FPU and CONFIG_FPU_SHARING, and just ignore options.
The function I modified is listed as below for your reference,

void arch_new_thread(struct k_thread *thread, k_thread_stack_t *stack,
size_t stack_size, k_thread_entry_t thread_func,
void *arg1, void *arg2, void *arg3,
int priority, unsigned int options)
{
char *stack_memory = Z_THREAD_STACK_BUFFER(stack);

struct __esf *stack_init;

z_new_thread_init(thread, stack_memory, stack_size);

/* Initial stack frame for thread */
stack_init = (struct __esf *)
	     Z_STACK_PTR_ALIGN(stack_memory +
			       stack_size - sizeof(struct __esf));

/* Setup the initial stack frame */
stack_init->a0 = (ulong_t)thread_func;
stack_init->a1 = (ulong_t)arg1;
stack_init->a2 = (ulong_t)arg2;
stack_init->a3 = (ulong_t)arg3;
/*
 * Following the RISC-V architecture,
 * the MSTATUS register (used to globally enable/disable interrupt),
 * as well as the MEPC register (used to by the core to save the
 * value of the program counter at which an interrupt/exception occcurs)
 * need to be saved on the stack, upon an interrupt/exception
 * and restored prior to returning from the interrupt/exception.
 * This shall allow to handle nested interrupts.
 *
 * Given that context switching is performed via a system call exception
 * within the RISCV architecture implementation, initially set:
 * 1) MSTATUS to MSTATUS_DEF_RESTORE in the thread stack to enable
 *    interrupts when the newly created thread will be scheduled;
 * 2) MEPC to the address of the z_thread_entry_wrapper in the thread
 *    stack.
 * Hence, when going out of an interrupt/exception/context-switch,
 * after scheduling the newly created thread:
 * 1) interrupts will be enabled, as the MSTATUS register will be
 *    restored following the MSTATUS value set within the thread stack;
 * 2) the core will jump to z_thread_entry_wrapper, as the program
 *    counter will be restored following the MEPC value set within the
 *    thread stack.
 */
stack_init->mstatus = MSTATUS_DEF_RESTORE;

#if defined(CONFIG_FPU) && defined(CONFIG_FPU_SHARING)
//if ((thread->base.user_options & K_FP_REGS) != 0)
{
stack_init->mstatus |= MSTATUS_FS_INIT;
}
stack_init->fp_state = 0;
#endif
stack_init->mepc = (ulong_t)z_thread_entry_wrapper;

thread->callee_saved.sp = (ulong_t)stack_init;

}

[katsuster]

@ite-ch
Which board are you using for? qemu_riscv32? or other real boards?

[ite-ch]

@katsuster real board.

[katsuster]

@ite-ch Thanks. I don't have such board, so cannot see the problem that is strictly same as you see...
But I can test FPU with QEMU and will continue to find the solution for FPU issue about tasks on work queue.

[katsuster]

Similar issue in ztest. ztest creates a thread for each unit tests.
But we cannot specify K_FP_REGS in thread_options of struct unit_test using current ztest macros.

[ite-ch]

Yes, that's what I thought K_FP_REGS option is not applicable for RISC-V arch.
I think enable HW float directly if user configure CONFIG_FPU when compiling is the most safest way to avoid system get stuck.

[katsuster]

Hmm, as you said, this problem will be gone if we forcedly set FS field of mstatus when CONFIG_FPU is set.
However, I'm not sure whether ignoring K_FP_REGS option is acceptable or not...