ppc64: fix mtmsr behavior on 64-bit targets

#if defined(TARGET_PPC64)

-        if (!ctx->sf_mode) {
             TCGv t0 = tcg_temp_new();
             TCGv t1 = tcg_temp_new();

Quoted 4 lines · Nathan Whitehorn

#if defined(TARGET_PPC64)

-        if (!ctx->sf_mode) {
              TCGv t0 = tcg_temp_new();
              TCGv t1 = tcg_temp_new();

You're removing a scope in which these variables were defined. That seems wrong, at minimum.

Quoted 8 lines · Richard Henderson

Quoted 4 lines · Nathan Whitehorn

#if defined(TARGET_PPC64)

-        if (!ctx->sf_mode) {
             TCGv t0 = tcg_temp_new();
             TCGv t1 = tcg_temp_new();

You're removing a scope in which these variables were defined. That seems wrong, at minimum.

I'll fix that (and resend the patch), thanks. A note on this: it looks like a lot of code here incorrectly changes behavior depending on the setting of MSR[SF]. While most of them aren't checking the condition the wrong way, like here, MSR[SF] actually changes very few aspects of the processor's operation. Turning MSR[SF] on or off on a 64-bit CPU basically only affects whether it pays attention to the high 32-bits of addresses when doing loads, stores, and branches -- 64-bit arithmetic, comparisons, registers, etc. are all available whatever the setting of MSR[SF].

Quoted 6 lines · Nathan Whitehorn

Quoted 5 lines · Richard Henderson

Quoted 4 lines · Nathan Whitehorn

#if defined(TARGET_PPC64)

-        if (!ctx->sf_mode) {
              TCGv t0 = tcg_temp_new();
              TCGv t1 = tcg_temp_new();

Quoted 4 lines

Quoted 3 lines

You're removing a scope in which these variables were defined. That seems wrong, at minimum.

I'll fix that (and resend the patch), thanks. A note on this: it looks like a lot of code here incorrectly changes behavior depending on the setting of MSR[SF]. While most of them aren't checking the condition the wrong way, like here, MSR[SF] actually changes very few aspects of the processor's operation. Turning MSR[SF] on or off on a 64-bit CPU basically only affects whether it pays attention to the high 32-bits of addresses when doing loads, stores, and branches -- 64-bit arithmetic, comparisons, registers, etc. are all available whatever the setting of MSR[SF].

Not sure I understand what you mean. You can't access the upper 32 bits since the instructions are not available when !SF. Also, some subtile behavior changes.

If you for example run "lis x,-1" in !SF on a 64-bit machine, the full register value becomes 0xffffffffffffffff while in SF mode it becomes 0xffffffff. Maybe there are some parts in the code that are not correct, but !SF on 64-bit is very subtile :).

Quoted 14 lines · Alexander Graf

Quoted 12 lines · Nathan Whitehorn

Quoted 8 lines · Richard Henderson

Quoted 4 lines · Nathan Whitehorn

#if defined(TARGET_PPC64)

-        if (!ctx->sf_mode) {
              TCGv t0 = tcg_temp_new();
              TCGv t1 = tcg_temp_new();

You're removing a scope in which these variables were defined. That seems wrong, at minimum.

I'll fix that (and resend the patch), thanks. A note on this: it looks like a lot of code here incorrectly changes behavior depending on the setting of MSR[SF]. While most of them aren't checking the condition the wrong way, like here, MSR[SF] actually changes very few aspects of the processor's operation. Turning MSR[SF] on or off on a 64-bit CPU basically only affects whether it pays attention to the high 32-bits of addresses when doing loads, stores, and branches -- 64-bit arithmetic, comparisons, registers, etc. are all available
whatever the setting of MSR[SF].

Quoted 3 lines

Not sure I understand what you mean. You can't access the upper 32 bits since the instructions are not available when !SF. Also, some subtile behavior changes.

If you for example run "lis x,-1" in !SF on a 64-bit machine, the full register value becomes 0xffffffffffffffff while in SF mode it becomes 0xffffffff. Maybe there are some parts in the code that are not correct, but !SF on 64-bit is very subtile :).

Is that actually true, though? The architecture manual says nothing about it, and on both Cell and 970 systems (the hardware I had handy), as well as IBM Systemsim, lis x,-1 gives 0xffffffffffffff00 irrespective of the value of MSR[SF]. -Nathan

Quoted 11 lines · Alexander Graf

Quoted 9 lines · Nathan Whitehorn

Quoted 8 lines · Richard Henderson

Quoted 4 lines · Nathan Whitehorn

#if defined(TARGET_PPC64)

-        if (!ctx->sf_mode) {
             TCGv t0 = tcg_temp_new();
             TCGv t1 = tcg_temp_new();

You're removing a scope in which these variables were defined. That seems wrong, at minimum.

Quoted 4 lines

Quoted 1 line

I'll fix that (and resend the patch), thanks. A note on this: it looks like a lot of code here incorrectly changes behavior depending on the setting of MSR[SF]. While most of them aren't checking the condition the wrong way, like here, MSR[SF] actually changes very few aspects of the processor's operation. Turning MSR[SF] on or off on a 64-bit CPU basically only affects whether it pays attention to the high 32-bits of addresses when doing loads, stores, and branches -- 64-bit arithmetic, comparisons, registers, etc. are all available whatever the setting of MSR[SF].

Not sure I understand what you mean. You can't access the upper 32 bits since the instructions are not available when !SF. Also, some subtile behavior changes.

If you for example run "lis x,-1" in !SF on a 64-bit machine, the full register value becomes 0xffffffffffffffff while in SF mode it becomes 0xffffffff. Maybe there are some parts in the code that are not correct, but !SF on 64-bit is very subtile :).

Is that actually true, though? The architecture manual says nothing about it, and on both Cell and 970 systems (the hardware I had handy), as well as IBM Systemsim, lis x,-1 gives 0xffffffffffffff00 irrespective of the value of MSR[SF].

The mtmsr instruction is required not to modify the upper 32-bits of the machine state register, but checks the current value of MSR[SF] to decide whether to do this. This has the effect of zeroing the upper 32 bits of the MSR whenever mtmsr is executed in 64-bit mode. Unconditionally preserve the upper 32-bits in mtmsr for TARGET_PPC64.

Signed-off-by Nathan Whitehorn <[email protected]>

---
target-ppc/translate.c | 5 ++---

1 files changed, 2 insertions(+), 3 deletions(-)

target-ppc/translate.c +0 -2

index 59aef85..38d2e2e 100644
@@ -3884,18 +3884,17 @@ static void gen_mtmsr(DisasContext *ctx)
         */
        gen_update_nip(ctx, ctx->nip);
 #if defined(TARGET_PPC64)
-        if (!ctx->sf_mode) {
-            TCGv t0 = tcg_temp_new();

target-ppc/translate.c +5 -1

+        tcg_temp_free(t1);
+        gen_helper_store_msr(t0);
+        tcg_temp_free(t0);
+#else
+        gen_helper_store_msr(cpu_gpr[rS(ctx->opcode)]);
 #endif
-            gen_helper_store_msr(cpu_gpr[rS(ctx->opcode)]);
        /* Must stop the translation as machine state (may have) changed */
        /* Note that mtmsr is not always defined as context-synchronizing */
        gen_stop_exception(ctx);

Am 12.06.2011 um 17:49 schrieb Nathan Whitehorn<[email protected]>:

Quoted 20 lines

The mtmsr instruction is required not to modify the upper 32-bits of the machine state register, but checks the current value of MSR[SF] to decide whether to do this. This has the effect of zeroing the upper 32 bits of the MSR whenever mtmsr is executed in 64-bit mode. Unconditionally preserve the upper 32-bits in mtmsr for TARGET_PPC64.

Signed-off-by Nathan Whitehorn<[email protected]>

---
target-ppc/translate.c | 5 ++---

1 files changed, 2 insertions(+), 3 deletions(-)

target-ppc/translate.c +0 -5

index 59aef85..38d2e2e 100644
@@ -3884,18 +3884,17 @@ static void gen_mtmsr(DisasContext *ctx)
          */
         gen_update_nip(ctx, ctx->nip);
 #if defined(TARGET_PPC64)
-        if (!ctx->sf_mode) {
-            TCGv t0 = tcg_temp_new();
-            TCGv t1 = tcg_temp_new();
-            tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL);
-            tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]);

target-ppc/translate.c static void gen_mtmsr(DisasContext *ctx) · line 3888

Quoted 7 lines

target-ppc/translate.c +2 -5

-            tcg_gen_or_tl(t0, t0, t1);
-            tcg_temp_free(t1);
-            gen_helper_store_msr(t0);
-            tcg_temp_free(t0);
-        } else
+        TCGv t0 = tcg_temp_new();
+        TCGv t1 = tcg_temp_new();

You're declaring variables in mid-scope. Please open a new scope :).

target-ppc/translate.c static void gen_mtmsr(DisasContext *ctx) · line 3891

Quoted 3 lines

target-ppc/translate.c +3 -0

+        tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL);
+        tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]);
+        tcg_gen_or_tl(t0, t0, t1);

