[Qemu-devel] [PATCH 1/3] Fix my email address

[Qemu-devel] [PATCH 1/3] Fix my email address

[Dongxue Zhang]

Fix my email address, last time it's wrong.

Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
---
 target-mips/dsp_helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
index 14daf91..536032b 100644
--- a/target-mips/dsp_helper.c
+++ b/target-mips/dsp_helper.c
@@ -2,7 +2,7 @@
  * MIPS ASE DSP Instruction emulation helpers for QEMU.
  *
  * Copyright (c) 2012  Jia Liu <proljc@gmail.com>
- *                     Dongxue Zhang <elat.era@gmail.com>
+ *                     Dongxue Zhang <elta.era@gmail.com>
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
  * License as published by the Free Software Foundation; either
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

[Dongxue Zhang]

The immediate value is 9bits, should sign-extend to 16bits. The return value to
register should sign-extend to target_long, as Richard says, removing an
unnecessary cast works fun.

Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
---
 target-mips/translate.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 65e6725..1701ca3 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -13769,9 +13769,10 @@ static void gen_mipsdsp_bitinsn(CPUMIPSState *env, DisasContext *ctx,
             check_dsp(ctx);
             {
                 imm = (ctx->opcode >> 16) & 0x03FF;
+                imm = (int16_t)(imm << 6) >> 6;
                 tcg_gen_movi_tl(cpu_gpr[ret], \
                                 (target_long)((int32_t)imm << 16 | \
-                                (uint32_t)(uint16_t)imm));
+                                (uint16_t)imm));
             }
             break;
         case OPC_REPLV_PH:
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/3] Fix-gen_HILO-to-make-it-adapt-each-arch-which-use-acc

[Dongxue Zhang]

The old gen_HILO was used by many arch, and they use acc[0] only. But now we
know the arch with dsp will have 4 acc reigster. So we need Fix gen_HILO.

When we add arch mipsdsp, we changed the gen_HILO, but now I found it may cause
a bug. Because we just get index of acc register from opcode, other arch to use
gen_HILO my have value at that bit, so, a bug cased.

Now I take acc to be a param of gen_HILO, other arch use gen_HILO with that
param set to 0, and dsp arch will set the value as which acc register it
selected.

TARGET_MIPS64 in gen_HILO changed into (TARGET_LONG_SIZE == 8), on 64bits arch,
acc register value sign-extended to 64bit value, save into register.

I don't have test log but there is something provide from manual. Instruction
mfhi in the two manual:

MD00375-2B-MIPS64DSP-AFP-02.34:
31     26  25      21   20 16  15 11  10   6   5
SPECIAL      0     ac     0      rd      0      MFHI
000000      000         00000          00000   010000

MD00764-2B-microMIPS32DSP-AFP-02.34:
31    26   25 21 20  16 15 14  13     6  5
POOL32A      0     rs    ac      MFHI    POOL32Axf
000000     00000               00000001    111100

Current gen_HILO get acc from bit 21/22, and check_dsp. If arch is
microMIPS32DSP, then a bug case. So I changed gen_HILO function for furture add
other arch with dsp.

MFHI MTHI MFLO MTLO of mipsdsp have been tested, they worked fun.

Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
---
 target-mips/translate.c |   64 ++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 1701ca3..2b0972b 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -2571,10 +2571,10 @@ static void gen_shift (CPUMIPSState *env, DisasContext *ctx, uint32_t opc,
 }
 
 /* Arithmetic on HI/LO registers */
-static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
+static void gen_HILO(DisasContext *ctx, uint32_t opc, int reg,
+                     unsigned int acc)
 {
     const char *opn = "hilo";
-    unsigned int acc;
 
     if (reg == 0 && (opc == OPC_MFHI || opc == OPC_MFLO)) {
         /* Treat as NOP. */
@@ -2582,19 +2582,9 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
         return;
     }
 
-    if (opc == OPC_MFHI || opc == OPC_MFLO) {
-        acc = ((ctx->opcode) >> 21) & 0x03;
-    } else {
-        acc = ((ctx->opcode) >> 11) & 0x03;
-    }
-
-    if (acc != 0) {
-        check_dsp(ctx);
-    }
-
     switch (opc) {
     case OPC_MFHI:
-#if defined(TARGET_MIPS64)
+#if (TARGET_LONG_SIZE == 8)
         if (acc != 0) {
             tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[acc]);
         } else
@@ -2605,7 +2595,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
         opn = "mfhi";
         break;
     case OPC_MFLO:
-#if defined(TARGET_MIPS64)
+#if (TARGET_LONG_SIZE == 8)
         if (acc != 0) {
             tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[acc]);
         } else
