[PATCH] Assembly suppport for context switch and bug fixes
Utkarsh Rai
utkarsh.rai60 at gmail.com
Tue Jun 30 03:06:25 UTC 2020
On Mon, Jun 29, 2020 at 11:00 PM Gedare Bloom <gedare at rtems.org> 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 utkarsh.rai60 at gmail.com
> <utkarsh.rai60 at gmail.com> wrote:
> >
> > From: Utkarsh Rai <utkarsh.rai60 at gmail.com>
> >
> > ---
> > 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
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200630/3a3131f0/attachment-0001.html>
More information about the devel
mailing list