diff mbox series

[03/19] target/hppa: Convert move to/from system registers

Message ID 20180217203132.31780-4-richard.henderson@linaro.org
State Superseded
Headers show
Series target/hppa: Convert to decodetree.py | expand

Commit Message

Richard Henderson Feb. 17, 2018, 8:31 p.m. UTC 
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/hppa/translate.c  | 57 +++++++++++++++++++++---------------------------
 target/hppa/insns.decode | 15 +++++++++++++
 2 files changed, 40 insertions(+), 32 deletions(-)

-- 
2.14.3

Comments

Bastian Koppelmann April 6, 2018, 1:14 p.m. UTC | #1 
On 02/17/2018 09:31 PM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/hppa/translate.c  | 57 +++++++++++++++++++++---------------------------

>  target/hppa/insns.decode | 15 +++++++++++++

>  2 files changed, 40 insertions(+), 32 deletions(-)

> 

[...]
> @@ -2267,24 +2265,26 @@ static void trans_ssm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)

>      ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;

>      nullify_end(ctx);

>  }

> +#endif /* !CONFIG_USER_ONLY */


This seems to not belong to this patch.

>  

> -static void trans_mtsm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)

> +static void trans_mtsm(DisasContext *ctx, arg_mtsm *a, uint32_t insn)

>  {

> -    unsigned rr = extract32(insn, 16, 5);

> -    TCGv_reg tmp, reg;

> -

>      CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);

> +#ifndef CONFIG_USER_ONLY


Why do you need to make this softmmu only in a simple convert patch?
This makes it at least confusing for the reviewer.

Otherwise it looks good to me.
Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>


Cheers,
Bastian
Richard Henderson April 6, 2018, 1:33 p.m. UTC | #2 
On 04/06/2018 11:14 PM, Bastian Koppelmann wrote:
> On 02/17/2018 09:31 PM, Richard Henderson wrote:

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> ---

>>  target/hppa/translate.c  | 57 +++++++++++++++++++++---------------------------

>>  target/hppa/insns.decode | 15 +++++++++++++

>>  2 files changed, 40 insertions(+), 32 deletions(-)

>>

> [...]

>> @@ -2267,24 +2265,26 @@ static void trans_ssm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)

>>      ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;

>>      nullify_end(ctx);

>>  }

>> +#endif /* !CONFIG_USER_ONLY */

> 

> This seems to not belong to this patch.


It does though.

>> -static void trans_mtsm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)

>> +static void trans_mtsm(DisasContext *ctx, arg_mtsm *a, uint32_t insn)

>>  {

>> -    unsigned rr = extract32(insn, 16, 5);

>> -    TCGv_reg tmp, reg;

>> -

>>      CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);

>> +#ifndef CONFIG_USER_ONLY

> 

> Why do you need to make this softmmu only in a simple convert patch?

> This makes it at least confusing for the reviewer.


Actually, it moves the function *out* of the softmmu only block.
That's the #endif being added above.

(1) The patterns in insns.decode are not (and cannot be) conditional
    on softmmu, so this function is now always called.

(2) This also enables a fix to a (trivial) emulation error between
    SIGILL (ILL_ILLOPC) and SIGILL (ILL_PRVOPC).  Although I think
    I would need an additional change in linux-user/ to effect this.


r~
Bastian Koppelmann April 6, 2018, 1:41 p.m. UTC | #3 
On 04/06/2018 03:33 PM, Richard Henderson wrote:
> On 04/06/2018 11:14 PM, Bastian Koppelmann wrote:

>> On 02/17/2018 09:31 PM, Richard Henderson wrote:

>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>>> ---

>>>  target/hppa/translate.c  | 57 +++++++++++++++++++++---------------------------

>>>  target/hppa/insns.decode | 15 +++++++++++++

>>>  2 files changed, 40 insertions(+), 32 deletions(-)

>>>

>> [...]

>>> @@ -2267,24 +2265,26 @@ static void trans_ssm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)

>>>      ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;

>>>      nullify_end(ctx);

>>>  }

>>> +#endif /* !CONFIG_USER_ONLY */

>>

>> This seems to not belong to this patch.

> 

> It does though.

> 

>>> -static void trans_mtsm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)