@@ -2617,7 +2607,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
         break;
     case OPC_MTHI:
         if (reg != 0) {
-#if defined(TARGET_MIPS64)
+#if (TARGET_LONG_SIZE == 8)
             if (acc != 0) {
                 tcg_gen_ext32s_tl(cpu_HI[acc], cpu_gpr[reg]);
             } else
@@ -2632,7 +2622,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
         break;
     case OPC_MTLO:
         if (reg != 0) {
-#if defined(TARGET_MIPS64)
+#if (TARGET_LONG_SIZE == 8)
             if (acc != 0) {
                 tcg_gen_ext32s_tl(cpu_LO[acc], cpu_gpr[reg]);
             } else
@@ -10134,7 +10124,7 @@ static int decode_mips16_opc (CPUMIPSState *env, DisasContext *ctx,
             gen_logic(env, ctx, OPC_NOR, rx, ry, 0);
             break;
         case RR_MFHI:
-            gen_HILO(ctx, OPC_MFHI, rx);
+            gen_HILO(ctx, OPC_MFHI, rx, 0);
             break;
         case RR_CNVT:
             switch (cnvt_op) {
@@ -10166,7 +10156,7 @@ static int decode_mips16_opc (CPUMIPSState *env, DisasContext *ctx,
             }
             break;
         case RR_MFLO:
-            gen_HILO(ctx, OPC_MFLO, rx);
+            gen_HILO(ctx, OPC_MFLO, rx, 0);
             break;
 #if defined (TARGET_MIPS64)
         case RR_DSRA:
@@ -10922,11 +10912,11 @@ static void gen_pool16c_insn (CPUMIPSState *env, DisasContext *ctx, int *is_bran
         break;
     case MFHI16 + 0:
     case MFHI16 + 1:
-        gen_HILO(ctx, OPC_MFHI, uMIPS_RS5(ctx->opcode));
+        gen_HILO(ctx, OPC_MFHI, uMIPS_RS5(ctx->opcode), 0);
         break;
     case MFLO16 + 0:
     case MFLO16 + 1:
-        gen_HILO(ctx, OPC_MFLO, uMIPS_RS5(ctx->opcode));
+        gen_HILO(ctx, OPC_MFLO, uMIPS_RS5(ctx->opcode), 0);
         break;
     case BREAK16:
         generate_exception(ctx, EXCP_BREAK);
@@ -11286,16 +11276,16 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs,
     case 0x35:
         switch (minor) {
         case MFHI32:
-            gen_HILO(ctx, OPC_MFHI, rs);
+            gen_HILO(ctx, OPC_MFHI, rs, 0);
             break;
         case MFLO32:
-            gen_HILO(ctx, OPC_MFLO, rs);
+            gen_HILO(ctx, OPC_MFLO, rs, 0);
             break;
         case MTHI32:
-            gen_HILO(ctx, OPC_MTHI, rs);
+            gen_HILO(ctx, OPC_MTHI, rs, 0);
             break;
         case MTLO32:
-            gen_HILO(ctx, OPC_MTLO, rs);
+            gen_HILO(ctx, OPC_MTLO, rs, 0);
             break;
         default:
             goto pool32axf_invalid;
@@ -14447,12 +14437,30 @@ static void decode_opc (CPUMIPSState *env, DisasContext *ctx, int *is_branch)
             break;
         case OPC_MFHI:          /* Move from HI/LO */
         case OPC_MFLO:
-            gen_HILO(ctx, op1, rd);
-            break;
+            {
+                unsigned int acc;
+                acc = ((ctx->opcode) >> 21) & 0x03;
+
+                if (acc != 0) {
+                    check_dsp(ctx);
+                }
+
+                gen_HILO(ctx, op1, rd, acc);
+                break;
+            }
         case OPC_MTHI:
         case OPC_MTLO:          /* Move to HI/LO */
-            gen_HILO(ctx, op1, rs);
-            break;
+            {
+                unsigned int acc;
+                acc = ((ctx->opcode) >> 11) & 0x03;
+
+                if (acc != 0) {
+                    check_dsp(ctx);
+                }
+
+                gen_HILO(ctx, op1, rs, acc);
+                break;
+            }
         case OPC_PMON:          /* Pmon entry point, also R4010 selsl */
 #ifdef MIPS_STRICT_STANDARD
             MIPS_INVAL("PMON / selsl");
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

[Jovanovic, Petar]

lgtm, though I wish there was a test for this in repl_ph.c.

+ cc Aurelien J.

Petar
________________________________________
From: Dongxue Zhang [elta.era@gmail.com]
Sent: Tuesday, December 11, 2012 3:28 PM
To: qemu-devel@nongnu.org
Cc: chenwj@iis.sinica.edu.tw; Jovanovic, Petar; rth@twiddle.net; Dongxue Zhang
Subject: [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

The immediate value is 9bits, should sign-extend to 16bits. The return value to
register should sign-extend to target_long, as Richard says, removing an
unnecessary cast works fun.

Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
---
 target-mips/translate.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 65e6725..1701ca3 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -13769,9 +13769,10 @@ static void gen_mipsdsp_bitinsn(CPUMIPSState *env, DisasContext *ctx,
             check_dsp(ctx);
             {
                 imm = (ctx->opcode >> 16) & 0x03FF;
+                imm = (int16_t)(imm << 6) >> 6;
                 tcg_gen_movi_tl(cpu_gpr[ret], \
                                 (target_long)((int32_t)imm << 16 | \
-                                (uint32_t)(uint16_t)imm));
+                                (uint16_t)imm));
             }
             break;
         case OPC_REPLV_PH:
--
1.7.10.4

Re: [Qemu-devel] [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

[Markus Armbruster]

Please-separate-words-with-spaces-in-your-subject-line-thank-you-:)

Re: [Qemu-devel] [PATCH 1/3] Fix my email address

[Stefan Weil]

Am 11.12.2012 15:28, schrieb Dongxue Zhang:
> Fix my email address, last time it's wrong.
>
> Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
> ---
>   target-mips/dsp_helper.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index 14daf91..536032b 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -2,7 +2,7 @@
>    * MIPS ASE DSP Instruction emulation helpers for QEMU.
>    *
>    * Copyright (c) 2012  Jia Liu <proljc@gmail.com>
> - *                     Dongxue Zhang <elat.era@gmail.com>
> + *                     Dongxue Zhang <elta.era@gmail.com>
>    * This library is free software; you can redistribute it and/or
>    * modify it under the terms of the GNU Lesser General Public
>    * License as published by the Free Software Foundation; either


Maybe removing the email address completely would be even
better. There is already an email address in eachcommit,
some addresses are in file MAINTAINERS, and you can also
add a personal entry in the QEMU wiki (linked to this URL:
http://wiki.qemu.org/Contribute/StartHere#Developers_and_Maintainers).

Currently there are copyright statements with and without
email address. I personally prefer those without, especially
when email address are known to be volatile. Even email
addresses at redhat.com (or other companies) are only valid
as long as the owner is related to RedHat.

Regards
Stefan Weil

Re: [Qemu-devel] [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

[Andreas Färber]

Am 11.12.2012 16:13, schrieb Markus Armbruster:
> Please-separate-words-with-spaces-in-your-subject-line-thank-you-:)

Also all three patches should have a "target-mips: " prefix in the
subject to make clear what they touch and who must review and later
commit these.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

[Dongxue Zhang]

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

Thanks for your review, now I know this problem but don't have gcc can
compile repl_ph instruction. I will build a gcc and resend this patch later.


2012/12/12 Andreas Färber <afaerber@suse.de>

> Am 11.12.2012 16:13, schrieb Markus Armbruster:
> > Please-separate-words-with-spaces-in-your-subject-line-thank-you-:)
>
> Also all three patches should have a "target-mips: " prefix in the
> subject to make clear what they touch and who must review and later
> commit these.
>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

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

Re: [Qemu-devel] [PATCH 1/3] Fix my email address

[Stefan Weil]

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

Am 12.12.2012 08:24, schrieb Dongxue Zhang:
>
> Sorry I don't really catch your idea. This email address is wrong and 
> i just
> want to correct it. I just want to do something to qemu. When I add
> target-mips/dsp_helper.c I got lots of help from other people, so I 
> also want to
> help others who need help from me. But with a wrong email, they send 
> mail to me
> will never get reply, this will make me looks like a cold guy, It's 
> not what I
> want. So I want to correct my email address. :)

Of course fixing a wrong email address is better than keeping a wrong 
address
in the code. But will you also fix the address when you change it some day,
maybe years later? What about old versions of QEMU? They will still show an
old email address. Most email addresses have a limit life time.

That's why I think that _no_ email address in source code might be better.
It is still possible to find authors by looking at the Signed-off-by in the
commit message or by searching the QEMU wiki or other sources of 
information.

Stefan Weil


>
>
> 2012/12/12 Stefan Weil <sw@weilnetz.de <mailto:sw@weilnetz.de>>
>
>     Am 11.12.2012 15:28, schrieb Dongxue Zhang:
>
>         Fix my email address, last time it's wrong.
>
>         Signed-off-by: Dongxue Zhang <elta.era@gmail.com
>         <mailto:elta.era@gmail.com>>
>         ---
>           target-mips/dsp_helper.c |    2 +-
>           1 file changed, 1 insertion(+), 1 deletion(-)
>
>         diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
>         index 14daf91..536032b 100644
>         --- a/target-mips/dsp_helper.c
>         +++ b/target-mips/dsp_helper.c
>         @@ -2,7 +2,7 @@
>            * MIPS ASE DSP Instruction emulation helpers for QEMU.
>            *
>            * Copyright (c) 2012  Jia Liu <proljc@gmail.com
>         <mailto:proljc@gmail.com>>
>         - *                     Dongxue Zhang <elat.era@gmail.com
>         <mailto:elat.era@gmail.com>>
>         + *                     Dongxue Zhang <elta.era@gmail.com
>         <mailto:elta.era@gmail.com>>
>            * This library is free software; you can redistribute it and/or
>            * modify it under the terms of the GNU Lesser General Public
>            * License as published by the Free Software Foundation; either
>
>
>
>     Maybe removing the email address completely would be even
>     better. There is already an email address in eachcommit,
>     some addresses are in file MAINTAINERS, and you can also
>     add a personal entry in the QEMU wiki (linked to this URL:
>     http://wiki.qemu.org/Contribute/StartHere#Developers_and_Maintainers).
>
>     Currently there are copyright statements with and without
>     email address. I personally prefer those without, especially
>     when email address are known to be volatile. Even email
>     addresses at redhat.com <http://redhat.com> (or other companies)
>     are only valid
>     as long as the owner is related to RedHat.
>
>     Regards
>     Stefan Weil
>
>
>


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

Re: [Qemu-devel] [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

[Jovanovic, Petar]

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

What does it mean you do not have gcc that can compile repl.ph instruction?
How do you test your patches?

If you want to append the repl_ph.c, you may want to add something like this:

diff --git a/tests/tcg/mips/mips32-dsp/repl_ph.c b/tests/tcg/mips/mips32-dsp/repl_ph.c
index 2107495..07d137d
--- a/tests/tcg/mips/mips32-dsp/repl_ph.c
+++ b/tests/tcg/mips/mips32-dsp/repl_ph.c
@@ -19,5 +19,12 @@ int main()
         );
     assert(rd == result);

+    result = 0xFFFFFFFF;
+    __asm
+        ("repl.ph %0, -1\n\t"
+         : "=r"(rd)
+        );
+    assert(rd == result);
+
     return 0;
 }


Petar

p.s. +cc Aurelien J.
________________________________
From: Dongxue Zhang [elta.era@gmail.com]
Sent: Wednesday, December 12, 2012 7:43 AM
To: Andreas Färber
Cc: Markus Armbruster; Jovanovic, Petar; qemu-devel@nongnu.org; 陳韋任; rth@twiddle.net
Subject: Re: [Qemu-devel] [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

Thanks for your review, now I know this problem but don't have gcc can compile repl_ph instruction. I will build a gcc and resend this patch later.


2012/12/12 Andreas Färber <afaerber@suse.de<mailto:afaerber@suse.de>>
Am 11.12.2012 16:13, schrieb Markus Armbruster:
> Please-separate-words-with-spaces-in-your-subject-line-thank-you-:)

Also all three patches should have a "target-mips: " prefix in the
subject to make clear what they touch and who must review and later
commit these.

Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


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

Re: [Qemu-devel] [PATCH 1/3] Fix my email address

[陳韋任 (Wei-Ren Chen)]

On Wed, Dec 12, 2012 at 11:02:59PM +0100, Stefan Weil wrote:
> Am 12.12.2012 08:24, schrieb Dongxue Zhang:
> 
> 
>     Sorry I don't really catch your idea. This email address is wrong and i
>     just
>     want to correct it. I just want to do something to qemu. When I add
>     target-mips/dsp_helper.c I got lots of help from other people, so I also
>     want to
>     help others who need help from me. But with a wrong email, they send mail
>     to me
>     will never get reply, this will make me looks like a cold guy, It's not
>     what I
>     want. So I want to correct my email address. :)
> 
> 
> Of course fixing a wrong email address is better than keeping a wrong address
> in the code. But will you also fix the address when you change it some day,
> maybe years later? What about old versions of QEMU? They will still show an
> old email address. Most email addresses have a limit life time.

  Well... Just let him fix his mail address (he want it in the source
code). Maybe we can discuss if we want the mail address in the code
while reviewing new coming patch next time. ;)

Regards,
chenwj
 
-- 
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj

Re: [Qemu-devel] [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long

[aurelien]

On Tue, Dec 11, 2012 at 03:06:35PM +0000, Jovanovic, Petar wrote:
> lgtm, though I wish there was a test for this in repl_ph.c.
> 
> + cc Aurelien J.
> 
> Petar
> ________________________________________
> From: Dongxue Zhang [elta.era@gmail.com]
> Sent: Tuesday, December 11, 2012 3:28 PM
> To: qemu-devel@nongnu.org
> Cc: chenwj@iis.sinica.edu.tw; Jovanovic, Petar; rth@twiddle.net; Dongxue Zhang
> Subject: [PATCH 2/3] Make-repl_ph-to-sign-extended-to-target_long
> 
> The immediate value is 9bits, should sign-extend to 16bits. The return value to
> register should sign-extend to target_long, as Richard says, removing an
> unnecessary cast works fun.
> 
> Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
> ---
>  target-mips/translate.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 65e6725..1701ca3 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -13769,9 +13769,10 @@ static void gen_mipsdsp_bitinsn(CPUMIPSState *env, DisasContext *ctx,
>              check_dsp(ctx);
>              {
>                  imm = (ctx->opcode >> 16) & 0x03FF;
> +                imm = (int16_t)(imm << 6) >> 6;
>                  tcg_gen_movi_tl(cpu_gpr[ret], \
>                                  (target_long)((int32_t)imm << 16 | \
> -                                (uint32_t)(uint16_t)imm));
> +                                (uint16_t)imm));
>              }
>              break;
>          case OPC_REPLV_PH:

Thanks, I have applied the patch after fixing the subject.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 3/3] Fix-gen_HILO-to-make-it-adapt-each-arch-which-use-acc

[Aurelien Jarno]

On Tue, Dec 11, 2012 at 10:28:30PM +0800, Dongxue Zhang wrote:
> The old gen_HILO was used by many arch, and they use acc[0] only. But now we
> know the arch with dsp will have 4 acc reigster. So we need Fix gen_HILO.
> 
> When we add arch mipsdsp, we changed the gen_HILO, but now I found it may cause
> a bug. Because we just get index of acc register from opcode, other arch to use
> gen_HILO my have value at that bit, so, a bug cased.

The MIPS32, MIPS64 and even the R4000 and R4400 bits define these bits
as being 0. They should not be ignored as they must be 0.

What is wrong there though is that a DSP exception is generated instead
of a reserved one. This is a more generic problem than MFHI/MFLO, and
should be fixed by the following patch:

http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg01497.html

I am working on a rework though, given the comments this patch got.

> Now I take acc to be a param of gen_HILO, other arch use gen_HILO with that
> param set to 0, and dsp arch will set the value as which acc register it
> selected.
> 
> TARGET_MIPS64 in gen_HILO changed into (TARGET_LONG_SIZE == 8), on 64bits arch,
> acc register value sign-extended to 64bit value, save into register.

I don't get whey you want to change TARGET_MIPS64 in (TARGET_LONG_SIZE
== 8). What's the goal of that? 

> I don't have test log but there is something provide from manual. Instruction
> mfhi in the two manual:
> 
> MD00375-2B-MIPS64DSP-AFP-02.34:
> 31     26  25      21   20 16  15 11  10   6   5
> SPECIAL      0     ac     0      rd      0      MFHI
> 000000      000         00000          00000   010000
> 
> MD00764-2B-microMIPS32DSP-AFP-02.34:
> 31    26   25 21 20  16 15 14  13     6  5
> POOL32A      0     rs    ac      MFHI    POOL32Axf
> 000000     00000               00000001    111100
> 
> Current gen_HILO get acc from bit 21/22, and check_dsp. If arch is
> microMIPS32DSP, then a bug case. So I changed gen_HILO function for furture add
> other arch with dsp.
> 
> MFHI MTHI MFLO MTLO of mipsdsp have been tested, they worked fun.
> 
> Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
> ---
>  target-mips/translate.c |   64 ++++++++++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 28 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 1701ca3..2b0972b 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -2571,10 +2571,10 @@ static void gen_shift (CPUMIPSState *env, DisasContext *ctx, uint32_t opc,
>  }
>  
>  /* Arithmetic on HI/LO registers */
> -static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
> +static void gen_HILO(DisasContext *ctx, uint32_t opc, int reg,
> +                     unsigned int acc)
>  {
>      const char *opn = "hilo";
> -    unsigned int acc;
>  
>      if (reg == 0 && (opc == OPC_MFHI || opc == OPC_MFLO)) {
>          /* Treat as NOP. */
> @@ -2582,19 +2582,9 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
>          return;
>      }
>  
> -    if (opc == OPC_MFHI || opc == OPC_MFLO) {
> -        acc = ((ctx->opcode) >> 21) & 0x03;
> -    } else {
> -        acc = ((ctx->opcode) >> 11) & 0x03;
> -    }
> -
> -    if (acc != 0) {
> -        check_dsp(ctx);
> -    }
> -
>      switch (opc) {
>      case OPC_MFHI:
> -#if defined(TARGET_MIPS64)
> +#if (TARGET_LONG_SIZE == 8)
>          if (acc != 0) {
>              tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[acc]);
>          } else
> @@ -2605,7 +2595,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
>          opn = "mfhi";
>          break;
>      case OPC_MFLO:
> -#if defined(TARGET_MIPS64)
> +#if (TARGET_LONG_SIZE == 8)
>          if (acc != 0) {
>              tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[acc]);
>          } else
> @@ -2617,7 +2607,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
>          break;
>      case OPC_MTHI:
>          if (reg != 0) {
> -#if defined(TARGET_MIPS64)
> +#if (TARGET_LONG_SIZE == 8)
>              if (acc != 0) {
>                  tcg_gen_ext32s_tl(cpu_HI[acc], cpu_gpr[reg]);
>              } else
> @@ -2632,7 +2622,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
>          break;
>      case OPC_MTLO:
>          if (reg != 0) {
> -#if defined(TARGET_MIPS64)
> +#if (TARGET_LONG_SIZE == 8)
>              if (acc != 0) {
>                  tcg_gen_ext32s_tl(cpu_LO[acc], cpu_gpr[reg]);
>              } else
> @@ -10134,7 +10124,7 @@ static int decode_mips16_opc (CPUMIPSState *env, DisasContext *ctx,
>              gen_logic(env, ctx, OPC_NOR, rx, ry, 0);
>              break;
>          case RR_MFHI:
> -            gen_HILO(ctx, OPC_MFHI, rx);
> +            gen_HILO(ctx, OPC_MFHI, rx, 0);
>              break;
>          case RR_CNVT:
>              switch (cnvt_op) {
> @@ -10166,7 +10156,7 @@ static int decode_mips16_opc (CPUMIPSState *env, DisasContext *ctx,
>              }
>              break;
>          case RR_MFLO:
> -            gen_HILO(ctx, OPC_MFLO, rx);
> +            gen_HILO(ctx, OPC_MFLO, rx, 0);
>              break;
>  #if defined (TARGET_MIPS64)
>          case RR_DSRA:
> @@ -10922,11 +10912,11 @@ static void gen_pool16c_insn (CPUMIPSState *env, DisasContext *ctx, int *is_bran
>          break;
>      case MFHI16 + 0:
>      case MFHI16 + 1:
> -        gen_HILO(ctx, OPC_MFHI, uMIPS_RS5(ctx->opcode));
> +        gen_HILO(ctx, OPC_MFHI, uMIPS_RS5(ctx->opcode), 0);
>          break;
>      case MFLO16 + 0:
>      case MFLO16 + 1:
> -        gen_HILO(ctx, OPC_MFLO, uMIPS_RS5(ctx->opcode));
> +        gen_HILO(ctx, OPC_MFLO, uMIPS_RS5(ctx->opcode), 0);
>          break;
>      case BREAK16:
>          generate_exception(ctx, EXCP_BREAK);
> @@ -11286,16 +11276,16 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs,
>      case 0x35:
>          switch (minor) {
>          case MFHI32:
> -            gen_HILO(ctx, OPC_MFHI, rs);
> +            gen_HILO(ctx, OPC_MFHI, rs, 0);
>              break;
>          case MFLO32:
> -            gen_HILO(ctx, OPC_MFLO, rs);
> +            gen_HILO(ctx, OPC_MFLO, rs, 0);
>              break;
>          case MTHI32:
> -            gen_HILO(ctx, OPC_MTHI, rs);
> +            gen_HILO(ctx, OPC_MTHI, rs, 0);
>              break;
>          case MTLO32:
> -            gen_HILO(ctx, OPC_MTLO, rs);
> +            gen_HILO(ctx, OPC_MTLO, rs, 0);
>              break;
>          default:
>              goto pool32axf_invalid;
> @@ -14447,12 +14437,30 @@ static void decode_opc (CPUMIPSState *env, DisasContext *ctx, int *is_branch)
>              break;
>          case OPC_MFHI:          /* Move from HI/LO */
>          case OPC_MFLO:
> -            gen_HILO(ctx, op1, rd);
> -            break;
> +            {
> +                unsigned int acc;
> +                acc = ((ctx->opcode) >> 21) & 0x03;
> +
> +                if (acc != 0) {
> +                    check_dsp(ctx);
> +                }
> +
> +                gen_HILO(ctx, op1, rd, acc);
> +                break;
> +            }
>          case OPC_MTHI:
>          case OPC_MTLO:          /* Move to HI/LO */
> -            gen_HILO(ctx, op1, rs);
> -            break;
> +            {
> +                unsigned int acc;
> +                acc = ((ctx->opcode) >> 11) & 0x03;
> +
> +                if (acc != 0) {
> +                    check_dsp(ctx);
> +                }
> +
> +                gen_HILO(ctx, op1, rs, acc);
> +                break;
> +            }
>          case OPC_PMON:          /* Pmon entry point, also R4010 selsl */
>  #ifdef MIPS_STRICT_STANDARD
>              MIPS_INVAL("PMON / selsl");
> -- 
> 1.7.10.4
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net