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(-)

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
-- 
2.14.3

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

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~

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