[PATCH] Strict thread-stack isolation
Gedare Bloom
gedare at rtems.org
Wed Jul 15 04:00:52 UTC 2020
On Tue, Jul 14, 2020 at 10:59 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
>
>
> On Tue, Jul 14, 2020 at 7:36 PM Gedare Bloom <gedare at rtems.org> wrote:
>>
>> I won't comment on the namespace and coding conventions, other than to
>> say you should focus on doing them correctly as you code, rather than
>> going back and fixing them later. More below.
>>
>> On Mon, Jul 13, 2020 at 10:34 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>> >
>> > - This is the complete set of changes for strict isolation of thread stacks.
>> > - There needs to be a confiuration operation,(#if defined(USE_THREAD_STACK_PROTECTION) for simple configuration can be used)
>> > - The stack attributes are allocated through malloc, this needs to be done through score unlimited objects.
>> > ---
>> > bsps/arm/headers.am | 1 +
>> > .../include/bsp/arm-cp15-set-ttb-entries.h | 7 +
>> > .../shared/cp15/arm-cp15-set-ttb-entries.c | 3 +
>> > bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c | 72 +++++++++
>> > bsps/shared/start/stackalloc.c | 20 ++-
>> > c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am | 5 +-
>> > cpukit/Makefile.am | 1 +
>> > cpukit/headers.am | 2 +
>> > cpukit/include/rtems/score/memorymanagement.h | 22 +++
>> > cpukit/include/rtems/score/stackmanagement.h | 49 ++++++
>> > cpukit/score/cpu/arm/cpu.c | 3 +
>> > cpukit/score/cpu/arm/cpu_asm.S | 22 ++-
>> > .../score/cpu/arm/include/rtems/score/cpu.h | 20 +++
>> > cpukit/score/src/stackmanagement.c | 143 ++++++++++++++++++
>> > 14 files changed, 365 insertions(+), 5 deletions(-)
>> > create mode 100644 bsps/arm/include/bsp/arm-cp15-set-ttb-entries.h
>> > create mode 100644 bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c
>> > create mode 100644 cpukit/include/rtems/score/memorymanagement.h
>> > create mode 100644 cpukit/include/rtems/score/stackmanagement.h
>> > create mode 100644 cpukit/score/src/stackmanagement.c
>> >
>> > diff --git a/bsps/arm/headers.am b/bsps/arm/headers.am
>> > index 3d2b09effa..b1e86f3385 100644
>> > --- a/bsps/arm/headers.am
>> > +++ b/bsps/arm/headers.am
>> > @@ -15,6 +15,7 @@ include_bsp_HEADERS += ../../../../../bsps/arm/include/bsp/arm-a9mpcore-clock.h
>> > include_bsp_HEADERS += ../../../../../bsps/arm/include/bsp/arm-a9mpcore-irq.h
>> > include_bsp_HEADERS += ../../../../../bsps/arm/include/bsp/arm-a9mpcore-regs.h
>> > include_bsp_HEADERS += ../../../../../bsps/arm/include/bsp/arm-a9mpcore-start.h
>> > +include_bsp_HEADERS += ../../../../../bsps/arm/include/bsp/arm-cp15-set-ttb-entries.h
>> > include_bsp_HEADERS += ../../../../../bsps/arm/include/bsp/arm-cp15-start.h
>> > include_bsp_HEADERS += ../../../../../bsps/arm/include/bsp/arm-errata.h
>> > include_bsp_HEADERS += ../../../../../bsps/arm/include/bsp/arm-gic-irq.h
>> > diff --git a/bsps/arm/include/bsp/arm-cp15-set-ttb-entries.h b/bsps/arm/include/bsp/arm-cp15-set-ttb-entries.h
>> > new file mode 100644
>> > index 0000000000..39170927da
>> > --- /dev/null
>> > +++ b/bsps/arm/include/bsp/arm-cp15-set-ttb-entries.h
>> > @@ -0,0 +1,7 @@
>> > +#include<bsp/arm-cp15-start.h>
>> > +
>> > +uint32_t arm_cp15_set_translation_table_entries(
>> > + const void *begin,
>> > + const void *end,
>> > + uint32_t section_flags
>> > +);
>> > \ No newline at end of file
>> > diff --git a/bsps/arm/shared/cp15/arm-cp15-set-ttb-entries.c b/bsps/arm/shared/cp15/arm-cp15-set-ttb-entries.c
>> > index 507277dca1..f5d0494167 100644
>> > --- a/bsps/arm/shared/cp15/arm-cp15-set-ttb-entries.c
>> > +++ b/bsps/arm/shared/cp15/arm-cp15-set-ttb-entries.c
>> > @@ -14,6 +14,7 @@
>> >
>> > #include <rtems.h>
>> > #include <libcpu/arm-cp15.h>
>> > +#include <bsp/arm-cp15-set-ttb-entries.h>
>> > #include <bspopts.h>
>> >
>> > /*
>> > @@ -30,6 +31,8 @@
>> > * ARM DDI 0406C.b (ID072512)
>> > */
>> >
>> > +#define ARM_MMU_USE_SMALL_PAGES
>> > +
>> > static uint32_t set_translation_table_entries(
>> > const void *begin,
>> > const void *end,
>> > diff --git a/bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c b/bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c
>> > new file mode 100644
>> > index 0000000000..978e35b86c
>> > --- /dev/null
>> > +++ b/bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c
>> > @@ -0,0 +1,72 @@
>> > +#include <bsp/arm-cp15-start.h>
>> > +#include <bsp/arm-cp15-set-ttb-entries.h>
>> > +#include <rtems/score/memorymanagement.h>
>> > +#include <libcpu/arm-cp15.h>
>> > +#include <rtems.h>
>> > +
>> > +#ifdef USE_THREAD_STACK_PROTECTION
>> > + #define ARM_MMU_USE_SMALL_PAGES
>> > +#endif
>> > +
>> > +void memory_entries_set(uintptr_t begin, size_t size, memory_flags flags)
>> > +{
>> > +
>> > + uintptr_t end;
>> > + rtems_interrupt_level irq_level;
>> > + uint32_t access_flags;
>> > +
>> > + end = begin + size;
>> > + access_flags = memory_translate_flags(flags);
>> > +
>> > + rtems_interrupt_local_disable(irq_level);
>>
>> is a local isr critical section sufficient to protect changing the ttb?
>
>
> On close inspection of the reference manual, I realize the mistake. The ARM reference manual says that we have to disable all the interrupts, so I guess rtems_interrupt_disable() would be better.
>
Honestly, I don't know the answer. Are the TTEs core-local or shared
in multicore scenarios?
>>
>>
>> > + arm_cp15_set_translation_table_entries(begin, end, access_flags);
>> > + rtems_interrupt_local_enable(irq_level);
>> > +}
>> > +
>> > +void memory_entries_unset(uintptr_t begin, size_t size)
>> > +{
>> > + uint32_t access_flags;
>> > + uintptr_t end;
>> > + rtems_interrupt_level irq_level;
>> > +
>> > + end = begin + size;
>> > + access_flags = memory_translate_flags(NO_ACCESS);
>> > +
>> > + rtems_interrupt_local_disable(irq_level);
>> > + arm_cp15_set_translation_table_entries(begin, end, access_flags);
>> > + rtems_interrupt_local_enable(irq_level);
>> > +}
>> > +
>> > +
>> > +uint32_t memory_translate_flags(memory_flags attr_flags)
>>
>> This helper function could be static (and at the top of the file).
>>
>> > +{
>> > + uint32_t flags;
>> > + switch (attr_flags)
>> > + {
>> > + case READ_WRITE:
>> > + flags = ARMV7_MMU_READ_WRITE;
>> > + break;
>> > +
>> > + case READ_WRITE_CACHED:
>> > + flags = ARMV7_MMU_DATA_READ_WRITE_CACHED;
>> > + break;
>> > +
>> > + case READ_ONLY:
>> > + flags = ARMV7_MMU_READ_ONLY;
>> > + break;
>> > +
>> > + case READ_ONLY_CACHED:
>> > + flags = ARMV7_MMU_READ_ONLY_CACHED;
>> > + break;
>> > +
>> > + case NO_ACCESS:
>> > + flags = 0;
>> > + break;
>> > +
>> > + default:
>> > + return -1;
>>
>> It's not great to return a negative value in an unsigned return.
>> Especially considering you're not checking them.
>>
>> The "safe default" to use when modifying protection state is default-deny.
>>
>> > + break;
>> > + }
>> > +
>> > + return flags;
>> > +}
>> > \ No newline at end of file
>> > diff --git a/bsps/shared/start/stackalloc.c b/bsps/shared/start/stackalloc.c
>> > index f7cf7be0f1..2ce0d6573b 100644
>> > --- a/bsps/shared/start/stackalloc.c
>> > +++ b/bsps/shared/start/stackalloc.c
>> > @@ -25,6 +25,7 @@
>> > #include <rtems.h>
>> > #include <rtems/score/heapimpl.h>
>> > #include <rtems/score/wkspace.h>
>> > +#include <rtems/score/stackmanagement.h>
>> >
>> > #include <bsp/linker-symbols.h>
>> >
>> > @@ -42,7 +43,8 @@ void bsp_stack_allocate_init(size_t stack_space_size)
>> >
>> > void *bsp_stack_allocate(size_t size)
>> > {
>> > - void *stack = NULL;
>> > + void *stack = NULL;
>> > + uintptr_t page_table_base;
>> These should be two spaces indented
>>
>> >
>> > if (bsp_stack_heap.area_begin != 0) {
>> > stack = _Heap_Allocate(&bsp_stack_heap, size);
>> > @@ -51,11 +53,23 @@ void *bsp_stack_allocate(size_t size)
>> > if (stack == NULL) {
>> > stack = _Workspace_Allocate(size);
>> > }
>> > -
>> > +
>> Not sure why the above two lines are removing and adding seeming the
>> same thing. Watch out for this.
>>
>> > +#ifdef USE_THREAD_STACK_PROTECTION
>> > + /*
>> > + This is a hard coded address, assigned just for the
>> > + purpose of consistency
>> > + */
>> Not sure what "consistency" means here. the comment is unclear.
>>
>> > + page_table_base = (uintptr_t)0x1000;
>> Avoid adding extra whitespaces at the end of the line
>>
>> > + /*
>> > + The current way to allocate protected stack is to assign memory attributes
>> > + to the allocated memory and remove memory-entry of every other stack
>> > + */
>> > + prot_stack_allocate( (uintptr_t)stack, size, page_table_base );
>> if you're not actually allocating something, you may not want to call
>> it "allocate" -- think about what it does when you go through your
>> renaming
>>
>> > +#endif
>> > return stack;
>> > }
>> >
>> > -void bsp_stack_free(void *stack)
>> > +void bsp_stack_free(void *stack)
>> avoid (spurious) changes in existing code
>>
>> > {
>> > bool ok = _Heap_Free(&bsp_stack_heap, stack);
>> >
>> > diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am b/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
>> > index cfd59475c2..490f99792e 100644
>> > --- a/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
>> > +++ b/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
>> > @@ -74,6 +74,9 @@ librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/i2c/cadence-i2c.
>> > # Cache
>> > librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/shared/cache/cache-l2c-310.c
>> >
>> > +#MMU
>> > +librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c
>> > +
>> > # Start hooks
>> > librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/start/bspstarthooks.c
>> > librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/start/bspstartmmu.c
>> > @@ -85,4 +88,4 @@ librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/start/bspstartmm
>> >
>> > include $(srcdir)/../../../../../../bsps/shared/irq-sources.am
>> > include $(srcdir)/../../../../../../bsps/shared/shared-sources.am
>> > -include $(srcdir)/../../../../../../bsps/arm/xilinx-zynq/headers.am
>> > +include $(srcdir)/../../../../../../bsps/arm/xilinx-zynq/headers.am
>> > \ No newline at end of file
>> > diff --git a/cpukit/Makefile.am b/cpukit/Makefile.am
>> > index 51f38c84c7..f3b373376b 100644
>> > --- a/cpukit/Makefile.am
>> > +++ b/cpukit/Makefile.am
>> > @@ -929,6 +929,7 @@ librtemscpu_a_SOURCES += score/src/schedulercbsgetserverid.c
>> > librtemscpu_a_SOURCES += score/src/schedulercbssetparameters.c
>> > librtemscpu_a_SOURCES += score/src/schedulercbsreleasejob.c
>> > librtemscpu_a_SOURCES += score/src/schedulercbsunblock.c
>> > +librtemscpu_a_SOURCES += score/src/stackmanagement.c
>> > librtemscpu_a_SOURCES += score/src/stackallocator.c
>> > librtemscpu_a_SOURCES += score/src/pheapallocate.c
>> > librtemscpu_a_SOURCES += score/src/pheapextend.c
>> > diff --git a/cpukit/headers.am b/cpukit/headers.am
>> > index fcf679f09d..9388aa0c8c 100644
>> > --- a/cpukit/headers.am
>> > +++ b/cpukit/headers.am
>> > @@ -352,6 +352,7 @@ include_rtems_score_HEADERS += include/rtems/score/isr.h
>> > include_rtems_score_HEADERS += include/rtems/score/isrlevel.h
>> > include_rtems_score_HEADERS += include/rtems/score/isrlock.h
>> > include_rtems_score_HEADERS += include/rtems/score/memory.h
>> > +include_rtems_score_HEADERS += include/rtems/score/memorymanagement.h
>> > include_rtems_score_HEADERS += include/rtems/score/mpci.h
>> > include_rtems_score_HEADERS += include/rtems/score/mpciimpl.h
>> > include_rtems_score_HEADERS += include/rtems/score/mppkt.h
>> > @@ -405,6 +406,7 @@ include_rtems_score_HEADERS += include/rtems/score/smplockstats.h
>> > include_rtems_score_HEADERS += include/rtems/score/smplockticket.h
>> > include_rtems_score_HEADERS += include/rtems/score/stack.h
>> > include_rtems_score_HEADERS += include/rtems/score/stackimpl.h
>> > +include_rtems_score_HEADERS += include/rtems/score/stackmanagement.h
>> > include_rtems_score_HEADERS += include/rtems/score/states.h
>> > include_rtems_score_HEADERS += include/rtems/score/statesimpl.h
>> > include_rtems_score_HEADERS += include/rtems/score/status.h
>> > diff --git a/cpukit/include/rtems/score/memorymanagement.h b/cpukit/include/rtems/score/memorymanagement.h
>> > new file mode 100644
>> > index 0000000000..c712e7301a
>> > --- /dev/null
>> > +++ b/cpukit/include/rtems/score/memorymanagement.h
>> > @@ -0,0 +1,22 @@
>> > +#ifndef _RTEMS_SCORE_MEMORYMANAGEMENT_H
>> > +#define _RTEMS_SCORE_MEMORYMANAGEMENT_H
>> > +
>> > +#include <stdlib.h>
>> What do you need this header for?
>>
>> > +#include <stdint.h>
>> This should get picked up if you include basedefs like other score headers do.
>>
>> > +
>> > +typedef enum
>> > +{
>> > + READ_WRITE,
>> > + READ_WRITE_CACHED,
>> > + READ_ONLY,
>> > + READ_ONLY_CACHED,
>> > + NO_ACCESS
>> > +} memory_flags;
>> > +
>>
>> Need documentation. Look at other score headers to guide you.
>>
>
> The memory_entries_* need to be implemented for each BSP to expose the low-level MMU implementation of that BSP. Is score the appropriate location for this API?
>
Take guidance maybe from the cache manager.
Even so, you should document this stuff.
>>
>> > +void memory_entries_set(uintptr_t begin_addr, size_t size, memory_flags flags);
>> > +
>> > +void memory_entries_unset(uintptr_t begin_addr, size_t size);
>> > +
>> > +uint32_t memory_translate_flags(memory_flags flags);
>>
>> Does this need to be part of the public-facing interface?
>>
>>
>> > +
>> > +#endif
>> > \ No newline at end of file
>> > diff --git a/cpukit/include/rtems/score/stackmanagement.h b/cpukit/include/rtems/score/stackmanagement.h
>> > new file mode 100644
>> > index 0000000000..53b6a23af0
>> > --- /dev/null
>> > +++ b/cpukit/include/rtems/score/stackmanagement.h
>> > @@ -0,0 +1,49 @@
>> > +#ifndef _RTEMS_SCORE_STACKMANAGEMENT_H
>> > +#define _RTEMS_SCORE_STACKMANAGEMENT_H
>> > +
>> > +#if defined (ASM)
>> > +#include <rtems/asm.h>
>> > +#else
>> This also gets handled by basedefs.
>>
>> > +
>> > +#include <stdint.h>
>> > +#include <stdbool.h>
>> > +#include <rtems/score/memorymanagement.h>
>> > +#include <rtems/score/chainimpl.h>
>> > +
>> > +typedef struct stack_attr
>> > +{
>> > + Chain_Node node;
>> > + uintptr_t stack_address;
>> > + size_t size;
>> > + uint32_t page_table_base;
>> > + memory_flags access_flags;
>> > +}stack_attr;
>> space after }
>>
>> > +
>> > +typedef struct stack_attr_shared
>> > +{
>> > + stack_attr Base;
>> > + Chain_Control shared_node_control;
>> > +}stack_attr_shared;
>> > +
>> > +typedef struct stack_attr_prot
>> > +{
>> > + stack_attr_shared *shared_stacks;
>> > + stack_attr Base;
>> Typically 'Base' should come first in the struct definition. This way,
>> the start of the struct is identical to its Base
>>
>> > + bool current_stack;
>> > +} stack_attr_prot;
>> > +
>> > +
>> > +void prot_stack_allocate(uintptr_t stack_address, size_t size, uintptr_t page_table_base);
>> > +
>> > +
>> > +void prot_stack_share(stack_attr_shared *shared, stack_attr_prot prot_stack);
>> > +
>> > +stack_attr_prot *prot_stack_context_initialize(void);
>> > +
>> > +void prot_stack_context_switch(stack_attr_prot *stack_attr);
>> > +
>> > +void prot_stack_context_restore(stack_attr_prot *stack_attr);
>> > +
>> > +#endif
>> > +
>> > +#endif
>> > \ No newline at end of file
>> > diff --git a/cpukit/score/cpu/arm/cpu.c b/cpukit/score/cpu/arm/cpu.c
>> > index 07b9588afd..aa3626965a 100644
>> > --- a/cpukit/score/cpu/arm/cpu.c
>> > +++ b/cpukit/score/cpu/arm/cpu.c
>> > @@ -97,6 +97,9 @@ void _CPU_Context_Initialize(
>> > {
>> > (void) new_level;
>> >
>> > + #if defined (USE_THREAD_STACK_PROTECTION)
>> > + the_context->stack_attr = prot_stack_context_initialize();
>> > + #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 66f8ba6032..0133746b82 100644
>> > --- a/cpukit/score/cpu/arm/cpu_asm.S
>> > +++ b/cpukit/score/cpu/arm/cpu_asm.S
>> > @@ -36,7 +36,6 @@
>> > #ifdef ARM_MULTILIB_ARCH_V4
>> >
>> > .text
>> > -
>> spurious
>>
>> > /*
>> > * void _CPU_Context_switch( run_context, heir_context )
>> > * void _CPU_Context_restore( run_context, heir_context )
>> > @@ -65,6 +64,16 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
>> > #endif
>> >
>> > str r3, [r0, #ARM_CONTEXT_CONTROL_ISR_DISPATCH_DISABLE]
>> > +#ifdef USE_THREAD_STACK_PROTECTION
>> > + mov r8, r0
>> > + mov r9, r1
>> check alignment
>>
>> > + mov r10, r2
>> > + ldr r0, [r0, #112]
>> > + bl prot_stack_context_switch
>>
>> Is this a function call from inside the context switch code? That does
>> not seem like a good design.
>>
>> > + mov r0, r8
>> > + mov r1, r9
>> > + mov r2, r10
>> > +#endif
>> >
>> > #ifdef RTEMS_SMP
>> > /*
>> > @@ -133,6 +142,17 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
>> > */
>> > DEFINE_FUNCTION_ARM(_CPU_Context_restore)
>> > mov r1, r0
>> > +#if defined( USE_THREAD_STACK_PROTECTION )
>> > + mov r8, r0
>> > + mov r9, r1
>> > + mov r10, r2
>> > + ldr r0, [r0, #ARM_STACK_PROT_ATTR_OFFSET]
>> > + bl prot_stack_context_restore
>> ditto
>>
>> > + ldr lr, [r2]
>> > + mov r0, r8
>> > + mov r1, r9
>> > + mov r2, r10
>> > +#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 b7b48a3ac3..fb91142f35 100644
>> > --- a/cpukit/score/cpu/arm/include/rtems/score/cpu.h
>> > +++ b/cpukit/score/cpu/arm/include/rtems/score/cpu.h
>> > @@ -34,6 +34,7 @@
>> > #include <rtems/score/paravirt.h>
>> > #endif
>> > #include <rtems/score/arm.h>
>> > +#include <rtems/score/stackmanagement.h>
>> >
>> > /**
>> > * @addtogroup RTEMSScoreCPUARM
>> > @@ -157,6 +158,21 @@
>> >
>> > #ifdef ARM_MULTILIB_HAS_THREAD_ID_REGISTER
>> > #define ARM_CONTEXT_CONTROL_THREAD_ID_OFFSET 44
>> > + #ifdef ARM_MULITLIB_VFP
>> > + #define ARM_STACK_PROT_ATTR_OFFSET 112
>> > + #else
>> > + #define ARM_STACK_PROT_ATTR_OFFSET 48
>> > + #endif
>> > +#endif
>> > +
>> > +#ifdef USE_THREAD_STACK_PROTECTION
>> > + #if defined ARM_MULITLIB_VFP
>> > + #define ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET 112
>> > + #elif ARM_MULTILIB_HAS_THREAD_ID_REGISTER
>> > + #define ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET 48
>> > + #else
>> > + #define ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET 44
>> > + #endif
>> > #endif
>> >
>> > #ifdef ARM_MULTILIB_VFP
>> > @@ -185,6 +201,7 @@
>> >
>> > #define ARM_VFP_CONTEXT_SIZE 264
>> >
>> > +
>> ws
>>
>> > #ifndef ASM
>> >
>> > #ifdef __cplusplus
>> > @@ -235,6 +252,9 @@ typedef struct {
>> > #ifdef RTEMS_SMP
>> > volatile bool is_executing;
>> > #endif
>> > +#ifdef USE_THREAD_STACK_PROTECTION
>> > +stack_attr_prot *stack_attr;
>> > +#endif
>> > } Context_Control;
>> >
>> > static inline void _ARM_Data_memory_barrier( void )
>> > diff --git a/cpukit/score/src/stackmanagement.c b/cpukit/score/src/stackmanagement.c
>> > new file mode 100644
>> > index 0000000000..ae66099b14
>> > --- /dev/null
>> > +++ b/cpukit/score/src/stackmanagement.c
>> > @@ -0,0 +1,143 @@
>> > +#include <rtems/score/stackmanagement.h>
>> > +#include <rtems/score/chainimpl.h>
>> > +#include <rtems/score/basedefs.h>
>> > +
>> > +Chain_Control prot_node_control = CHAIN_INITIALIZER_EMPTY(prot_node_control);
>> > +
>> > +Chain_Control *shared_node_control;
>> > +
>> > +static void prot_stack_prev_entry_remove(void)
>> > +{
>> > +
>> > + Chain_Node *node;
>> > + stack_attr_prot *stack_attr;
>> > +
>> > + if( _Chain_Is_empty(&prot_node_control) == true ) {
>> > + _Chain_Initialize_empty(&prot_node_control);
>> > + }
>> > + node = _Chain_First( &prot_node_control );
>> > +
>> > + 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 == false ) {
>>
>> Is there more than one current_stack?
>>
>> If not, why can't you just keep track of it in a variable rather than
>> iterate through a list looking for it?
>>
>> > + memory_entries_unset(stack_attr->Base.stack_address, stack_attr->Base.size);
>> > + // shared_stack_entry_remove(stack_attr->shared_stacks);
>> > +
>> > + }
>> > + node = _Chain_Immutable_next( node );
>> > + }
>> > +
>> > +}
>> > +
>> > +/*
>> > +Iterate to the end of the chain and mark all the 'currnet' stack as false
>> > +Append the current stack attribute to the end of the chain
>> > +*/
>> > +static void prot_stack_chain_append (Chain_Control *control, stack_attr_prot *stack_append_attr)
>> > +{
>> > + Chain_Node *node;
>> > + stack_attr_prot *present_stacks_attr;
>> > +
>> > + if(_Chain_Is_empty(&prot_node_control) == true ) {
>> > +
>> > + _Chain_Initialize_one(&prot_node_control, &stack_append_attr->Base.node);
>> > + } else {
>> > + node = _Chain_First(&prot_node_control);
>> > +
>> > + 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(&prot_node_control, &stack_append_attr->Base.node);
>> > + }
>> > +
>> > +}
>> > +
>> > +void prot_stack_allocate(uintptr_t stack_address, size_t size, uintptr_t page_table_base)
>> > +{
>> > + stack_attr_prot *stack_attr;
>> > +
>> > +/*This field will be refactored and score objects will be used for dynamic allocation*/
>> > + stack_attr = malloc(sizeof(stack_attr_prot));
>> > +
>> > + if(stack_attr != NULL) {
>> > + stack_attr->Base.stack_address = stack_address;
>> > + stack_attr->Base.size = size;
>> > + stack_attr->Base.page_table_base = page_table_base;
>> > + stack_attr->Base.access_flags = READ_WRITE_CACHED;
>> > + stack_attr->current_stack = true;
>> > + }
>> > +
>> > + /*
>> > + Add the attribute field to the end of the chain, remove the memory entries of
>> > + previously allocated stack and set the memory entry of the currnet stack.
>> > + */
>> > + prot_stack_chain_append(&prot_node_control, stack_attr );
>> > + prot_stack_prev_entry_remove();
>> > + memory_entries_set(stack_address, size, READ_WRITE_CACHED);
>> > +
>> > +}
>> > +
>> > +stack_attr_prot *prot_stack_context_initialize(void)
>> > +{
>> > + Chain_Node *node;
>> > + stack_attr_prot *stack_attr;
>> > +
>> > + if( _Chain_Is_empty(&prot_node_control) == false ) {
>> > + node = _Chain_First( &prot_node_control );
>> > +
>> > + 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 = _Chain_Immutable_next( node );
>> > + }
>> > + }
>> > + }
>> > +
>> > + return stack_attr;
>> > +}
>> > +
>> > +void prot_stack_context_switch(stack_attr_prot *stack_attr)
>> > +{
>> > + void *stack_address;
>> > + size_t size;
>> > + Chain_Node *node;
>> > + Chain_Control *shared_node_control;
>> > +
>> > + /*
>> > + Remove the stacks shared with the current stack by iterating the chain
>> > + */
>> > + if( stack_attr != NULL) {
>> > +
>> > + stack_address = stack_attr->Base.stack_address;
>> > + size = stack_attr->Base.size;
>> > +
>> > + if(stack_attr->current_stack == true) {
>> > + memory_entries_unset(stack_address, size);
>> > + stack_attr->current_stack = false;
>> > + }
>> > +
>> > + shared_node_control = &stack_attr->shared_stacks->shared_node_control;
>> > + }
>> > +}
>> > +
>> > +void prot_stack_context_restore(stack_attr_prot *stack_attr)
>> > +{
>> > + void *stack_address;
>> > + size_t size;
>> > + Chain_Node *node;
>> > + Chain_Control *shared_node_control;
>> > + memory_flags flags;
>> > +
>> > + if(stack_attr->current_stack == true) {
>> > + memory_entries_set(stack_address, size, READ_WRITE_CACHED);
>>
>> Should you be reading these flags from somewhere
>> (stack_attr->Base.access_flags?), instead of hard-coding them?
>>
>> > + }
>> > +
>> > +}
>> > --
>> > 2.17.1
>> >
More information about the devel
mailing list