[PATCH] Assembly suppport for context switch and bug fixes
Gedare Bloom
gedare at rtems.org
Tue Jun 30 04:55:24 UTC 2020
On Mon, Jun 29, 2020 at 9:06 PM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
>
>
> 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?
>
When you submit for merge, we'll want to have a series of patches that
each change ideally one thing without breaking any compilation.
Smaller is better, but compiling is necessary (and working is best!).
Smaller commits are easier to merge, while commits that don't break
builds make it possible to use tools like git-bisect without too much
difficulty.
>>
>> 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
More information about the devel
mailing list