[PATCH 0/4] target/m68k: MacOS supervisor/user mode switch fixes

[PATCH 0/4] target/m68k: MacOS supervisor/user mode switch fixes

[Mark Cave-Ayland]

This series fixes a couple of bugs that were discovered when trying to boot
MacOS on my github q800 branch with virtual memory enabled.

Patch 1 renames M68K_FEATURE_M68000 to M68K_FEATURE_M68K in order to clarify
that this feature indicates any Motorola 68K CPU rather than the 68000
specifically [1].

Patch 2 increases the size of the M68K features bitmap since there are already
32 features present, and we need to add one more.

Patch 3 fixes up the MOVE-from-SR instruction which is privileged from the
68010 CPU onwards to use a newly introduced M68K_FEATURE_MOVEFROMSR_PRIV
feature [2].

Patch 4 ensures that we always call gen_exit_tb() after writes to the SR
register since any change of the S bit can change the security context.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Notes:

[1] The m68k code currently contains a mix of real CPU features and pseudo
    features that represent each 680X0 CPU. In general QEMU maps features to
    CPUs which is why I've introduced the new M68K_FEATURE_MOVEFROMSR_PRIV
    feature, but there are still checks for specific 680X0 CPU models. This
    could do with a tidy-up, but without a specific set of test images across
    68K and Coldfire I don't feel I'm confident enough to do this.
    
[2] The existing code in MOVE-from-SR uses !m68k_feature(env, M68K_FEATURE_M68000)
    to suggest that the condition should match for any CPU that isn't a 68000 (i.e.
    68010 and later) but as we see from this series, this is not the case according
    to the code. Some of the Mac 68K folk have suggested there are likely other
    cases in target/m68k where the same assumption has been used and the check
    logic is incorrect, but again without specific examples it's difficult for me to
    test.


Mark Cave-Ayland (4):
  target/m68k: rename M68K_FEATURE_M68000 to M68K_FEATURE_M68K
  target/m68k: increase size of m68k CPU features from uint32_t to
    uint64_t
  target/m68k: use M68K_FEATURE_MOVEFROMSR_PRIV feature for move_from_sr
    privilege check
  target/m68k: always call gen_exit_tb() after writes to SR

 target/m68k/cpu.c       |  11 +++-
 target/m68k/cpu.h       |  13 ++--
 target/m68k/helper.c    |   2 +-
 target/m68k/op_helper.c |   2 +-
 target/m68k/translate.c | 142 +++++++++++++++++++++-------------------
 5 files changed, 91 insertions(+), 79 deletions(-)

-- 
2.30.2

[PATCH 1/4] target/m68k: rename M68K_FEATURE_M68000 to M68K_FEATURE_M68K

[Mark Cave-Ayland]

The M68K_FEATURE_M68000 feature is misleading in that its name suggests the feature
is defined just for Motorola 68000 CPUs, whilst in fact it is defined for all
Motorola 680X0 CPUs.

