Bug #4923: ARM VFP context switch fix

[Sebastian Huber]

Hello Cedric,

thanks for having a look at this.

On 06.02.24 16:37, Cedric Berger wrote:
> Hello,
> 
> I've started to investigate bug #4923.
> 
> A first problem can be found after applying the following patch to the 
> paranoia.exe test:
> 
> https://devel.rtems.org/attachment/ticket/4923/bug-1-make-paranoia-test-fail.patch
> 
> The test fails because of the added sleep(1) at the beginning of Init() 
> and the CONFIGURE_STACK_CHECKER_ENABLED option which poisons the stack.
> 
> What I have been able to observe is something like this:
> 
> 1) Because of the sleep(1) call, RTEMS goes through a INIT->IDLE->INIT 
> pair of context switches.
> 
> 2) The first context switch goes through _Thread_Dispatch_direct, which 
> does not use an ARMv7 exception frame save/restore.
> 
> 3) In _CPU_Context_switch(), the FPSCR register is not saved/restored.

This is related to:

http://devel.rtems.org/ticket/4027

> 
> 4) After the 1s sleep ends, the second context switch originate with 
> _ARMV7M_Pendable_service_call()
> 
> 5) In _ARMV7M_Pendable_service_call(), a new stack frame is generaterd 
> (--ef;) but only partially initalized with PC and XPSR. Everything else 
> on that exception frame comes from the poisonned stack.

The _ARMV7M_Pendable_service_call() and _ARMV7M_Supervisor_call() work 
as a team. The --ef and ++ef is there to preserve the original exception 
frame across the jump to and from _ARMV7M_Thread_dispatch().

void _ARMV7M_Pendable_service_call( void )
{
   Per_CPU_Control *cpu_self = _Per_CPU_Get();

   /*
    * We must check here if a thread dispatch is allowed.  Right after a
    * "msr basepri_max, %[basepri]" instruction an interrupt service may 
still
    * take place.  However, pendable service calls that are activated during
    * this interrupt service may be delayed until interrupts are enable 
again.
    */
   if (
     ( cpu_self->isr_nest_level | 
cpu_self->thread_dispatch_disable_level ) == 0
   ) {
     ARMV7M_Exception_frame *ef;

     cpu_self->isr_nest_level = 1;

     _ARMV7M_SCB->icsr = ARMV7M_SCB_ICSR_PENDSVCLR;
     _ARMV7M_Trigger_lazy_floating_point_context_save();

At this point, the floating point context should be saved on the 
exception frame. The FPCCR.LSPACT bit should be 0, to indicate that lazy 
state preservation is no longer active.

     ef = (ARMV7M_Exception_frame *) _ARMV7M_Get_PSP();
     --ef;
     _ARMV7M_Set_PSP( (uint32_t) ef );

This new exception frame is just there to jump to 
_ARMV7M_Thread_dispatch(). Here could be the problem, maybe we have to 
set FPCCR.LSPACT to 1, so that we do not restore a garbage FP state, see 
also:

https://developer.arm.com/documentation/ddi0403/d/System-Level-Architecture/System-Level-Programmers--Model/ARMv7-M-exception-model/Exception-return-behavior?lang=en

     /*
      * According to "ARMv7-M Architecture Reference Manual" section B1.5.6
      * "Exception entry behavior" the return address is half-word aligned.
      */
     ef->register_pc = (void *)
       ((uintptr_t) _ARMV7M_Thread_dispatch & ~((uintptr_t) 1));

     ef->register_xpsr = 0x01000000U;
   }
}

void _ARMV7M_Supervisor_call( void )
{
   Per_CPU_Control *cpu_self = _Per_CPU_Get();
   ARMV7M_Exception_frame *ef;

   _ARMV7M_Trigger_lazy_floating_point_context_save();

   ef = (ARMV7M_Exception_frame *) _ARMV7M_Get_PSP();
   ++ef;
   _ARMV7M_Set_PSP( (uint32_t) ef );

   cpu_self->isr_nest_level = 0;

   if ( cpu_self->dispatch_necessary ) {
     _ARMV7M_Pendable_service_call();
   }
}


> 
> 6) Because of that, the FPU registers S0-S15 and FPSCR is loaded with 
> garbage.
> 
> 7) This garbage is transfered to the Init task after 
> _CPU_Context_switch(). It does not matter for S0-S15 because they don't 
> need to be preserved across function calls, but for FPSCR it matters.
> 
> 8) With the bogus FPSCR, pow(2.0, 1.0) returns 1.9999999999, which 
> breaks the paranoia FPU tests and our javascript engine :)
> 
> I might have missed something, but with the two following fixes, the 
> problem goes away:
> 
> 1) 
> https://devel.rtems.org/attachment/ticket/4923/fix-1-save-and-restore-fpscr.patch
> 
> This patch explicitely save/restore the FPSCR in _CPU_Context_switch()
> 
> 2) 
> https://devel.rtems.org/attachment/ticket/4923/fix-2-clean-initial-thread-frame.patch
> 
> This patch prevents the uninitialized or poisoned stack data to be 
> loaded into the VFP registers.
> 
> There is still a lot that I need to test and what I don't understand, 
> but what do you guys think about these 2 patches?

Saving and restoring the FPSCR could make sense independent of the 
current issue. However, I am not sure if we really need it.

Could you please test also if this patch alone fixes the problem?

diff --git a/cpukit/score/cpu/arm/armv7m-isr-dispatch.c 
b/cpukit/score/cpu/arm/armv7m-isr-dispatch.c
index 5ab9d1fae2..b30197c45b 100644
--- a/cpukit/score/cpu/arm/armv7m-isr-dispatch.c
+++ b/cpukit/score/cpu/arm/armv7m-isr-dispatch.c
@@ -78,6 +78,9 @@ void _ARMV7M_Pendable_service_call( void )

      _ARMV7M_SCB->icsr = ARMV7M_SCB_ICSR_PENDSVCLR;
      _ARMV7M_Trigger_lazy_floating_point_context_save();
+#ifdef ARM_MULTILIB_VFP
+    _ARMV7M_SCB->fpccr |= 0x1;
+#endif

      ef = (ARMV7M_Exception_frame *) _ARMV7M_Get_PSP();
      --ef;

-- 
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/