fix two ARC issues and improve stack object checking #15132
Conversation
There was a problem hiding this comment.
Don't know what the best is, here. I would prefer : (start + size - 1) < r_addr_end
It's a style thing.
There was a problem hiding this comment.
you've lost me.
they are the same.
There was a problem hiding this comment.
I know, I said it is a style thing :) We usually have "something" -1 < "sthg else", not <=.
Just style, ignore at will.
There was a problem hiding this comment.
my suggestion would be to directly compare sizes and not calculate end addresses: no need to add the start addresses, less risk for 1-off mistakes.
ioannisg
left a comment
ioannisg
left a comment
There was a problem hiding this comment.
Looks ok
NACK - just until I test it in ARMv8-M
There was a problem hiding this comment.
Could we simply use the Z_SYSCALL_MEMORY_WRITE macros here, too? Isn't it effectively the same?
There was a problem hiding this comment.
what is the advantage of doing that
There was a problem hiding this comment.
Where does the name tls come from?
There was a problem hiding this comment.
Isn't this a more generic thing, i.e. getting the pointer to the userspace local data?
There was a problem hiding this comment.
it's not.
userspace local data is a poorly named implementation of thread-local storage.
There was a problem hiding this comment.
Right! So tls is thread-local storage? I confused it to TLS, actually, remembering you did some mbedTLS stuff few days ago.
Yes, I have a task for 1.15 to rename that, actually.
There was a problem hiding this comment.
Report interesting information ) :)
There was a problem hiding this comment.
Correct me if I am wrong, but I think this could be strickt equality, except for the case of ARMv7-M MPU. Right @andrewboie ?
Update: IMHO, for ARM builds, this should always be a strict equality check, if obj_size is the sizeof an object created by K_THREAD_STACK_DEFINE(obj, STEST_STACKSIZE), right? This is because the privileged stack for ARM is allocated elsewhere.
For ARC the obj_size is, trivially, larger than STEST_STACKSIZE, because we also allocate the privilege area in the same object, AFAIU. So this check has little value. Instead we should perhaps check whether the created stack object can hold a thread buffer of STEST_STACKSIZE excluding any priv stack size...
There was a problem hiding this comment.
I need to work with the objects as they are currently defined.
I don't want to further iterate on this until after 1.14 is out.
There was a problem hiding this comment.
Should we be more consistent on types for pos and val? E.g. char val and char *pos, or using u8_t...
There was a problem hiding this comment.
why is this change needed?
There was a problem hiding this comment.
Again, I suppose, we can't use a strict equality due to ARMv7-MPU, right?
There was a problem hiding this comment.
I know, it might not be of great value, and would make the test case not so easy to read, but would it make sense to transform some of the inequalities to strict equality (whenever possible), in order to get higher coverage level?
There was a problem hiding this comment.
can we do this some other time?
I have very little bandwidth to further enhance this test until after 1.14 is out
There was a problem hiding this comment.
Can we repeat the test using k_thread_create with THREAD_STACK_SIZEOF, to confirm that that is also working?
There was a problem hiding this comment.
can we do this some other time?
I have very little bandwidth to further enhance this test until after 1.14 is out
ioannisg
left a comment
ioannisg
left a comment
There was a problem hiding this comment.
The test as-is is passing on all ARM-based MPU architectures.
Left some feedback to be addressed.
|
@ioannisg it's not clear to me what items in your feedback are gating acceptance. I fixed the call to check both read and write permissions, and removed the TLS pointer syscall as I haven't written a test yet to use it. The other feedback seems to be either further enhancements you desire, or fall under the category of "six of one, half dozen of the other" |
Similar issue to what was fixed earlier in the MPUv3 code. start + size should be <= r_addr_end. Fixes a problem where the last byte of an MPU region is incorrectly reported as out-of-bounds. Fixes: zephyrproject-rtos#15131 Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
Unlike the others, this macro was not taking into account minimum MPU region sizes by filtering through STACK_SIZE_ALIGN(). Fixes: zephyrproject-rtos#15130 Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
The stack information stored in the thread->stack_info fields need to represent the actual writable area for its associated thread. Perform various tests to ensure that the various reported and specified values are in agreement. Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
andrewboie
force-pushed
the
arc-fixes-stacks-test
branch
from
1e1b6d8 to
c612522
Compare
ioannisg
left a comment
ioannisg
left a comment
There was a problem hiding this comment.
Good to go for ARM, thanks for the test - really helpful.
6 participants
Fixes: #15130
Fixes: #15131