Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARM: fix TLS pointer issue (1.14 branch) #25636

Merged
merged 2 commits into from
Jun 4, 2020

Conversation

andrewboie
Copy link
Contributor

arm: fix stack size accounting

On ARM Cortex-M, which enforces MPU power-of-two alignment,
the memory for stack guards is 'carved-out' of the stack buffer,
and not reserved permanently.

However, the Z_ARCH_THREAD_STACK_BUFFER and
Z_ARCH_THREAD_STACK_SIZEOF implementations were adding/subtracting
the guard size, which is incorrect for carved-out space. This can
only be done if the space is permanently reserved.

Fix this so that these APIs work correctly in the carve-out case.
Adjust thread->stack_info values on transition to user mode correctly.

See: #25635

Signed-off-by: Andrew Boie andrew.p.boie@intel.com

@andrewboie andrewboie added bug The issue is a bug, or the PR is fixing a bug backport v1.14-branch labels May 26, 2020
@andrewboie andrewboie requested a review from ioannisg May 26, 2020 18:40
@zephyrbot zephyrbot added area: ARM ARM (32-bit) Architecture area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Kernel labels May 26, 2020
@andrewboie
Copy link
Contributor Author

andrewboie commented May 26, 2020

This is fixed in master branch by #24714 but I wanted a more targeted fix to backport to 1.14.

This needs HW testing on frdm-k64f (or some other board with the custom NXP MPU)

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me you have changed the semantics of Z_ARCH_THREAD_STACK_BUFFER and Z_THREAD_STACK_SIZEOF

Currently:

  • Z_ARCH_THREAD_STACK_BUFFER points to the start of the available thread stack, i.e. after the MPU guard (if that one exists).
  • Z_ARCH_THREAD_STACK_SIZEOF is the size of the stack available for the thread, i.e. after subtracting the MPU guard (if that one exists).

To facilitate review, pls, explain what you intend to reflect with the macros:
Z_ARCH_THREAD_STACK_BUFFER and Z_ARCH_THREAD_STACK_SIZEOF, then I 'll take a second look.

arch/arm/core/thread.c Outdated Show resolved Hide resolved
arch/arm/core/thread.c Outdated Show resolved Hide resolved
@andrewboie
Copy link
Contributor Author

Z_ARCH_THREAD_STACK_BUFFER points to the start of the available thread stack, i.e. after the MPU guard (if that one exists).

This a the problem here. You cannot do this if the guard is "carved out" because you don't know whether the thread is running in user mode or not.

If the thread is running in user mode, there is no guard. The entire stack object is sized to a power of two and the entire stack object is available to the user.

ARCH_THREAD_STACK_BUFFER() can only work properly if it skips permanently reserved memory. Not a guard that may or may not be there depending on what mode the thread is running in. You can't have a constant definition for something that changes at runtime.

ARCH_THREAD_STACK_SIZEOF is the size of the stack available for the thread, i.e. after subtracting the MPU guard (if that one exists).

This was changed for the same reason as ARCH_THREAD_STACK_BUFFER for power-of-two systems: it should only take into account permanently reserved space. Not transient guard memory.

What you are thinking of is actually tracked in thread->stack_info.
thread->stack_info is supposed to accurately reflect the accessible part of the thread stack buffer, minus any memory currently used for reserved data or guard memory. It gets updated when threads change state, such as dropping to user node.

There were several inconsistencies in implementation and semantics for all of this between X86, ARC, and ARM. I have rewritten all of this in #24714 so that it finally is the same everywhere. I did not want to back port all of that to 1.14.

@andrewboie
Copy link
Contributor Author

@ioannisg I looked at your patch against master and it is less invasive, which is desirable for 1.14 -- this PR pulls in a bit too much stuff from #24714 to acheive its goals.

I'm going to backport your fix instead to touch the code as little as possible. Updated PR coming.

ioannisg and others added 2 commits May 27, 2020 10:48
The workaround for ARMv7-M architecture (which proactively
decreases the available thread stack by the size of the MPU
guard) needs to be placed before we calculate the pointer of
the user-space local thread data, otherwise this pointer will
fall beyond the boundary of the thread stack area.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
Ensure that the TLS region is within the stack object.

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the fix and the test.

I'd like to have a full run all -userspace tests for e.g. sam_e70_xplained before merging this to the LTS

@andrewboie
Copy link
Contributor Author

@ioannisg agreed. Would like some testing on frdm_k64f too since the NXP MPU isn't under emulation.

I have a board but have been having trouble flashing it. I am going to fiddle with it more this afternoon.

I also have a sam_e70 board, I'll give that a go too.

@andrewboie andrewboie added the block: HW Test Testing on hardware required before merging label May 27, 2020
@ioannisg
Copy link
Member

I can do all this for you @andrewboie. By the way, although I am fine with testing it on nxp mpu, I find it less important since this workaround does not apply to the nxp mph as far as I remember. Nxp mpu doesn't require power of 2 size alignment

@andrewboie
Copy link
Contributor Author

I can do all this for you @andrewboie. By the way, although I am fine with testing it on nxp mpu, I find it less important since this workaround does not apply to the nxp mph as far as I remember. Nxp mpu doesn't require power of 2 size alignment

Thanks appreciate it!
You're right, NXP should be unchanged with this PR.

@ioannisg
Copy link
Member

Testing on sam_e70_xplained and frdm_k64f:

  • tests/kernel
  • tests/lib
  • tests/benchmarks
  • tests/cmsis_rtos_v1, v2

tests are passing

@andrewboie andrewboie removed the block: HW Test Testing on hardware required before merging label May 27, 2020
@andrewboie andrewboie requested a review from carlescufi May 27, 2020 22:23
@nashif nashif merged commit 355ba2a into zephyrproject-rtos:v1.14-branch Jun 4, 2020
@andrewboie andrewboie deleted the arm-tls-fix-114 branch September 24, 2020 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: ARM ARM (32-bit) Architecture area: Kernel area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants