Patchwork PPC: Fix sync instructions problem in SMP

login
register
mail settings
Submitter Elie Richa
Date July 22, 2011, 3:58 p.m.
Message ID <1311350319-29706-1-git-send-email-richa@adacore.com>
Download mbox | patch
Permalink /patch/106324/
State New
Headers show

Comments

Elie Richa - July 22, 2011, 3:58 p.m.
In the current emulation of the load-and-reserve (lwarx) and
store-conditional (stwcx.) instructions, the internal reservation
mechanism is taken into account, however each CPU has its own
reservation information and this information is not synchronized between
CPUs to perform proper synchronization.
The following test case with 2 CPUs shows that the semantics of the
"lwarx" and "stwcx." instructions are not preserved by the emulation.
The test case does the following :
	- CPU0: reserve a memory location
	- CPU1: reserve the same memory location
	- CPU0: perform stwcx. on the location 
The last store-conditional operation succeeds while it is supposed to
fail since the reservation was supposed to be lost at the second reserve
operation.

This (one line) patch fixes this problem in a very simple manner by
removing the reservation of a CPU every time it is scheduled (in
cpu_exec()). While this is a harsh workaround, it does not affect the
guest code much because reservations are usually held for a very short
time, that is an lwarx is almost always followed by an stwcx. a few
instructions below. Therefore, in most cases, the reservation will be
taken and consumed before a CPU switch occurs. However in the rare case
where a CPU switch does occur between the lwarx and its corresponding
stwcx.  this patch solves a potential erroneous behavior of the
synchronization instructions.

Signed-off-by: Elie Richa <richa@adacore.com>
---
 cpu-exec.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Elie Richa - Aug. 4, 2011, 10:34 a.m.
bump.

Anybody have opinions on this?
I can provide a more thorough explanation of the problem if needed. I 
wrote the test case in Ada for an MPC8641D processor (2 e600 cores) on a 
board that is not yet submitted to the list, therefore I cannot provide 
a ready to run binary. I can submit my board if needed.

Even though my patch is rather harsh, it has the advantage of solving
the problem for any number of CPUs without touching the translation
code. And it does not affect normal execution as I explained in the
previous mail.

Regards,
Elie

On 07/22/2011 05:58 PM, Elie Richa wrote:
> In the current emulation of the load-and-reserve (lwarx) and
> store-conditional (stwcx.) instructions, the internal reservation
> mechanism is taken into account, however each CPU has its own
> reservation information and this information is not synchronized between
> CPUs to perform proper synchronization.
> The following test case with 2 CPUs shows that the semantics of the
> "lwarx" and "stwcx." instructions are not preserved by the emulation.
> The test case does the following :
> 	- CPU0: reserve a memory location
> 	- CPU1: reserve the same memory location
> 	- CPU0: perform stwcx. on the location
> The last store-conditional operation succeeds while it is supposed to
> fail since the reservation was supposed to be lost at the second reserve
> operation.
>
> This (one line) patch fixes this problem in a very simple manner by
> removing the reservation of a CPU every time it is scheduled (in
> cpu_exec()). While this is a harsh workaround, it does not affect the
> guest code much because reservations are usually held for a very short
> time, that is an lwarx is almost always followed by an stwcx. a few
> instructions below. Therefore, in most cases, the reservation will be
> taken and consumed before a CPU switch occurs. However in the rare case
> where a CPU switch does occur between the lwarx and its corresponding
> stwcx.  this patch solves a potential erroneous behavior of the
> synchronization instructions.
>
> Signed-off-by: Elie Richa<richa@adacore.com>
> ---
>   cpu-exec.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index de0d716..d1ac3b0 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -216,6 +216,7 @@ int cpu_exec(CPUState *env)
>   #elif defined(TARGET_ARM)
>   #elif defined(TARGET_UNICORE32)
>   #elif defined(TARGET_PPC)
> +    env->reserve_addr = -1;
>   #elif defined(TARGET_LM32)
>   #elif defined(TARGET_MICROBLAZE)
>   #elif defined(TARGET_MIPS)
Alexander Graf - Aug. 9, 2011, 12:44 p.m.
On 08/04/2011 12:34 PM, Elie Richa wrote:
> bump.
>
> Anybody have opinions on this?
> I can provide a more thorough explanation of the problem if needed. I 
> wrote the test case in Ada for an MPC8641D processor (2 e600 cores) on 
> a board that is not yet submitted to the list, therefore I cannot 
> provide a ready to run binary. I can submit my board if needed.
>
> Even though my patch is rather harsh, it has the advantage of solving
> the problem for any number of CPUs without touching the translation
> code. And it does not affect normal execution as I explained in the
> previous mail.

No, the patch looks reasonable. I'll try to apply it ASAP.


Alex

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index de0d716..d1ac3b0 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -216,6 +216,7 @@  int cpu_exec(CPUState *env)
 #elif defined(TARGET_ARM)
 #elif defined(TARGET_UNICORE32)
 #elif defined(TARGET_PPC)
+    env->reserve_addr = -1;
 #elif defined(TARGET_LM32)
 #elif defined(TARGET_MICROBLAZE)
 #elif defined(TARGET_MIPS)