Skip to content

Conversation

@ioannisg
Copy link
Member

ip register holds the stack_info.size (it is passed as argument
into z_arch_user_mode_enter(.)). We trust that the value of
stack_info.size contains the accurate size of the writable
stack buffer, above stack_info.start (as specified in kernel.h).
Therefore, we do not need to subtract any bytes for the MPU
stack guard. This allows us to clean-up one more occurrence of
CONFIG_MPU_REQUIRES_POWER_OF_TWO_ALIGNMENT in userspace.S.

Signed-off-by: Ioannis Glaropoulos [email protected]

@ioannisg
Copy link
Member Author

Yet another removal of the unwanted CONFIG_MPU_REQUIRES_POWER_OF_TWO_ALIGNMENT from arch/arm code. This seems to be actually a minor bug, eating up 32 bytes from the stack of a user thread.

@ioannisg
Copy link
Member Author

@andrewboie please, take a quick look; do you think we should back-port this one?

@ioannisg ioannisg added the area: ARM ARM (32-bit) Architecture label May 24, 2019
@ioannisg ioannisg requested a review from carlescufi May 24, 2019 19:38
ip register holds the stack_info.size (it is passed as argument
into z_arch_user_mode_enter(.)). We trust that the value of
stack_info.size contains the accurate size of the writable
stack buffer, above stack_info.start (as specified in kernel.h).
Therefore, we do not need to subtract any bytes for the MPU
stack guard. This allows us to clean-up one more occurrence of
CONFIG_MPU_REQUIRES_POWER_OF_TWO_ALIGNMENT in userspace.S.

Signed-off-by: Ioannis Glaropoulos <[email protected]>
@ioannisg ioannisg force-pushed the arch_arm_userspace_remove_power_of_two_alignment_req branch from f703e19 to 85691a3 Compare May 25, 2019 11:25
@wentongwu
Copy link
Contributor

Yet another removal of the unwanted CONFIG_MPU_REQUIRES_POWER_OF_TWO_ALIGNMENT from arch/arm code. This seems to be actually a minor bug, eating up 32 bytes from the stack of a user thread.

Yes, the mpu guard for thread stack has already been take care in z_new_thread when thread is setup.

@wentongwu
Copy link
Contributor

The stack_info is under CONFIG_THREAD_STACK_INFO, we can see this in function z_new_thread_init, but I see several places using stack_info directly without CONFIG_THREAD_STACK_INFO wrapped like function z_arch_user_mode_enter, do I miss something?
@ioannisg

@ioannisg
Copy link
Member Author

The stack_info is under CONFIG_THREAD_STACK_INFO, we can see this in function z_new_thread_init, but I see several places using stack_info directly without CONFIG_THREAD_STACK_INFO wrapped like function z_arch_user_mode_enter, do I miss something?
@ioannisg

@wentongwu For ARM, THREAD_STACK_INFO is selected under two cases

  • build with MPU support (ARM_MPU)
  • build with BUILTIN_STACK_GUARD support (ARMV8-M), not necessarily with MPU support.

If you see it being used outside these two cases, this is a (serious) bug.

@wentongwu
Copy link
Contributor

The stack_info is under CONFIG_THREAD_STACK_INFO, we can see this in function z_new_thread_init, but I see several places using stack_info directly without CONFIG_THREAD_STACK_INFO wrapped like function z_arch_user_mode_enter, do I miss something?
@ioannisg

@wentongwu For ARM, THREAD_STACK_INFO is selected under two cases

* build with MPU support (ARM_MPU)

* build with BUILTIN_STACK_GUARD support (ARMV8-M), not necessarily with MPU support.

If you see it being used outside these two cases, this is a (serious) bug.

OK, thanks for the info.

@ioannisg ioannisg added bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug labels May 27, 2019
@ioannisg ioannisg added this to the v2.0.0 milestone May 28, 2019
@ioannisg
Copy link
Member Author

ioannisg commented Jun 3, 2019

@andrewboie could you review this minor patch, too?

@andrewboie andrewboie merged commit b3114ef into zephyrproject-rtos:master Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ARM ARM (32-bit) Architecture bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants