pussuw commented on a change in pull request #5782:
URL: https://github.com/apache/incubator-nuttx/pull/5782#discussion_r835034944
##########
File path: arch/risc-v/src/common/riscv_assert.c
##########
@@ -338,6 +338,10 @@ static void riscv_dumpstate(void)
else
{
riscv_saveusercontext(rtcb->xcp.regs);
+
+ /* riscv_saveusercontext modifies the local context (a0), restore it */
+
+ rtcb = running_task();
Review comment:
Now that I have had a good nights sleep I agree what you say above
should be true. However if the line is removed the rtcb pointer gets wiped by
riscv_saveusercontext and I don't quite realize why this happens.
I took a look at the assembly listing and it seems that rtcb is stored into
s0 like this:
0000000080008998: auipc s0,0xfa
000000008000899c: ld s0,-392(s0) # 0x80102810 <g_running_tasks>
And the argument is set like this:
00000000800089ac: addi a0,s0,304 // <- Here a0 is set up for
riscv_saveusercontext() or for memcpy below
And then rtcb is accessed later via s0:
353 ustackbase = (uintptr_t)rtcb->stack_base_ptr;
0000000080008b18: ld s2,112(s0)
354 ustacksize = (uintptr_t)rtcb->adj_stack_size;
0000000080008b1c: ld s4,96(s0)
However what I see is that the SW crashes because rtcb (s0) gets corrupted
somehow. I need to check if there is a bug in the riscv_saveusercontext() asm
function.
##########
File path: arch/risc-v/src/common/riscv_internal.h
##########
@@ -32,6 +32,7 @@
# include <nuttx/arch.h>
# include <sys/types.h>
# include <stdint.h>
+# include <syscall.h>
Review comment:
To declare the sys_call() functions and SYS_xx names
##########
File path: arch/risc-v/include/mode.h
##########
@@ -0,0 +1,91 @@
+/****************************************************************************
+ * arch/risc-v/include/mode.h
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+#ifndef __ARCH_RISCV_INCLUDE_MODE_H
+#define __ARCH_RISCV_INCLUDE_MODE_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <arch/csr.h>
Review comment:
Yes, this is fixed now
##########
File path: arch/risc-v/src/common/supervisor/riscv_context.S
##########
@@ -0,0 +1,234 @@
+/****************************************************************************
+ * arch/risc-v/src/common/supervisor/riscv_context.S
Review comment:
I think having the separate folders is more clear. There are two files
that have the same name and different implementations:
- riscv_exception_common.S
- riscv_vectors.S
Those are selected in Make.defs without reference to the folder. However,
there should not be any more common file names.
I think most of the other files are the .asm functions that are only needed
in S-mode
##########
File path: arch/risc-v/src/common/supervisor/riscv_exception_macros.S
##########
@@ -0,0 +1,136 @@
+/****************************************************************************
+ * arch/risc-v/src/common/supervisor/riscv_exception_macros.S
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+.file "riscv_exception_macros.S"
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <arch/irq.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_RV32
+# define REGLOAD lw
+# define REGSTORE sw
+#else
+# define REGLOAD ld
+# define REGSTORE sd
+#endif
+
+/****************************************************************************
+ * Name: save_ctx
+ *
+ * Parameter:
+ * in - Pointer to where the save is performed (e.g. sp)
+ *
+ * Description:
+ * Save the common context registers (i.e. work / temp / etc).
+ *
+ ****************************************************************************/
+
+.macro save_ctx in
+
+ REGSTORE x1, REG_X1(\in) /* ra */
+#ifdef RISCV_SAVE_GP
Review comment:
I don't think this macro is defined anywhere. I just copy&pasted the
implementation of exception_common here.
##########
File path: arch/risc-v/src/mpfs/Make.defs
##########
@@ -34,6 +34,14 @@ CMN_CSRCS += riscv_mdelay.c riscv_udelay.c
riscv_copyfullstate.c
CMN_CSRCS += riscv_idle.c riscv_tcbinfo.c riscv_getnewintctx.c
CMN_CSRCS += riscv_cpuindex.c
+# If the NuttX kernel runs in S-mode
+
+ifeq ($(CONFIG_ARCH_USE_S_MODE),y)
+CMN_ASRCS += riscv_exception_macros.S riscv_machine_trap.S riscv_context.S
Review comment:
What I meant including a defconfig with CONFIG_ARCH_USE_S_MODE=y will
very likely result in a build error as then this function will be compiled and
will produce the error
```
void riscv_sbi_ack_timer(void)
{
#error "Missing functionality..."
}
```
I promise to you that the intermediate steps are only temporal and a fully
functional configuration with CONFIG_BUILD_KERNEL=y with a defconfig that
includes CONFIG_ARCH_USE_S_MODE=y will come very soon.
##########
File path: arch/risc-v/src/common/riscv_assert.c
##########
@@ -338,6 +338,10 @@ static void riscv_dumpstate(void)
else
{
riscv_saveusercontext(rtcb->xcp.regs);
+
+ /* riscv_saveusercontext modifies the local context (a0), restore it */
+
+ rtcb = running_task();
Review comment:
And now I see what happens :) The value is destroyed by this:
csrr s0, CSR_STATUS // s0 gets wiped
Value of s0 is destroyed by the function... My bad...
I'll fix the asm function and remove rtcb = running_task(); from here
##########
File path: arch/risc-v/include/syscall.h
##########
@@ -391,6 +396,64 @@ static inline uintptr_t sys_call6(unsigned int nbr,
uintptr_t parm1,
return r0;
}
+#ifdef CONFIG_ARCH_USE_S_MODE
+
+/* In S-mode the kernel cannot ecall to itself, because ecall will raise
+ * privileges to M-mode, which is not what we want. Instead, use the syscall
+ * trampoline to do the same.
+ */
+
+static inline uintptr_t ksys_call3(unsigned int nbr, uintptr_t parm1,
Review comment:
I thought of this and the reason I did the separate functions is that
the original sys_call() functions should be for the user. IMHO this is not the
correct place to define them either, they should be in libc.
##########
File path: arch/risc-v/src/common/riscv_internal.h
##########
@@ -279,6 +288,63 @@ int riscv_pause_handler(int irq, void *c, void *arg);
uintptr_t riscv_mhartid(void);
+#ifdef CONFIG_ARCH_USE_S_MODE
+/* If kernel runs in Supervisor mode, declare proper function prototypes,
+ * this is because it is not possible to ecall from S mode to S mode
+ */
+
+int riscv_saveusercontext(uintptr_t *saveregs);
+void riscv_fullcontextrestore(uintptr_t *restoreregs) noreturn_function;
+void riscv_switchcontext(uintptr_t *saveregs, uintptr_t *restoreregs);
+void riscv_syscall_return(void);
+void riscv_syscall_dispatch(void) noreturn_function;
+
+#else
+
+#ifndef __ASSEMBLY__
Review comment:
Yes but IMO it does not hurt to have it here too. If line 30 is ever
removed, this still guards the functions below
##########
File path: arch/risc-v/include/syscall.h
##########
@@ -78,6 +79,7 @@
*/
#define SYS_switch_context (2)
+#endif /* CONFIG_ARCH_USE_S_MODE */
Review comment:
SYS_syscall_return is needed
##########
File path: arch/risc-v/src/common/supervisor/riscv_sbi.c
##########
@@ -0,0 +1,58 @@
+/****************************************************************************
+ * arch/risc-v/src/common/supervisor/riscv_sbi.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <nuttx/arch.h>
+
+#include <stdint.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: riscv_sbi_ack_timer
+ *
+ * Description:
+ * Clear supervisor timer interrupt pending
+ *
+ ****************************************************************************/
+
+void riscv_sbi_ack_timer(void)
+{
+#error "Missing functionality..."
Review comment:
Yes I did the option / Kconfig already but into a separate patch
##########
File path: arch/risc-v/src/common/supervisor/riscv_exception_macros.S
##########
@@ -0,0 +1,136 @@
+/****************************************************************************
+ * arch/risc-v/src/common/supervisor/riscv_exception_macros.S
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+.file "riscv_exception_macros.S"
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <arch/irq.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_RV32
+# define REGLOAD lw
+# define REGSTORE sw
+#else
+# define REGLOAD ld
+# define REGSTORE sd
+#endif
+
+/****************************************************************************
+ * Name: save_ctx
+ *
+ * Parameter:
+ * in - Pointer to where the save is performed (e.g. sp)
+ *
+ * Description:
+ * Save the common context registers (i.e. work / temp / etc).
+ *
+ ****************************************************************************/
+
+.macro save_ctx in
+
+ REGSTORE x1, REG_X1(\in) /* ra */
+#ifdef RISCV_SAVE_GP
+ REGSTORE x3, REG_X3(\in) /* gp */
Review comment:
Because that is the stack pointer and is handled separately.
##########
File path: arch/risc-v/include/syscall.h
##########
@@ -391,6 +396,64 @@ static inline uintptr_t sys_call6(unsigned int nbr,
uintptr_t parm1,
return r0;
}
+#ifdef CONFIG_ARCH_USE_S_MODE
+
+/* In S-mode the kernel cannot ecall to itself, because ecall will raise
+ * privileges to M-mode, which is not what we want. Instead, use the syscall
+ * trampoline to do the same.
+ */
+
+static inline uintptr_t ksys_call3(unsigned int nbr, uintptr_t parm1,
Review comment:
The reason I chose the name ksys_call() and made the separate functions
is analogous to how for example the memory allocators are implemented now.
There are separate malloc/kmalloc functions for example. Both are needed. This
is the case with the syscall functions also.
##########
File path: arch/risc-v/include/mode.h
##########
@@ -0,0 +1,91 @@
+/****************************************************************************
+ * arch/risc-v/include/mode.h
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+#ifndef __ARCH_RISC_V_INCLUDE_MODE_H
+#define __ARCH_RISC_V_INCLUDE_MODE_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <arch/csr.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_USE_S_MODE
+
+/* CSR definitions */
+
+# define CSR_STATUS sstatus /* Global status register */
Review comment:
I'll keep it like this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
