Re: [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

["Andreas Färber" <andreas.faerber@web.de>]

Am 29.12.2011 08:55, schrieb Khansa Butt:
> On Fri, Dec 9, 2011 at 5:04 AM, Andreas Färber <andreas.faerber@web.de> 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.

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