arch/arm: syscall SYS_switch_context and SYS_restore_context use 0 para #14881

hujun260 · 2024-11-20T14:30:02Z

Summary

arch/arm: syscall SYS_switch_context and SYS_restore_context use 0 para
reason:
simplify context switch
sys_call0(SYS_switch_context)
sys_call0(SYS_restore_context)

Impact

arm

Testing

ostest
mps2-an500:nsh
qemu-armv7a:smp

nuttxpr · 2024-11-20T14:36:52Z

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details. Here's a breakdown:

Insufficient Summary: While it states the what, it's missing the why and how. Why is simplifying the context switch necessary? What specific changes were made to achieve this? How does using 0 parameters simplify it? What were the parameters before?
Incomplete Impact: Simply stating "arm" is not enough. Which ARM architectures? Which boards? Does this change affect any existing drivers? Does this have performance implications? Does it change any APIs that applications might use? It needs more specifics. Address all impact categories explicitly (even if the answer is NO with a brief justification).
Missing Testing Details: Listing platforms is insufficient. "ostest", "mps2-an500:nsh", and "qemu-armv7a:smp" are not descriptive enough. What specific tests were run? Crucially, the testing logs are empty. The logs demonstrating the before and after behavior are mandatory for demonstrating the change's correctness and verifying its impact. Provide actual command-line invocations and their output. Also specify the Build Host details.

Example of how to improve this PR description:

Summary

arch/arm: Simplify context switching by removing parameters from SYS_switch_context and SYS_restore_context

This change removes the parameters previously required for SYS_switch_context and SYS_restore_context syscalls. These parameters were redundant as the necessary context information is already managed internally within the scheduler. This simplification reduces overhead in context switching and improves code clarity. No related NuttX issue.

Impact

Is new feature added? Is existing feature changed? Existing feature changed (optimization).
Impact on user: YES. Applications directly using these syscalls (which is unlikely as these are typically used internally) will need to be updated to remove the parameters.
Impact on build: NO.
Impact on hardware: YES. This change affects ARM architectures (ARMv7-M, ARMv7-A, Cortex-M4 tested). Tested on mps2-an500 and simulated qemu-armv7a:smp.
Impact on documentation: YES. The syscall documentation needs to be updated to reflect the parameter removal. A documentation update is included in this PR.
Impact on security: NO.
Impact on compatibility: Backward compatibility is broken for applications directly using these syscalls. Forward compatibility is maintained.
Anything else to consider: Potential performance improvements.

Testing

I confirm that changes are verified on local setup and works as intended:

Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
Target(s): ARMv7-M (sim:qemu-armv7m), ARMv7-A (sim:qemu-armv7a:smp), Cortex-M4 (mps2-an500:nsh)

Testing logs before change:

# On mps2-an500:nsh
nsh> <command to demonstrate previous behavior with parameters>
<output demonstrating previous behavior>

Testing logs after change:

# On mps2-an500:nsh
nsh> <command to demonstrate new behavior without parameters>
<output demonstrating new behavior>

# On qemu-armv7a:smp
<commands and output demonstrating SMP functionality>

# ostest output (relevant sections) before and after
<ostest output before>
<ostest output after>

By providing complete and detailed information, the reviewers can effectively evaluate the PR and ensure its quality and correctness. Remember, the goal is to make the change clear, easy to understand, and demonstrate its impact and correctness.

arch/arm/include/irq.h

arch/arm/src/armv6-m/arm_sigdeliver.c

arch/arm/src/armv7-a/arm_sigdeliver.c

arch/arm/src/armv7-m/arm_sigdeliver.c

arch/arm/src/armv7-r/arm_sigdeliver.c

arch/arm/src/armv8-m/arm_sigdeliver.c

arch/arm/src/common/arm_internal.h

arch/arm/src/armv8-r/arm_sigdeliver.c

arch/arm/src/armv8-m/arm_svcall.c

arch/arm/src/common/arm_internal.h

arch/arm/src/armv6-m/arm_svcall.c

arch/arm/src/armv7-m/arm_svcall.c

arch/arm/src/armv8-m/arm_svcall.c

arch/arm/include/tlsr82/irq.h

arch/arm/src/tlsr82/Make.defs

arch/arm/src/armv8-r/arm_syscall.c

reason: We hope to keep g_running_tasks valid forever. Signed-off-by: hujun5 <[email protected]>

