* 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
@ 2011-12-29 7:55 Khansa Butt
2011-12-29 11:17 ` Andreas Färber
0 siblings, 1 reply; 6+ messages in thread
From: Khansa Butt @ 2011-12-29 7:55 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, Richard Henderson
On Fri, Dec 9, 2011 at 5:04 AM, Andreas Färber <andreas.faerber@web.de> wrote:
> Thanks for extending the commit description. Please see this for a
> template though:
>
> http://live.gnome.org/Git/CommitMessages
>
> Looks like there's an empty line missing between subject and description
> (and the space after "target-mips:").
>
> Am 08.12.2011 06:25, schrieb khansa@kics.edu.pk:
>> From: Khansa Butt <khansa@kics.edu.pk>
>>
>>
>> Signed-off-by: Abdul Qadeer <qadeer@kics.edu.pk>
>> ---
>> target-mips/translate.c | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-mips/translate.c b/target-mips/translate.c
>> index d5b1c76..452a63b 100644
>> --- a/target-mips/translate.c
>> +++ b/target-mips/translate.c
>> @@ -12779,6 +12779,10 @@ void cpu_reset (CPUMIPSState *env)
>> env->hflags |= MIPS_HFLAG_FPU;
>> }
>> #ifdef TARGET_MIPS64
>> + env->hflags |= MIPS_HFLAG_UX;
>
> So for those of us not knowing mips, it's defined as:
>
> #define MIPS_HFLAG_UX 0x00200 /* 64-bit user mode */
>
> The code above is inside CONFIG_USER_ONLY, so this looks right for n64
> but not for n32 ABI.
>
> If you put this into its own patch with a description of
>
> ---8<---
> target-mips: Enable 64 bit user mode for n64
>
> For user mode n64 ABI emulation, MIPS_HFLAG_UX is included in
> env->hflags so that the address computation for LD instruction does not
> get treated as 32 bit code, see gen_op_addr_add() in translate.c.
>
> Signed-off-by: Abdul Qadeer <qadeer@kics.edu.pk>
> Signed-off-by: (you)
> ---8<---
>
> and make it depend on TARGET_ABI_MIPSN64 then I will happily add my
> Acked-by.
>
>
>> + /* 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.
>
> Andreas
^ permalink raw reply [flat|nested] 6+ messages in thread* 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
2011-12-29 7:55 [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
@ 2011-12-29 11:17 ` Andreas Färber
[not found] ` <CAAoJSP5wPz_rL9x0ZZBZJz1A9RuGhohLKSg_nChyo96mvuJ70w@mail.gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Färber @ 2011-12-29 11:17 UTC (permalink / raw)
To: Khansa Butt; +Cc: qemu-devel, Richard Henderson
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* 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
@ 2011-12-29 8:06 Khansa Butt
0 siblings, 0 replies; 6+ messages in thread
From: Khansa Butt @ 2011-12-29 8:06 UTC (permalink / raw)
To: Richard Henderson; +Cc: Andreas Färber, qemu-devel
On Wed, Dec 14, 2011 at 10:05 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 12/08/2011 04:04 PM, Andreas Färber 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]
>
> That said, there's still something missing, e.g. MIPS_HFLAG_COP1X.
> My first guess is simply
>
> if (env->insn_flags & (ISA_MIPS32 | ISA_MIPS4)) {
> env->hflags |= MIPS_HFLAG_COP1X;
> }
I don't understand why we add above lines. I think this issue is some
what related to cpu_model not with ISA
I've explained why I add "env->active_fpu.fcr0 =
env->cpu_model->CP1_fcr0;" line in reply to this patch to Andreas
Färber
and cc'ed you as well
>
> immediately after this MIPS64 hunk.
>
>
> r~
^ permalink raw reply [flat|nested] 6+ messages in thread[parent not found: <CAAoJSP677G-bsiZ3W=D_O5DiwyKv=XtEWgU_6wH2LztGBK2s8A@mail.gmail.com>]
* 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
[not found] <CAAoJSP677G-bsiZ3W=D_O5DiwyKv=XtEWgU_6wH2LztGBK2s8A@mail.gmail.com>
@ 2011-12-31 11:54 ` Andreas Färber
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Färber @ 2011-12-31 11:54 UTC (permalink / raw)
To: Khansa Butt; +Cc: qemu-devel Developers
Am 31.12.2011 08:42, schrieb Khansa Butt:
> On Fri, Dec 9, 2011 at 5:04 AM, Andreas Färber <andreas.faerber@web.de> wrote:
>> Am 08.12.2011 06:25, schrieb khansa@kics.edu.pk:
>>> diff --git a/target-mips/translate.c b/target-mips/translate.c
>>> index d5b1c76..452a63b 100644
>>> --- a/target-mips/translate.c
>>> +++ b/target-mips/translate.c
>>> @@ -12779,6 +12779,10 @@ void cpu_reset (CPUMIPSState *env)
>>> env->hflags |= MIPS_HFLAG_FPU;
>>> }
>>> #ifdef TARGET_MIPS64
>>> + env->hflags |= MIPS_HFLAG_UX;
>>
>> So for those of us not knowing mips, it's defined as:
>>
>> #define MIPS_HFLAG_UX 0x00200 /* 64-bit user mode */
>>
>> The code above is inside CONFIG_USER_ONLY, so this looks right for n64
>> but not for n32 ABI.
>>
>> If you put this into its own patch with a description of
>>
>> ---8<---
>> target-mips: Enable 64 bit user mode for n64
>>
>> For user mode n64 ABI emulation, MIPS_HFLAG_UX is included in
>> env->hflags so that the address computation for LD instruction does not
>> get treated as 32 bit code, see gen_op_addr_add() in translate.c.
>>
>> Signed-off-by: Abdul Qadeer <qadeer@kics.edu.pk>
>> Signed-off-by: (you)
>> ---8<---
>>
>> and make it depend on TARGET_ABI_MIPSN64 then I will happily add my
>> Acked-by.
>>
> Why this is necessary to put "env->hflags |= MIPS_HFLAG_UX;" line under
> TARGET_ABI_MIPSN64? as this was already put under #if TARGET_MIPS64, is not
> it suffient?
You're right. I was under the impression that both n32 and n64 were
based off mips64, but mipsn32 is in fact based off mips. Adding NUBI64
support (as opposed to NUBI64W) would then probably be based off mips as
well then. Quite confusing.
So yes, no need to add #if defined(TARGET_ABI_MIPSN64) there, but do put
it in its own patch with a description explaining why.
Andreas
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-12-31 11:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-29 7:55 [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
2011-12-29 11:17 ` Andreas Färber
[not found] ` <CAAoJSP5wPz_rL9x0ZZBZJz1A9RuGhohLKSg_nChyo96mvuJ70w@mail.gmail.com>
2011-12-30 7:52 ` Khansa Butt
2011-12-30 12:39 ` Andreas Färber
-- strict thread matches above, loose matches on Subject: below --
2011-12-29 8:06 Khansa Butt
[not found] <CAAoJSP677G-bsiZ3W=D_O5DiwyKv=XtEWgU_6wH2LztGBK2s8A@mail.gmail.com>
2011-12-31 11:54 ` Andreas Färber
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).