Le 14/08/2017 à 10:00, Igor Mammedov a écrit : > 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()? >
I agree. Acked-by: Laurent Vivier <laur...@vivier.eu>