lcd/st7565.c:107:4: warning: #warning "Optimal setting of CONFIG_LCD_MAXCONTRAST is 255" [-Wcpp] 107 | # warning "Optimal setting of CONFIG_LCD_MAXCONTRAST is 255" | ^~~~~~~ chip/lc823450_cpuindex.c:36: warning: "CORE_COREID" redefined 36 | #define CORE_COREID (LC823450_CORE_BASE + 0x0) | In file included from /home/hujun5/downloads1/vela_sim/nuttx/arch/arm/src/common/arm_internal.h:37, from chip/lc823450_cpuindex.c:29: /home/hujun5/downloads1/vela_sim/nuttx/arch/arm/src/chip/chip.h:48: note: this is the location of the previous definition 48 | #define CORE_COREID (LC823450_CORE_BASE + 0) | chip/lc823450_cpuindex.c:37: warning: "CORE_COREID_ID" redefined 37 | #define CORE_COREID_ID (0x1 << 0) | /home/hujun5/downloads1/vela_sim/nuttx/arch/arm/src/chip/chip.h:49: note: this is the location of the previous definition 49 | #define CORE_COREID_ID (1 << 0) Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfault Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfault Background The origin of this issue is our desire to eliminate the function of storing "regs" in g_current_regs and instead utilize (*running_task)->xcp.regs for storage. The benefits of this approach include faster storage speed and avoiding multiple accesses to g_current_regs during context switching, thus ensuring that whether returning from an interrupt or an exception, we consistently use this_task()->xcp.regs Issue Encountered However, when storing registers, we must ensure that (running_task)->xcp.regs is invalid so that it can be safely overwritten. According to the existing logic, the only scenario where (running_task)->xcp.regs is valid is during restore_context. We must accurately identify this scenario. Initially, we used the condition (running_task)==NULL for this purpose, but we deemed this approach unsatisfactory as it did not align well with the actual logic. (running_task) should not be NULL. Consequently, we adopted other arch-specific methods for judgment, but due to special logic in some arch, the judgment was not accurate, leading to this issue. Solution: For armv6-m, we haven't found a more suitable solution, so we are sticking with (*running_task)==NULL. For armv7-m/armv8-m, by removing support for primask, we can achieve accurate judgment. PRIMASK is a bad design in armv6-m, that's why arm introduce BASEPRI from armv7-m. It's wrong to provide this option for armv7-m/armv8-m arch. Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfault Background The origin of this issue is our desire to eliminate the function of storing "regs" in g_current_regs and instead utilize (*running_task)->xcp.regs for storage. The benefits of this approach include faster storage speed and avoiding multiple accesses to g_current_regs during context switching, thus ensuring that whether returning from an interrupt or an exception, we consistently use this_task()->xcp.regs Issue Encountered However, when storing registers, we must ensure that (running_task)->xcp.regs is invalid so that it can be safely overwritten. According to the existing logic, the only scenario where (running_task)->xcp.regs is valid is during restore_context. We must accurately identify this scenario. Initially, we used the condition (running_task)==NULL for this purpose, but we deemed this approach unsatisfactory as it did not align well with the actual logic. (running_task) should not be NULL. Consequently, we adopted other arch-specific methods for judgment, but due to special logic in some arch, the judgment was not accurate, leading to this issue. Solution: For armv6-m, we haven't found a more suitable solution, so we are sticking with (*running_task)==NULL. For armv7-m/armv8-m, by removing support for primask, we can achieve accurate judgment. PRIMASK is a bad design in armv6-m, that's why arm introduce BASEPRI from armv7-m. It's wrong to provide this option for armv7-m/armv8-m arch. Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfault Background The origin of this issue is our desire to eliminate the function of storing "regs" in g_current_regs and instead utilize (*running_task)->xcp.regs for storage. The benefits of this approach include faster storage speed and avoiding multiple accesses to g_current_regs during context switching, thus ensuring that whether returning from an interrupt or an exception, we consistently use this_task()->xcp.regs Issue Encountered However, when storing registers, we must ensure that (running_task)->xcp.regs is invalid so that it can be safely overwritten. According to the existing logic, the only scenario where (running_task)->xcp.regs is valid is during restore_context. We must accurately identify this scenario. Initially, we used the condition (running_task)==NULL for this purpose, but we deemed this approach unsatisfactory as it did not align well with the actual logic. (running_task) should not be NULL. Consequently, we adopted other arch-specific methods for judgment, but due to special logic in some arch, the judgment was not accurate, leading to this issue. Solution: For armv6-m, we haven't found a more suitable solution, so we are sticking with (*running_task)==NULL. For armv7-m/armv8-m, by removing support for primask, we can achieve accurate judgment. PRIMASK is a design in armv6-m, that's why arm introduce BASEPRI from armv7-m. It's wrong to provide this option for armv7-m/armv8-m arch. Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfault Background The origin of this issue is our desire to eliminate the function of storing "regs" in g_current_regs and instead utilize (*running_task)->xcp.regs for storage. The benefits of this approach include faster storage speed and avoiding multiple accesses to g_current_regs during context switching, thus ensuring that whether returning from an interrupt or an exception, we consistently use this_task()->xcp.regs Issue Encountered However, when storing registers, we must ensure that (running_task)->xcp.regs is invalid so that it can be safely overwritten. According to the existing logic, the only scenario where (running_task)->xcp.regs is valid is during restore_context. We must accurately identify this scenario. Initially, we used the condition (running_task)==NULL for this purpose, but we deemed this approach unsatisfactory as it did not align well with the actual logic. (running_task) should not be NULL. Consequently, we adopted other arch-specific methods for judgment, but due to special logic in some arch, the judgment was not accurate, leading to this issue. Solution: For armv6-m, we haven't found a more suitable solution, so we are sticking with (*running_task)==NULL. For armv7-m/armv8-m, by removing support for primask, we can achieve accurate judgment. PRIMASK is a design in armv6-m, that's why arm introduce BASEPRI from armv7-m. It's wrong to provide this option for armv7-m/armv8-m arch. Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfault Background The origin of this issue is our desire to eliminate the function of storing "regs" in g_current_regs and instead utilize (*running_task)->xcp.regs for storage. The benefits of this approach include faster storage speed and avoiding multiple accesses to g_current_regs during context switching, thus ensuring that whether returning from an interrupt or an exception, we consistently use this_task()->xcp.regs Issue Encountered However, when storing registers, we must ensure that (running_task)->xcp.regs is invalid so that it can be safely overwritten. According to the existing logic, the only scenario where (running_task)->xcp.regs is valid is during restore_context. We must accurately identify this scenario. Initially, we used the condition (running_task)==NULL for this purpose, but we deemed this approach unsatisfactory as it did not align well with the actual logic. (running_task) should not be NULL. Consequently, we adopted other arch-specific methods for judgment, but due to special logic in some arch, the judgment was not accurate, leading to this issue. Solution: For armv6-m, we haven't found a more suitable solution, so we are sticking with (*running_task)==NULL. For armv7-m/armv8-m, by removing support for primask, we can achieve accurate judgment. PRIMASK is a design in armv6-m, that's why arm introduce BASEPRI from armv7-m. It's wrong to provide this option for armv7-m/armv8-m arch. Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

