[PATCH] Assembly suppport for context switch and bug fixes
Gedare Bloom
gedare at rtems.org
Mon Jun 29 17:30:12 UTC 2020
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. 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. 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