>>> +static void trans_mtsm(DisasContext *ctx, arg_mtsm *a, uint32_t insn)

>>>  {

>>> -    unsigned rr = extract32(insn, 16, 5);

>>> -    TCGv_reg tmp, reg;

>>> -

>>>      CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);

>>> +#ifndef CONFIG_USER_ONLY

>>

>> Why do you need to make this softmmu only in a simple convert patch?

>> This makes it at least confusing for the reviewer.

> 

> Actually, it moves the function *out* of the softmmu only block.

> That's the #endif being added above.


Ah sorry. It does match the conventionalized trans_mtsm function in
table_system[].

Cheers,
Bastian
diff mbox series

Patch

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index a503ae38d4..9b2de2fa2a 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -844,7 +844,7 @@  static unsigned assemble_rc64(uint32_t insn)
     return r2 * 32 + r1 * 4 + r0;
 }
 
-static unsigned assemble_sr3(uint32_t insn)
+static inline unsigned assemble_sr3(uint32_t insn)
 {
     unsigned s2 = extract32(insn, 13, 1);
     unsigned s0 = extract32(insn, 14, 2);
@@ -2015,9 +2015,9 @@  static void trans_sync(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
     cond_free(&ctx->null_cond);
 }
 
-static void trans_mfia(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
+static void trans_mfia(DisasContext *ctx, arg_mfia *a, uint32_t insn)
 {
-    unsigned rt = extract32(insn, 0, 5);
+    unsigned rt = a->t;
     TCGv_reg tmp = dest_gpr(ctx, rt);
     tcg_gen_movi_reg(tmp, ctx->iaoq_f);
     save_gpr(ctx, rt, tmp);
@@ -2025,10 +2025,10 @@  static void trans_mfia(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
     cond_free(&ctx->null_cond);
 }
 
-static void trans_mfsp(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
+static void trans_mfsp(DisasContext *ctx, arg_mfsp *a, uint32_t insn)
 {
-    unsigned rt = extract32(insn, 0, 5);
-    unsigned rs = assemble_sr3(insn);
+    unsigned rt = a->t;
+    unsigned rs = a->sp;
     TCGv_i64 t0 = tcg_temp_new_i64();
     TCGv_reg t1 = tcg_temp_new();
 
@@ -2043,16 +2043,16 @@  static void trans_mfsp(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
     cond_free(&ctx->null_cond);
 }
 
-static void trans_mfctl(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
+static void trans_mfctl(DisasContext *ctx, arg_mfctl *a, uint32_t insn)
 {
-    unsigned rt = extract32(insn, 0, 5);
-    unsigned ctl = extract32(insn, 21, 5);
+    unsigned rt = a->t;
+    unsigned ctl = a->r;
     TCGv_reg tmp;
 
     switch (ctl) {
     case CR_SAR:
 #ifdef TARGET_HPPA64
-        if (extract32(insn, 14, 1) == 0) {
+        if (a->e == 0) {
             /* MFSAR without ,W masks low 5 bits.  */
             tmp = dest_gpr(ctx, rt);
             tcg_gen_andi_reg(tmp, cpu_sar, 31);
@@ -2094,10 +2094,10 @@  static void trans_mfctl(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
     cond_free(&ctx->null_cond);
 }
 
-static void trans_mtsp(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
+static void trans_mtsp(DisasContext *ctx, arg_mtsp *a, uint32_t insn)
 {
-    unsigned rr = extract32(insn, 16, 5);
-    unsigned rs = assemble_sr3(insn);
+    unsigned rr = a->r;
+    unsigned rs = a->sp;
     TCGv_i64 t64;
 
     if (rs >= 5) {
@@ -2120,11 +2120,10 @@  static void trans_mtsp(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
     nullify_end(ctx);
 }
 
-static void trans_mtctl(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
+static void trans_mtctl(DisasContext *ctx, arg_mtctl *a, uint32_t insn)
 {
-    unsigned rin = extract32(insn, 16, 5);
-    unsigned ctl = extract32(insn, 21, 5);
-    TCGv_reg reg = load_gpr(ctx, rin);
+    unsigned ctl = a->t;
+    TCGv_reg reg = load_gpr(ctx, a->r);
     TCGv_reg tmp;
 
     if (ctl == CR_SAR) {
@@ -2176,12 +2175,11 @@  static void trans_mtctl(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
 #endif
 }
 
-static void trans_mtsarcm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
+static void trans_mtsarcm(DisasContext *ctx, arg_mtsarcm *a, uint32_t insn)
 {
-    unsigned rin = extract32(insn, 16, 5);
     TCGv_reg tmp = tcg_temp_new();
 
-    tcg_gen_not_reg(tmp, load_gpr(ctx, rin));
+    tcg_gen_not_reg(tmp, load_gpr(ctx, a->r));
     tcg_gen_andi_reg(tmp, tmp, TARGET_REGISTER_BITS - 1);
     save_or_nullify(ctx, cpu_sar, tmp);
     tcg_temp_free(tmp);
@@ -2267,24 +2265,26 @@  static void trans_ssm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
     ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
     nullify_end(ctx);
 }
+#endif /* !CONFIG_USER_ONLY */
 
-static void trans_mtsm(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
+static void trans_mtsm(DisasContext *ctx, arg_mtsm *a, uint32_t insn)
 {
-    unsigned rr = extract32(insn, 16, 5);
-    TCGv_reg tmp, reg;
-
     CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);
+#ifndef CONFIG_USER_ONLY
+    TCGv_reg tmp, reg;
     nullify_over(ctx);
 
-    reg = load_gpr(ctx, rr);
+    reg = load_gpr(ctx, a->r);
     tmp = get_temp(ctx);
     gen_helper_swap_system_mask(tmp, cpu_env, reg);
 
     /* Exit the TB to recognize new interrupts.  */
     ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
     nullify_end(ctx);
+#endif
 }
 
+#ifndef CONFIG_USER_ONLY
 static void trans_rfi(DisasContext *ctx, uint32_t insn, const DisasInsn *di)
 {
     unsigned comp = extract32(insn, 5, 4);
@@ -2323,19 +2323,12 @@  static void gen_hlt(DisasContext *ctx, int reset)
 #endif /* !CONFIG_USER_ONLY */
 
 static const DisasInsn table_system[] = {
-    { 0x00001820u, 0xffe01fffu, trans_mtsp },
-    { 0x00001840u, 0xfc00ffffu, trans_mtctl },
-    { 0x016018c0u, 0xffe0ffffu, trans_mtsarcm },
-    { 0x000014a0u, 0xffffffe0u, trans_mfia },
-    { 0x000004a0u, 0xffff1fe0u, trans_mfsp },
-    { 0x000008a0u, 0xfc1fbfe0u, trans_mfctl },
     { 0x00000400u, 0xffffffffu, trans_sync },  /* sync */
     { 0x00100400u, 0xffffffffu, trans_sync },  /* syncdma */
     { 0x000010a0u, 0xfc1f3fe0u, trans_ldsid },
 #ifndef CONFIG_USER_ONLY
     { 0x00000e60u, 0xfc00ffe0u, trans_rsm },
     { 0x00000d60u, 0xfc00ffe0u, trans_ssm },
-    { 0x00001860u, 0xffe0ffffu, trans_mtsm },
     { 0x00000c00u, 0xfffffe1fu, trans_rfi },
 #endif
 };
diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
index 6c2d3a3a52..01b8a52ca5 100644
--- a/target/hppa/insns.decode
+++ b/target/hppa/insns.decode
@@ -17,8 +17,23 @@ 
 # License along with this library; if not, see <http://www.gnu.org/licenses/>.
 #
 
+####
+# Field definitions
+####
+
+%assemble_sr3	13:1 14:2
+
 ####
 # System
 ####
 
 break		000000 ----- ----- --- 00000000 -----
+
+mtsp		000000 ----- r:5   ... 11000001 00000	sp=%assemble_sr3
+mtctl		000000 t:5   r:5   --- 11000010 00000
+mtsarcm		000000 01011 r:5   --- 11000110 00000
+mtsm		000000 00000 r:5   000 11000011 00000
+
+mfia		000000 ----- 00000 ---   10100101 t:5
+mfsp		000000 ----- 00000 ...   00100101 t:5	sp=%assemble_sr3
+mfctl		000000 r:5   00000- e:1 -01000101 t:5