[Qemu-devel] [PATCH 2/3] target-mips:enabling of 64 bit user mode and floating point operations MIPS_HFLAG_UX is included in env->hflags so that the address computation for LD instruction does not treated as 32 bit code see gen_op_addr_add() in t

[Khansa Butt]

On Thu, Dec 29, 2011 at 4:17 PM, Andreas Färber <address@hidden> wrote:
> Am 29.12.2011 08:55, schrieb Khansa Butt:
>> On Fri, Dec 9, 2011 at 5:04 AM, Andreas Färber <address@hidden> wrote:
>>>> +    /* if cpu has FPU, MIPS_HFLAG_F64 must be included in env->hflags
>>>> +       so that floating point operations can be emulated */
>>>> +    env->active_fpu.fcr0 = env->cpu_model->CP1_fcr0;
>>>>      if (env->active_fpu.fcr0 & (1 << FCR0_F64)) {
>>>>          env->hflags |= MIPS_HFLAG_F64;
>>>>      }
>>>
>>> Nack. env->active_fpu.fcr0 gets initialized in translate_init.c based on
>>> cpu_model->CR1_fcr0, where FCR0_F64 is set only for 24Kf, 34Kf,
>>> MIPS64R2-generic. TARGET_ABI_MIPSN64 linux-user defaults to 20Kc. So it
>>> seems to rather be an issue of using the right -cpu parameter or
>>> changing the default for n64. [cc'ing Nathan, who introduced the if]
>>
>> The reason why I add this line " env->active_fpu.fcr0 =
>> env->cpu_model->CP1_fcr0" is as follows
>> in translate_init.c fpu_init() initializes active_fpu for given cpu
>> model afterwards cpu_reset() reset the values
>> to zero using this
>> memset(env, 0, offsetof(CPUMIPSState, breakpoints));
>> so whatever the value of  cpu_model->CR1_fcr0 was , the value of
>> env->active_fpu.fcr0 will be zero now  thats why I add above
>> line to retrieve the correct env->active_fpu.fcr0 value according to
>> CPU model( whether it is 24Kf or 20Kc or something else)
>> During the development of mips64-linux-user I observed this issue. I
>> gave qemu-mips64 command with -cpu option equal to MIPS64R2-generic
>> and an illegal instruction error occurred, so I used above hunk.
>
> Well, that sounds like a different and more generic problem that
> shouldn't be fixed inside CONFIG_USER_ONLY && TARGET_MIPS64.
> And your reasoning should've definitely been in the commit message!
>
> The problem here is not whether the patches observably work for you but
> whether it's the correct way to fix it. "We did this because it works
> for us" is never a convincing justification of a change.
> If it doesn't work for you in linux-user it won't work in softmmu
> either, so doing that before #if defined(CONFIG_USER_ONLY) where lots of
> env->cpu_model stuff is being copyied (esp. before env->HABITS to honor
> mips_def_t order) seems better.
>
> Also, given your observation, does it even make sense for
> cpu_mips_init() to call fpu_init() when all CPUState members it
> initializes get cleared in cpu_reset()? Maybe just move that call into
> cpu_reset() and rename it to fpu_reset()? mmu_init() and mvp_init() seem
> okay by contrast.

why cpu_reset() calls memset? it does not reset all the members of CPUState only
those which are in the range of offsetof(CPUMIPSState, breakpoints).
what if I remove
memset line?

>
> When you've figured this out, please again put it into a separate patch
> titled, e.g., "target-mips: Fix FPU reset" with appropriate explanation.
>
> Andreas