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