[Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Nathan Whitehorn]

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(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 9b3f90c..a60dbe9 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -3886,7 +3886,6 @@ 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);
@@ -3895,9 +3894,9 @@ static void gen_mtmsr(DisasContext *ctx)
              tcg_temp_free(t1);
              gen_helper_store_msr(t0);
              tcg_temp_free(t0);
-        } else
-#endif
+#else
              gen_helper_store_msr(cpu_gpr[rS(ctx->opcode)]);
+#endif
          /* Must stop the translation as machine state (may have) 
changed */
          /* Note that mtmsr is not always defined as 
context-synchronizing */
          gen_stop_exception(ctx);

Re: [Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Alexander Graf]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]


On 31.05.2011, at 16:56, Nathan Whitehorn wrote:

> 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.

This patch is missing a Signed-off-by line. Without, I unfortunately can't take the code upstream yet. See http://wiki.qemu.org/Contribute/SubmitAPatch for details.

> 
> ---
> target-ppc/translate.c |    5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 9b3f90c..a60dbe9 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -3886,7 +3886,6 @@ 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();

This code needs reindenting.

>             TCGv t1 = tcg_temp_new();
>             tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL);
> @@ -3895,9 +3894,9 @@ static void gen_mtmsr(DisasContext *ctx)
>             tcg_temp_free(t1);
>             gen_helper_store_msr(t0);
>             tcg_temp_free(t0);
> -        } else
> -#endif
> +#else
>             gen_helper_store_msr(cpu_gpr[rS(ctx->opcode)]);

This does too

Functionality-wise it looks good to me.


Alex


[-- Attachment #2: Type: text/html, Size: 2689 bytes --]

Re: [Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Richard Henderson]

On 05/31/2011 07:56 AM, Nathan Whitehorn wrote:
>  #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.


r~

Re: [Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Nathan Whitehorn]

On 05/31/11 12:40, Richard Henderson wrote:
> On 05/31/2011 07:56 AM, Nathan Whitehorn wrote:
>>   #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].
-Nathan

Re: [Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Alexander Graf]

On 04.06.2011, at 21:28, Nathan Whitehorn wrote:

> On 05/31/11 12:40, Richard Henderson wrote:
>> On 05/31/2011 07:56 AM, Nathan Whitehorn wrote:
>>>  #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].

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 :).


Alex

Re: [Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Nathan Whitehorn]

On 06/05/11 04:00, Alexander Graf wrote:
> On 04.06.2011, at 21:28, Nathan Whitehorn wrote:
>
>> On 05/31/11 12:40, Richard Henderson wrote:
>>> On 05/31/2011 07:56 AM, Nathan Whitehorn wrote:
>>>>   #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].
> 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

Re: [Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Nathan Whitehorn]

On 06/05/11 08:33, Nathan Whitehorn wrote:
> On 06/05/11 04:00, Alexander Graf wrote:
>> On 04.06.2011, at 21:28, Nathan Whitehorn wrote:
>>
>>> On 05/31/11 12:40, Richard Henderson wrote:
>>>> On 05/31/2011 07:56 AM, Nathan Whitehorn wrote:
>>>>>   #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].
>> 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

Also, those instructions *are* available. That's the only way to turn 
64-bit mode on, for instance: you need to grab the MSR, set one of the 
high bits, then mtmsrd (which is a 64-bit instruction).
-Nathan

Re: [Qemu-devel] [PATCH] ppc64: fix mtmsr behavior on 64-bit targets

[Alexander Graf]

On 05.06.2011, at 15:33, Nathan Whitehorn wrote:

> On 06/05/11 04:00, Alexander Graf wrote:
>> On 04.06.2011, at 21:28, Nathan Whitehorn wrote:
>> 
>>> On 05/31/11 12:40, Richard Henderson wrote:
>>>> On 05/31/2011 07:56 AM, Nathan Whitehorn wrote:
>>>>>  #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].
>> 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].

Hrm, right. I don't remember the details, but back when I implemented book3s_32 guest support on book3s_64 KVM, I was running guest code with MSR_SF enabled which resulted in very odd results with negative values. Either way, I'll gladly compare patches to real hardware and the spec whenever you fix one of the misinterpretations of msr_sf :).


Alex

[Qemu-devel] [PATCH2] ppc64: fix mtmsr behavior on 64-bit targets

[Nathan Whitehorn]

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 <nwhitehorn@freebsd.org>
---
  target-ppc/translate.c |    5 ++---
  1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 59aef85..38d2e2e 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -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)]);
-            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();
+        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);
+        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);

Re: [Qemu-devel] [PATCH2] ppc64: fix mtmsr behavior on 64-bit targets

[Alexander Graf]

Am 12.06.2011 um 17:49 schrieb Nathan Whitehorn <nwhitehorn@freebsd.org>:

> 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 <nwhitehorn@freebsd.org>
> ---
> target-ppc/translate.c |    5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 59aef85..38d2e2e 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -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)]);
> -            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 :). 

> +        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 :)


Alex

> +        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);
> 

Re: [Qemu-devel] [PATCH2] ppc64: fix mtmsr behavior on 64-bit targets

[Nathan Whitehorn]

On 06/13/11 05:20, Alexander Graf wrote:
>
>
>
> Am 12.06.2011 um 17:49 schrieb Nathan Whitehorn<nwhitehorn@freebsd.org>:
>
>> 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<nwhitehorn@freebsd.org>
>> ---
>> target-ppc/translate.c |    5 ++---
>> 1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> index 59aef85..38d2e2e 100644
>> --- a/target-ppc/translate.c
>> +++ b/target-ppc/translate.c
>> @@ -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)]);
>> -            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.
>> +        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 :)
-Nathan

Re: [Qemu-devel] [PATCH2] ppc64: fix mtmsr behavior on 64-bit targets

[Alexander Graf]

On 13.06.2011, at 14:52, Nathan Whitehorn wrote:

> On 06/13/11 05:20, Alexander Graf wrote:
>> 
>> 
>> 
>> Am 12.06.2011 um 17:49 schrieb Nathan Whitehorn<nwhitehorn@freebsd.org>:
>> 
>>> 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<nwhitehorn@freebsd.org>
>>> ---
>>> target-ppc/translate.c |    5 ++---
>>> 1 files changed, 2 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>> index 59aef85..38d2e2e 100644
>>> --- a/target-ppc/translate.c
>>> +++ b/target-ppc/translate.c
>>> @@ -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)]);
>>> -            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.

It makes sure that we know the current instruction pointer in case an interrupt gets delivered for example. There's a hack to not require this for data protection interrupts, but others might. In general, we need this whenever we call a helper function that could manually raise an interrupt. helper_store_msr does this:

void helper_store_msr (target_ulong val)
{
    val = hreg_store_msr(env, val, 0);
    if (val != 0) {
        env->interrupt_request |= CPU_INTERRUPT_EXITTB;
        helper_raise_exception(val);
    }
}

So it needs to come before any call to gen_helper_store_msr :).

>>> +        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 :)

It's a new tcg call that makes the above code a lot easier. Do you want to go for v3 without changing deposit then? That would be awesome :).


Alex