While at it, this is a perfect scenario for the deposit tcg op! :)

If you feel like this is too cumbersome work for such a small patch, please let me know and I'll do the changes for you :)

Quoted 25 lines · Alexander Graf

Am 12.06.2011 um 17:49 schrieb Nathan Whitehorn<[email protected]>:

Quoted 20 lines

The mtmsr instruction is required not to modify the upper 32-bits of the machine state register, but checks the current value of MSR[SF] to decide whether to do this. This has the effect of zeroing the upper 32 bits of the MSR whenever mtmsr is executed in 64-bit mode. Unconditionally preserve the upper 32-bits in mtmsr for TARGET_PPC64.

Signed-off-by Nathan Whitehorn<[email protected]>

---
target-ppc/translate.c | 5 ++---

1 files changed, 2 insertions(+), 3 deletions(-)

target-ppc/translate.c +0 -5

index 59aef85..38d2e2e 100644
@@ -3884,18 +3884,17 @@ static void gen_mtmsr(DisasContext *ctx)
         */
        gen_update_nip(ctx, ctx->nip);
 #if defined(TARGET_PPC64)
-        if (!ctx->sf_mode) {
-            TCGv t0 = tcg_temp_new();
-            TCGv t1 = tcg_temp_new();
-            tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL);
-            tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]);

Quoted 8 lines

target-ppc/translate.c static void gen_mtmsr(DisasContext *ctx) · line 3888

Quoted 7 lines

target-ppc/translate.c +2 -5

-            tcg_gen_or_tl(t0, t0, t1);
-            tcg_temp_free(t1);
-            gen_helper_store_msr(t0);
-            tcg_temp_free(t0);
-        } else
+        TCGv t0 = tcg_temp_new();
+        TCGv t1 = tcg_temp_new();

You're declaring variables in mid-scope. Please open a new scope :).

Does the gen_update_nip(ctx, ctx->nip); need to be first here? If not, we can just move it to the end and avoid the scoping issue.

Quoted 6 lines

target-ppc/translate.c static void gen_mtmsr(DisasContext *ctx) · line 3891

Quoted 3 lines

target-ppc/translate.c +3 -0

+        tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL);
+        tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]);
+        tcg_gen_or_tl(t0, t0, t1);

While at it, this is a perfect scenario for the deposit tcg op! :)

If you feel like this is too cumbersome work for such a small patch, please let me know and I'll do the changes for you :)

I have no idea what that is, so I'd prefer you do it :)

The mtmsr instruction is required not to modify the upper 32-bits of the machine state register, but checks the current value of MSR[SF] to decide whether to do this. This has the effect of zeroing the upper 32 bits of the MSR whenever mtmsr is executed in 64-bit mode. Unconditionally preserve the upper 32-bits in mtmsr for TARGET_PPC64.

---
target-ppc/translate.c | 5 ++---

1 files changed, 2 insertions(+), 3 deletions(-)

target-ppc/translate.c +0 -0

index 9b3f90c..a60dbe9 100644