<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 29, 2020 at 11:00 PM Gedare Bloom <<a href="mailto:gedare@rtems.org">gedare@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">high-level comment: this patch is not complete wrt current RTEMS<br>
master branch. This makes a complete review impossible. I'm not even<br>
sure what I'm supposed to be reviewing here. </blockquote><div><br></div><div>Sorry, I was undecided between sending all of my changes in multiple patches or squashing the last few which had the context-switching mechanism that I wanted to get reviewed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I did my best to provide<br>
you with some guidance, but if you want something specific<br>
reviewed/evaluated, please make a clean patch that demonstrates your<br>
idea for review.</blockquote><div><br></div><div>Should I squash all of my commits and send it as a single patch (along with the changes suggested in this one) or break it into multiple patches?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> More below:<br>
<br>
On Mon, Jun 29, 2020 at 8:48 AM <a href="mailto:utkarsh.rai60@gmail.com" target="_blank">utkarsh.rai60@gmail.com</a><br>
<<a href="mailto:utkarsh.rai60@gmail.com" target="_blank">utkarsh.rai60@gmail.com</a>> wrote:<br>
><br>
> From: Utkarsh Rai <<a href="mailto:utkarsh.rai60@gmail.com" target="_blank">utkarsh.rai60@gmail.com</a>><br>
><br>
> ---<br>
> cpukit/score/cpu/arm/cpu.c | 3 +-<br>
> cpukit/score/cpu/arm/cpu_asm.S | 14 ++-<br>
> .../score/cpu/arm/include/rtems/score/cpu.h | 12 ++-<br>
> cpukit/score/src/stackmanagement.c | 98 +++++++++++--------<br>
> cpukit/score/src/threadloadenv.c | 11 +++<br>
> 5 files changed, 92 insertions(+), 46 deletions(-)<br>
><br>
> diff --git a/cpukit/score/cpu/arm/cpu.c b/cpukit/score/cpu/arm/cpu.c<br>
> index 5ee1acede2..39b5a33c03 100644<br>
> --- a/cpukit/score/cpu/arm/cpu.c<br>
> +++ b/cpukit/score/cpu/arm/cpu.c<br>
> @@ -97,8 +97,9 @@ void _CPU_Context_Initialize(<br>
> )<br>
> {<br>
> (void) new_level;<br>
> -<br>
Don't remove/add blank lines in existing files without good reason. I<br>
don't know if it is codified, but we usually have a blank line after<br>
variable decls.<br>
<br>
> + #if defined (USE_THREAD_STACK_PROTECTION)<br>
> the_context->stack_attr = prot_stack_context_initialize();<br>
This appears to be a line that was added prior? Please check out how<br>
to "squash" multiple patches together to a single patch. That will<br>
help with review. I like to use the "git rebase -i" interactive rebase<br>
mode for patch management.<br>
<br>
An "attribute" is normally a property of something. So is this storing<br>
some (set of) properties for the thread's stack?<br>
<br>
We prefer to avoid abbreviations in function names when possible. We<br>
also need to be aware of namespace issues. this "prot_" should belong<br>
to some namespace that is cleanly separated from applications.<br>
<br>
> + #endif<br>
> the_context->register_sp = (uint32_t) stack_area_begin + stack_area_size;<br>
> the_context->register_lr = (uint32_t) entry_point;<br>
> the_context->isr_dispatch_disable = 0;<br>
> diff --git a/cpukit/score/cpu/arm/cpu_asm.S b/cpukit/score/cpu/arm/cpu_asm.S<br>
> index e7cdd24c2f..2678589483 100644<br>
> --- a/cpukit/score/cpu/arm/cpu_asm.S<br>
> +++ b/cpukit/score/cpu/arm/cpu_asm.S<br>
> @@ -65,9 +65,13 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)<br>
> #endif<br>
><br>
> str r3, [r0, #ARM_CONTEXT_CONTROL_ISR_DISPATCH_DISABLE]<br>
> - str r0, [r0, #44]<br>
why this is removed? what is it from? this line is something from a<br>
prior patch you have not sent?<br>
<br>
> +#if defined ( USE_THREAD_STACK_PROTECTION )<br>
> + mov r2, r0<br>
> + ldr r0, [r0, #ARM_STACK_PROT_ATTR_OFFSET]<br>
check number of white spaces in alignment here. why it is indented<br>
more than surrounding lines?<br>
<br>
Should this macro should be consistent with similar macros? Like<br>
ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET<br>
<br>
> bl prot_stack_context_switch<br>
again, this is from some prior patch that you haven't sent, so I can't<br>
review it here.<br>
<br>
> -<br>
> + mov r0, r2<br>
> +#endif<br>
> +<br>
> #ifdef RTEMS_SMP<br>
> /*<br>
> * The executing thread no longer executes on this processor. Switch<br>
> @@ -135,6 +139,12 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)<br>
> */<br>
> DEFINE_FUNCTION_ARM(_CPU_Context_restore)<br>
> mov r1, r0<br>
> +#if defined( USE_THREAD_STACK_PROTECTION )<br>
> + ldr r2, [lr]<br>
> + ldr r0, [r0, #ARM_STACK_PROT_ATTR_OFFSET]<br>
indent?<br>
<br>
> + bl prot_stack_context_restore<br>
> + ldr lr, [r2]<br>
> +#endif<br>
> GET_SELF_CPU_CONTROL r2<br>
> b .L_restore<br>
><br>
> diff --git a/cpukit/score/cpu/arm/include/rtems/score/cpu.h b/cpukit/score/cpu/arm/include/rtems/score/cpu.h<br>
> index 481d50f427..f0af63e532 100644<br>
> --- a/cpukit/score/cpu/arm/include/rtems/score/cpu.h<br>
> +++ b/cpukit/score/cpu/arm/include/rtems/score/cpu.h<br>
> @@ -160,6 +160,16 @@<br>
> #define ARM_CONTEXT_CONTROL_THREAD_ID_OFFSET 44<br>
> #endif<br>
><br>
> +#ifdef USE_THREAD_STACK_PROTECTION<br>
> + #if defined ARM_MULITLIB_VFP<br>
> + #define ARM_STACK_PROT_ATTR_OFFSET 112<br>
> + #elif ARM_MULTILIB_HAS_THREAD_ID_REGISTER<br>
> + #define ARM_STACK_PROT_ATTR_OFFSET 48<br>
> + #else<br>
> + #define ARM_STACK_PROT_ATTR_OFFSET 44<br>
fix macro name<br>
<br>
> + #endif<br>
> +#endif<br>
> +<br>
> #ifdef ARM_MULTILIB_VFP<br>
> #define ARM_CONTEXT_CONTROL_D8_OFFSET 48<br>
> #endif<br>
> @@ -184,8 +194,6 @@<br>
><br>
> #define ARM_EXCEPTION_FRAME_VFP_CONTEXT_OFFSET 72<br>
><br>
> -#define ARM_STACK_PROT_ATTR_OFFSET 44<br>
> -<br>
> #define ARM_VFP_CONTEXT_SIZE 264<br>
><br>
><br>
> diff --git a/cpukit/score/src/stackmanagement.c b/cpukit/score/src/stackmanagement.c<br>
> index 5ed8242c50..eff1b59add 100644<br>
> --- a/cpukit/score/src/stackmanagement.c<br>
> +++ b/cpukit/score/src/stackmanagement.c<br>
> @@ -1,7 +1,8 @@<br>
> #include <rtems/score/stackmanagement.h><br>
> #include <rtems/score/chainimpl.h><br>
> +#include <rtems/score/basedefs.h><br>
><br>
> -Chain_Control *node_control;<br>
> +Chain_Control prot_node_control = CHAIN_INITIALIZER_EMPTY(prot_node_control);<br>
Use CHAIN_DEFINE_EMPTY?<br>
<br>
><br>
> Chain_Control *shared_node_control;<br>
><br>
> @@ -20,71 +21,82 @@ static void shared_stack_entry_remove(stack_attr_shared *shared_stack)<br>
> memory_entries_unset(shared_stack->Base.stack_address, shared_stack->Base.size);<br>
> node = node->next;<br>
> }<br>
> -<br>
> }<br>
> }<br>
><br>
> -static void prot_stack_prev_entry_remove(stack_attr_prot *stack_attr)<br>
> +static void prot_stack_prev_entry_remove(void)<br>
Please document what this function is supposed to be doing. It's not<br>
clear to me just by inspection.<br>
<br>
> {<br>
> -<br>
> +<br>
> Chain_Node *node;<br>
> -/*<br>
> - if(node_control == NULL) {<br>
> - _Chain_Initialize_empty(node_control);<br>
> + stack_attr_prot *stack_attr;<br>
> +<br>
> + if( _Chain_Is_empty(&prot_node_control) == true ) {<br>
fix your whitespaces<br>
e.g., if ( _Chain_Is_empty( &prot_node_control ) == true ) {<br>
<br>
> + _Chain_Initialize_empty(&prot_node_control);<br>
ditto<br>
<br>
> }<br>
> - */<br>
> - while(!_Chain_Is_tail(node_control, node)) {<br>
> - stack_attr = (stack_attr_prot*) node;<br>
> + node = _Chain_First( &prot_node_control );<br>
> +<br>
> + while(_Chain_Is_tail(&prot_node_control, node) == false) {<br>
><br>
> - if( stack_attr->current_stack == false ) {<br>
> + stack_attr = RTEMS_CONTAINER_OF(node,stack_attr_prot, Base.node);<br>
> +<br>
> + if( stack_attr->current_stack == false && _Chain_Is_head(&prot_node_control, node) == false ) {<br>
> memory_entries_unset(stack_attr->Base.stack_address, stack_attr->Base.size);<br>
> - shared_stack_entry_remove(stack_attr->shared_stacks);<br>
> - node = node->next;<br>
> + // shared_stack_entry_remove(stack_attr->shared_stacks);<br>
It is easier to review a squashed patch, I don't need to see these<br>
kind of interim changes of your work.<br>
<br>
> +<br>
> }<br>
> + node = _Chain_Immutable_next( node );<br>
> }<br>
> -<br>
> -<br>
> +<br>
> + return ;<br>
not needed<br>
<br>
> }<br>
><br>
> -static void prot_stack_chain_append (Chain_Control *control, stack_attr_prot *stack_attr) //maybe we don't need the chain control parameter to be passed<br>
> +static void prot_stack_chain_append (Chain_Control *control, stack_attr_prot *stack_append_attr) //maybe we don't need the chain control parameter to be passed<br>
fix line length, put function doco prior to the function decl, and use<br>
/* */ comments<br>
<br>
<a href="https://docs.rtems.org/branches/master/eng/coding-conventions.html#source-documentation" rel="noreferrer" target="_blank">https://docs.rtems.org/branches/master/eng/coding-conventions.html#source-documentation</a><br>
<br>
> {<br>
> Chain_Node *node;<br>
> + stack_attr_prot *present_stacks_attr;<br>
> +<br>
> + if(_Chain_Is_empty(&prot_node_control) == true ) {<br>
><br>
> - if( control == NULL ) {<br>
> - _Chain_Initialize_one(control, &stack_attr->Base.node);<br>
> + _Chain_Initialize_one(&prot_node_control, &stack_append_attr->Base.node);<br>
> } else {<br>
> - node = _Chain_Head(control);<br>
> + node = _Chain_First(&prot_node_control);<br>
><br>
> /*<br>
> This is done to ensure that we mark all the remaining<br>
> entries as not-current so that they can be removed.<br>
> */<br>
> - while(!_Chain_Is_tail(control,node)) {<br>
> - stack_attr = (stack_attr_prot*) node;<br>
> - stack_attr->current_stack = false;<br>
> - node = node->next;<br>
> + while(_Chain_Is_tail(&prot_node_control,node) == false) {<br>
> +<br>
> + present_stacks_attr = RTEMS_CONTAINER_OF(node, stack_attr_prot, Base.node);<br>
> + present_stacks_attr->current_stack = false;<br>
> + node = _Chain_Immutable_next( node );<br>
> }<br>
> - _Chain_Append_unprotected(control, &stack_attr->Base.node);<br>
> + _Chain_Append_unprotected(&prot_node_control, &stack_append_attr->Base.node);<br>
> }<br>
> -<br>
> + return ;<br>
> }<br>
Does this function do anything more than just append a node on a<br>
chain? Why you can't just call the _Chain_Append function?<br>
<br>
Maybe there needs to be a helper function to set all 'current_stack'<br>
fields to false in the prot stack chain?<br>
<br>
><br>
> void prot_stack_allocate(uint32_t *stack_address, size_t size, uint32_t *page_table_base)<br>
> {<br>
> stack_attr_prot *stack_attr;<br>
><br>
> - stack_attr = malloc(sizeof(stack_attr_prot));<br>
> + // Have a condition for the case when the same stack is allocated twice, do not allocate a new node, will cause memory leaks.<br>
use /* */ comments with multi-line formatting<br>
<br>
><br>
> + stack_attr = malloc(sizeof(stack_attr_prot));<br>
malloc is not allowed here. need to configure the workspace to<br>
allocate these dynamically if dynamic memory is required. Why isn't<br>
this field allocated with the TCB or stack itself?<br>
<br>
> +<br>
> + if(stack_attr != NULL) {<br>
> stack_attr->Base.stack_address = stack_address;<br>
> stack_attr->Base.size = size;<br>
are these redundant to other stack metadata stored elsewhere in the TCB?<br>
<br>
> stack_attr->Base.page_table_base = page_table_base;<br>
> stack_attr->Base.access_flags = READ_WRITE_CACHED;<br>
> stack_attr->current_stack = true;<br>
> -<br>
> - prot_stack_chain_append( node_control, stack_attr ); // Add the stack attr. at the end of the chain<br>
> - prot_stack_prev_entry_remove( stack_attr ); // Remove the previous stack entry<br>
> + }<br>
> + prot_stack_chain_append(&prot_node_control, stack_attr ); // Add the stack attr. at the end of the chain<br>
> + prot_stack_prev_entry_remove(); // Remove the previous stack entry<br>
> +<br>
> + memory_entries_set(stack_address, size, READ_WRITE_CACHED);<br>
<br>
I could use with a /* */ block prior to these 3 function calls that<br>
explains what they are accomplishing.<br>
<br>
><br>
> - memory_entries_set(stack_address, size, READ_WRITE);<br>
> + return;<br>
remove empty return, we don't usually put those, unless it is an early<br>
return to get out of the function prior to fall-through the end of<br>
scope. Maybe this needs to be added to coding conventions, probably<br>
<a href="https://docs.rtems.org/branches/master/eng/coding-conventions.html#readability" rel="noreferrer" target="_blank">https://docs.rtems.org/branches/master/eng/coding-conventions.html#readability</a><br>
? perhaps you can add it and send a patch for review.<br>
<br>
><br>
> }<br>
><br>
> @@ -93,16 +105,16 @@ stack_attr_prot *prot_stack_context_initialize(void)<br>
> Chain_Node *node;<br>
> stack_attr_prot *stack_attr;<br>
><br>
> - if( node_control != NULL && _Chain_Is_empty(node_control) == false ) {<br>
> - node = _Chain_Head( node_control );<br>
> + if( _Chain_Is_empty(&prot_node_control) == false ) {<br>
> + node = _Chain_First( &prot_node_control );<br>
><br>
> - while( _Chain_Is_tail( node_control, node ) == false) {<br>
> - stack_attr = (stack_attr_prot*) node;<br>
> + while( _Chain_Is_tail(&prot_node_control, node ) == false) {<br>
> + stack_attr = RTEMS_CONTAINER_OF( node, stack_attr_prot, Base.node);<br>
><br>
> if(stack_attr->current_stack == true) {<br>
> return stack_attr;<br>
> } else {<br>
> - node = node->next;<br>
> + node = _Chain_Immutable_next( node );<br>
> }<br>
> }<br>
> }<br>
<br>
Instead of always iterating through the structure to find the<br>
current_stack, maybe you can store a pointer to it somewhere<br>
convenient?<br>
<br>
> @@ -132,8 +144,10 @@ void prot_stack_context_switch(stack_attr_prot *stack_attr)<br>
><br>
> shared_node_control = &stack_attr->shared_stacks->shared_node_control;<br>
> }<br>
> +/*<br>
> +<br>
> + The shared node control structure will be initialized during stack sharing<br>
><br>
> -<br>
> if( shared_node_control != NULL && _Chain_Is_empty( shared_node_control ) == false) {<br>
> node = _Chain_Head(shared_node_control);<br>
><br>
> @@ -147,7 +161,7 @@ void prot_stack_context_switch(stack_attr_prot *stack_attr)<br>
> node = node->next;<br>
> }<br>
> }<br>
> -<br>
> +*/<br>
don't do this, it is fragile at best, and since we use /* */ comments<br>
it will lead you to bad development practices (like not using comments<br>
so you can comment out code).<br>
<br>
If you want, use<br>
#if 0<br>
<br>
<br>
#endif<br>
<br>
to disable some chunk of code. But don't send patches with that either.<br>
<br>
> }<br>
><br>
> void prot_stack_context_restore(stack_attr_prot *stack_attr)<br>
> @@ -162,16 +176,18 @@ void prot_stack_context_restore(stack_attr_prot *stack_attr)<br>
> */<br>
> if(stack_attr != NULL){<br>
><br>
> - stack_attr->current_stack = true;<br>
> + /* stack_attr->current_stack = true;<br>
> stack_address = stack_attr->Base.stack_address;<br>
> size = stack_attr->Base.size;<br>
> -<br>
> + */<br>
ditto<br>
<br>
> if(stack_attr->current_stack == true) {<br>
> memory_entries_set(stack_address, size, READ_WRITE_CACHED);<br>
> }<br>
> + return ;<br>
><br>
> - shared_node_control = &stack_attr->shared_stacks->shared_node_control;<br>
> + // shared_node_control = &stack_attr->shared_stacks->shared_node_control;<br>
> }<br>
> +/* The shared node control structure will be initialized during stack sharing<br>
><br>
> if( shared_node_control !=NULL && _Chain_Is_empty( shared_node_control ) == false ) {<br>
> node = _Chain_Head( shared_node_control );<br>
> @@ -188,7 +204,7 @@ void prot_stack_context_restore(stack_attr_prot *stack_attr)<br>
> }<br>
> }<br>
> // Possible bug<br>
?<br>
<br>
> -<br>
> + */<br>
> }<br>
><br>
> /*<br>
> diff --git a/cpukit/score/src/threadloadenv.c b/cpukit/score/src/threadloadenv.c<br>
> index 9b0808cbf8..44ff5c3a28 100644<br>
> --- a/cpukit/score/src/threadloadenv.c<br>
> +++ b/cpukit/score/src/threadloadenv.c<br>
> @@ -21,6 +21,10 @@<br>
><br>
> #include <rtems/score/threadimpl.h><br>
><br>
> +//#if defined( THREAD_STACK_PROTECTION )<br>
> +#include <rtems/score/stackmanagement.h><br>
> +//#endif<br>
> +<br>
> void _Thread_Load_environment(<br>
> Thread_Control *the_thread<br>
> )<br>
> @@ -35,6 +39,13 @@ void _Thread_Load_environment(<br>
> the_thread->is_preemptible = the_thread->Start.is_preemptible;<br>
> the_thread->budget_algorithm = the_thread->Start.budget_algorithm;<br>
> the_thread->budget_callout = the_thread->Start.budget_callout;<br>
> +<br>
> + // #if defined ( USE_THREAD_STACK_PROTECTION )<br>
> + /*prot_stack_allocate(<br>
> + the_thread->Start.Initial_stack.area,<br>
> + the_thread->Start.Initial_stack.size,<br>
> + (uint32_t *) 0x1000);*/<br>
> + // #endif<br>
><br>
> _Context_Initialize(<br>
> &the_thread->Registers,<br>
> --<br>
> 2.17.1<br>
><br>
> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</blockquote></div></div>