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