You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/03/13 12:32:55 UTC
[GitHub] [incubator-nuttx] no1wudi opened a new pull request #5731: arch/risc-v: Improve speed of context switch
no1wudi opened a new pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731
## Summary
Follow #5645
## Impact
RISC-V
## Testing
maix-bit/rv-virt:nsh/smp on qemu
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080564175
Can someone please explain to me how this change is supposed to work with MMU and address environments ?
Please note the following example
```
struct tcb_s *nexttcb = this_task();
#ifdef CONFIG_ARCH_ADDRENV
/* Make sure that the address environment for the previously
* running task is closed down gracefully (data caches dump,
* MMU flushed) and set up the address environment for the new
* thread at the head of the ready-to-run list.
*/
(void)group_addrenv(nexttcb);
#endif
/* Reset scheduler parameters */
nxsched_resume_scheduler(nexttcb);
/* Then switch contexts */
riscv_switchcontext(&rtcb->xcp.regs, nexttcb->xcp.regs);
```
If rtcb->xpc.regs points to the old task's stack, which resides in its address environment, how can we perform a register save there ?
The old implementation saved the task context in the kernel space memory, which is why it is safe to change the address environment as the memory for xcp.regs[] exists in the kernel space and is always mapped ?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1078755319
> @no1wudi
>
> It seems that the PR has only 1 commit which also contains some style changes. So could you please separate such the changes to another commit?
OK, style changes removed.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080795149
Oops, you are also right, the original code is suspicious too, as the first thing exception_common does is this:
```
exception_common:
addi sp, sp, -XCPTCONTEXT_SIZE
```
I assume the same issue is in the ARM implementation too.
In this case the new task's address environment is already in use, but sp still points to the old task.
This is not an issue with the new S-mode function however:
riscv_switchcontext:
```
/* Save old context to arg[0] */
save_ctx a0 /* save context */
```
Saving the context does not use stack, and this is why the code works on my table.
But indeed, the location for calling group_addrenv() is suspicious, as the context switch is done by using the new tasks environment, even though the old task's registers are still in use.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080996343
> Oops, you are also right, the original code is suspicious too, as the first thing exception_common does is this:
>
> ```
> exception_common:
> addi sp, sp, -XCPTCONTEXT_SIZE
> ```
>
> I assume the same issue is in the ARM implementation too. In this case the new task's address environment is already in use, but sp still points to the old task.
Yes, ARM has the similar issue, @anchao hit the problem before like this:
1. The first task wait on a named semaphore
2. The second task post this semaphore to wake up the first
3. And the system panic suddenly
he made a temp fix like this:
1. Move group_addrenv to arm_syscall::SYS_switch_context
2. Disable FPU since the lazy fpu save may corrupt the next task memory space
The above problem is fixed. So, to fix this issue:
1. We need review all place which call group_addrenv and move to the right location
2. Save and restore FPU in exception_common
@no1wudi please prepare two patches for RISCV tomorrow, so @pussuw can verify with his S-mode change.
@anchao will fix the similar issue on ARM in the next couple days.
>
> This is not an issue with the new S-mode function however:
>
> ```
> riscv_switchcontext:
>
> /* Save old context to arg[0] */
>
> save_ctx a0 /* save context */
> ```
>
> Saving the context does not use stack, and this is why the old code works on my table.
>
Yes, but it isn't reliable since we can't control how C compiler generate the machine code which may insert push/pop operation between group_addrenv and riscv_switchcontext in the new version or different option.
> But indeed, the location for calling group_addrenv() is suspicious, as the context switch is done by using the new tasks environment, even though the old task's registers are still in use.
Yes, we move need do the address space change with the interrupt or kernel stack, otherwise we have to write the whole process in assemble code, so we can control the stack usage manually.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080795149
Oops, you are also right, the original code is suspicious too, as the first thing exception_common does is this:
```
exception_common:
addi sp, sp, -XCPTCONTEXT_SIZE
```
I assume the same issue is in the ARM implementation too.
In this case the new task's address environment is already in use, but sp still points to the old task.
This is not an issue with the new S-mode function however:
```
riscv_switchcontext:
/* Save old context to arg[0] */
save_ctx a0 /* save context */
```
Saving the context does not use stack, and this is why the old code works on my table.
But indeed, the location for calling group_addrenv() is suspicious, as the context switch is done by using the new tasks environment, even though the old task's registers are still in use.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#discussion_r835328175
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -265,55 +283,80 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
* been delivered.
*/
- tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ tcb->xcp.sigdeliver = sigdeliver;
/* Then set up vector to the trampoline with interrupts
* disabled. We must already be in privileged thread mode
* to be here.
*/
- tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ /* Save the current register context location */
+
+ tcb->xcp.saved_regs = tcb->xcp.regs;
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ tcb->xcp.regs = (uintptr_t *)((uintptr_t)tcb->xcp.regs -
+ XCPTCONTEXT_SIZE);
- int_ctx = tcb->xcp.regs[REG_INT_CTX];
- int_ctx &= ~MSTATUS_MPIE;
+ memcpy(tcb->xcp.regs, tcb->xcp.saved_regs,
+ XCPTCONTEXT_SIZE);
+ tcb->xcp.regs[REG_SP] = (uintptr_t)tcb->xcp.regs +
+ XCPTCONTEXT_SIZE;
+
+ tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ int_ctx &= ~MSTATUS_MPIE;
+#ifdef CONFIG_BUILD_PROTECTED
+ int_ctx |= MSTATUS_MPPM;
+#endif
tcb->xcp.regs[REG_INT_CTX] = int_ctx;
}
else
{
/* tcb is running on the same CPU */
- /* Save the return EPC and STATUS registers. These will be
+ /* Save the context registers. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ tcb->xcp.sigdeliver = (void *)sigdeliver;
+
+ tcb->xcp.saved_regs = (uintptr_t *)CURRENT_REGS;
+
+ /* Duplicate the register context. These will be
* restored by the signal trampoline after the signal has
* been delivered.
*/
- tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = CURRENT_REGS[REG_EPC];
- tcb->xcp.saved_int_ctx = CURRENT_REGS[REG_INT_CTX];
+ CURRENT_REGS = (uintptr_t *)((uintptr_t)CURRENT_REGS -
+ XCPTCONTEXT_SIZE);
+
+ memcpy((uintptr_t *)CURRENT_REGS, tcb->xcp.saved_regs,
+ XCPTCONTEXT_SIZE);
+
+ CURRENT_REGS[REG_SP] = (uintptr_t)CURRENT_REGS +
+ XCPTCONTEXT_SIZE;
/* Then set up vector to the trampoline with interrupts
* disabled. The kernel-space trampoline must run in
* privileged thread mode.
*/
- CURRENT_REGS[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ CURRENT_REGS[REG_EPC] = (uintptr_t)riscv_sigdeliver;
Review comment:
The above line is a style change. (just remove spaces).
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -265,55 +283,80 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
* been delivered.
*/
- tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ tcb->xcp.sigdeliver = sigdeliver;
/* Then set up vector to the trampoline with interrupts
* disabled. We must already be in privileged thread mode
* to be here.
*/
- tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ /* Save the current register context location */
+
+ tcb->xcp.saved_regs = tcb->xcp.regs;
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ tcb->xcp.regs = (uintptr_t *)((uintptr_t)tcb->xcp.regs -
+ XCPTCONTEXT_SIZE);
- int_ctx = tcb->xcp.regs[REG_INT_CTX];
- int_ctx &= ~MSTATUS_MPIE;
+ memcpy(tcb->xcp.regs, tcb->xcp.saved_regs,
+ XCPTCONTEXT_SIZE);
+ tcb->xcp.regs[REG_SP] = (uintptr_t)tcb->xcp.regs +
+ XCPTCONTEXT_SIZE;
+
+ tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ int_ctx &= ~MSTATUS_MPIE;
+#ifdef CONFIG_BUILD_PROTECTED
+ int_ctx |= MSTATUS_MPPM;
+#endif
tcb->xcp.regs[REG_INT_CTX] = int_ctx;
}
else
{
/* tcb is running on the same CPU */
- /* Save the return EPC and STATUS registers. These will be
+ /* Save the context registers. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ tcb->xcp.sigdeliver = (void *)sigdeliver;
+
+ tcb->xcp.saved_regs = (uintptr_t *)CURRENT_REGS;
+
+ /* Duplicate the register context. These will be
* restored by the signal trampoline after the signal has
* been delivered.
*/
- tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = CURRENT_REGS[REG_EPC];
- tcb->xcp.saved_int_ctx = CURRENT_REGS[REG_INT_CTX];
+ CURRENT_REGS = (uintptr_t *)((uintptr_t)CURRENT_REGS -
+ XCPTCONTEXT_SIZE);
+
+ memcpy((uintptr_t *)CURRENT_REGS, tcb->xcp.saved_regs,
+ XCPTCONTEXT_SIZE);
+
+ CURRENT_REGS[REG_SP] = (uintptr_t)CURRENT_REGS +
+ XCPTCONTEXT_SIZE;
/* Then set up vector to the trampoline with interrupts
* disabled. The kernel-space trampoline must run in
* privileged thread mode.
*/
- CURRENT_REGS[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ CURRENT_REGS[REG_EPC] = (uintptr_t)riscv_sigdeliver;
- int_ctx = CURRENT_REGS[REG_INT_CTX];
- int_ctx &= ~MSTATUS_MPIE;
+ int_ctx = CURRENT_REGS[REG_INT_CTX];
+ int_ctx &= ~MSTATUS_MPIE;
#ifndef CONFIG_BUILD_FLAT
- int_ctx |= MSTATUS_MPPM;
+ int_ctx |= MSTATUS_MPPM;
Review comment:
ditto.
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -370,7 +428,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
int_ctx = tcb->xcp.regs[REG_INT_CTX];
int_ctx &= ~MSTATUS_MPIE;
-
+#ifdef CONFIG_BUILD_PROTECTED
Review comment:
ditto
##########
File path: arch/risc-v/src/common/riscv_sigdeliver.c
##########
@@ -114,7 +110,7 @@ void riscv_sigdeliver(void)
*/
sinfo("Resuming EPC: %" PRIxREG " INT_CTX: %" PRIxREG "\n",
- regs[REG_EPC], regs[REG_INT_CTX]);
+ regs[REG_EPC], regs[REG_INT_CTX]);
Review comment:
ditto.
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -170,25 +179,34 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
* been delivered.
*/
- tcb->xcp.sigdeliver = sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ tcb->xcp.sigdeliver = sigdeliver;
- /* Then set up to vector to the trampoline with interrupts
- * disabled. We must already be in privileged thread mode to be
- * here.
+ /* Save the current register context location */
+
+ tcb->xcp.saved_regs = tcb->xcp.regs;
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has been
+ * delivered.
*/
- tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ tcb->xcp.regs = (uintptr_t *)((uintptr_t)tcb->xcp.regs -
+ XCPTCONTEXT_SIZE);
+
+ memcpy(tcb->xcp.regs, tcb->xcp.saved_regs, XCPTCONTEXT_SIZE);
+ tcb->xcp.regs[REG_SP] = (uintptr_t)tcb->xcp.regs +
+ XCPTCONTEXT_SIZE;
+
+ tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
int_ctx = tcb->xcp.regs[REG_INT_CTX];
int_ctx &= ~MSTATUS_MPIE;
-
tcb->xcp.regs[REG_INT_CTX] = int_ctx;
Review comment:
The above change is a style change. (just remove a blank line)
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -265,55 +283,80 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
* been delivered.
*/
- tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ tcb->xcp.sigdeliver = sigdeliver;
/* Then set up vector to the trampoline with interrupts
* disabled. We must already be in privileged thread mode
* to be here.
*/
- tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ /* Save the current register context location */
+
+ tcb->xcp.saved_regs = tcb->xcp.regs;
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ tcb->xcp.regs = (uintptr_t *)((uintptr_t)tcb->xcp.regs -
+ XCPTCONTEXT_SIZE);
- int_ctx = tcb->xcp.regs[REG_INT_CTX];
- int_ctx &= ~MSTATUS_MPIE;
+ memcpy(tcb->xcp.regs, tcb->xcp.saved_regs,
+ XCPTCONTEXT_SIZE);
+ tcb->xcp.regs[REG_SP] = (uintptr_t)tcb->xcp.regs +
+ XCPTCONTEXT_SIZE;
+
+ tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ int_ctx &= ~MSTATUS_MPIE;
+#ifdef CONFIG_BUILD_PROTECTED
+ int_ctx |= MSTATUS_MPPM;
+#endif
tcb->xcp.regs[REG_INT_CTX] = int_ctx;
}
else
{
/* tcb is running on the same CPU */
- /* Save the return EPC and STATUS registers. These will be
+ /* Save the context registers. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ tcb->xcp.sigdeliver = (void *)sigdeliver;
+
+ tcb->xcp.saved_regs = (uintptr_t *)CURRENT_REGS;
+
+ /* Duplicate the register context. These will be
* restored by the signal trampoline after the signal has
* been delivered.
*/
- tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = CURRENT_REGS[REG_EPC];
- tcb->xcp.saved_int_ctx = CURRENT_REGS[REG_INT_CTX];
+ CURRENT_REGS = (uintptr_t *)((uintptr_t)CURRENT_REGS -
+ XCPTCONTEXT_SIZE);
+
+ memcpy((uintptr_t *)CURRENT_REGS, tcb->xcp.saved_regs,
+ XCPTCONTEXT_SIZE);
+
+ CURRENT_REGS[REG_SP] = (uintptr_t)CURRENT_REGS +
+ XCPTCONTEXT_SIZE;
/* Then set up vector to the trampoline with interrupts
* disabled. The kernel-space trampoline must run in
* privileged thread mode.
*/
- CURRENT_REGS[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ CURRENT_REGS[REG_EPC] = (uintptr_t)riscv_sigdeliver;
- int_ctx = CURRENT_REGS[REG_INT_CTX];
- int_ctx &= ~MSTATUS_MPIE;
+ int_ctx = CURRENT_REGS[REG_INT_CTX];
Review comment:
The above lines are style changes. (just remove spaces).
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#discussion_r835386707
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -370,7 +428,9 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
int_ctx = tcb->xcp.regs[REG_INT_CTX];
int_ctx &= ~MSTATUS_MPIE;
-
+#ifdef CONFIG_BUILD_PROTECTED
Review comment:
This isn't a style change
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080996343
> Oops, you are also right, the original code is suspicious too, as the first thing exception_common does is this:
>
> ```
> exception_common:
> addi sp, sp, -XCPTCONTEXT_SIZE
> ```
>
> I assume the same issue is in the ARM implementation too. In this case the new task's address environment is already in use, but sp still points to the old task.
Yes, ARM has the similar issue, @anchao hit the problem before like this:
1. The first task wait on a named semaphore
2. The second task post this semaphore to wake up the first
3. And the system panic suddenly
he made a temp fix like this:
1. Move group_addrenv to arm_syscall::SYS_switch_context
2. Disable FPU since the lazy fpu save may corrupt the next task memory space
The above problem is fixed. So, to fix this issue:
1. We need review all place which call group_addrenv and move to the right location
2. Save and restore FPU in exception_common
@no1wudi please prepare two patches for RISCV tomorrow, so @pussuw can verify with his S-mode change.
@anchao will fix the similar issue on ARM in the next couple days.
>
> This is not an issue with the new S-mode function however:
>
> ```
> riscv_switchcontext:
>
> /* Save old context to arg[0] */
>
> save_ctx a0 /* save context */
> ```
>
> Saving the context does not use stack, and this is why the old code works on my table.
If we postpone the address space switch until save_ctx return, this PR should work with your S-mode change, I think.
>
Yes, but it isn't reliable since we can't control how C compiler generate the machine code which may insert push/pop operation between group_addrenv and riscv_switchcontext in the new version or different option.
> But indeed, the location for calling group_addrenv() is suspicious, as the context switch is done by using the new tasks environment, even though the old task's registers are still in use.
Yes, we move need do the address space change with the interrupt or kernel stack, otherwise we have to write the whole process in assemble code, so we can control the stack usage manually.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080646436
I can confirm that for RISC-V the flat and protected targets work (with #5881) but the kernel mode target crashes with a load page fault. I will investigate further but the code was working before i.e. I could enter nsh and run things on the console. Now the SW crashes before nsh is launched.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1066286230
@masayuki2009 Emm, I meet another problem with qemu-6.2 with mainline or this PR:
```
> Executing task: qemu-system-riscv64 -M sifive_u -device loader,file=nuttx/nuttx.bin,addr=0x80000000 -device loader,file=nuttx/nuttx_user.bin,addr=0x80100000 -nographic <
ABCDriscv_fault: EPC: 00000000800001ae
riscv_fault: Fault IRQ=2
riscv_fault: A0: 00000000801005ec A1: 0000000000000001 A2: 0000000080401fa0 A3: 0000000080401fa0
riscv_fault: A4: 00000000801005ec A5: 0000000000000001 A6: 0000000000000101 A7: 0000000000000000
riscv_fault: T0: 0000000000000000 T1: 0000000000000000 T2: 0000000000000000 T3: 0000000000000000
riscv_fault: T4: 0000000000000000 T5: 0000000000000000 T6: 0000000000000000
riscv_fault: S0: 0000000000000000 S1: 0000000000000000 S2: 0000000000000000 S3: 0000000000000000
riscv_fault: S4: 0000000000000000 S5: 0000000000000000 S6: 0000000000000000 S7: 0000000000000000
riscv_fault: S8: 0000000000000000 S9: 0000000000000000 S10: 0000000000000000 S11: 0000000000000000
riscv_fault: SP: 0000000080402b80 FP: 0000000000000000 TP: 0000000000000000 RA: 00000000800019f0
up_assert: Assertion failed at file:chip/k210_irq_dispatch.c line: 91 task: ostest_main
riscv_registerdump: EPC: 00000000800001ae
riscv_registerdump: A0: 00000000801005ec A1: 0000000000000001 A2: 0000000080401fa0 A3: 0000000080401fa0
riscv_registerdump: A4: 00000000801005ec A5: 0000000000000001 A6: 0000000000000101 A7: 0000000000000000
riscv_registerdump: T0: 0000000000000000 T1: 0000000000000000 T2: 0000000000000000 T3: 0000000000000000
riscv_registerdump: T4: 0000000000000000 T5: 0000000000000000 T6: 0000000000000000
riscv_registerdump: S0: 0000000000000000 S1: 0000000000000000 S2: 0000000000000000 S3: 0000000000000000
riscv_registerdump: S4: 0000000000000000 S5: 0000000000000000 S6: 0000000000000000 S7: 0000000000000000
riscv_registerdump: S8: 0000000000000000 S9: 0000000000000000 S10: 0000000000000000 S11: 0000000000000000
riscv_registerdump: SP: 0000000080402b80 FP: 0000000000000000 TP: 0000000000000000 RA: 00000000800019f0
riscv_dumpstate: sp: 0000000080400840
riscv_dumpstate: IRQ stack:
riscv_dumpstate: base: 00000000804000c0
riscv_dumpstate: size: 0000000000000800
riscv_stackdump: 0000000080400840: 0000005b 00000000 8000ffb8 00000000 00000000 00000000 00000002 00000000
riscv_stackdump: 0000000080400860: 80402a78 00000000 804012a0 00000000 00000002 00000000 8000475c 00000000
riscv_stackdump: 0000000080400880: 00000002 00000000 80000260 00000000 deadbeef deadbeef 00000000 00000000
riscv_stackdump: 00000000804008a0: 00000000 00000000 00000000 00000000 00000000 00000000 80000164 00000000
riscv_dumpstate: sp: 0000000080402b80
riscv_dumpstate: User stack:
riscv_dumpstate: base: 0000000080401fc0
riscv_dumpstate: size: 0000000000000be0
riscv_dumpstate: ERROR: Stack pointer is not within allocated stack
riscv_stackdump: 0000000080401fc0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080401fe0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402000: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402020: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402040: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402060: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402080: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804020a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804020c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804020e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402100: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402120: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402140: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402160: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402180: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804021a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804021c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804021e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402200: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402220: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402240: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402260: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402280: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804022a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804022c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804022e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402300: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402320: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402340: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402360: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402380: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804023a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804023c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804023e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402400: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402420: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402440: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402460: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402480: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804024a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804024c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804024e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402500: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402520: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402540: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402560: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402580: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804025a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804025c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804025e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402600: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402620: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402640: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402660: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402680: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804026a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804026c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804026e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402700: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402720: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402740: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402760: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402780: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804027a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804027c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804027e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402800: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402820: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402840: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402860: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402880: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804028a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804028c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804028e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402900: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402920: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402940: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402960: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402980: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804029a0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804029c0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 00000000804029e0: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402a00: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402a20: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402a40: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402a60: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef 800001ae 00000000
riscv_stackdump: 0000000080402a80: 800019f0 00000000 80402b80 00000000 deadbeef deadbeef 00000000 00000000
riscv_stackdump: 0000000080402aa0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
riscv_stackdump: 0000000080402ac0: 00000000 00000000 801005ec 00000000 00000001 00000000 80401fa0 00000000
riscv_stackdump: 0000000080402ae0: 80401fa0 00000000 801005ec 00000000 00000001 00000000 00000101 00000000
riscv_stackdump: 0000000080402b00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
riscv_stackdump: 0000000080402b20: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
riscv_stackdump: 0000000080402b40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
riscv_stackdump: 0000000080402b60: 00000000 00000000 00000000 00000000 00000000 00000000 00001800 0000000a
riscv_stackdump: 0000000080402b80: deadbeef deadbeef 800019f0 00000000 deadbeef deadbeef 00000000 00000000
riscv_showtasks: PID PRI USED STACK FILLED COMMAND
riscv_showtasks: ---- ---- 656 2048 32.0% irq
riscv_dump_task: 0 0 528 2000 26.4% Idle Task
riscv_dump_task: 1 100 296 3040 9.7% ostest_main
```
I'll try it again with qemu 5.2
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi removed a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi removed a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1076166983
@masayuki2009 Could you try this PR again ?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080774112
But the thread stack also switch out. For example, let's assume that the user space thread call semwait and the count of sem is zero:
1. Task 1 run on stack 1 call sem_wait and then trigger ecall
2. The ecall handler switch to S-mode and dispatch to nxsem_wait
3. nxsem_wait continue execute on stack 1
4. up_block_task is called since the count is zero
5. group_addrenv switch the address space to the next task
6. But the current SP isn't changed and still point to the stack 1(of course, it isn't the memory of stack 1 anymore, but somewhere in task 2 memory space).
So, I think the problem exist in the origin code.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080654327
> Thank you for your response, yes it is clear that xcp.regs is still in kernel memory and is addressable. However, the memory for the context itself exists in MMU mapped memory only if I'm right. As `tcb->stack_alloc_ptr = kumm_malloc(stack_size); `uses the userspace heap allocator which works with virtual addresses that exist in the user task's own memory. Obviously the mapping exists for the kernel as well, when the task is in execution.
>
Yes, I think so.
> What I'm concerned about is that calling `(void)group_addrenv(nexttcb);` prior to the context switch changes the MMU mappings already, and the address environment that is used to store the task that is being preempted is not in place any more.
call group_addrenv here is too early, even without @no1wudi 's change, it still can't work as expect, because the current stack is swapped out. The right place is in riscv_swint::SYS_switch_context.
> So where does e.g. arm_switchcontext(a,b) actually save the task's context (the task that is removed from execution) ? It pushes it to the stack, but to me it will be the new task's address environment (task that will continue) and the old register set is saved into task b:s stack instead of task a:s stack.
>
Since the similar code also exist on ARM, @anchao will try to provide a patch fixing this issue.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080751884
> Yes, xcp.regs[] is in the kernel space, but the thread stack pointed by SP isn't.
Exactly, and now the memory for the task context is taken from the stack. Previously the memory was in tcb, as xcp.regs was a buffer, not a pointer. This is why the old code works, because the buffer is in kernel memory, for both tasks.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080599016
> Can someone please explain to me how this change is supposed to work with MMU and address environments ? Am I misunderstanding something ? The xcp.regs are now allocated from the user task's stack memory, prior to this change they were in the kernel's memory space.
>
> Please note the following example
>
> ```
>
> struct tcb_s *nexttcb = this_task();
>
> #ifdef CONFIG_ARCH_ADDRENV
> /* Make sure that the address environment for the previously
> * running task is closed down gracefully (data caches dump,
> * MMU flushed) and set up the address environment for the new
> * thread at the head of the ready-to-run list.
> */
>
> (void)group_addrenv(nexttcb);
> #endif
> /* Reset scheduler parameters */
>
> nxsched_resume_scheduler(nexttcb);
>
> /* Then switch contexts */
>
> riscv_switchcontext(&rtcb->xcp.regs, nexttcb->xcp.regs);
> ```
>
> If rtcb->xpc.regs points to the old task's stack, which resides in its address environment, how can we perform a register save there ?
>
since rtcb->xcp.regs itself is still in the kernel space, riscv_switchcontext can update rtcb->xcp.regs to the new context on the stack:
https://github.com/apache/incubator-nuttx/blob/master/arch/risc-v/src/common/riscv_swint.c#L247-L254
> The old implementation saved the task context in the kernel space memory, which is why it is safe to change the address environment as the memory for xcp.regs[] exists in the kernel space and is always mapped ?
The problem we can see is that up_schedule_sigaction may touch the stack of non current thread directly:
- https://github.com/apache/incubator-nuttx/pull/5731/files#diff-6959df667f4307ac5bd81dbd07981927ab5612fe0590f0dfca26665bcde96ec6R307-R318
- https://github.com/apache/incubator-nuttx/pull/5731/files#diff-6959df667f4307ac5bd81dbd07981927ab5612fe0590f0dfca26665bcde96ec6R399-R415
Since RISCV kernel mode isn't mainlined yet, @anchao will test the same change(#5645) on ARM.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080676592
> > I can confirm that for RISC-V the flat and protected targets work (with #5881) but the kernel mode target crashes with a load page fault. I will investigate further but the code was working before i.e. I could enter nsh and run things on the console. Now the SW crashes before nsh is launched.
>
> Do you have try the context switch between two user space thread without this change? It work before may just because the next thread is kernel thread.
I had several user tasks running so yes it got tested. As I said previously the memory for tcb.xcp.regs[] was in the tcb itself, which means it is in kernel memory for kernel an user tasks both.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080751884
> Yes, xcp.regs[] is in the kernel space, but the thread stack pointed by SP isn't.
Exactly, and now the memory for the task context is taken from the stack.
```
xcp->regs = (uintptr_t *)(
(uintptr_t)tcb->stack_base_ptr + tcb->adj_stack_size - XCPTCONTEXT_SIZE);
```
Previously the memory was in tcb, as xcp.regs was a buffer, not a pointer: `uintptr_t regs[XCPTCONTEXT_REGS];`. This is why the old code works, because the buffer is in tcb which is kernel memory, for both tasks.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080656533
> I can confirm that for RISC-V the flat and protected targets work (with #5881) but the kernel mode target crashes with a load page fault. I will investigate further but the code was working before i.e. I could enter nsh and run things on the console. Now the SW crashes before nsh is launched.
Do you have try the context switch between two user space thread without this change? It work before may just because the next thread is kernel thread.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080610588
Thank you for your response, yes it is clear that xcp.regs is still in kernel memory and is addressable. However, the memory for the context itself exists in MMU mapped memory only if I'm right. As `tcb->stack_alloc_ptr = kumm_malloc(stack_size); `uses the userspace heap allocator which works with virtual addresses that exist in the user task's own memory. Obviously the mapping exists for the kernel as well, when the task is in execution.
What I'm concerned about is that calling `(void)group_addrenv(nexttcb);` prior to the context switch changes the MMU mappings already, and the address environment that is used to store the task that is being preempted is not in place any more. So where does the exception entry actually save the task's context (the task that is removed from execution) ? To me it is the new task's address environment (task that will continue). Unless I'm totally missing something here.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1077018379
Emm, smp is still not work with latest update.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1079078557
>
The conflict fixed now.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1076653910
@no1wudi is it ready for reviewing?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1081012244
> Yes, but it isn't reliable since we can't control how C compiler generate the machine code which may insert push/pop operation between group_addrenv<->riscv_switchcontext and riscv_switchcontext<->riscv_swint in the new version or different compiler options.
Yes only option is to make sure interrupt or kernel stack is in use. I'm not even sure how the old implementation seemingly works for me. If the return address is put into stack, the SW should get lost when it attempts to return from group_addrenv(), as the memory pointed to by SP is no longer the same.
Unless the next task is always a kernel task when a user task is being suspended, then the address is still mapped, like you already said. I must have gotten lucky in my testing or something.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080751884
> Yes, xcp.regs[] is in the kernel space, but the thread stack pointed by SP isn't.
Exactly, and now the memory for the task context is taken from the stack. ```
xcp->regs = (uintptr_t *)(
(uintptr_t)tcb->stack_base_ptr + tcb->adj_stack_size - XCPTCONTEXT_SIZE);
```
Previously the memory was in tcb, as xcp.regs was a buffer, not a pointer: `uintptr_t regs[XCPTCONTEXT_REGS];`. This is why the old code works, because the buffer is in tcb which is kernel memory, for both tasks.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080599016
> Can someone please explain to me how this change is supposed to work with MMU and address environments ? Am I misunderstanding something ? The xcp.regs are now allocated from the user task's stack memory, prior to this change they were in the kernel's memory space.
>
> Please note the following example
>
> ```
>
> struct tcb_s *nexttcb = this_task();
>
> #ifdef CONFIG_ARCH_ADDRENV
> /* Make sure that the address environment for the previously
> * running task is closed down gracefully (data caches dump,
> * MMU flushed) and set up the address environment for the new
> * thread at the head of the ready-to-run list.
> */
>
> (void)group_addrenv(nexttcb);
> #endif
> /* Reset scheduler parameters */
>
> nxsched_resume_scheduler(nexttcb);
>
> /* Then switch contexts */
>
> riscv_switchcontext(&rtcb->xcp.regs, nexttcb->xcp.regs);
> ```
>
> If rtcb->xpc.regs points to the old task's stack, which resides in its address environment, how can we perform a register save there ?
>
since rtcb->xcp.regs itself is still in the kernel space, riscv_switchcontext can update rtcb->xcp.regs to the new context on the stack:
https://github.com/apache/incubator-nuttx/blob/master/arch/risc-v/src/common/riscv_swint.c#L247-L254
> The old implementation saved the task context in the kernel space memory, which is why it is safe to change the address environment as the memory for xcp.regs[] exists in the kernel space and is always mapped ?
The problem we can see is that up_schedule_sigaction may touch the stack of non current thread directly:
- https://github.com/apache/incubator-nuttx/pull/5731/files#diff-6959df667f4307ac5bd81dbd07981927ab5612fe0590f0dfca26665bcde96ec6R307-R318
- https://github.com/apache/incubator-nuttx/pull/5731/files#diff-6959df667f4307ac5bd81dbd07981927ab5612fe0590f0dfca26665bcde96ec6R399-R415
We haven't found other places will touch the memory pointed by xcp.regs of the non current thread, if you find some please point out.
Since RISCV kernel mode isn't mainlined yet, @anchao will test the same change(#5645) on ARM.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#discussion_r835125326
##########
File path: arch/risc-v/src/common/riscv_exception_common.S
##########
@@ -119,12 +119,23 @@ exception_common:
la sp, g_intstacktop
#endif
-#endif
+ /* Call interrupt handler in C */
+
+ jal x1, riscv_dispatch_irq
+
+#else
+ /* Reserve some space for CURRENT_REGS if interrupt stack disabled */
Review comment:
add blank line
##########
File path: arch/risc-v/src/qemu-rv/qemu_rv_head.S
##########
@@ -77,11 +77,20 @@ __start:
ld sp, 0(t0)
#endif
- /* sp (stack top) = sp + idle stack size */
-
- li t0, CONFIG_IDLETHREAD_STACKSIZE
+ /*
+ * sp (stack top) = sp + idle stack size - XCPTCONTEXT_SIZE
+ *
+ * Note: Reserve some space used by up_initial_state since we are already
+ * running and using the per CPU idle stack.
+ */
+
+ li t0, CONFIG_IDLETHREAD_STACKSIZE - XCPTCONTEXT_SIZE
Review comment:
change to STACK_ALIGN_UP(CONFIG_IDLETHREAD_STACKSIZE - XCPTCONTEXT_SIZE)
and remove line 90-93
##########
File path: arch/risc-v/src/k210/k210_head.S
##########
@@ -73,11 +73,21 @@ __start:
ld sp, 0(t0)
- /* sp (stack top) = sp + idle stack size */
-
- li t0, CONFIG_IDLETHREAD_STACKSIZE
+ /*
+ * sp (stack top) = sp + idle stack size - XCPTCONTEXT_SIZE
+ *
+ * Note: Reserve some space used by up_initial_state since we are already
+ * running and using the per CPU idle stack.
+ */
+
+ li t0, CONFIG_IDLETHREAD_STACKSIZE - XCPTCONTEXT_SIZE
Review comment:
change to STACK_ALIGN_UP(CONFIG_IDLETHREAD_STACKSIZE - XCPTCONTEXT_SIZE)
and remove line 90-93
##########
File path: arch/risc-v/src/common/riscv_exception_common.S
##########
@@ -119,12 +119,23 @@ exception_common:
la sp, g_intstacktop
#endif
-#endif
+ /* Call interrupt handler in C */
+
+ jal x1, riscv_dispatch_irq
+
+#else
+ /* Reserve some space for CURRENT_REGS if interrupt stack disabled */
+ addi sp, sp, -XCPTCONTEXT_SIZE
/* Call interrupt handler in C */
jal x1, riscv_dispatch_irq
+ /* Restore sp */
Review comment:
add blank line
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1079024831
@masayuki2009 could you try with your fail case again?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1078740606
@no1wudi
It seems that the PR has only 1 commit which also contains some style changes.
So could you please separate such the changes to another commit?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080774112
But the thread stack also switch out. For example, let's assume that the user space thread call semwait and the count of sem is zero:
1. Task 1 run on stack 1 call sem_wait and then trigger ecall
2. The ecall handler switch to S-mode and dispatch to nxsem_wait
3. nxsem_wait continue execute on stack 1
4. up_block_task is called since the count is zero
5. group_addrenv switch the address space to the next task
6. But the current SP isn't changed and still point to the stack 1(of course, it isn't the memory of stack 1 anymore, but somewhere in task 2 memory space).
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1066322171
@masayuki2009 I can't reproduce this issue but the problem mentioned in #5672 occured:
```
user_main: nested signal handler test
signest_test: Starting signal waiter task at priority 101
waiter_main: Waiter started
waiter_main: Setting signal mask
waiter_main: Registering signal handler
waiter_main: Waiting on semaphore
signest_test: Started waiter_main pid=86
signest_test: Starting interfering task at priority 102
interfere_main: Waiting on semaphore
signest_test: Started interfere_main pid=87
[CPU1] riscv_fault: EPC: 0000000080004fe8
[CPU1] riscv_fault: Fault IRQ=7
[CPU1] riscv_fault: A0: 0000000080481480 A1: 0000000080483368 A2: 0000000080402588 A3: 0000000080402578
[CPU1] riscv_fault: A4: 0000000000000000 A5: 0000000000000000 A6: 000000000000000c A7: 000000008010a03e
[CPU1] riscv_fault: T0: 0000000080004c58 T1: 0000000080003da2 T2: 0000000000000000 T3: 0000000000000000
[CPU1] riscv_fault: T4: 0000000000000000 T5: 0000000000000000 T6: 0000000000000000
[CPU1] riscv_fault: S0: 00000000804832d0 S1: 0000000080481480 S2: 0000000000000000 S3: 0000000000000000
[CPU1] riscv_fault: S4: 0000000080483368 S5: 0000000000000004 S6: 0000000000000004 S7: 0000000080483358
[CPU1] riscv_fault: S8: 0000000000000002 S9: 0000000000000088 S10: 0000000000000000 S11: 0000000000000000
[CPU1] riscv_fault: SP: 000000008040b1d0 FP: 00000000804832d0 TP: 0000000000000000 RA: 0000000080001dca
[CPU1] up_assert: Assertion failed CPU1 at file:chip/k210_irq_dispatch.c line: 91 task: waiter
[CPU1] riscv_registerdump: EPC: 0000000080004fe8
[CPU1] riscv_registerdump: A0: 0000000080481480 A1: 0000000080483368 A2: 0000000080402588 A3: 0000000080402578
[CPU1] riscv_registerdump: A4: 0000000000000000 A5: 0000000000000000 A6: 000000000000000c A7: 000000008010a03e
[CPU1] riscv_registerdump: T0: 0000000080004c58 T1: 0000000080003da2 T2: 0000000000000000 T3: 0000000000000000
[CPU1] riscv_registerdump: T4: 0000000000000000 T5: 0000000000000000 T6: 0000000000000000
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1066240709
@no1wudi
I tried this PR with maix-bit:kostest on QEMU but the ASSERTION happened.
```
$ ~/opensource/QEMU/qemu-5.2/build/riscv64-softmmu/qemu-system-riscv64 -nographic -machine sifive_u -device loader,file=nuttx.bin,addr=0x80000000 -device loader,file=nuttx_user.bin,addr=0x80100000
ABCDstdio_test: write fd=1
stdio_test: Standard I/O Check: printf
stdio_test: write fd=2
stdio_test: Standard I/O Check: fprintf to stderr
ostest_main: putenv(Variable1=BadValue3)
ostest_main: setenv(Variable1, GoodValue1, TRUE)
ostest_main: setenv(Variable2, BadValue1, FALSE)
ostest_main: setenv(Variable2, GoodValue2, TRUE)
ostest_main: setenv(Variable3, Variable3, FALSE)
ostest_main: setenv(Variable3, Variable3, FALSE)
show_variable: Variable=Variable1 has value=GoodValue1
show_variable: Variable=Variable2 has value=GoodValue2
show_variable: Variable=Variable3 has value=GoodValue3
ostest_main: Started user_main at PID=2
...
user_main: waitpid test
Test waitpid()
waitpid_start_child: Started waitpid_main at PID=7
waitpid_start_child: Started waitpid_main at PID=11
waitpid_main: PID 7 Started
waitpid_main: PID 11 Started
waitpid_main: PID 12 Started
up_assert: Assertion failed at file:stdio/lib_libfilesem.c line: 106 task: ostest
riscv_registerdump: EPC: 00000000800062be
riscv_registerdump: A0: 0000000000000000 A1: 0000000080404960 A2: 000000000000003f A3: 0000000000000002
riscv_registerdump: A4: 0000000010010000 A5: 0000000080404960 A6: 000000000000006c A7: 0000000000000068
riscv_registerdump: T0: 0000000000000030 T1: 0000000000000009 T2: 0000000000000020 T3: 000000000000002a
riscv_registerdump: T4: 000000000000002e T5: 00000000000001ff T6: 000000000000002d
riscv_registerdump: S0: 00000000800008fe S1: 0000000080404908 S2: 0000000080481930 S3: 0000000080401188
riscv_registerdump: S4: 0000000080102502 S5: 000000008010a210 S6: 0000000080404b48 S7: 000000008010a27b
riscv_registerdump: S8: 0000000000000a00 S9: 0000000000001000 S10: 0000000000000800 S11: deadbeefdeadbeef
riscv_registerdump: SP: 0000000080404908 FP: 00000000800008fe TP: deadbeefdeadbeef RA: 0000000080005fbe
riscv_dumpstate: sp: 0000000080404908
riscv_dumpstate: IRQ stack:
riscv_dumpstate: base: 00000000804000b0
riscv_dumpstate: size: 0000000000000800
riscv_dumpstate: User stack:
riscv_dumpstate: base: 0000000080402df0
riscv_dumpstate: size: 0000000000001fb0
riscv_dumpstate: ERROR: Stack pointer is not within allocated stack
riscv_stackdump: 0000000080402de0: 67724100 72410033 00003467 00000000 deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402e00: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
riscv_stackdump: 0000000080402e20: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
``
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1076166983
@masayuki2009 Could you try this PR again ?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080610588
Thank you for your response, yes it is clear that xcp.regs is still in kernel memory and is addressable. However, the memory for the context itself exists in MMU mapped memory only if I'm right. As `tcb->stack_alloc_ptr = kumm_malloc(stack_size); `uses the userspace heap allocator which works with virtual addresses that exist in the user task's own memory. Obviously the mapping exists for the kernel as well, when the task is in execution.
What I'm concerned about is that calling `(void)group_addrenv(nexttcb);` prior to the context switch changes the MMU mappings already, and the address environment that is used to store the task that is being preempted is not in place any more. So where does e.g. arm_switchcontext(a,b) actually save the task's context (the task that is removed from execution) ? It pushes it to the stack, but to me it will be the new task's address environment (task that will continue) and the old register set is saved into task b:s stack instead of task a:s stack.
Unless I'm totally missing something here.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080681445
Yes, xcp.regs[] is in the kernel space, but the thread stack pointed by SP isn't.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 merged pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
masayuki2009 merged pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080996343
> Oops, you are also right, the original code is suspicious too, as the first thing exception_common does is this:
>
> ```
> exception_common:
> addi sp, sp, -XCPTCONTEXT_SIZE
> ```
>
> I assume the same issue is in the ARM implementation too. In this case the new task's address environment is already in use, but sp still points to the old task.
Yes, ARM has the similar issue, @anchao hit the problem before like this:
1. The first task wait on a named semaphore
2. The second task post this semaphore to wake up the first
3. And the system panic suddenly
he made a temp fix like this:
1. Move group_addrenv to arm_syscall::SYS_switch_context
2. Disable FPU since the lazy fpu save may corrupt the next task memory space
The above problem is fixed. So, to fix this issue:
1. We need review all place which call group_addrenv and move to the right location
2. Save and restore FPU in exception_common
@no1wudi please prepare two patches for RISCV tomorrow, so @pussuw can verify with his S-mode change.
@anchao will fix the similar issue on ARM in the next couple days.
>
> This is not an issue with the new S-mode function however:
>
> ```
> riscv_switchcontext:
>
> /* Save old context to arg[0] */
>
> save_ctx a0 /* save context */
> ```
>
> Saving the context does not use stack, and this is why the old code works on my table.
If we postpone the address space switch until save_ctx return, this PR should work with your S-mode change, I think.
>
Yes, but it isn't reliable since we can't control how C compiler generate the machine code which may insert push/pop operation between group_addrenv<->riscv_switchcontext and riscv_switchcontext<->riscv_swint in the new version or different compiler options.
> But indeed, the location for calling group_addrenv() is suspicious, as the context switch is done by using the new tasks environment, even though the old task's registers are still in use.
Yes, we move need do the address space change with the interrupt or kernel stack, otherwise we have to write the whole process in assemble code, so we can control the stack usage manually.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1081012244
> Yes, but it isn't reliable since we can't control how C compiler generate the machine code which may insert push/pop operation between group_addrenv<->riscv_switchcontext and riscv_switchcontext<->riscv_swint in the new version or different compiler options.
Yes only option is to make sure interrupt or kernel stack is in use. I'm not even sure how the old implementation seemingly works for me. If the return address is put into stack, the SW should get lost when it attempts to return from group_addrenv(), as the memory pointed to by SP is no longer the same.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1081012244
> Yes, but it isn't reliable since we can't control how C compiler generate the machine code which may insert push/pop operation between group_addrenv<->riscv_switchcontext and riscv_switchcontext<->riscv_swint in the new version or different compiler options.
Yes only option is to make sure interrupt or kernel stack is in use. I'm not even sure how the old implementation seemingly works for me. If the return address is put into stack, the SW should get lost when it attempts to return from group_addrenv(), as the memory pointed to by SP is no longer the same.
Unless the next task is always a kernel task, then the address is still mapped, like you already said. I must have gotten lucky in my testing or something.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#discussion_r825443766
##########
File path: arch/risc-v/src/qemu-rv/qemu_rv_head.S
##########
@@ -77,9 +77,9 @@ __start:
ld sp, 0(t0)
#endif
- /* sp (stack top) = sp + idle stack size */
+ /* sp (stack top) = sp + idle stack size - XCPTCONTEXT_SIZE */
- li t0, CONFIG_IDLETHREAD_STACKSIZE
+ li t0, CONFIG_IDLETHREAD_STACKSIZE - XCPTCONTEXT_SIZE
Review comment:
don't need reserve xcpt for idle thread
##########
File path: arch/risc-v/src/k210/k210_head.S
##########
@@ -73,9 +73,9 @@ __start:
ld sp, 0(t0)
- /* sp (stack top) = sp + idle stack size */
+ /* sp (stack top) = sp + idle stack size - XCPTCONTEXT_SIZE */
- li t0, CONFIG_IDLETHREAD_STACKSIZE
+ li t0, CONFIG_IDLETHREAD_STACKSIZE - XCPTCONTEXT_SIZE
Review comment:
don't preserve
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -267,20 +258,34 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
* been delivered.
*/
- tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ tcb->xcp.sigdeliver = sigdeliver;
/* Then set up vector to the trampoline with interrupts
* disabled. We must already be in privileged thread mode
* to be here.
*/
- tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ /* Save the current register context location */
+
+ tcb->xcp.saved_regs = tcb->xcp.regs;
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ tcb->xcp.regs = (uintptr_t *)STACK_ALIGN_DOWN(
+ (uintptr_t)tcb->xcp.regs -
+ XCPTCONTEXT_SIZE);
Review comment:
merge to line 278
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -172,26 +162,27 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
* been delivered.
*/
- tcb->xcp.sigdeliver = sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ tcb->xcp.sigdeliver = sigdeliver;
Review comment:
remove the extra space before =
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -353,8 +370,22 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
*/
tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+
+ /* Save the current register context location */
+
+ tcb->xcp.saved_regs = tcb->xcp.regs;
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has been
+ * delivered.
+ */
+
+ tcb->xcp.regs = (uintptr_t *)STACK_ALIGN_DOWN(
+ (uintptr_t)tcb->xcp.regs -
+ XCPTCONTEXT_SIZE);
Review comment:
merge to line 384
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -293,8 +298,26 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
*/
tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = CURRENT_REGS[REG_EPC];
- tcb->xcp.saved_int_ctx = CURRENT_REGS[REG_INT_CTX];
+
+ /* And make sure that the saved context in the TCB is the
+ * same as the interrupt return context.
+ */
+
+ riscv_savestate(tcb->xcp.saved_regs);
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ CURRENT_REGS = (uintptr_t *)STACK_ALIGN_DOWN(
+ (uintptr_t)CURRENT_REGS -
+ XCPTCONTEXT_SIZE);
Review comment:
align with line 314
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -172,26 +162,27 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
* been delivered.
*/
- tcb->xcp.sigdeliver = sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ tcb->xcp.sigdeliver = sigdeliver;
- /* Then set up to vector to the trampoline with interrupts
- * disabled. We must already be in privileged thread mode to be
- * here.
+ /* Save the current register context location */
+
+ tcb->xcp.saved_regs = tcb->xcp.regs;
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has been
+ * delivered.
*/
- tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ tcb->xcp.regs = (uintptr_t *)STACK_ALIGN_DOWN(
+ (uintptr_t)tcb->xcp.regs - XCPTCONTEXT_SIZE);
+ memcpy(tcb->xcp.regs, tcb->xcp.saved_regs, XCPTCONTEXT_SIZE);
+
+ tcb->xcp.regs[REG_SP] = (uintptr_t)tcb->xcp.regs;
Review comment:
keep = at line 180 align with line 182
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -267,20 +258,34 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
* been delivered.
*/
- tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = tcb->xcp.regs[REG_EPC];
- tcb->xcp.saved_int_ctx = tcb->xcp.regs[REG_INT_CTX];
+ tcb->xcp.sigdeliver = sigdeliver;
/* Then set up vector to the trampoline with interrupts
* disabled. We must already be in privileged thread mode
* to be here.
*/
- tcb->xcp.regs[REG_EPC] = (uintptr_t)riscv_sigdeliver;
+ /* Save the current register context location */
+
+ tcb->xcp.saved_regs = tcb->xcp.regs;
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ tcb->xcp.regs = (uintptr_t *)STACK_ALIGN_DOWN(
+ (uintptr_t)tcb->xcp.regs -
+ XCPTCONTEXT_SIZE);
- int_ctx = tcb->xcp.regs[REG_INT_CTX];
- int_ctx &= ~MSTATUS_MPIE;
+ memcpy(tcb->xcp.regs, tcb->xcp.saved_regs,
+ XCPTCONTEXT_SIZE);
Review comment:
merge to line 281
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -293,8 +298,26 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
*/
tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = CURRENT_REGS[REG_EPC];
- tcb->xcp.saved_int_ctx = CURRENT_REGS[REG_INT_CTX];
+
+ /* And make sure that the saved context in the TCB is the
+ * same as the interrupt return context.
+ */
+
+ riscv_savestate(tcb->xcp.saved_regs);
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ CURRENT_REGS = (uintptr_t *)STACK_ALIGN_DOWN(
+ (uintptr_t)CURRENT_REGS -
+ XCPTCONTEXT_SIZE);
+
+ memcpy((uintptr_t *)CURRENT_REGS, tcb->xcp.saved_regs,
+ XCPTCONTEXT_SIZE);
+
+ CURRENT_REGS[REG_SP] = (uintptr_t)CURRENT_REGS;
Review comment:
align = with line 327
##########
File path: arch/risc-v/src/common/riscv_initialstate.c
##########
@@ -74,24 +78,21 @@ void up_initial_state(struct tcb_s *tcb)
riscv_stack_color(tcb->stack_alloc_ptr, 0);
#endif /* CONFIG_STACK_COLORATION */
+ return;
}
- /* Initialize the initial exception register context structure */
-
- memset(xcp, 0, sizeof(struct xcptcontext));
+ xcp->regs = (uintptr_t *)(STACK_ALIGN_DOWN(
+ (uintptr_t)tcb->stack_base_ptr +
+ tcb->adj_stack_size -
+ XCPTCONTEXT_SIZE));
- /* Save the initial stack pointer. Hmmm.. the stack is set to the very
- * beginning of the stack region. Some functions may want to store data on
- * the caller's stack and it might be good to reserve some space. However,
- * only the start function would do that and we have control over that one
- */
+ /* Save the initial stack pointer */
- xcp->regs[REG_SP] = (uintptr_t)tcb->stack_base_ptr +
- tcb->adj_stack_size;
+ xcp->regs[REG_SP] = (uintptr_t)xcp->regs;
/* Save the task entry point */
- xcp->regs[REG_EPC] = (uintptr_t)tcb->start;
+ xcp->regs[REG_EPC] = (uintptr_t)tcb->start;
Review comment:
remove the space at before = at line 100 too
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1079089384
@no1wudi
There still remains style changes that should be in a separate commit
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1079070581
>@masayuki2009 could you try with your fail case again?
@xiaoxiang781216 @no1wudi
I think that this PR should be rebased to fix the conflicts reported by the github.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080795149
Oops, you are also right, the original code is suspicious too, as the first thing exception_common does is this:
```
exception_common:
addi sp, sp, -XCPTCONTEXT_SIZE
```
I assume the same issue is in the ARM implementation too.
In this case the new task's address environment is already in use, but sp still points to the old task.
This is not an issue with the new S-mode function however:
```
riscv_switchcontext:
/* Save old context to arg[0] */
save_ctx a0 /* save context */
```
Saving the context does not use stack, and this is why the code works on my table.
But indeed, the location for calling group_addrenv() is suspicious, as the context switch is done by using the new tasks environment, even though the old task's registers are still in use.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080670618
> call group_addrenv here is too early, even without @no1wudi 's change, it still can't work as expect, because the current stack is swapped out. The right place is in riscv_swint::SYS_switch_context.
Yes it is too early, but only with the new context switch change. It worked previously since the tcb.xcp contained memory for the task's context. So calling riscv_switchcontext(a,b) memory for both a and b exist exists in kernel space, so it is always mapped, and safe to use even after calling group_addrenv().
After this riscv_switchcontext(a,b) is using b's stack, which is imo correct.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080564175
Can someone please explain to me how this change is supposed to work with MMU and address environments ? Am I misunderstanding something ? The xcp.regs are now allocated from the user task's stack memory, prior to this change they were in the kernel's memory space.
Please note the following example
```
struct tcb_s *nexttcb = this_task();
#ifdef CONFIG_ARCH_ADDRENV
/* Make sure that the address environment for the previously
* running task is closed down gracefully (data caches dump,
* MMU flushed) and set up the address environment for the new
* thread at the head of the ready-to-run list.
*/
(void)group_addrenv(nexttcb);
#endif
/* Reset scheduler parameters */
nxsched_resume_scheduler(nexttcb);
/* Then switch contexts */
riscv_switchcontext(&rtcb->xcp.regs, nexttcb->xcp.regs);
```
If rtcb->xpc.regs points to the old task's stack, which resides in its address environment, how can we perform a register save there ?
The old implementation saved the task context in the kernel space memory, which is why it is safe to change the address environment as the memory for xcp.regs[] exists in the kernel space and is always mapped ?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1078604901
@xiaoxiang781216 @masayuki2009 This patch is ready to review now.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080610588
Thank you for your response, yes it is clear that xcp.regs is still in kernel memory and is addressable. However, the memory for the context itself exists in MMU mapped memory only if I'm right. As `tcb->stack_alloc_ptr = kumm_malloc(stack_size); `uses the userspace heap allocator which works with virtual addresses.
What I'm concerned about is that calling `(void)group_addrenv(nexttcb);` prior to the context switch changes the MMU mappings already, and the address environment that is used to store the task that is being preempted is not in place any more. So where does the exception entry actually save the task's context (the task that is removed from execution) ? To me it is the new task's address environment (task that will continue). Unless I'm totally missing something here.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080610588
Thank you for your response, yes it is clear that xcp.regs is still in kernel memory and is addressable. However, the memory for the context itself exists in MMU mapped memory only if I'm right. As `tcb->stack_alloc_ptr = kumm_malloc(stack_size); `uses the userspace heap allocator which works with virtual addresses.
What I'm concerned about is that calling `(void)group_addrenv(nexttcb);` prior to the context switch changes the MMU mappings already, and the address environment that is used to store the task that is being preempted is not in place any more. So where does the exception entry actually save the current task's context ? To me it is the new task's address environment. Unless I'm totally missing something here.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080654327
> Thank you for your response, yes it is clear that xcp.regs is still in kernel memory and is addressable. However, the memory for the context itself exists in MMU mapped memory only if I'm right. As `tcb->stack_alloc_ptr = kumm_malloc(stack_size); `uses the userspace heap allocator which works with virtual addresses that exist in the user task's own memory. Obviously the mapping exists for the kernel as well, when the task is in execution.
>
Yes, I think so.
> What I'm concerned about is that calling `(void)group_addrenv(nexttcb);` prior to the context switch changes the MMU mappings already, and the address environment that is used to store the task that is being preempted is not in place any more.
call group_addrenv here is too early, even without @no1wudi 's change, it still can't work as expect, because the current stack is swapped out.
> So where does e.g. arm_switchcontext(a,b) actually save the task's context (the task that is removed from execution) ? It pushes it to the stack, but to me it will be the new task's address environment (task that will continue) and the old register set is saved into task b:s stack instead of task a:s stack.
>
Since the similar code also exist on ARM, @anchao will try to provide a patch fixing this issue.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080751884
> Yes, xcp.regs[] is in the kernel space, but the thread stack pointed by SP isn't.
Exactly, and now the memory for the task context is taken from the stack. Previously the memory was in tcb, as xcp.regs was a buffer, not a pointer: `uintptr_t regs[XCPTCONTEXT_REGS];`. This is why the old code works, because the buffer is in tcb which is kernel memory, for both tasks.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1080751884
> Yes, xcp.regs[] is in the kernel space, but the thread stack pointed by SP isn't.
Exactly, and now the memory for saving the task context is taken from the stack.
```
xcp->regs = (uintptr_t *)(
(uintptr_t)tcb->stack_base_ptr + tcb->adj_stack_size - XCPTCONTEXT_SIZE);
```
Previously the memory was in tcb, as xcp.regs was a buffer, not a pointer: `uintptr_t regs[XCPTCONTEXT_REGS];`. This is why the old code works, because the buffer is in tcb which is kernel memory, for both tasks.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw edited a comment on pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
pussuw edited a comment on pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#issuecomment-1081012244
> Yes, but it isn't reliable since we can't control how C compiler generate the machine code which may insert push/pop operation between group_addrenv<->riscv_switchcontext and riscv_switchcontext<->riscv_swint in the new version or different compiler options.
Yes only option is to make sure interrupt or kernel stack is in use. I'm not even sure how the old implementation seemingly works for me. If the return address is put into stack, the SW should get lost when it attempts to return from group_addrenv(), as the memory pointed to by SP is no longer the same.
Unless the next task is always a kernel task when a user task is being suspended, then the address is still mapped, like you already said. I must have gotten lucky in my testing or something.
I guess this is what saves it
```
/* Does the group have an address environment? */
if ((group->tg_flags & GROUP_FLAG_ADDRENV) == 0)
{
/* No... just return perhaps leaving a different address environment
* intact.
*/
return OK;
}
```
So when a kernel task is resumed and a user task is suspended, the user task's environment is left in place, I guess..
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5731: arch/risc-v: Improve speed of context switch
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5731:
URL: https://github.com/apache/incubator-nuttx/pull/5731#discussion_r825468065
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -293,8 +298,26 @@ void up_schedule_sigaction(struct tcb_s *tcb, sig_deliver_t sigdeliver)
*/
tcb->xcp.sigdeliver = (void *)sigdeliver;
- tcb->xcp.saved_epc = CURRENT_REGS[REG_EPC];
- tcb->xcp.saved_int_ctx = CURRENT_REGS[REG_INT_CTX];
+
+ /* And make sure that the saved context in the TCB is the
+ * same as the interrupt return context.
+ */
+
+ riscv_savestate(tcb->xcp.saved_regs);
+
+ /* Duplicate the register context. These will be
+ * restored by the signal trampoline after the signal has
+ * been delivered.
+ */
+
+ CURRENT_REGS = (uintptr_t *)STACK_ALIGN_DOWN(
+ (uintptr_t)CURRENT_REGS -
+ XCPTCONTEXT_SIZE);
Review comment:
merge to previous line
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org