reason: svc call may trigger hardfault Background The origin of this issue is our desire to eliminate the function of storing "regs" in g_current_regs and instead utilize (*running_task)->xcp.regs for storage. The benefits of this approach include faster storage speed and avoiding multiple accesses to g_current_regs during context switching, thus ensuring that whether returning from an interrupt or an exception, we consistently use this_task()->xcp.regs Issue Encountered However, when storing registers, we must ensure that (running_task)->xcp.regs is invalid so that it can be safely overwritten. According to the existing logic, the only scenario where (running_task)->xcp.regs is valid is during restore_context. We must accurately identify this scenario. Initially, we used the condition (running_task)==NULL for this purpose, but we deemed this approach unsatisfactory as it did not align well with the actual logic. (running_task) should not be NULL. Consequently, we adopted other arch-specific methods for judgment, but due to special logic in some arch, the judgment was not accurate, leading to this issue. Solution: For armv6-m, we haven't found a more suitable solution, so we are sticking with (*running_task)==NULL. For armv7-m/armv8-m, by removing support for primask, we can achieve accurate judgment. PRIMASK is a design in armv6-m, that's why arm introduce BASEPRI from armv7-m. It's wrong to provide this option for armv7-m/armv8-m arch. Signed-off-by: hujun5 <[email protected]>

* apache/nuttx/master: risc-v/mpfs: make cache clearing optional arch/arm64/src/imx9/imx9_usdhc.c: Simplify eventwait logic and remove race condition [POSIX][Bug] `mqueue.h`: Include file does not conform the standard zcu111: add support for user led build(CMAKE): fix pac sim elf ONLY in Linux platform armv7/8m: fix regresion from apache#14881 armv6m: fix regresion from apache#14881

hartmannathan · 2024-12-09T22:06:20Z

Please see Issue #15100

github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: M The size of the change in this PR is medium labels Nov 20, 2024

xiaoxiang781216 reviewed Nov 20, 2024

View reviewed changes

arch/arm/include/irq.h Show resolved Hide resolved

hujun260 force-pushed the apache_20 branch 2 times, most recently from e120df4 to ea9129f Compare November 21, 2024 08:51

github-actions bot added Size: L The size of the change in this PR is large and removed Size: M The size of the change in this PR is medium labels Nov 21, 2024

xiaoxiang781216 reviewed Nov 23, 2024

View reviewed changes

hujun260 force-pushed the apache_20 branch 5 times, most recently from b1d9bfc to ed48c90 Compare November 26, 2024 13:10

github-actions bot added Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. and removed Size: L The size of the change in this PR is large labels Nov 26, 2024

hujun260 force-pushed the apache_20 branch from ed48c90 to f0ec61b Compare November 26, 2024 13:14

