[Qemu-devel] [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I

Craig Janeczek via Qemu-devel posted 8 patches 7 years, 2 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I
Posted by Craig Janeczek via Qemu-devel 7 years, 2 months ago
This commit makes the MXU registers and the utility functions for
reading/writing to them. This is required for full MXU instruction
support.

Adds support for emulating the S32I2M and S32M2I MXU instructions.

Signed-off-by: Craig Janeczek <jancraig@amazon.com>
---
 v1
    - initial patch
 v2
    - Fix checkpatch.pl errors
    - remove mips64 ifdef
    - changed bitfield usage to extract32
    - squashed register addition patch into this one
 v3
    - Split register addition and opcode enum definition into seperate patches
    - Split gen_mxu function into command specific gen_mxu_<ins> functions

 target/mips/translate.c | 62 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index ae6b16ecd7..f6991aa8ef 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -1610,6 +1610,23 @@ static inline void gen_store_gpr (TCGv t, int reg)
         tcg_gen_mov_tl(cpu_gpr[reg], t);
 }
 
+/* MXU General purpose registers moves. */
+static inline void gen_load_mxu_gpr(TCGv t, int reg)
+{
+    if (reg == 0) {
+        tcg_gen_movi_tl(t, 0);
+    } else {
+        tcg_gen_mov_tl(t, mxu_gpr[reg - 1]);
+    }
+}
+
+static inline void gen_store_mxu_gpr(TCGv t, int reg)
+{
+    if (reg != 0) {
+        tcg_gen_mov_tl(mxu_gpr[reg - 1], t);
+    }
+}
+
 /* Moves to/from shadow registers. */
 static inline void gen_load_srsgpr (int from, int to)
 {
@@ -3798,6 +3815,42 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
     }
 }
 
+/* MXU Instructions */
+
+/* S32I2M XRa, rb - Register move from GRF to XRF */
+static void gen_mxu_s32i2m(DisasContext *ctx, uint32_t opc)
+{
+    TCGv t0;
+    uint32_t xra, rb;
+
+    t0 = tcg_temp_new();
+
+    xra = extract32(ctx->opcode, 6, 5);
+    rb = extract32(ctx->opcode, 16, 5);
+
+    gen_load_gpr(t0, rb);
+    gen_store_mxu_gpr(t0, xra);
+
+    tcg_temp_free(t0);
+}
+
+/* S32M2I XRa, rb - Register move from XRF to GRF */
+static void gen_mxu_s32m2i(DisasContext *ctx, uint32_t opc)
+{
+    TCGv t0;
+    uint32_t xra, rb;
+
+    t0 = tcg_temp_new();
+
+    xra = extract32(ctx->opcode, 6, 5);
+    rb = extract32(ctx->opcode, 16, 5);
+
+    gen_load_mxu_gpr(t0, xra);
+    gen_store_gpr(t0, rb);
+
+    tcg_temp_free(t0);
+}
+
 /* Godson integer instructions */
 static void gen_loongson_integer(DisasContext *ctx, uint32_t opc,
                                  int rd, int rs, int rt)
@@ -17894,6 +17947,15 @@ static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx)
         check_insn(ctx, INSN_LOONGSON2F);
         gen_loongson_integer(ctx, op1, rd, rs, rt);
         break;
+
+    case OPC_MXU_S32I2M:
+        gen_mxu_s32i2m(ctx, op1);
+        break;
+
+    case OPC_MXU_S32M2I:
+        gen_mxu_s32m2i(ctx, op1);
+        break;
+
     case OPC_CLO:
     case OPC_CLZ:
         check_insn(ctx, ISA_MIPS32);
-- 
2.18.0


