Re: [PATCH-V2 1/1] sparc: Fix context switch on SMP

[Sebastian Huber]

On 16/11/15 13:14, Daniel Cederman wrote:
I was unsure if the ET bit was always set or not for newly created 
task contexts, or if this was the first place that traps got enabled 
for a new task. If it is always set we can remove that instruction.


The PSR is initialized like this (_CPU_Context_Initialize()):

[...]
/*
 *  Build the PSR for the task.  Most everything can be 0 and the
 *  CWP is corrected during the context switch.
 *
 *  The EF bit determines if the floating point unit is available.
 *  The FPU is ONLY enabled if the context is associated with an FP 
task

 *  and this SPARC model has an FPU.
 */

sparc_get_psr( tmp_psr );
tmp_psr &= ~SPARC_PSR_PIL_MASK;
tmp_psr |= (new_level << 8) & SPARC_PSR_PIL_MASK;
tmp_psr &= ~SPARC_PSR_EF_MASK;  /* disabled by default */

#if (SPARC_HAS_FPU == 1)
/*
 *  If this bit is not set, then a task gets a fault when it accesses
 *  a floating point register.  This is a nice way to detect floating
 *  point tasks which are not currently declared as such.
 */

if ( is_fp )
  tmp_psr |= SPARC_PSR_EF_MASK;
#endif
the_context->psr = tmp_psr;
[...]

Since traps must be enabled at C level, this should be fine.

Maybe add this to the patch for documentation purposes:

diff --git a/cpukit/score/cpu/sparc/cpu.c b/cpukit/score/cpu/sparc/cpu.c
index 569b6f8..5434355 100644
--- a/cpukit/score/cpu/sparc/cpu.c
+++ b/cpukit/score/cpu/sparc/cpu.c
@@ -356,6 +356,9 @@ void _CPU_Context_Initialize(
 tmp_psr |= (new_level << 8) & SPARC_PSR_PIL_MASK;
 tmp_psr &= ~SPARC_PSR_EF_MASK;  /* disabled by default */

+/* _CPU_Context_restore_heir() relies on this */
+_Assert( ( tmp_psr & SPARC_PSR_ET_MASK ) != 0 );
+
 #if (SPARC_HAS_FPU == 1)
 /*
  *  If this bit is not set, then a task gets a fault when it accesse

--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail  : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: Memory access between application task and driver using printk

[Gedare Bloom]

There appears to be problems in the code you copy-pasted, so it is
hard to say for sure what is going on in your actual code. See below
for a few notes.

On Mon, Nov 16, 2015 at 11:28 AM, Inderjit Singh
 wrote:
> Hi,
>
> I have a strange behaviour regarding memory access between my user
> application and the driver i have developed I'd like to get some
> clarification on.
>
> The development is all used on or1k arch.
>
> On the application side, I have a buffer declared as:
> #define PACKET_SIZE 256
> volatile uint8_t __attribute__ ((aligned (32))) buf_rx[256];
>
> Then I use the posix api function read to access the data I want to get from
> my driver as following:
>
> rtems_task test_task_rx(rtems_task_argument arg)
> {
>   info_rx_t *info_rx;
>   info_rx = (info_rx_t*)arg;
>
>   while(true){
> memset((uint8_t*)buf_rx, 0xaa, SPWN_TS_PACKET_SIZE);
> printk("Waiting for packet...@0x%08X\r\n", buf_rx);
> size = read(info_rx->fd, (uint8_t*)_rx[0], PACKET_SIZE);
>
Check size. If it is 1, then the behavior you get may be expected, since...

> if(size > 0) {
>   printk("Packet received with size <%d>", buf_rx[0], size);
>   /* Print info */
>   if(spwn_info_rx->show_header == true) {
> for(i = 0;i < info_rx->packet_len;i++) {
>   printk("%02X ", buf_rx[1]);
array index should be i, not 1.

>   if((i%16) == 0) {
> printk("\r\n");
>   }
>  }
>  printk("\b>\r\n");
>}
>  }
>}
>  }
>
> From my driver side I'm just iterating the buffer values by one to see the
> update, i.e: First time it starts with [01 02 03..." and next time it starts
> with [02 03 04...]. The strange thing is the first time I get a 'packet' it
> looks great. But the second time, It shows a bunch of 0xaa values (from
> memset before reading):
>
You should also validate behavior within the driver 'read' function. I
don't know what you mean by "iterating the buffer values" and if your
driver is accessing the buf_rx array that is probably not a great
design choice. I don't quite understand what your driver does to help
you debug this issue.

> Packet received with size <256> header @ 0004D960: AA
> AA AA AA ...
> 11 12 13 ...
> AA AA AA ...
> ...
>
> But looking at the memory area in gdb I see the correct values:
> 01 02 03 ...
> 11 12 13 ...
> 21 22 23 ...
> ...
>
> Removing memset call only shows that the buffer is the same after first call
> to read. I can't figure out why. Does anyone have a explanation. Any help
> would be appreciated.
>
> Thanks,
> Inderjit
>
>
>
>
>
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Memory access between application task and driver using printk

[Inderjit Singh]

Hi,

I have a strange behaviour regarding memory access between my user application 
and the driver i have developed I'd like to get some clarification on.

The development is all used on or1k arch.

On the application side, I have a buffer declared as:
#define PACKET_SIZE 256
volatile uint8_t __attribute__ ((aligned (32))) buf_rx[256];

Then I use the posix api function read to access the data I want to get from my 
driver as following:

rtems_task test_task_rx(rtems_task_argument arg)
{
  info_rx_t *info_rx;
  info_rx = (info_rx_t*)arg;

  while(true){
memset((uint8_t*)buf_rx, 0xaa, SPWN_TS_PACKET_SIZE);
printk("Waiting for packet...@0x%08X\r\n", buf_rx);
size = read(info_rx->fd, (uint8_t*)_rx[0], PACKET_SIZE);

if(size > 0) {
  printk("Packet received with size <%d>", buf_rx[0], size);
  /* Print info */
  if(spwn_info_rx->show_header == true) {
for(i = 0;i < info_rx->packet_len;i++) {
  printk("%02X ", buf_rx[1]);
  if((i%16) == 0) {
printk("\r\n");
  }
 }
 printk("\b>\r\n");
   }
 }
   }
 }

>From my driver side I'm just iterating the buffer values by one to see the 
>update, i.e: First time it starts with [01 02 03..." and next time it starts 
>with [02 03 04...]. The strange thing is the first time I get a 'packet' it 
>looks great. But the second time, It shows a bunch of 0xaa values (from memset 
>before reading):

Packet received with size <256> header @ 0004D960: AA
AA AA AA ...
11 12 13 ...
AA AA AA ...
...

But looking at the memory area in gdb I see the correct values:
01 02 03 ...
11 12 13 ...
21 22 23 ...
...

Removing memset call only shows that the buffer is the same after first call to 
read. I can't figure out why. Does anyone have a explanation. Any help would be 
appreciated.

Thanks,
Inderjit




___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH-V3 1/1] sparc: Fix context switch on SMP

[Sebastian Huber]

Looks good, we should probably apply it to the 4.11 branch as well.

--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail  : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH-V3 1/1] sparc: Fix context switch on SMP

[Daniel Cederman]

Yes, definitely. Would you mind doing it? Daniel is away from office 
this week and I do not have access.


On 2015-11-16 15:57, Sebastian Huber wrote:

Looks good, we should probably apply it to the 4.11 branch as well.



--
Daniel Cederman
Software Engineer
Cobham Gaisler
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH-V2 1/1] sparc: Fix context switch on SMP

[Daniel Cederman]

I was unsure if the ET bit was always set or not for newly created task 
contexts, or if this was the first place that traps got enabled for a 
new task. If it is always set we can remove that instruction.


On 2015-11-16 11:27, Sebastian Huber wrote:



On 16/11/15 11:06, Daniel Cederman wrote:

@@ -202,6 +193,13 @@ try_update_is_executing:
  ! The next load is in a delay slot, which is all right
  #endif
+ld  [%o1 + PSR_OFFSET], %g1   ! g1 = heir psr
+andn%g1, SPARC_PSR_CWP_MASK, %g1  ! g1 = heir psr w/o cwp
+or  %g1, %g3, %g1 ! g1 = heir psr with cwp
+or  %g1, SPARC_PSR_ET_MASK, %g1   ! g1 = heir psr traps
enabled


Do we really need the "or  %g1, SPARC_PSR_ET_MASK, %g1"? I think we
should simply use the value of the saved PSR and omit this step.


+mov %g1, %psr ! restore status
register and
+  !  ENABLE TRAPS 
+
  ld  [%o1 + G5_OFFSET], %g5! restore the global
registers
  ld  [%o1 + G7_OFFSET], %g7




--
Daniel Cederman
Software Engineer
Cobham Gaisler
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH-V3 1/1] sparc: Fix context switch on SMP

[Sebastian Huber]

- Am 16. Nov 2015 um 17:06 schrieb Gedare Bloom ged...@rtems.org:

> Does this bug have a ticket?
 
>From my point of view we need a ticket, if

1) a bug cannot be fixed immediately or requires a couple of patches, or

2) a new feature need several patches, accompanying 
documentation/reasons/motivation, or

3) a bug is in a release.

This is not the case here, since it fixes a problem introduced due to the SMP 
support.
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH-V3 1/1] sparc: Fix context switch on SMP

[Gedare Bloom]

On Mon, Nov 16, 2015 at 2:31 PM, Sebastian Huber
 wrote:
>
>
> - Am 16. Nov 2015 um 17:06 schrieb Gedare Bloom ged...@rtems.org:
>
>> Does this bug have a ticket?
>
> From my point of view we need a ticket, if
>
> 1) a bug cannot be fixed immediately or requires a couple of patches, or
>
> 2) a new feature need several patches, accompanying 
> documentation/reasons/motivation, or
>
> 3) a bug is in a release.
>
> This is not the case here, since it fixes a problem introduced due to the SMP 
> support.
Sure, as long is you slip it in 4.11 before we actually release.
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

[PATCH 1/1] sparc: Fix context switch on SMP

[Daniel Cederman]

We must not load registers (e.g. PSR) from the heir context area before
the heir stopped execution.
---
 c/src/lib/libbsp/sparc/shared/irq_asm.S | 30 +-
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/c/src/lib/libbsp/sparc/shared/irq_asm.S 
b/c/src/lib/libbsp/sparc/shared/irq_asm.S
index f7222e7..5931e77 100644
--- a/c/src/lib/libbsp/sparc/shared/irq_asm.S
+++ b/c/src/lib/libbsp/sparc/shared/irq_asm.S
@@ -127,14 +127,9 @@ SYM(_CPU_Context_restore_heir):
  *  g5 = scratch
  */
 
-ld  [%o1 + PSR_OFFSET], %g1   ! g1 = saved psr
-
 and %o2, SPARC_PSR_CWP_MASK, %g3  ! g3 = CWP
-  ! g1 = psr w/o cwp
-andn%g1, SPARC_PSR_ET_MASK | SPARC_PSR_CWP_MASK, %g1
-or  %g1, %g3, %g1 ! g1 = heirs psr
-mov %g1, %psr ! restore status register and
-  !  DISABLE TRAPS 
+andn%o2, SPARC_PSR_ET_MASK, %g1   ! g1 = psr with traps disabled
+mov %g1, %psr !  DISABLE TRAPS 
 mov %wim, %g2 ! g2 = wim
 mov 1, %g4
 sll %g4, %g3, %g4 ! g4 = WIM mask for CW invalid
@@ -173,19 +168,13 @@ save_frame_loop:
 
 done_flushing:
 
-add %g3, 1, %g3   ! calculate desired WIM
-and %g3, SPARC_NUMBER_OF_REGISTER_WINDOWS - 1, %g3
+mov %g1, %psr ! restore cwp
+add %g3, 1, %g2   ! calculate desired WIM
+and %g2, SPARC_NUMBER_OF_REGISTER_WINDOWS - 1, %g2
 mov 1, %g4
-sll %g4, %g3, %g4 ! g4 = new WIM
+sll %g4, %g2, %g4 ! g4 = new WIM
 mov %g4, %wim
 
-or  %g1, SPARC_PSR_ET_MASK, %g1
-mov %g1, %psr !  ENABLE TRAPS 
-  !   and restore CWP
-nop
-nop
-nop
-
 #if defined(RTEMS_SMP)
 ! The executing context no longer executes on this processor
 st  %g0, [%o0 + SPARC_CONTEXT_CONTROL_IS_EXECUTING_OFFSET]
@@ -202,6 +191,13 @@ try_update_is_executing:
 ! The next load is in a delay slot, which is all right
 #endif
 
+ld  [%o1 + PSR_OFFSET], %g1   ! g1 = heir psr
+andn%g1, SPARC_PSR_CWP_MASK, %g1  ! g1 = heir psr w/o cwp
+or  %g1, %g3, %g1 ! g1 = heir psr with cwp
+or  %g1, SPARC_PSR_ET_MASK, %g1   ! g1 = heir psr traps enabled
+mov %g1, %psr ! restore status register and
+  !  ENABLE TRAPS 
+
 ld  [%o1 + G5_OFFSET], %g5! restore the global registers
 ld  [%o1 + G7_OFFSET], %g7
 
-- 
2.1.4

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

[PATCH-V4 1/1] sparc: Fix context switch on SMP

[Daniel Cederman]

We must not load registers (e.g. PSR) from the heir context area before
the heir stopped execution.