github-actions bot added Size: L The size of the change in this PR is large and removed Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Nov 26, 2024

hujun260 force-pushed the apache_20 branch from f0ec61b to 6f44449 Compare December 2, 2024 02:02

xiaoxiang781216 reviewed Dec 2, 2024

View reviewed changes

arch/arm/src/common/arm_internal.h Outdated Show resolved Hide resolved

arch/arm/src/common/arm_internal.h Outdated Show resolved Hide resolved

hujun260 force-pushed the apache_20 branch 3 times, most recently from 6fca1b4 to ac3f75f Compare December 2, 2024 10:42

xiaoxiang781216 reviewed Dec 2, 2024

View reviewed changes

arch/arm/src/armv6-m/arm_svcall.c Outdated Show resolved Hide resolved

arch/arm/src/armv7-m/arm_svcall.c Outdated Show resolved Hide resolved

arch/arm/src/armv8-m/arm_svcall.c Outdated Show resolved Hide resolved

arch/arm/include/tlsr82/irq.h Outdated Show resolved Hide resolved

hujun260 force-pushed the apache_20 branch from ac3f75f to 94585fe Compare December 2, 2024 13:39

xiaoxiang781216 reviewed Dec 2, 2024

View reviewed changes

arch/arm/src/tlsr82/Make.defs Show resolved Hide resolved

arch/arm/src/armv8-r/arm_syscall.c Show resolved Hide resolved

arch/arm/src/armv8-r/arm_syscall.c Outdated Show resolved Hide resolved

arch/arm/src/armv8-r/arm_syscall.c Outdated Show resolved Hide resolved

hujun260 force-pushed the apache_20 branch from 94585fe to caa5791 Compare December 3, 2024 03:12

arm: remove g_running_tasks[this_cpu()] = NULL

9a697ab

reason: We hope to keep g_running_tasks valid forever. Signed-off-by: hujun5 <[email protected]>

hujun260 force-pushed the apache_20 branch from caa5791 to 9a697ab Compare December 3, 2024 03:46

hujun260 force-pushed the apache_20 branch from 34d0fec to e49edaf Compare December 3, 2024 06:54

xiaoxiang781216 approved these changes Dec 3, 2024

View reviewed changes

xiaoxiang781216 merged commit 4f3f975 into apache:master Dec 3, 2024
25 checks passed

hujun260 deleted the apache_20 branch December 5, 2024 04:23

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 6, 2024

fix regresion from apache#14881

36797cd

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

hujun260 mentioned this pull request Dec 6, 2024

fix regresion from https://github.com/apache/nuttx/pull/14881 #15073

Merged

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 6, 2024

fix regresion from apache#14881

e3aaf21

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 6, 2024

fix regresion from apache#14881

eb94949

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 6, 2024

fix regresion from apache#14881

92a684d

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 6, 2024

fix regresion from apache#14881

22ee615

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 6, 2024

fix regresion from apache#14881

545d0ce

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 6, 2024

armv6m: fix regresion from apache#14881

939be7a

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 6, 2024

armv7/8m: fix regresion from apache#14881

034edd0

reason: svc call may trigger hardfault Signed-off-by: hujun5 <[email protected]>

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 9, 2024

armv6m: fix regresion from apache#14881

c7dd473

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

xiaoxiang781216 pushed a commit that referenced this pull request Dec 9, 2024

armv6m: fix regresion from #14881

d20189b

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

jerpelea pushed a commit to jerpelea/nuttx that referenced this pull request Dec 9, 2024

armv6m: fix regresion from apache#14881

e6983f0

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

jerpelea mentioned this pull request Dec 9, 2024

[Bp-15073} fix regresion from https://github.com/apache/nuttx/pull/14881 #15086

Merged

xiaoxiang781216 pushed a commit that referenced this pull request Dec 9, 2024

armv6m: fix regresion from #14881

702affa

reason: svc call may trigger hardfalt Signed-off-by: hujun5 <[email protected]>

hartmannathan mentioned this pull request Dec 9, 2024

[BUG] PR-15073 PR-15086 broke High Priority Zero Latency Interrupts #15100

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arch/arm: syscall SYS_switch_context and SYS_restore_context use 0 para #14881

arch/arm: syscall SYS_switch_context and SYS_restore_context use 0 para #14881

hujun260 commented Nov 20, 2024

nuttxpr commented Nov 20, 2024

hartmannathan commented Dec 9, 2024

arch/arm: syscall SYS_switch_context and SYS_restore_context use 0 para #14881

arch/arm: syscall SYS_switch_context and SYS_restore_context use 0 para #14881

Conversation

hujun260 commented Nov 20, 2024

Summary

Impact

Testing

nuttxpr commented Nov 20, 2024

Summary

Impact

Testing

hartmannathan commented Dec 9, 2024