In order to avoid confusion with the other M68K_FEATURE_M680X0 constants which
define the features available for specific Motorola CPU models, rename
M68K_FEATURE_M68000 to M68K_FEATURE_M68K and add comments to clarify its usage.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/cpu.c       |   2 +-
 target/m68k/cpu.h       |   5 +-
 target/m68k/helper.c    |   2 +-
 target/m68k/op_helper.c |   2 +-
 target/m68k/translate.c | 138 ++++++++++++++++++++--------------------
 5 files changed, 75 insertions(+), 74 deletions(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 5bbefda575..f681be3a2a 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -110,7 +110,7 @@ static void m68000_cpu_initfn(Object *obj)
     M68kCPU *cpu = M68K_CPU(obj);
     CPUM68KState *env = &cpu->env;
 
-    m68k_set_feature(env, M68K_FEATURE_M68000);
+    m68k_set_feature(env, M68K_FEATURE_M68K);
     m68k_set_feature(env, M68K_FEATURE_USP);
     m68k_set_feature(env, M68K_FEATURE_WORD_INDEX);
     m68k_set_feature(env, M68K_FEATURE_MOVEP);
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 4d8f48e8c7..67b6c12c28 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -480,8 +480,9 @@ void do_m68k_semihosting(CPUM68KState *env, int nr);
  */
 
 enum m68k_features {
-    /* Base m68k instruction set */
-    M68K_FEATURE_M68000,
+    /* Base Motorola CPU set (not set for Coldfire CPUs) */
+    M68K_FEATURE_M68K,
+    /* Motorola CPU feature sets */
     M68K_FEATURE_M68010,
     M68K_FEATURE_M68020,
     M68K_FEATURE_M68030,
diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 5728e48585..4621cf2402 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -460,7 +460,7 @@ void m68k_switch_sp(CPUM68KState *env)
     int new_sp;
 
     env->sp[env->current_sp] = env->aregs[7];
-    if (m68k_feature(env, M68K_FEATURE_M68000)) {
+    if (m68k_feature(env, M68K_FEATURE_M68K)) {
         if (env->sr & SR_S) {
             /* SR:Master-Mode bit unimplemented then ISP is not available */
             if (!m68k_feature(env, M68K_FEATURE_MSP) || env->sr & SR_M) {
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index d9937ca8dc..99dc994fcb 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -433,7 +433,7 @@ static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
 
 static void do_interrupt_all(CPUM68KState *env, int is_hw)
 {
-    if (m68k_feature(env, M68K_FEATURE_M68000)) {
+    if (m68k_feature(env, M68K_FEATURE_M68K)) {
         m68k_interrupt_all(env, is_hw);
         return;
     }
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 5098f7e570..fad8af8f83 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -471,7 +471,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext *s, TCGv base)
     if ((ext & 0x800) == 0 && !m68k_feature(s->env, M68K_FEATURE_WORD_INDEX))
         return NULL_QREG;
 
-    if (m68k_feature(s->env, M68K_FEATURE_M68000) &&
+    if (m68k_feature(s->env, M68K_FEATURE_M68K) &&
         !m68k_feature(s->env, M68K_FEATURE_SCALED_INDEX)) {
         ext &= ~(3 << 9);
     }
@@ -804,7 +804,7 @@ static TCGv gen_lea_mode(CPUM68KState *env, DisasContext *s,
         reg = get_areg(s, reg0);
         tmp = mark_to_release(s, tcg_temp_new());
         if (reg0 == 7 && opsize == OS_BYTE &&
-            m68k_feature(s->env, M68K_FEATURE_M68000)) {
+            m68k_feature(s->env, M68K_FEATURE_M68K)) {
             tcg_gen_subi_i32(tmp, reg, 2);
         } else {
             tcg_gen_subi_i32(tmp, reg, opsize_bytes(opsize));
@@ -888,7 +888,7 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext *s, int mode, int reg0,
         if (what == EA_STORE || !addrp) {
             TCGv tmp = tcg_temp_new();
             if (reg0 == 7 && opsize == OS_BYTE &&
-                m68k_feature(s->env, M68K_FEATURE_M68000)) {
+                m68k_feature(s->env, M68K_FEATURE_M68K)) {
                 tcg_gen_addi_i32(tmp, reg, 2);
             } else {
                 tcg_gen_addi_i32(tmp, reg, opsize_bytes(opsize));
@@ -2210,7 +2210,7 @@ DISAS_INSN(bitop_im)
     op = (insn >> 6) & 3;
 
     bitnum = read_im16(env, s);
-    if (m68k_feature(s->env, M68K_FEATURE_M68000)) {
+    if (m68k_feature(s->env, M68K_FEATURE_M68K)) {
         if (bitnum & 0xfe00) {
             disas_undef(env, s, insn);
             return;
@@ -2875,7 +2875,7 @@ DISAS_INSN(mull)
         return;
     }
     SRC_EA(env, src1, OS_LONG, 0, NULL);
-    if (m68k_feature(s->env, M68K_FEATURE_M68000)) {
+    if (m68k_feature(s->env, M68K_FEATURE_M68K)) {
         tcg_gen_movi_i32(QREG_CC_C, 0);
         if (sign) {
             tcg_gen_muls2_i32(QREG_CC_N, QREG_CC_V, src1, DREG(ext, 12));
@@ -3470,7 +3470,7 @@ static inline void shift_im(DisasContext *s, uint16_t insn, int opsize)
          * while M68000 sets if the most significant bit is changed at
          * any time during the shift operation.
          */
-        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68000)) {
+        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68K)) {
             /* if shift count >= bits, V is (reg != 0) */
             if (count >= bits) {
                 tcg_gen_setcond_i32(TCG_COND_NE, QREG_CC_V, reg, QREG_CC_V);
@@ -3554,7 +3554,7 @@ static inline void shift_reg(DisasContext *s, uint16_t insn, int opsize)
          *     int64_t t = (int64_t)(intN_t)reg << count;
          *     V = ((s ^ t) & (-1 << (bits - 1))) != 0
          */
-        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68000)) {
+        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68K)) {
             TCGv_i64 tt = tcg_const_i64(32);
             /* if shift is greater than 32, use 32 */
             tcg_gen_movcond_i64(TCG_COND_GT, s64, s64, tt, tt, s64);
@@ -3647,7 +3647,7 @@ DISAS_INSN(shift_mem)
          * while M68000 sets if the most significant bit is changed at
          * any time during the shift operation
          */
-        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68000)) {
+        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68K)) {
             src = gen_extend(s, src, OS_WORD, 1);
             tcg_gen_xor_i32(QREG_CC_V, QREG_CC_N, src);
         }
@@ -4598,7 +4598,7 @@ DISAS_INSN(move_from_sr)
 {
     TCGv sr;
 
-    if (IS_USER(s) && !m68k_feature(env, M68K_FEATURE_M68000)) {
+    if (IS_USER(s) && !m68k_feature(env, M68K_FEATURE_M68K)) {
         gen_exception(s, s->base.pc_next, EXCP_PRIVILEGE);
         return;
     }
@@ -6011,7 +6011,7 @@ void register_m68k_insns (CPUM68KState *env)
     } while(0)
     BASE(undef,     0000, 0000);
     INSN(arith_im,  0080, fff8, CF_ISA_A);
-    INSN(arith_im,  0000, ff00, M68000);
+    INSN(arith_im,  0000, ff00, M68K);
     INSN(chk2,      00c0, f9c0, CHK2);
     INSN(bitrev,    00c0, fff8, CF_ISA_APLUSC);
     BASE(bitop_reg, 0100, f1c0);
@@ -6020,26 +6020,26 @@ void register_m68k_insns (CPUM68KState *env)
     BASE(bitop_reg, 01c0, f1c0);
     INSN(movep,     0108, f138, MOVEP);
     INSN(arith_im,  0280, fff8, CF_ISA_A);
-    INSN(arith_im,  0200, ff00, M68000);
-    INSN(undef,     02c0, ffc0, M68000);
+    INSN(arith_im,  0200, ff00, M68K);
+    INSN(undef,     02c0, ffc0, M68K);
     INSN(byterev,   02c0, fff8, CF_ISA_APLUSC);
     INSN(arith_im,  0480, fff8, CF_ISA_A);
-    INSN(arith_im,  0400, ff00, M68000);
-    INSN(undef,     04c0, ffc0, M68000);
-    INSN(arith_im,  0600, ff00, M68000);
-    INSN(undef,     06c0, ffc0, M68000);
+    INSN(arith_im,  0400, ff00, M68K);
+    INSN(undef,     04c0, ffc0, M68K);
+    INSN(arith_im,  0600, ff00, M68K);
+    INSN(undef,     06c0, ffc0, M68K);
     INSN(ff1,       04c0, fff8, CF_ISA_APLUSC);
     INSN(arith_im,  0680, fff8, CF_ISA_A);
     INSN(arith_im,  0c00, ff38, CF_ISA_A);
-    INSN(arith_im,  0c00, ff00, M68000);
+    INSN(arith_im,  0c00, ff00, M68K);
     BASE(bitop_im,  0800, ffc0);
     BASE(bitop_im,  0840, ffc0);
     BASE(bitop_im,  0880, ffc0);
     BASE(bitop_im,  08c0, ffc0);
     INSN(arith_im,  0a80, fff8, CF_ISA_A);
-    INSN(arith_im,  0a00, ff00, M68000);
+    INSN(arith_im,  0a00, ff00, M68K);
 #if defined(CONFIG_SOFTMMU)
-    INSN(moves,     0e00, ff00, M68000);
+    INSN(moves,     0e00, ff00, M68K);
 #endif
     INSN(cas,       0ac0, ffc0, CAS);
     INSN(cas,       0cc0, ffc0, CAS);
@@ -6049,44 +6049,44 @@ void register_m68k_insns (CPUM68KState *env)
     BASE(move,      1000, f000);
     BASE(move,      2000, f000);
     BASE(move,      3000, f000);
-    INSN(chk,       4000, f040, M68000);
+    INSN(chk,       4000, f040, M68K);
     INSN(strldsr,   40e7, ffff, CF_ISA_APLUSC);
     INSN(negx,      4080, fff8, CF_ISA_A);
-    INSN(negx,      4000, ff00, M68000);
-    INSN(undef,     40c0, ffc0, M68000);
+    INSN(negx,      4000, ff00, M68K);
+    INSN(undef,     40c0, ffc0, M68K);
     INSN(move_from_sr, 40c0, fff8, CF_ISA_A);
-    INSN(move_from_sr, 40c0, ffc0, M68000);
+    INSN(move_from_sr, 40c0, ffc0, M68K);
     BASE(lea,       41c0, f1c0);
     BASE(clr,       4200, ff00);
     BASE(undef,     42c0, ffc0);
     INSN(move_from_ccr, 42c0, fff8, CF_ISA_A);
-    INSN(move_from_ccr, 42c0, ffc0, M68000);
+    INSN(move_from_ccr, 42c0, ffc0, M68K);
     INSN(neg,       4480, fff8, CF_ISA_A);
-    INSN(neg,       4400, ff00, M68000);
-    INSN(undef,     44c0, ffc0, M68000);
+    INSN(neg,       4400, ff00, M68K);
+    INSN(undef,     44c0, ffc0, M68K);
     BASE(move_to_ccr, 44c0, ffc0);
     INSN(not,       4680, fff8, CF_ISA_A);
-    INSN(not,       4600, ff00, M68000);
+    INSN(not,       4600, ff00, M68K);
 #if defined(CONFIG_SOFTMMU)
     BASE(move_to_sr, 46c0, ffc0);
 #endif
-    INSN(nbcd,      4800, ffc0, M68000);
-    INSN(linkl,     4808, fff8, M68000);
+    INSN(nbcd,      4800, ffc0, M68K);
+    INSN(linkl,     4808, fff8, M68K);
     BASE(pea,       4840, ffc0);
     BASE(swap,      4840, fff8);
     INSN(bkpt,      4848, fff8, BKPT);
     INSN(movem,     48d0, fbf8, CF_ISA_A);
     INSN(movem,     48e8, fbf8, CF_ISA_A);
-    INSN(movem,     4880, fb80, M68000);
+    INSN(movem,     4880, fb80, M68K);
     BASE(ext,       4880, fff8);
     BASE(ext,       48c0, fff8);
     BASE(ext,       49c0, fff8);
     BASE(tst,       4a00, ff00);
     INSN(tas,       4ac0, ffc0, CF_ISA_B);
-    INSN(tas,       4ac0, ffc0, M68000);
+    INSN(tas,       4ac0, ffc0, M68K);
 #if defined(CONFIG_SOFTMMU)
     INSN(halt,      4ac8, ffff, CF_ISA_A);
-    INSN(halt,      4ac8, ffff, M68060);
+    INSN(halt,      4ac8, ffff, M68K);
 #endif
     INSN(pulse,     4acc, ffff, CF_ISA_A);
     BASE(illegal,   4afc, ffff);
@@ -6101,7 +6101,7 @@ void register_m68k_insns (CPUM68KState *env)
 #if defined(CONFIG_SOFTMMU)
     INSN(move_to_usp, 4e60, fff8, USP);
     INSN(move_from_usp, 4e68, fff8, USP);
-    INSN(reset,     4e70, ffff, M68000);
+    INSN(reset,     4e70, ffff, M68K);
     BASE(stop,      4e72, ffff);
     BASE(rte,       4e73, ffff);
     INSN(cf_movec,  4e7b, ffff, CF_ISA_A);
@@ -6110,15 +6110,15 @@ void register_m68k_insns (CPUM68KState *env)
     BASE(nop,       4e71, ffff);
     INSN(rtd,       4e74, ffff, RTD);
     BASE(rts,       4e75, ffff);
-    INSN(trapv,     4e76, ffff, M68000);
-    INSN(rtr,       4e77, ffff, M68000);
+    INSN(trapv,     4e76, ffff, M68K);
+    INSN(rtr,       4e77, ffff, M68K);
     BASE(jump,      4e80, ffc0);
     BASE(jump,      4ec0, ffc0);
-    INSN(addsubq,   5000, f080, M68000);
+    INSN(addsubq,   5000, f080, M68K);
     BASE(addsubq,   5080, f0c0);
     INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
-    INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
-    INSN(dbcc,      50c8, f0f8, M68000);
+    INSN(scc,       50c0, f0c0, M68K);     /* Scc.B <EA> */
+    INSN(dbcc,      50c8, f0f8, M68K);
     INSN(trapcc,    50fa, f0fe, TRAPCC);   /* opmode 010, 011 */
     INSN(trapcc,    50fc, f0ff, TRAPCC);   /* opmode 100 */
     INSN(trapcc,    51fa, fffe, CF_ISA_A); /* TPF (trapf) opmode 010, 011 */
@@ -6137,15 +6137,15 @@ void register_m68k_insns (CPUM68KState *env)
     INSN(mvzs,      7100, f100, CF_ISA_B);
     BASE(or,        8000, f000);
     BASE(divw,      80c0, f0c0);
-    INSN(sbcd_reg,  8100, f1f8, M68000);
-    INSN(sbcd_mem,  8108, f1f8, M68000);
+    INSN(sbcd_reg,  8100, f1f8, M68K);
+    INSN(sbcd_mem,  8108, f1f8, M68K);
     BASE(addsub,    9000, f000);
     INSN(undef,     90c0, f0c0, CF_ISA_A);
     INSN(subx_reg,  9180, f1f8, CF_ISA_A);
-    INSN(subx_reg,  9100, f138, M68000);
-    INSN(subx_mem,  9108, f138, M68000);
+    INSN(subx_reg,  9100, f138, M68K);
+    INSN(subx_mem,  9108, f138, M68K);
     INSN(suba,      91c0, f1c0, CF_ISA_A);
-    INSN(suba,      90c0, f0c0, M68000);
+    INSN(suba,      90c0, f0c0, M68K);
 
     BASE(undef_mac, a000, f000);
     INSN(mac,       a000, f100, CF_EMAC);
@@ -6166,41 +6166,41 @@ void register_m68k_insns (CPUM68KState *env)
     INSN(cmpa,      b0c0, f1c0, CF_ISA_B); /* cmpa.w */
     INSN(cmp,       b080, f1c0, CF_ISA_A);
     INSN(cmpa,      b1c0, f1c0, CF_ISA_A);
-    INSN(cmp,       b000, f100, M68000);
-    INSN(eor,       b100, f100, M68000);
-    INSN(cmpm,      b108, f138, M68000);
-    INSN(cmpa,      b0c0, f0c0, M68000);
+    INSN(cmp,       b000, f100, M68K);
+    INSN(eor,       b100, f100, M68K);
+    INSN(cmpm,      b108, f138, M68K);
+    INSN(cmpa,      b0c0, f0c0, M68K);
     INSN(eor,       b180, f1c0, CF_ISA_A);
     BASE(and,       c000, f000);
-    INSN(exg_dd,    c140, f1f8, M68000);
-    INSN(exg_aa,    c148, f1f8, M68000);
-    INSN(exg_da,    c188, f1f8, M68000);
+    INSN(exg_dd,    c140, f1f8, M68K);
+    INSN(exg_aa,    c148, f1f8, M68K);
+    INSN(exg_da,    c188, f1f8, M68K);
     BASE(mulw,      c0c0, f0c0);
-    INSN(abcd_reg,  c100, f1f8, M68000);
-    INSN(abcd_mem,  c108, f1f8, M68000);
+    INSN(abcd_reg,  c100, f1f8, M68K);
+    INSN(abcd_mem,  c108, f1f8, M68K);
     BASE(addsub,    d000, f000);
     INSN(undef,     d0c0, f0c0, CF_ISA_A);
     INSN(addx_reg,      d180, f1f8, CF_ISA_A);
-    INSN(addx_reg,  d100, f138, M68000);
-    INSN(addx_mem,  d108, f138, M68000);
+    INSN(addx_reg,  d100, f138, M68K);
+    INSN(addx_mem,  d108, f138, M68K);
     INSN(adda,      d1c0, f1c0, CF_ISA_A);
-    INSN(adda,      d0c0, f0c0, M68000);
+    INSN(adda,      d0c0, f0c0, M68K);
     INSN(shift_im,  e080, f0f0, CF_ISA_A);
     INSN(shift_reg, e0a0, f0f0, CF_ISA_A);
-    INSN(shift8_im, e000, f0f0, M68000);
-    INSN(shift16_im, e040, f0f0, M68000);
-    INSN(shift_im,  e080, f0f0, M68000);
-    INSN(shift8_reg, e020, f0f0, M68000);
-    INSN(shift16_reg, e060, f0f0, M68000);
-    INSN(shift_reg, e0a0, f0f0, M68000);
-    INSN(shift_mem, e0c0, fcc0, M68000);
-    INSN(rotate_im, e090, f0f0, M68000);
-    INSN(rotate8_im, e010, f0f0, M68000);
-    INSN(rotate16_im, e050, f0f0, M68000);
-    INSN(rotate_reg, e0b0, f0f0, M68000);
-    INSN(rotate8_reg, e030, f0f0, M68000);
-    INSN(rotate16_reg, e070, f0f0, M68000);
-    INSN(rotate_mem, e4c0, fcc0, M68000);
+    INSN(shift8_im, e000, f0f0, M68K);
+    INSN(shift16_im, e040, f0f0, M68K);
+    INSN(shift_im,  e080, f0f0, M68K);
+    INSN(shift8_reg, e020, f0f0, M68K);
+    INSN(shift16_reg, e060, f0f0, M68K);
+    INSN(shift_reg, e0a0, f0f0, M68K);
+    INSN(shift_mem, e0c0, fcc0, M68K);
+    INSN(rotate_im, e090, f0f0, M68K);
+    INSN(rotate8_im, e010, f0f0, M68K);
+    INSN(rotate16_im, e050, f0f0, M68K);
+    INSN(rotate_reg, e0b0, f0f0, M68K);
+    INSN(rotate8_reg, e030, f0f0, M68K);
+    INSN(rotate16_reg, e070, f0f0, M68K);
+    INSN(rotate_mem, e4c0, fcc0, M68K);
     INSN(bfext_mem, e9c0, fdc0, BITFIELD);  /* bfextu & bfexts */
     INSN(bfext_reg, e9c0, fdf8, BITFIELD);
     INSN(bfins_mem, efc0, ffc0, BITFIELD);
-- 
2.30.2

[PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

[Mark Cave-Ayland]

There are already 32 feature bits in use, so change the size of the m68k
CPU features to uint64_t (allong with the associated m68k_feature()
functions) to allow up to 64 feature bits to be used.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/cpu.c | 4 ++--
 target/m68k/cpu.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index f681be3a2a..7b4797e2f1 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
 
 static void m68k_set_feature(CPUM68KState *env, int feature)
 {
-    env->features |= (1u << feature);
+    env->features |= (1ul << feature);
 }
 
 static void m68k_unset_feature(CPUM68KState *env, int feature)
 {
-    env->features &= (-1u - (1u << feature));
+    env->features &= (-1ul - (1ul << feature));
 }
 
 static void m68k_cpu_reset(DeviceState *dev)
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 67b6c12c28..d3384e5d98 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -154,7 +154,7 @@ typedef struct CPUArchState {
     struct {} end_reset_fields;
 
     /* Fields from here on are preserved across CPU reset. */
-    uint32_t features;
+    uint64_t features;
 } CPUM68KState;
 
 /*
@@ -539,9 +539,9 @@ enum m68k_features {
     M68K_FEATURE_TRAPCC,
 };
 
-static inline int m68k_feature(CPUM68KState *env, int feature)
+static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
 {
-    return (env->features & (1u << feature)) != 0;
+    return (env->features & (1ul << feature)) != 0;
 }
 
 void m68k_cpu_list(void);
-- 
2.30.2

[PATCH 3/4] target/m68k: use M68K_FEATURE_MOVEFROMSR_PRIV feature for move_from_sr privilege check

[Mark Cave-Ayland]

Now that M68K_FEATURE_M68000 has been renamed to M68K_FEATURE_M68K it is easier
to see that the privilege exception check is wrong: it is currently only generated
for ColdFire CPUs when in fact it should also be generated for Motorola CPUs from
the 68010 onwards.

Introduce a new M68K_FEATURE_MOVEFROMSR_PRIV feature which is set for all non-
Motorola CPUs, and for all Motorola CPUs from the 68010 onwards and use it to
determine whether a privilege exception should be generated for the MOVE-from-SR
instruction.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/cpu.c       | 5 +++++
 target/m68k/cpu.h       | 2 ++
 target/m68k/translate.c | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 7b4797e2f1..cc5311a4ac 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -102,6 +102,7 @@ static void m5206_cpu_initfn(Object *obj)
     CPUM68KState *env = &cpu->env;
 
     m68k_set_feature(env, M68K_FEATURE_CF_ISA_A);
+    m68k_set_feature(env, M68K_FEATURE_MOVEFROMSR_PRIV);
 }
 
 /* Base feature set, including isns. for m68k family */
@@ -129,6 +130,7 @@ static void m68010_cpu_initfn(Object *obj)
     m68k_set_feature(env, M68K_FEATURE_RTD);
     m68k_set_feature(env, M68K_FEATURE_BKPT);
     m68k_set_feature(env, M68K_FEATURE_MOVEC);
+    m68k_set_feature(env, M68K_FEATURE_MOVEFROMSR_PRIV);
 }
 
 /*
@@ -241,6 +243,7 @@ static void m5208_cpu_initfn(Object *obj)
     m68k_set_feature(env, M68K_FEATURE_BRAL);
     m68k_set_feature(env, M68K_FEATURE_CF_EMAC);
     m68k_set_feature(env, M68K_FEATURE_USP);
+    m68k_set_feature(env, M68K_FEATURE_MOVEFROMSR_PRIV);
 }
 
 static void cfv4e_cpu_initfn(Object *obj)
@@ -254,6 +257,7 @@ static void cfv4e_cpu_initfn(Object *obj)
     m68k_set_feature(env, M68K_FEATURE_CF_FPU);
     m68k_set_feature(env, M68K_FEATURE_CF_EMAC);
     m68k_set_feature(env, M68K_FEATURE_USP);
+    m68k_set_feature(env, M68K_FEATURE_MOVEFROMSR_PRIV);
 }
 
 static void any_cpu_initfn(Object *obj)
@@ -275,6 +279,7 @@ static void any_cpu_initfn(Object *obj)
     m68k_set_feature(env, M68K_FEATURE_USP);
     m68k_set_feature(env, M68K_FEATURE_EXT_FULL);
     m68k_set_feature(env, M68K_FEATURE_WORD_INDEX);
+    m68k_set_feature(env, M68K_FEATURE_MOVEFROMSR_PRIV);
 }
 
 static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index d3384e5d98..57936ea780 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -537,6 +537,8 @@ enum m68k_features {
     M68K_FEATURE_UNALIGNED_DATA,
     /* TRAPcc insn. (680[2346]0, and CPU32) */
     M68K_FEATURE_TRAPCC,
+    /* MOVE from SR privileged (from 68010) */
+    M68K_FEATURE_MOVEFROMSR_PRIV,
 };
 
 static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index fad8af8f83..be5561e1e9 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -4598,7 +4598,7 @@ DISAS_INSN(move_from_sr)
 {
     TCGv sr;
 
-    if (IS_USER(s) && !m68k_feature(env, M68K_FEATURE_M68K)) {
+    if (IS_USER(s) && m68k_feature(env, M68K_FEATURE_MOVEFROMSR_PRIV)) {
         gen_exception(s, s->base.pc_next, EXCP_PRIVILEGE);
         return;
     }
-- 
2.30.2

[PATCH 4/4] target/m68k: always call gen_exit_tb() after writes to SR

[Mark Cave-Ayland]

Any write to SR can change the security state so always call gen_exit_tb() when
this occurs. In particular MacOS makes use of andiw/oriw in a few places to
handle the switch between user and supervisor mode.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/translate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index be5561e1e9..892473d01f 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -2373,6 +2373,7 @@ DISAS_INSN(arith_im)
         tcg_gen_or_i32(dest, src1, im);
         if (with_SR) {
             gen_set_sr(s, dest, opsize == OS_BYTE);
+            gen_exit_tb(s);
         } else {
             DEST_EA(env, insn, opsize, dest, &addr);
             gen_logic_cc(s, dest, opsize);
@@ -2382,6 +2383,7 @@ DISAS_INSN(arith_im)
         tcg_gen_and_i32(dest, src1, im);
         if (with_SR) {
             gen_set_sr(s, dest, opsize == OS_BYTE);
+            gen_exit_tb(s);
         } else {
             DEST_EA(env, insn, opsize, dest, &addr);
             gen_logic_cc(s, dest, opsize);
@@ -2405,6 +2407,7 @@ DISAS_INSN(arith_im)
         tcg_gen_xor_i32(dest, src1, im);
         if (with_SR) {
             gen_set_sr(s, dest, opsize == OS_BYTE);
+            gen_exit_tb(s);
         } else {
             DEST_EA(env, insn, opsize, dest, &addr);
             gen_logic_cc(s, dest, opsize);
@@ -4592,6 +4595,7 @@ DISAS_INSN(strldsr)
     }
     gen_push(s, gen_get_sr(s));
     gen_set_sr_im(s, ext, 0);
+    gen_exit_tb(s);
 }
 
 DISAS_INSN(move_from_sr)
-- 
2.30.2

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

[BALATON Zoltan]

On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
> There are already 32 feature bits in use, so change the size of the m68k
> CPU features to uint64_t (allong with the associated m68k_feature()
> functions) to allow up to 64 feature bits to be used.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> target/m68k/cpu.c | 4 ++--
> target/m68k/cpu.h | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index f681be3a2a..7b4797e2f1 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>
> static void m68k_set_feature(CPUM68KState *env, int feature)
> {
> -    env->features |= (1u << feature);
> +    env->features |= (1ul << feature);
> }
>
> static void m68k_unset_feature(CPUM68KState *env, int feature)
> {
> -    env->features &= (-1u - (1u << feature));
> +    env->features &= (-1ul - (1ul << feature));

Should these be ull instead of ul?

Regards,
BALATON Zoltan

> }
>
> static void m68k_cpu_reset(DeviceState *dev)
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index 67b6c12c28..d3384e5d98 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>     struct {} end_reset_fields;
>
>     /* Fields from here on are preserved across CPU reset. */
> -    uint32_t features;
> +    uint64_t features;
> } CPUM68KState;
>
> /*
> @@ -539,9 +539,9 @@ enum m68k_features {
>     M68K_FEATURE_TRAPCC,
> };
>
> -static inline int m68k_feature(CPUM68KState *env, int feature)
> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
> {
> -    return (env->features & (1u << feature)) != 0;
> +    return (env->features & (1ul << feature)) != 0;
> }
>
> void m68k_cpu_list(void);
>

Re: [PATCH 1/4] target/m68k: rename M68K_FEATURE_M68000 to M68K_FEATURE_M68K

[Philippe Mathieu-Daudé via]

On 17/9/22 13:25, Mark Cave-Ayland wrote:
> The M68K_FEATURE_M68000 feature is misleading in that its name suggests the feature
> is defined just for Motorola 68000 CPUs, whilst in fact it is defined for all
> Motorola 680X0 CPUs.
> 
> In order to avoid confusion with the other M68K_FEATURE_M680X0 constants which
> define the features available for specific Motorola CPU models, rename
> M68K_FEATURE_M68000 to M68K_FEATURE_M68K and add comments to clarify its usage.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/cpu.c       |   2 +-
>   target/m68k/cpu.h       |   5 +-
>   target/m68k/helper.c    |   2 +-
>   target/m68k/op_helper.c |   2 +-
>   target/m68k/translate.c | 138 ++++++++++++++++++++--------------------
>   5 files changed, 75 insertions(+), 74 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

[Philippe Mathieu-Daudé via]

On 17/9/22 14:09, BALATON Zoltan wrote:
> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>> There are already 32 feature bits in use, so change the size of the m68k
>> CPU features to uint64_t (allong with the associated m68k_feature()
>> functions) to allow up to 64 feature bits to be used.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> target/m68k/cpu.c | 4 ++--
>> target/m68k/cpu.h | 6 +++---
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>> index f681be3a2a..7b4797e2f1 100644
>> --- a/target/m68k/cpu.c
>> +++ b/target/m68k/cpu.c
>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>
>> static void m68k_set_feature(CPUM68KState *env, int feature)
>> {
>> -    env->features |= (1u << feature);
>> +    env->features |= (1ul << feature);

         env->features = deposit64(env->features, feature, 1, 1);

>> }
>>
>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>> {
>> -    env->features &= (-1u - (1u << feature));
>> +    env->features &= (-1ul - (1ul << feature));

         env->features = deposit64(env->features, feature, 1, 0);

> Should these be ull instead of ul?

Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.

>> }
>>
>> static void m68k_cpu_reset(DeviceState *dev)
>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>> index 67b6c12c28..d3384e5d98 100644
>> --- a/target/m68k/cpu.h
>> +++ b/target/m68k/cpu.h
>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>     struct {} end_reset_fields;
>>
>>     /* Fields from here on are preserved across CPU reset. */
>> -    uint32_t features;
>> +    uint64_t features;
>> } CPUM68KState;
>>
>> /*
>> @@ -539,9 +539,9 @@ enum m68k_features {
>>     M68K_FEATURE_TRAPCC,
>> };
>>
>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)

Why uint64_t? Can we simplify using a boolean?

>> {
>> -    return (env->features & (1u << feature)) != 0;
>> +    return (env->features & (1ul << feature)) != 0;

         return extract64(env->features, feature, 1) == 1;

>> }
>>
>> void m68k_cpu_list(void);
>>
> 

Re: [PATCH 4/4] target/m68k: always call gen_exit_tb() after writes to SR

[Philippe Mathieu-Daudé via]

On 17/9/22 13:25, Mark Cave-Ayland wrote:
> Any write to SR can change the security state so always call gen_exit_tb() when
> this occurs. In particular MacOS makes use of andiw/oriw in a few places to
> handle the switch between user and supervisor mode.

Shouldn't be safer to add the gen_exit_tb() call in gen_set_sr[_im]()?

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/translate.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index be5561e1e9..892473d01f 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -2373,6 +2373,7 @@ DISAS_INSN(arith_im)
>           tcg_gen_or_i32(dest, src1, im);
>           if (with_SR) {
>               gen_set_sr(s, dest, opsize == OS_BYTE);
> +            gen_exit_tb(s);
>           } else {
>               DEST_EA(env, insn, opsize, dest, &addr);
>               gen_logic_cc(s, dest, opsize);
> @@ -2382,6 +2383,7 @@ DISAS_INSN(arith_im)
>           tcg_gen_and_i32(dest, src1, im);
>           if (with_SR) {
>               gen_set_sr(s, dest, opsize == OS_BYTE);
> +            gen_exit_tb(s);
>           } else {
>               DEST_EA(env, insn, opsize, dest, &addr);
>               gen_logic_cc(s, dest, opsize);
> @@ -2405,6 +2407,7 @@ DISAS_INSN(arith_im)
>           tcg_gen_xor_i32(dest, src1, im);
>           if (with_SR) {
>               gen_set_sr(s, dest, opsize == OS_BYTE);
> +            gen_exit_tb(s);
>           } else {
>               DEST_EA(env, insn, opsize, dest, &addr);
>               gen_logic_cc(s, dest, opsize);
> @@ -4592,6 +4595,7 @@ DISAS_INSN(strldsr)
>       }
>       gen_push(s, gen_get_sr(s));
>       gen_set_sr_im(s, ext, 0);
> +    gen_exit_tb(s);
>   }
>   
>   DISAS_INSN(move_from_sr)

Re: [PATCH 4/4] target/m68k: always call gen_exit_tb() after writes to SR

[Richard Henderson]

On 9/18/22 00:29, Philippe Mathieu-Daudé wrote:
> On 17/9/22 13:25, Mark Cave-Ayland wrote:
>> Any write to SR can change the security state so always call gen_exit_tb() when
>> this occurs. In particular MacOS makes use of andiw/oriw in a few places to
>> handle the switch between user and supervisor mode.
> 
> Shouldn't be safer to add the gen_exit_tb() call in gen_set_sr[_im]()?

No.  For halt we need to raise EXCP_HLT.

r~

> 
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   target/m68k/translate.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index be5561e1e9..892473d01f 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -2373,6 +2373,7 @@ DISAS_INSN(arith_im)
>>           tcg_gen_or_i32(dest, src1, im);
>>           if (with_SR) {
>>               gen_set_sr(s, dest, opsize == OS_BYTE);
>> +            gen_exit_tb(s);
>>           } else {
>>               DEST_EA(env, insn, opsize, dest, &addr);
>>               gen_logic_cc(s, dest, opsize);
>> @@ -2382,6 +2383,7 @@ DISAS_INSN(arith_im)
>>           tcg_gen_and_i32(dest, src1, im);
>>           if (with_SR) {
>>               gen_set_sr(s, dest, opsize == OS_BYTE);
>> +            gen_exit_tb(s);
>>           } else {
>>               DEST_EA(env, insn, opsize, dest, &addr);
>>               gen_logic_cc(s, dest, opsize);
>> @@ -2405,6 +2407,7 @@ DISAS_INSN(arith_im)
>>           tcg_gen_xor_i32(dest, src1, im);
>>           if (with_SR) {
>>               gen_set_sr(s, dest, opsize == OS_BYTE);
>> +            gen_exit_tb(s);
>>           } else {
>>               DEST_EA(env, insn, opsize, dest, &addr);
>>               gen_logic_cc(s, dest, opsize);
>> @@ -4592,6 +4595,7 @@ DISAS_INSN(strldsr)
>>       }
>>       gen_push(s, gen_get_sr(s));
>>       gen_set_sr_im(s, ext, 0);
>> +    gen_exit_tb(s);
>>   }
>>   DISAS_INSN(move_from_sr)
> 

Re: [PATCH 4/4] target/m68k: always call gen_exit_tb() after writes to SR

[Richard Henderson]

On 9/17/22 13:25, Mark Cave-Ayland wrote:
> Any write to SR can change the security state so always call gen_exit_tb() when
> this occurs. In particular MacOS makes use of andiw/oriw in a few places to
> handle the switch between user and supervisor mode.
> 
> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/translate.c | 4 ++++
>   1 file changed, 4 insertions(+)

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

r~

Re: [PATCH 3/4] target/m68k: use M68K_FEATURE_MOVEFROMSR_PRIV feature for move_from_sr privilege check

[Richard Henderson]

On 9/17/22 13:25, Mark Cave-Ayland wrote:
> Now that M68K_FEATURE_M68000 has been renamed to M68K_FEATURE_M68K it is easier
> to see that the privilege exception check is wrong: it is currently only generated
> for ColdFire CPUs when in fact it should also be generated for Motorola CPUs from
> the 68010 onwards.
> 
> Introduce a new M68K_FEATURE_MOVEFROMSR_PRIV feature which is set for all non-
> Motorola CPUs, and for all Motorola CPUs from the 68010 onwards and use it to
> determine whether a privilege exception should be generated for the MOVE-from-SR
> instruction.
> 
> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/cpu.c       | 5 +++++
>   target/m68k/cpu.h       | 2 ++
>   target/m68k/translate.c | 2 +-
>   3 files changed, 8 insertions(+), 1 deletion(-)

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

r~

Re: [PATCH 1/4] target/m68k: rename M68K_FEATURE_M68000 to M68K_FEATURE_M68K

[Richard Henderson]

On 9/17/22 13:25, Mark Cave-Ayland wrote:
> The M68K_FEATURE_M68000 feature is misleading in that its name suggests the feature
> is defined just for Motorola 68000 CPUs, whilst in fact it is defined for all
> Motorola 680X0 CPUs.
> 
> In order to avoid confusion with the other M68K_FEATURE_M680X0 constants which
> define the features available for specific Motorola CPU models, rename
> M68K_FEATURE_M68000 to M68K_FEATURE_M68K and add comments to clarify its usage.
> 
> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/cpu.c       |   2 +-
>   target/m68k/cpu.h       |   5 +-
>   target/m68k/helper.c    |   2 +-
>   target/m68k/op_helper.c |   2 +-
>   target/m68k/translate.c | 138 ++++++++++++++++++++--------------------
>   5 files changed, 75 insertions(+), 74 deletions(-)

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

r~

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

[Mark Cave-Ayland]

On 17/09/2022 13:09, BALATON Zoltan wrote:

> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>> There are already 32 feature bits in use, so change the size of the m68k
>> CPU features to uint64_t (allong with the associated m68k_feature()
>> functions) to allow up to 64 feature bits to be used.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> target/m68k/cpu.c | 4 ++--
>> target/m68k/cpu.h | 6 +++---
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>> index f681be3a2a..7b4797e2f1 100644
>> --- a/target/m68k/cpu.c
>> +++ b/target/m68k/cpu.c
>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>
>> static void m68k_set_feature(CPUM68KState *env, int feature)
>> {
>> -    env->features |= (1u << feature);
>> +    env->features |= (1ul << feature);
>> }
>>
>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>> {
>> -    env->features &= (-1u - (1u << feature));
>> +    env->features &= (-1ul - (1ul << feature));
> 
> Should these be ull instead of ul?

Indeed, it looks like Windows needs ULL in order to work correctly with uint64_t - I 
can easily fix that in v2.

>> }
>>
>> static void m68k_cpu_reset(DeviceState *dev)
>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>> index 67b6c12c28..d3384e5d98 100644
>> --- a/target/m68k/cpu.h
>> +++ b/target/m68k/cpu.h
>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>     struct {} end_reset_fields;
>>
>>     /* Fields from here on are preserved across CPU reset. */
>> -    uint32_t features;
>> +    uint64_t features;
>> } CPUM68KState;
>>
>> /*
>> @@ -539,9 +539,9 @@ enum m68k_features {
>>     M68K_FEATURE_TRAPCC,
>> };
>>
>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
>> {
>> -    return (env->features & (1u << feature)) != 0;
>> +    return (env->features & (1ul << feature)) != 0;
>> }
>>
>> void m68k_cpu_list(void);


ATB,

Mark.

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

[Mark Cave-Ayland]

On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:

> On 17/9/22 14:09, BALATON Zoltan wrote:
>> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>>> There are already 32 feature bits in use, so change the size of the m68k
>>> CPU features to uint64_t (allong with the associated m68k_feature()
>>> functions) to allow up to 64 feature bits to be used.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> target/m68k/cpu.c | 4 ++--
>>> target/m68k/cpu.h | 6 +++---
>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>>> index f681be3a2a..7b4797e2f1 100644
>>> --- a/target/m68k/cpu.c
>>> +++ b/target/m68k/cpu.c
>>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>>
>>> static void m68k_set_feature(CPUM68KState *env, int feature)
>>> {
>>> -    env->features |= (1u << feature);
>>> +    env->features |= (1ul << feature);
> 
>          env->features = deposit64(env->features, feature, 1, 1);
> 
>>> }
>>>
>>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>>> {
>>> -    env->features &= (-1u - (1u << feature));
>>> +    env->features &= (-1ul - (1ul << feature));
> 
>          env->features = deposit64(env->features, feature, 1, 0);
> 
>> Should these be ull instead of ul?
> 
> Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.

I must admit I find the deposit64() variants not particularly easy to read: if we're 
considering alterations rather than changing the constant suffix then I'd much rather 
go for:

     env->features |= (1ULL << feature);

and:

     env->features &= ~(1ULL << feature);

Laurent, what would be your preference?

>>> }
>>>
>>> static void m68k_cpu_reset(DeviceState *dev)
>>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>>> index 67b6c12c28..d3384e5d98 100644
>>> --- a/target/m68k/cpu.h
>>> +++ b/target/m68k/cpu.h
>>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>>     struct {} end_reset_fields;
>>>
>>>     /* Fields from here on are preserved across CPU reset. */
>>> -    uint32_t features;
>>> +    uint64_t features;
>>> } CPUM68KState;
>>>
>>> /*
>>> @@ -539,9 +539,9 @@ enum m68k_features {
>>>     M68K_FEATURE_TRAPCC,
>>> };
>>>
>>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
> 
> Why uint64_t? Can we simplify using a boolean?

I don't really feel strongly either way here. Again I'm happy to go with whatever 
Laurent would prefer as maintainer.

>>> {
>>> -    return (env->features & (1u << feature)) != 0;
>>> +    return (env->features & (1ul << feature)) != 0;
> 
>          return extract64(env->features, feature, 1) == 1;
> 
>>> }
>>>
>>> void m68k_cpu_list(void);


ATB,

Mark.

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

[Philippe Mathieu-Daudé via]

On Tue, Sep 20, 2022 at 6:30 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:
> > On 17/9/22 14:09, BALATON Zoltan wrote:
> >> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
> >>> There are already 32 feature bits in use, so change the size of the m68k
> >>> CPU features to uint64_t (allong with the associated m68k_feature()
> >>> functions) to allow up to 64 feature bits to be used.
> >>>
> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>> ---
> >>> target/m68k/cpu.c | 4 ++--
> >>> target/m68k/cpu.h | 6 +++---
> >>> 2 files changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> >>> index f681be3a2a..7b4797e2f1 100644
> >>> --- a/target/m68k/cpu.c
> >>> +++ b/target/m68k/cpu.c
> >>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
> >>>
> >>> static void m68k_set_feature(CPUM68KState *env, int feature)
> >>> {
> >>> -    env->features |= (1u << feature);
> >>> +    env->features |= (1ul << feature);
> >
> >          env->features = deposit64(env->features, feature, 1, 1);
> >
> >>> }
> >>>
> >>> static void m68k_unset_feature(CPUM68KState *env, int feature)
> >>> {
> >>> -    env->features &= (-1u - (1u << feature));
> >>> +    env->features &= (-1ul - (1ul << feature));
> >
> >          env->features = deposit64(env->features, feature, 1, 0);
> >
> >> Should these be ull instead of ul?
> >
> > Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.
>
> I must admit I find the deposit64() variants not particularly easy to read: if we're
> considering alterations rather than changing the constant suffix then I'd much rather
> go for:
>
>      env->features |= (1ULL << feature);
>
> and:
>
>      env->features &= ~(1ULL << feature);
>
> Laurent, what would be your preference?

OK, no need to change then.

> >>> -static inline int m68k_feature(CPUM68KState *env, int feature)
> >>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
> >
> > Why uint64_t? Can we simplify using a boolean?
>
> I don't really feel strongly either way here. Again I'm happy to go with whatever
> Laurent would prefer as maintainer.

Preferably using boolean:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 4/4] target/m68k: always call gen_exit_tb() after writes to SR

[Philippe Mathieu-Daudé via]

On 19/9/22 10:13, Richard Henderson wrote:
> On 9/18/22 00:29, Philippe Mathieu-Daudé wrote:
>> On 17/9/22 13:25, Mark Cave-Ayland wrote:
>>> Any write to SR can change the security state so always call 
>>> gen_exit_tb() when
>>> this occurs. In particular MacOS makes use of andiw/oriw in a few 
>>> places to
>>> handle the switch between user and supervisor mode.
>>
>> Shouldn't be safer to add the gen_exit_tb() call in gen_set_sr[_im]()?
> 
> No.  For halt we need to raise EXCP_HLT.

Right, I should have looked at translate.c; I also noticed the ccr_only
flag. So:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

[BALATON Zoltan]

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

On Tue, 20 Sep 2022, Mark Cave-Ayland wrote:
> On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:
>
>> On 17/9/22 14:09, BALATON Zoltan wrote:
>>> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>>>> There are already 32 feature bits in use, so change the size of the m68k
>>>> CPU features to uint64_t (allong with the associated m68k_feature()
>>>> functions) to allow up to 64 feature bits to be used.
>>>> 
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> target/m68k/cpu.c | 4 ++--
>>>> target/m68k/cpu.h | 6 +++---
>>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>>>> index f681be3a2a..7b4797e2f1 100644
>>>> --- a/target/m68k/cpu.c
>>>> +++ b/target/m68k/cpu.c
>>>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>>> 
>>>> static void m68k_set_feature(CPUM68KState *env, int feature)
>>>> {
>>>> -    env->features |= (1u << feature);
>>>> +    env->features |= (1ul << feature);
>>
>>          env->features = deposit64(env->features, feature, 1, 1);
>> 
>>>> }
>>>> 
>>>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>>>> {
>>>> -    env->features &= (-1u - (1u << feature));
>>>> +    env->features &= (-1ul - (1ul << feature));
>>
>>          env->features = deposit64(env->features, feature, 1, 0);
>> 
>>> Should these be ull instead of ul?
>> 
>> Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.
>
> I must admit I find the deposit64() variants not particularly easy to read:

I agree with that and also dislike the dposit/extract functions that would 
not bring much here. Maybe they are useful for multiple bits but for a 
single bit they just add overhead and obfuscation.

> if we're considering alterations rather than changing the constant suffix 
> then I'd much rather go for:
>
>    env->features |= (1ULL << feature);
>
> and:
>
>    env->features &= ~(1ULL << feature);

There's also a BIT_ULL macro which could be used but it's up to you, 
shifting 1ULL is also simple enough to read.

Regards,
BALATON Zoltan

> Laurent, what would be your preference?
>
>>>> }
>>>> 
>>>> static void m68k_cpu_reset(DeviceState *dev)
>>>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>>>> index 67b6c12c28..d3384e5d98 100644
>>>> --- a/target/m68k/cpu.h
>>>> +++ b/target/m68k/cpu.h
>>>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>>>     struct {} end_reset_fields;
>>>> 
>>>>     /* Fields from here on are preserved across CPU reset. */
>>>> -    uint32_t features;
>>>> +    uint64_t features;
>>>> } CPUM68KState;
>>>> 
>>>> /*
>>>> @@ -539,9 +539,9 @@ enum m68k_features {
>>>>     M68K_FEATURE_TRAPCC,
>>>> };
>>>> 
>>>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>>>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
>> 
>> Why uint64_t? Can we simplify using a boolean?
>
> I don't really feel strongly either way here. Again I'm happy to go with 
> whatever Laurent would prefer as maintainer.
>
>>>> {
>>>> -    return (env->features & (1u << feature)) != 0;
>>>> +    return (env->features & (1ul << feature)) != 0;
>>
>>          return extract64(env->features, feature, 1) == 1;
>> 
>>>> }
>>>> 
>>>> void m68k_cpu_list(void);
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH 1/4] target/m68k: rename M68K_FEATURE_M68000 to M68K_FEATURE_M68K

[Laurent Vivier]

Le 17/09/2022 à 13:25, Mark Cave-Ayland a écrit :
> The M68K_FEATURE_M68000 feature is misleading in that its name suggests the feature
> is defined just for Motorola 68000 CPUs, whilst in fact it is defined for all
> Motorola 680X0 CPUs.
> 
> In order to avoid confusion with the other M68K_FEATURE_M680X0 constants which
> define the features available for specific Motorola CPU models, rename
> M68K_FEATURE_M68000 to M68K_FEATURE_M68K and add comments to clarify its usage.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/cpu.c       |   2 +-
>   target/m68k/cpu.h       |   5 +-
>   target/m68k/helper.c    |   2 +-
>   target/m68k/op_helper.c |   2 +-
>   target/m68k/translate.c | 138 ++++++++++++++++++++--------------------
>   5 files changed, 75 insertions(+), 74 deletions(-)
> 
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 5bbefda575..f681be3a2a 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -110,7 +110,7 @@ static void m68000_cpu_initfn(Object *obj)
>       M68kCPU *cpu = M68K_CPU(obj);
>       CPUM68KState *env = &cpu->env;
>   
> -    m68k_set_feature(env, M68K_FEATURE_M68000);
> +    m68k_set_feature(env, M68K_FEATURE_M68K);
>       m68k_set_feature(env, M68K_FEATURE_USP);
>       m68k_set_feature(env, M68K_FEATURE_WORD_INDEX);
>       m68k_set_feature(env, M68K_FEATURE_MOVEP);
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index 4d8f48e8c7..67b6c12c28 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -480,8 +480,9 @@ void do_m68k_semihosting(CPUM68KState *env, int nr);
>    */
>   
>   enum m68k_features {
> -    /* Base m68k instruction set */
> -    M68K_FEATURE_M68000,
> +    /* Base Motorola CPU set (not set for Coldfire CPUs) */
> +    M68K_FEATURE_M68K,
> +    /* Motorola CPU feature sets */
>       M68K_FEATURE_M68010,
>       M68K_FEATURE_M68020,
>       M68K_FEATURE_M68030,
> diff --git a/target/m68k/helper.c b/target/m68k/helper.c
> index 5728e48585..4621cf2402 100644
> --- a/target/m68k/helper.c
> +++ b/target/m68k/helper.c
> @@ -460,7 +460,7 @@ void m68k_switch_sp(CPUM68KState *env)
>       int new_sp;
>   
>       env->sp[env->current_sp] = env->aregs[7];
> -    if (m68k_feature(env, M68K_FEATURE_M68000)) {
> +    if (m68k_feature(env, M68K_FEATURE_M68K)) {
>           if (env->sr & SR_S) {
>               /* SR:Master-Mode bit unimplemented then ISP is not available */
>               if (!m68k_feature(env, M68K_FEATURE_MSP) || env->sr & SR_M) {
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index d9937ca8dc..99dc994fcb 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -433,7 +433,7 @@ static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
>   
>   static void do_interrupt_all(CPUM68KState *env, int is_hw)
>   {
> -    if (m68k_feature(env, M68K_FEATURE_M68000)) {
> +    if (m68k_feature(env, M68K_FEATURE_M68K)) {
>           m68k_interrupt_all(env, is_hw);
>           return;
>       }
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 5098f7e570..fad8af8f83 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -471,7 +471,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext *s, TCGv base)
>       if ((ext & 0x800) == 0 && !m68k_feature(s->env, M68K_FEATURE_WORD_INDEX))
>           return NULL_QREG;
>   
> -    if (m68k_feature(s->env, M68K_FEATURE_M68000) &&
> +    if (m68k_feature(s->env, M68K_FEATURE_M68K) &&
>           !m68k_feature(s->env, M68K_FEATURE_SCALED_INDEX)) {
>           ext &= ~(3 << 9);
>       }
> @@ -804,7 +804,7 @@ static TCGv gen_lea_mode(CPUM68KState *env, DisasContext *s,
>           reg = get_areg(s, reg0);
>           tmp = mark_to_release(s, tcg_temp_new());
>           if (reg0 == 7 && opsize == OS_BYTE &&
> -            m68k_feature(s->env, M68K_FEATURE_M68000)) {
> +            m68k_feature(s->env, M68K_FEATURE_M68K)) {
>               tcg_gen_subi_i32(tmp, reg, 2);
>           } else {
>               tcg_gen_subi_i32(tmp, reg, opsize_bytes(opsize));
> @@ -888,7 +888,7 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext *s, int mode, int reg0,
>           if (what == EA_STORE || !addrp) {
>               TCGv tmp = tcg_temp_new();
>               if (reg0 == 7 && opsize == OS_BYTE &&
> -                m68k_feature(s->env, M68K_FEATURE_M68000)) {
> +                m68k_feature(s->env, M68K_FEATURE_M68K)) {
>                   tcg_gen_addi_i32(tmp, reg, 2);
>               } else {
>                   tcg_gen_addi_i32(tmp, reg, opsize_bytes(opsize));
> @@ -2210,7 +2210,7 @@ DISAS_INSN(bitop_im)
>       op = (insn >> 6) & 3;
>   
>       bitnum = read_im16(env, s);
> -    if (m68k_feature(s->env, M68K_FEATURE_M68000)) {
> +    if (m68k_feature(s->env, M68K_FEATURE_M68K)) {
>           if (bitnum & 0xfe00) {
>               disas_undef(env, s, insn);
>               return;
> @@ -2875,7 +2875,7 @@ DISAS_INSN(mull)
>           return;
>       }
>       SRC_EA(env, src1, OS_LONG, 0, NULL);
> -    if (m68k_feature(s->env, M68K_FEATURE_M68000)) {
> +    if (m68k_feature(s->env, M68K_FEATURE_M68K)) {
>           tcg_gen_movi_i32(QREG_CC_C, 0);
>           if (sign) {
>               tcg_gen_muls2_i32(QREG_CC_N, QREG_CC_V, src1, DREG(ext, 12));
> @@ -3470,7 +3470,7 @@ static inline void shift_im(DisasContext *s, uint16_t insn, int opsize)
>            * while M68000 sets if the most significant bit is changed at
>            * any time during the shift operation.
>            */
> -        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68000)) {
> +        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68K)) {
>               /* if shift count >= bits, V is (reg != 0) */
>               if (count >= bits) {
>                   tcg_gen_setcond_i32(TCG_COND_NE, QREG_CC_V, reg, QREG_CC_V);
> @@ -3554,7 +3554,7 @@ static inline void shift_reg(DisasContext *s, uint16_t insn, int opsize)
>            *     int64_t t = (int64_t)(intN_t)reg << count;
>            *     V = ((s ^ t) & (-1 << (bits - 1))) != 0
>            */
> -        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68000)) {
> +        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68K)) {
>               TCGv_i64 tt = tcg_const_i64(32);
>               /* if shift is greater than 32, use 32 */
>               tcg_gen_movcond_i64(TCG_COND_GT, s64, s64, tt, tt, s64);
> @@ -3647,7 +3647,7 @@ DISAS_INSN(shift_mem)
>            * while M68000 sets if the most significant bit is changed at
>            * any time during the shift operation
>            */
> -        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68000)) {
> +        if (!logical && m68k_feature(s->env, M68K_FEATURE_M68K)) {
>               src = gen_extend(s, src, OS_WORD, 1);
>               tcg_gen_xor_i32(QREG_CC_V, QREG_CC_N, src);
>           }
> @@ -4598,7 +4598,7 @@ DISAS_INSN(move_from_sr)
>   {
>       TCGv sr;
>   
> -    if (IS_USER(s) && !m68k_feature(env, M68K_FEATURE_M68000)) {
> +    if (IS_USER(s) && !m68k_feature(env, M68K_FEATURE_M68K)) {
>           gen_exception(s, s->base.pc_next, EXCP_PRIVILEGE);
>           return;
>       }
> @@ -6011,7 +6011,7 @@ void register_m68k_insns (CPUM68KState *env)
>       } while(0)
>       BASE(undef,     0000, 0000);
>       INSN(arith_im,  0080, fff8, CF_ISA_A);
> -    INSN(arith_im,  0000, ff00, M68000);
> +    INSN(arith_im,  0000, ff00, M68K);
>       INSN(chk2,      00c0, f9c0, CHK2);
>       INSN(bitrev,    00c0, fff8, CF_ISA_APLUSC);
>       BASE(bitop_reg, 0100, f1c0);
> @@ -6020,26 +6020,26 @@ void register_m68k_insns (CPUM68KState *env)
>       BASE(bitop_reg, 01c0, f1c0);
>       INSN(movep,     0108, f138, MOVEP);
>       INSN(arith_im,  0280, fff8, CF_ISA_A);
> -    INSN(arith_im,  0200, ff00, M68000);
> -    INSN(undef,     02c0, ffc0, M68000);
> +    INSN(arith_im,  0200, ff00, M68K);
> +    INSN(undef,     02c0, ffc0, M68K);
>       INSN(byterev,   02c0, fff8, CF_ISA_APLUSC);
>       INSN(arith_im,  0480, fff8, CF_ISA_A);
> -    INSN(arith_im,  0400, ff00, M68000);
> -    INSN(undef,     04c0, ffc0, M68000);
> -    INSN(arith_im,  0600, ff00, M68000);
> -    INSN(undef,     06c0, ffc0, M68000);
> +    INSN(arith_im,  0400, ff00, M68K);
> +    INSN(undef,     04c0, ffc0, M68K);
> +    INSN(arith_im,  0600, ff00, M68K);
> +    INSN(undef,     06c0, ffc0, M68K);
>       INSN(ff1,       04c0, fff8, CF_ISA_APLUSC);
>       INSN(arith_im,  0680, fff8, CF_ISA_A);
>       INSN(arith_im,  0c00, ff38, CF_ISA_A);
> -    INSN(arith_im,  0c00, ff00, M68000);
> +    INSN(arith_im,  0c00, ff00, M68K);
>       BASE(bitop_im,  0800, ffc0);
>       BASE(bitop_im,  0840, ffc0);
>       BASE(bitop_im,  0880, ffc0);
>       BASE(bitop_im,  08c0, ffc0);
>       INSN(arith_im,  0a80, fff8, CF_ISA_A);
> -    INSN(arith_im,  0a00, ff00, M68000);
> +    INSN(arith_im,  0a00, ff00, M68K);
>   #if defined(CONFIG_SOFTMMU)
> -    INSN(moves,     0e00, ff00, M68000);
> +    INSN(moves,     0e00, ff00, M68K);
>   #endif
>       INSN(cas,       0ac0, ffc0, CAS);
>       INSN(cas,       0cc0, ffc0, CAS);
> @@ -6049,44 +6049,44 @@ void register_m68k_insns (CPUM68KState *env)
>       BASE(move,      1000, f000);
>       BASE(move,      2000, f000);
>       BASE(move,      3000, f000);
> -    INSN(chk,       4000, f040, M68000);
> +    INSN(chk,       4000, f040, M68K);
>       INSN(strldsr,   40e7, ffff, CF_ISA_APLUSC);
>       INSN(negx,      4080, fff8, CF_ISA_A);
> -    INSN(negx,      4000, ff00, M68000);
> -    INSN(undef,     40c0, ffc0, M68000);
> +    INSN(negx,      4000, ff00, M68K);
> +    INSN(undef,     40c0, ffc0, M68K);
>       INSN(move_from_sr, 40c0, fff8, CF_ISA_A);
> -    INSN(move_from_sr, 40c0, ffc0, M68000);
> +    INSN(move_from_sr, 40c0, ffc0, M68K);
>       BASE(lea,       41c0, f1c0);
>       BASE(clr,       4200, ff00);
>       BASE(undef,     42c0, ffc0);
>       INSN(move_from_ccr, 42c0, fff8, CF_ISA_A);
> -    INSN(move_from_ccr, 42c0, ffc0, M68000);
> +    INSN(move_from_ccr, 42c0, ffc0, M68K);
>       INSN(neg,       4480, fff8, CF_ISA_A);
> -    INSN(neg,       4400, ff00, M68000);
> -    INSN(undef,     44c0, ffc0, M68000);
> +    INSN(neg,       4400, ff00, M68K);
> +    INSN(undef,     44c0, ffc0, M68K);
>       BASE(move_to_ccr, 44c0, ffc0);
>       INSN(not,       4680, fff8, CF_ISA_A);
> -    INSN(not,       4600, ff00, M68000);
> +    INSN(not,       4600, ff00, M68K);
>   #if defined(CONFIG_SOFTMMU)
>       BASE(move_to_sr, 46c0, ffc0);
>   #endif
> -    INSN(nbcd,      4800, ffc0, M68000);
> -    INSN(linkl,     4808, fff8, M68000);
> +    INSN(nbcd,      4800, ffc0, M68K);
> +    INSN(linkl,     4808, fff8, M68K);
>       BASE(pea,       4840, ffc0);
>       BASE(swap,      4840, fff8);
>       INSN(bkpt,      4848, fff8, BKPT);
>       INSN(movem,     48d0, fbf8, CF_ISA_A);
>       INSN(movem,     48e8, fbf8, CF_ISA_A);
> -    INSN(movem,     4880, fb80, M68000);
> +    INSN(movem,     4880, fb80, M68K);
>       BASE(ext,       4880, fff8);
>       BASE(ext,       48c0, fff8);
>       BASE(ext,       49c0, fff8);
>       BASE(tst,       4a00, ff00);
>       INSN(tas,       4ac0, ffc0, CF_ISA_B);
> -    INSN(tas,       4ac0, ffc0, M68000);
> +    INSN(tas,       4ac0, ffc0, M68K);
>   #if defined(CONFIG_SOFTMMU)
>       INSN(halt,      4ac8, ffff, CF_ISA_A);
> -    INSN(halt,      4ac8, ffff, M68060);
> +    INSN(halt,      4ac8, ffff, M68K);
>   #endif
>       INSN(pulse,     4acc, ffff, CF_ISA_A);
>       BASE(illegal,   4afc, ffff);
> @@ -6101,7 +6101,7 @@ void register_m68k_insns (CPUM68KState *env)
>   #if defined(CONFIG_SOFTMMU)
>       INSN(move_to_usp, 4e60, fff8, USP);
>       INSN(move_from_usp, 4e68, fff8, USP);
> -    INSN(reset,     4e70, ffff, M68000);
> +    INSN(reset,     4e70, ffff, M68K);
>       BASE(stop,      4e72, ffff);
>       BASE(rte,       4e73, ffff);
>       INSN(cf_movec,  4e7b, ffff, CF_ISA_A);
> @@ -6110,15 +6110,15 @@ void register_m68k_insns (CPUM68KState *env)
>       BASE(nop,       4e71, ffff);
>       INSN(rtd,       4e74, ffff, RTD);
>       BASE(rts,       4e75, ffff);
> -    INSN(trapv,     4e76, ffff, M68000);
> -    INSN(rtr,       4e77, ffff, M68000);
> +    INSN(trapv,     4e76, ffff, M68K);
> +    INSN(rtr,       4e77, ffff, M68K);
>       BASE(jump,      4e80, ffc0);
>       BASE(jump,      4ec0, ffc0);
> -    INSN(addsubq,   5000, f080, M68000);
> +    INSN(addsubq,   5000, f080, M68K);
>       BASE(addsubq,   5080, f0c0);
>       INSN(scc,       50c0, f0f8, CF_ISA_A); /* Scc.B Dx   */
> -    INSN(scc,       50c0, f0c0, M68000);   /* Scc.B <EA> */
> -    INSN(dbcc,      50c8, f0f8, M68000);
> +    INSN(scc,       50c0, f0c0, M68K);     /* Scc.B <EA> */
> +    INSN(dbcc,      50c8, f0f8, M68K);
>       INSN(trapcc,    50fa, f0fe, TRAPCC);   /* opmode 010, 011 */
>       INSN(trapcc,    50fc, f0ff, TRAPCC);   /* opmode 100 */
>       INSN(trapcc,    51fa, fffe, CF_ISA_A); /* TPF (trapf) opmode 010, 011 */
> @@ -6137,15 +6137,15 @@ void register_m68k_insns (CPUM68KState *env)
>       INSN(mvzs,      7100, f100, CF_ISA_B);
>       BASE(or,        8000, f000);
>       BASE(divw,      80c0, f0c0);
> -    INSN(sbcd_reg,  8100, f1f8, M68000);
> -    INSN(sbcd_mem,  8108, f1f8, M68000);
> +    INSN(sbcd_reg,  8100, f1f8, M68K);
> +    INSN(sbcd_mem,  8108, f1f8, M68K);
>       BASE(addsub,    9000, f000);
>       INSN(undef,     90c0, f0c0, CF_ISA_A);
>       INSN(subx_reg,  9180, f1f8, CF_ISA_A);
> -    INSN(subx_reg,  9100, f138, M68000);
> -    INSN(subx_mem,  9108, f138, M68000);
> +    INSN(subx_reg,  9100, f138, M68K);
> +    INSN(subx_mem,  9108, f138, M68K);
>       INSN(suba,      91c0, f1c0, CF_ISA_A);
> -    INSN(suba,      90c0, f0c0, M68000);
> +    INSN(suba,      90c0, f0c0, M68K);
>   
>       BASE(undef_mac, a000, f000);
>       INSN(mac,       a000, f100, CF_EMAC);
> @@ -6166,41 +6166,41 @@ void register_m68k_insns (CPUM68KState *env)
>       INSN(cmpa,      b0c0, f1c0, CF_ISA_B); /* cmpa.w */
>       INSN(cmp,       b080, f1c0, CF_ISA_A);
>       INSN(cmpa,      b1c0, f1c0, CF_ISA_A);
> -    INSN(cmp,       b000, f100, M68000);
> -    INSN(eor,       b100, f100, M68000);
> -    INSN(cmpm,      b108, f138, M68000);
> -    INSN(cmpa,      b0c0, f0c0, M68000);
> +    INSN(cmp,       b000, f100, M68K);
> +    INSN(eor,       b100, f100, M68K);
> +    INSN(cmpm,      b108, f138, M68K);
> +    INSN(cmpa,      b0c0, f0c0, M68K);
>       INSN(eor,       b180, f1c0, CF_ISA_A);
>       BASE(and,       c000, f000);
> -    INSN(exg_dd,    c140, f1f8, M68000);
> -    INSN(exg_aa,    c148, f1f8, M68000);
> -    INSN(exg_da,    c188, f1f8, M68000);
> +    INSN(exg_dd,    c140, f1f8, M68K);
> +    INSN(exg_aa,    c148, f1f8, M68K);
> +    INSN(exg_da,    c188, f1f8, M68K);
>       BASE(mulw,      c0c0, f0c0);
> -    INSN(abcd_reg,  c100, f1f8, M68000);
> -    INSN(abcd_mem,  c108, f1f8, M68000);
> +    INSN(abcd_reg,  c100, f1f8, M68K);
> +    INSN(abcd_mem,  c108, f1f8, M68K);
>       BASE(addsub,    d000, f000);
>       INSN(undef,     d0c0, f0c0, CF_ISA_A);
>       INSN(addx_reg,      d180, f1f8, CF_ISA_A);
> -    INSN(addx_reg,  d100, f138, M68000);
> -    INSN(addx_mem,  d108, f138, M68000);
> +    INSN(addx_reg,  d100, f138, M68K);
> +    INSN(addx_mem,  d108, f138, M68K);
>       INSN(adda,      d1c0, f1c0, CF_ISA_A);
> -    INSN(adda,      d0c0, f0c0, M68000);
> +    INSN(adda,      d0c0, f0c0, M68K);
>       INSN(shift_im,  e080, f0f0, CF_ISA_A);
>       INSN(shift_reg, e0a0, f0f0, CF_ISA_A);
> -    INSN(shift8_im, e000, f0f0, M68000);
> -    INSN(shift16_im, e040, f0f0, M68000);
> -    INSN(shift_im,  e080, f0f0, M68000);
> -    INSN(shift8_reg, e020, f0f0, M68000);
> -    INSN(shift16_reg, e060, f0f0, M68000);
> -    INSN(shift_reg, e0a0, f0f0, M68000);
> -    INSN(shift_mem, e0c0, fcc0, M68000);
> -    INSN(rotate_im, e090, f0f0, M68000);
> -    INSN(rotate8_im, e010, f0f0, M68000);
> -    INSN(rotate16_im, e050, f0f0, M68000);
> -    INSN(rotate_reg, e0b0, f0f0, M68000);
> -    INSN(rotate8_reg, e030, f0f0, M68000);
> -    INSN(rotate16_reg, e070, f0f0, M68000);
> -    INSN(rotate_mem, e4c0, fcc0, M68000);
> +    INSN(shift8_im, e000, f0f0, M68K);
> +    INSN(shift16_im, e040, f0f0, M68K);
> +    INSN(shift_im,  e080, f0f0, M68K);
> +    INSN(shift8_reg, e020, f0f0, M68K);
> +    INSN(shift16_reg, e060, f0f0, M68K);
> +    INSN(shift_reg, e0a0, f0f0, M68K);
> +    INSN(shift_mem, e0c0, fcc0, M68K);
> +    INSN(rotate_im, e090, f0f0, M68K);
> +    INSN(rotate8_im, e010, f0f0, M68K);
> +    INSN(rotate16_im, e050, f0f0, M68K);
> +    INSN(rotate_reg, e0b0, f0f0, M68K);
> +    INSN(rotate8_reg, e030, f0f0, M68K);
> +    INSN(rotate16_reg, e070, f0f0, M68K);
> +    INSN(rotate_mem, e4c0, fcc0, M68K);
>       INSN(bfext_mem, e9c0, fdc0, BITFIELD);  /* bfextu & bfexts */
>       INSN(bfext_reg, e9c0, fdf8, BITFIELD);
>       INSN(bfins_mem, efc0, ffc0, BITFIELD);

Applied to my m68k-for-7.2 branch

Thanks,
Laurent

Re: [PATCH 4/4] target/m68k: always call gen_exit_tb() after writes to SR

[Laurent Vivier]

Le 17/09/2022 à 13:25, Mark Cave-Ayland a écrit :
> Any write to SR can change the security state so always call gen_exit_tb() when
> this occurs. In particular MacOS makes use of andiw/oriw in a few places to
> handle the switch between user and supervisor mode.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/translate.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index be5561e1e9..892473d01f 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -2373,6 +2373,7 @@ DISAS_INSN(arith_im)
>           tcg_gen_or_i32(dest, src1, im);
>           if (with_SR) {
>               gen_set_sr(s, dest, opsize == OS_BYTE);
> +            gen_exit_tb(s);
>           } else {
>               DEST_EA(env, insn, opsize, dest, &addr);
>               gen_logic_cc(s, dest, opsize);
> @@ -2382,6 +2383,7 @@ DISAS_INSN(arith_im)
>           tcg_gen_and_i32(dest, src1, im);
>           if (with_SR) {
>               gen_set_sr(s, dest, opsize == OS_BYTE);
> +            gen_exit_tb(s);
>           } else {
>               DEST_EA(env, insn, opsize, dest, &addr);
>               gen_logic_cc(s, dest, opsize);
> @@ -2405,6 +2407,7 @@ DISAS_INSN(arith_im)
>           tcg_gen_xor_i32(dest, src1, im);
>           if (with_SR) {
>               gen_set_sr(s, dest, opsize == OS_BYTE);
> +            gen_exit_tb(s);
>           } else {
>               DEST_EA(env, insn, opsize, dest, &addr);
>               gen_logic_cc(s, dest, opsize);
> @@ -4592,6 +4595,7 @@ DISAS_INSN(strldsr)
>       }
>       gen_push(s, gen_get_sr(s));
>       gen_set_sr_im(s, ext, 0);
> +    gen_exit_tb(s);
>   }
>   
>   DISAS_INSN(move_from_sr)

Applied to my m68k-for-7.2 branch

Thanks,
Laurent

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

[Laurent Vivier]

Le 20/09/2022 à 18:30, Mark Cave-Ayland a écrit :
> On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:
> 
>> On 17/9/22 14:09, BALATON Zoltan wrote:
>>> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>>>> There are already 32 feature bits in use, so change the size of the m68k
>>>> CPU features to uint64_t (allong with the associated m68k_feature()
>>>> functions) to allow up to 64 feature bits to be used.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> target/m68k/cpu.c | 4 ++--
>>>> target/m68k/cpu.h | 6 +++---
>>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>>>> index f681be3a2a..7b4797e2f1 100644
>>>> --- a/target/m68k/cpu.c
>>>> +++ b/target/m68k/cpu.c
>>>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>>>
>>>> static void m68k_set_feature(CPUM68KState *env, int feature)
>>>> {
>>>> -    env->features |= (1u << feature);
>>>> +    env->features |= (1ul << feature);
>>
>>          env->features = deposit64(env->features, feature, 1, 1);
>>
>>>> }
>>>>
>>>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>>>> {
>>>> -    env->features &= (-1u - (1u << feature));
>>>> +    env->features &= (-1ul - (1ul << feature));
>>
>>          env->features = deposit64(env->features, feature, 1, 0);
>>
>>> Should these be ull instead of ul?
>>
>> Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.
> 
> I must admit I find the deposit64() variants not particularly easy to read: if we're considering 
> alterations rather than changing the constant suffix then I'd much rather go for:
> 
>      env->features |= (1ULL << feature);
> 
> and:
> 
>      env->features &= ~(1ULL << feature);
> 
> Laurent, what would be your preference?

I have no preference, do as you prefer.

> 
>>>> }
>>>>
>>>> static void m68k_cpu_reset(DeviceState *dev)
>>>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>>>> index 67b6c12c28..d3384e5d98 100644
>>>> --- a/target/m68k/cpu.h
>>>> +++ b/target/m68k/cpu.h
>>>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>>>     struct {} end_reset_fields;
>>>>
>>>>     /* Fields from here on are preserved across CPU reset. */
>>>> -    uint32_t features;
>>>> +    uint64_t features;
>>>> } CPUM68KState;
>>>>
>>>> /*
>>>> @@ -539,9 +539,9 @@ enum m68k_features {
>>>>     M68K_FEATURE_TRAPCC,
>>>> };
>>>>
>>>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>>>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
>>
>> Why uint64_t? Can we simplify using a boolean?
> 
> I don't really feel strongly either way here. Again I'm happy to go with whatever Laurent would 
> prefer as maintainer.

A boolean seems more logic, I think.

Thanks,
Laurent