With this patch the write to PSR is divided into two steps. We first update
the current window pointer and then we restore the status registers and
enable traps. This allows us to move the first write to PSR to be before
the write to WIM, as there is now no risk that we get an interrupt where
the CWP and WIM would be inconsistent. We only need to make sure that we
do not use any of the non-global registers or instructions that affects
CWP for three instructions after the write.

In the earlier code the non-global %o1 register was used right after the
write to PSR, which required the use of three nop:s.

Fixes #2472
---
 c/src/lib/libbsp/sparc/shared/irq_asm.S | 31 ++-
 cpukit/score/cpu/sparc/cpu.c|  3 +++
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/c/src/lib/libbsp/sparc/shared/irq_asm.S 
b/c/src/lib/libbsp/sparc/shared/irq_asm.S
index f7222e7..6074130 100644
--- a/c/src/lib/libbsp/sparc/shared/irq_asm.S
+++ b/c/src/lib/libbsp/sparc/shared/irq_asm.S
@@ -127,14 +127,9 @@ SYM(_CPU_Context_restore_heir):
  *  g5 = scratch
  */
 
-ld  [%o1 + PSR_OFFSET], %g1   ! g1 = saved psr
-
 and %o2, SPARC_PSR_CWP_MASK, %g3  ! g3 = CWP
-  ! g1 = psr w/o cwp
-andn%g1, SPARC_PSR_ET_MASK | SPARC_PSR_CWP_MASK, %g1
-or  %g1, %g3, %g1 ! g1 = heirs psr
-mov %g1, %psr ! restore status register and
-  !  DISABLE TRAPS 
+andn%o2, SPARC_PSR_ET_MASK, %g1   ! g1 = psr with traps disabled
+mov %g1, %psr !  DISABLE TRAPS 
 mov %wim, %g2 ! g2 = wim
 mov 1, %g4
 sll %g4, %g3, %g4 ! g4 = WIM mask for CW invalid
@@ -173,19 +168,15 @@ save_frame_loop:
 
 done_flushing:
 
-add %g3, 1, %g3   ! calculate desired WIM
-and %g3, SPARC_NUMBER_OF_REGISTER_WINDOWS - 1, %g3
+! Wait three instructions after the write to PSR before using
+! non-global registers or instructions affecting the CWP
+mov %g1, %psr ! restore cwp
+add %g3, 1, %g2   ! calculate desired WIM
+and %g2, SPARC_NUMBER_OF_REGISTER_WINDOWS - 1, %g2
 mov 1, %g4
-sll %g4, %g3, %g4 ! g4 = new WIM
+sll %g4, %g2, %g4 ! g4 = new WIM
 mov %g4, %wim
 
-or  %g1, SPARC_PSR_ET_MASK, %g1
-mov %g1, %psr !  ENABLE TRAPS 
-  !   and restore CWP
-nop
-nop
-nop
-
 #if defined(RTEMS_SMP)
 ! The executing context no longer executes on this processor
 st  %g0, [%o0 + SPARC_CONTEXT_CONTROL_IS_EXECUTING_OFFSET]
@@ -202,6 +193,12 @@ try_update_is_executing:
 ! The next load is in a delay slot, which is all right
 #endif
 
+ld  [%o1 + PSR_OFFSET], %g1   ! g1 = heir psr with traps 
enabled
+andn%g1, SPARC_PSR_CWP_MASK, %g1  ! g1 = heir psr w/o cwp
+or  %g1, %g3, %g1 ! g1 = heir psr with cwp
+mov %g1, %psr ! restore status register and
+  !  ENABLE TRAPS 
+
 ld  [%o1 + G5_OFFSET], %g5! restore the global registers
 ld  [%o1 + G7_OFFSET], %g7
 
diff --git a/cpukit/score/cpu/sparc/cpu.c b/cpukit/score/cpu/sparc/cpu.c
index 569b6f8..5434355 100644
--- a/cpukit/score/cpu/sparc/cpu.c
+++ b/cpukit/score/cpu/sparc/cpu.c
@@ -356,6 +356,9 @@ void _CPU_Context_Initialize(
 tmp_psr |= (new_level << 8) & SPARC_PSR_PIL_MASK;
 tmp_psr &= ~SPARC_PSR_EF_MASK;  /* disabled by default */
 
+/* _CPU_Context_restore_heir() relies on this */
+_Assert( ( tmp_psr & SPARC_PSR_ET_MASK ) != 0 );
+
 #if (SPARC_HAS_FPU == 1)
 /*
  *  If this bit is not set, then a task gets a fault when it accesses
-- 
2.1.4

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel