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<address@hidden>:
>> 
>>> 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<address@hidden>
>>> ---
>>> 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