Re: [Qemu-devel] [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I
Posted by Aleksandar Markovic 7 years, 2 months ago
> From: Craig Janeczek <jancraig@amazon.com>
> Sent: Tuesday, August 28, 2018 3:00 PM
> To: qemu-devel@nongnu.org
> Cc: Aleksandar Markovic; aurelien@aurel32.net; Craig Janeczek
> Subject: [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I
> 
> This commit makes the MXU registers and the utility functions for
> reading/writing to them. This is required for full MXU instruction
> support.
> 
> Adds support for emulating the S32I2M and S32M2I MXU instructions.
> 
> Signed-off-by: Craig Janeczek <jancraig@amazon.com>
> ---
>  v1
>     - initial patch
>  v2
>     - Fix checkpatch.pl errors
>     - remove mips64 ifdef
>     - changed bitfield usage to extract32
>     - squashed register addition patch into this one
>  v3
>     - Split register addition and opcode enum definition into seperate patches
>     - Split gen_mxu function into command specific gen_mxu_<ins> functions
> 
>  target/mips/translate.c | 62 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index ae6b16ecd7..f6991aa8ef 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -1610,6 +1610,23 @@ static inline void gen_store_gpr (TCGv t, int reg)
>          tcg_gen_mov_tl(cpu_gpr[reg], t);
>  }
> 
> +/* MXU General purpose registers moves. */
> +static inline void gen_load_mxu_gpr(TCGv t, int reg)
> +{
> +    if (reg == 0) {
> +        tcg_gen_movi_tl(t, 0);
> +    } else {
> +        tcg_gen_mov_tl(t, mxu_gpr[reg - 1]);
> +    }
> +}

What happens if reg > 16? Also, the argument reg should be unsigned.

> +
> +static inline void gen_store_mxu_gpr(TCGv t, int reg)
> +{
> +    if (reg != 0) {
> +        tcg_gen_mov_tl(mxu_gpr[reg - 1], t);
> +    }
> +}
> +

What happens if reg > 16? Also, the argument reg should be unsigned.

>  /* Moves to/from shadow registers. */
>  static inline void gen_load_srsgpr (int from, int to)
>  {
> @@ -3798,6 +3815,42 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
>      }
>  }
> 
> +/* MXU Instructions */
> +
> +/* S32I2M XRa, rb - Register move from GRF to XRF */
> +static void gen_mxu_s32i2m(DisasContext *ctx, uint32_t opc)
> +{
> +    TCGv t0;
> +    uint32_t xra, rb;
> +
> +    t0 = tcg_temp_new();
> +
> +    xra = extract32(ctx->opcode, 6, 5);
> +    rb = extract32(ctx->opcode, 16, 5);
> +
> +    gen_load_gpr(t0, rb);
> +    gen_store_mxu_gpr(t0, xra);
> +
> +    tcg_temp_free(t0);
> +}

This does not handle the case xra == XR16. From the doc:

"In MXU, a dedicated register file named XRF comprises sixteen 32-bit general purpose registers – XR0~XR15. XR0 is a special one, which always is read as zero. Moreover, XR16 is an alias of MXU_CR described below and it can only be accessed by S32I2M/S32M2I instructions."

> +
> +/* S32M2I XRa, rb - Register move from XRF to GRF */
> +static void gen_mxu_s32m2i(DisasContext *ctx, uint32_t opc)
> +{
> +    TCGv t0;
> +    uint32_t xra, rb;
> +
> +    t0 = tcg_temp_new();
> +
> +    xra = extract32(ctx->opcode, 6, 5);
> +    rb = extract32(ctx->opcode, 16, 5);
> +
> +    gen_load_mxu_gpr(t0, xra);
> +    gen_store_gpr(t0, rb);
> +
> +    tcg_temp_free(t0);
> +}
> +
>  /* Godson integer instructions */
>  static void gen_loongson_integer(DisasContext *ctx, uint32_t opc,
>                                   int rd, int rs, int rt)
> @@ -17894,6 +17947,15 @@ static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx)
>          check_insn(ctx, INSN_LOONGSON2F);
>          gen_loongson_integer(ctx, op1, rd, rs, rt);
>          break;
> +
> +    case OPC_MXU_S32I2M:
> +        gen_mxu_s32i2m(ctx, op1);
> +        break;
> +
> +    case OPC_MXU_S32M2I:
> +        gen_mxu_s32m2i(ctx, op1);
> +        break;
> +
>      case OPC_CLO:
>      case OPC_CLZ:
>          check_insn(ctx, ISA_MIPS32);

