[PATCH] Strict thread-stack isolation

Utkarsh Rai utkarsh.rai60 at gmail.com
Tue Jul 14 16:59:02 UTC 2020


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.


>
> > +    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?


> > +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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200714/89a72ad1/attachment-0001.html>


More information about the devel mailing list