On Mon, 17 Jul 2017 17:23:22 +0200 Igor Mammedov <imamm...@redhat.com> wrote:
> On Mon, 17 Jul 2017 17:05:15 +0200 > Andreas Färber <afaer...@suse.de> wrote: > > > Am 17.07.2017 um 12:41 schrieb Igor Mammedov: > > > On Sat, 15 Jul 2017 08:08:58 -1000 > > > Richard Henderson <r...@twiddle.net> wrote: > > > > > >> On 07/14/2017 03:52 AM, Igor Mammedov wrote: > > >>> @@ -230,6 +230,8 @@ static void m68k_cpu_realizefn(DeviceState *dev, > > >>> Error **errp) > > >>> M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev); > > >>> Error *local_err = NULL; > > >>> > > >>> + register_m68k_insns(&cpu->env); > > >>> + > > >> > > >> I think it would make more sense to do this during m68k_tcg_init. > > >> > > > it seems that m68k_cpu_initfn accesses 'env' via some global, > > > while cpu_mk68k_init() used to access concrete pointer of just created > > > cpu, > > > > > > how about moving register_m68k_insns() to m68k_cpu_initfn(), instead? > > > it should be equivalent to what cpu_mk68k_init() used to do. > > > > As a general note, realize should be re-entrant. Can't tell from the > > above diff whether that is the case here. > Looking at > > void register_m68k_insns (CPUM68KState *env) > > { > > /* Build the opcode table only once to avoid > > multithreading issues. */ > > if (opcode_table[0] != NULL) { > > return; > > } > > it is save to use multiple times, > > also looking further in it: > > #define BASE(name, opcode, mask) \ > > register_opcode(disas_##name, 0x##opcode, 0x##mask) > > #define INSN(name, opcode, mask, feature) do { \ > > if (m68k_feature(env, M68K_FEATURE_##feature)) \ > > BASE(name, opcode, mask); \ > > } while(0) > > BASE(undef, 0000, 0000); > > INSN(arith_im, 0080, fff8, CF_ISA_A); > > INSN macro depends on enabled features, it might work with current code that > has no user settable features but it will break once that is available. > > So I retract my suggestion to move register_m68k_insns() into > m68k_cpu_initfn() > and keep it as it's in this patch (in m68k_cpu_realizefn()), > that way features theoretically set between initfn(and m68k_tcg_init) and > realize() will > have effect on created cpu and we won't have to fix it in future. Richard, Laurent, Do you agree with keeping register_m68k_insns() in realize()?