Re: [Qemu-devel] [PATCH 14/28] m68k: replace cpu_m68k_init() with cpu_generic_init()

[Igor Mammedov]

On Mon, 17 Jul 2017 17:23:22 +0200
Igor Mammedov <address@hidden> wrote:

> On Mon, 17 Jul 2017 17:05:15 +0200
> Andreas Färber <address@hidden> wrote:
> 
> > Am 17.07.2017 um 12:41 schrieb Igor Mammedov:  
> > > On Sat, 15 Jul 2017 08:08:58 -1000
> > > Richard Henderson <address@hidden> 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()?