Re: [Qemu-devel] [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I
Posted by Janeczek, Craig via Qemu-devel 7 years, 2 months ago
What happens if reg > 16? Also, the argument reg should be unsigned.
If rev > 16 the instruction is invalid. What type of error can/should I throw here.

This does not handle the case xra == XR16. From the doc:
I do not see where the case is un-handled. XR16 maps to index 15 in the mxu_gpr array. 

-----Original Message-----
From: Aleksandar Markovic <amarkovic@wavecomp.com> 
Sent: Tuesday, August 28, 2018 10:43 AM
To: Janeczek, Craig <jancraig@amazon.com>; qemu-devel@nongnu.org
Cc: aurelien@aurel32.net
Subject: Re: [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I

> From: Craig Janeczek <jancraig@amazon.com>
> Sent: Tuesday, August 28, 2018 3:00 PM
> To: qemu-devel@nongnu.org
> Cc: Aleksandar Markovic; aurelien@aurel32.net; Craig Janeczek
> Subject: [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and 
> S32M2I
> 
> This commit makes the MXU registers and the utility functions for 
> reading/writing to them. This is required for full MXU instruction 
> support.
> 
> Adds support for emulating the S32I2M and S32M2I MXU instructions.
> 
> Signed-off-by: Craig Janeczek <jancraig@amazon.com>
> ---
>  v1
>     - initial patch
>  v2
>     - Fix checkpatch.pl errors
>     - remove mips64 ifdef
>     - changed bitfield usage to extract32
>     - squashed register addition patch into this one
>  v3
>     - Split register addition and opcode enum definition into seperate patches
>     - Split gen_mxu function into command specific gen_mxu_<ins> 
> functions
> 
>  target/mips/translate.c | 62 
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c index 
> ae6b16ecd7..f6991aa8ef 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -1610,6 +1610,23 @@ static inline void gen_store_gpr (TCGv t, int reg)
>          tcg_gen_mov_tl(cpu_gpr[reg], t);  }
> 
> +/* MXU General purpose registers moves. */ static inline void 
> +gen_load_mxu_gpr(TCGv t, int reg) {
> +    if (reg == 0) {
> +        tcg_gen_movi_tl(t, 0);
> +    } else {
> +        tcg_gen_mov_tl(t, mxu_gpr[reg - 1]);
> +    }
> +}

What happens if reg > 16? Also, the argument reg should be unsigned.

> +
> +static inline void gen_store_mxu_gpr(TCGv t, int reg) {
> +    if (reg != 0) {
> +        tcg_gen_mov_tl(mxu_gpr[reg - 1], t);
> +    }
> +}
> +

What happens if reg > 16? Also, the argument reg should be unsigned.

>  /* Moves to/from shadow registers. */  static inline void 
> gen_load_srsgpr (int from, int to)  { @@ -3798,6 +3815,42 @@ static 
> void gen_cl (DisasContext *ctx, uint32_t opc,
>      }
>  }
> 
> +/* MXU Instructions */
> +
> +/* S32I2M XRa, rb - Register move from GRF to XRF */ static void 
> +gen_mxu_s32i2m(DisasContext *ctx, uint32_t opc) {
> +    TCGv t0;
> +    uint32_t xra, rb;
> +
> +    t0 = tcg_temp_new();
> +
> +    xra = extract32(ctx->opcode, 6, 5);
> +    rb = extract32(ctx->opcode, 16, 5);
> +
> +    gen_load_gpr(t0, rb);
> +    gen_store_mxu_gpr(t0, xra);
> +
> +    tcg_temp_free(t0);
> +}

This does not handle the case xra == XR16. From the doc:

"In MXU, a dedicated register file named XRF comprises sixteen 32-bit general purpose registers - XR0~XR15. XR0 is a special one, which always is read as zero. Moreover, XR16 is an alias of MXU_CR described below and it can only be accessed by S32I2M/S32M2I instructions."

> +
> +/* S32M2I XRa, rb - Register move from XRF to GRF */ static void 
> +gen_mxu_s32m2i(DisasContext *ctx, uint32_t opc) {
> +    TCGv t0;
> +    uint32_t xra, rb;
> +
> +    t0 = tcg_temp_new();
> +
> +    xra = extract32(ctx->opcode, 6, 5);
> +    rb = extract32(ctx->opcode, 16, 5);
> +
> +    gen_load_mxu_gpr(t0, xra);
> +    gen_store_gpr(t0, rb);
> +
> +    tcg_temp_free(t0);
> +}
> +
>  /* Godson integer instructions */
>  static void gen_loongson_integer(DisasContext *ctx, uint32_t opc,
>                                   int rd, int rs, int rt) @@ -17894,6 
> +17947,15 @@ static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx)
>          check_insn(ctx, INSN_LOONGSON2F);
>          gen_loongson_integer(ctx, op1, rd, rs, rt);
>          break;
> +
> +    case OPC_MXU_S32I2M:
> +        gen_mxu_s32i2m(ctx, op1);
> +        break;
> +
> +    case OPC_MXU_S32M2I:
> +        gen_mxu_s32m2i(ctx, op1);
> +        break;
> +
>      case OPC_CLO:
>      case OPC_CLZ:
>          check_insn(ctx, ISA_MIPS32);


Re: [Qemu-devel] [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I
Posted by Aleksandar Markovic 7 years, 2 months ago
> > This does not handle the case xra == XR16.

> I do not see where the case is un-handled. XR16 maps to index 15 in the mxu_gpr array.

But, XR16 has its own rules for read/write, and you are treating it just as a regular register. 
Re: [Qemu-devel] [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I
Posted by Janeczek, Craig via Qemu-devel 7 years, 2 months ago
The only commands that have the 5th bit required to address XR16 are S32M2I/S32I2M.

I can split it out into a separate utility function and put a conditional into the S32M2I/S32I2M functions if you are more comfortable with that.

-----Original Message-----
From: Aleksandar Markovic <amarkovic@wavecomp.com> 
Sent: Tuesday, August 28, 2018 12:53 PM
To: Janeczek, Craig <jancraig@amazon.com>; qemu-devel@nongnu.org
Cc: aurelien@aurel32.net
Subject: Re: [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I

> > This does not handle the case xra == XR16.

> I do not see where the case is un-handled. XR16 maps to index 15 in the mxu_gpr array.

But, XR16 has its own rules for read/write, and you are treating it just as a regular register. 

Re: [Qemu-devel] [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I
Posted by Aleksandar Markovic 7 years, 2 months ago
> The only commands that have the 5th bit required to address XR16 are S32M2I/S32I2M.
> 
> I can split it out into a separate utility function and put a conditional into the S32M2I/S32I2M functions if you are more comfortable with that.

It is not a bad idea. (preventing all instructions other than S32M2I/S32I2M from accessing XR16)

-----Original Message-----
From: Aleksandar Markovic <amarkovic@wavecomp.com>
Sent: Tuesday, August 28, 2018 12:53 PM
To: Janeczek, Craig <jancraig@amazon.com>; qemu-devel@nongnu.org
Cc: aurelien@aurel32.net
Subject: Re: [PATCH v3 3/8] target/mips: Add MXU instructions S32I2M and S32M2I

> > This does not handle the case xra == XR16.

> I do not see where the case is un-handled. XR16 maps to index 15 in the mxu_gpr array.

But, XR16 has its own rules for read/write, and you are treating it just as a regular register.