| Message ID | 1500040339-119465-15-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am Fri, 14 Jul 2017 15:52:05 +0200 schrieb Igor Mammedov <imammedo@redhat.com>: > call register_m68k_insns() at realize time which makes > cpu_m68k_init() typical object creation function. > As result we can replace it with cpu_generic_init() > which does the same job, reducing code duplication a bit. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > CC: Thomas Huth <huth@tuxfamily.org> > CC: Laurent Vivier <laurent@vivier.eu> > --- > target/m68k/cpu.h | 3 +-- > hw/m68k/an5206.c | 2 +- > hw/m68k/mcf5208.c | 2 +- > target/m68k/cpu.c | 2 ++ > target/m68k/helper.c | 20 -------------------- > 5 files changed, 5 insertions(+), 24 deletions(-) Patch looks good, and the Coldfire images that I have still boot fine: Tested-by: Thomas Huth <huth@tuxfamily.org>
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. r~
Le 15/07/2017 à 20:08, Richard Henderson a écrit : > 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. I agree. Laurent
On Sat, 15 Jul 2017 08:08:58 -1000 Richard Henderson <rth@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.
Am 17.07.2017 um 12:41 schrieb Igor Mammedov: > On Sat, 15 Jul 2017 08:08:58 -1000 > Richard Henderson <rth@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. Regards, Andreas
On Mon, 17 Jul 2017 17:05:15 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 17.07.2017 um 12:41 schrieb Igor Mammedov: > > On Sat, 15 Jul 2017 08:08:58 -1000 > > Richard Henderson <rth@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. > Regards, > Andreas >
On Mon, 17 Jul 2017 17:23:22 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Mon, 17 Jul 2017 17:05:15 +0200 > Andreas Färber <afaerber@suse.de> wrote: > > > Am 17.07.2017 um 12:41 schrieb Igor Mammedov: > > > On Sat, 15 Jul 2017 08:08:58 -1000 > > > Richard Henderson <rth@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()?
Le 14/08/2017 à 10:00, Igor Mammedov a écrit : > On Mon, 17 Jul 2017 17:23:22 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > >> On Mon, 17 Jul 2017 17:05:15 +0200 >> Andreas Färber <afaerber@suse.de> wrote: >> >>> Am 17.07.2017 um 12:41 schrieb Igor Mammedov: >>>> On Sat, 15 Jul 2017 08:08:58 -1000 >>>> Richard Henderson <rth@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 <laurent@vivier.eu>
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h index 38a7e11..d936547 100644 --- a/target/m68k/cpu.h +++ b/target/m68k/cpu.h @@ -163,7 +163,6 @@ int m68k_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg); void m68k_tcg_init(void); void m68k_cpu_init_gdb(M68kCPU *cpu); -M68kCPU *cpu_m68k_init(const char *cpu_model); /* you can call this signal handler from your SIGBUS and SIGSEGV signal handlers to inform the virtual CPU of exceptions. non zero is returned if the signal was handled by the virtual CPU. */ @@ -322,7 +321,7 @@ void register_m68k_insns (CPUM68KState *env); #define TARGET_PHYS_ADDR_SPACE_BITS 32 #define TARGET_VIRT_ADDR_SPACE_BITS 32 -#define cpu_init(cpu_model) CPU(cpu_m68k_init(cpu_model)) +#define cpu_init(cpu_model) cpu_generic_init(TYPE_M68K_CPU, cpu_model) #define cpu_signal_handler cpu_m68k_signal_handler #define cpu_list m68k_cpu_list diff --git a/hw/m68k/an5206.c b/hw/m68k/an5206.c index 142bab9..23c23df 100644 --- a/hw/m68k/an5206.c +++ b/hw/m68k/an5206.c @@ -42,7 +42,7 @@ static void an5206_init(MachineState *machine) if (!cpu_model) { cpu_model = "m5206"; } - cpu = cpu_m68k_init(cpu_model); + cpu = M68K_CPU(cpu_generic_init(TYPE_M68K_CPU, cpu_model)); if (!cpu) { error_report("Unable to find m68k CPU definition"); exit(1); diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c index 6563518..cca2e73 100644 --- a/hw/m68k/mcf5208.c +++ b/hw/m68k/mcf5208.c @@ -232,7 +232,7 @@ static void mcf5208evb_init(MachineState *machine) if (!cpu_model) { cpu_model = "m5208"; } - cpu = cpu_m68k_init(cpu_model); + cpu = M68K_CPU(cpu_generic_init(TYPE_M68K_CPU, cpu_model)); if (!cpu) { fprintf(stderr, "Unable to find m68k CPU definition\n"); exit(1); diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c index a14b6dd..55bf24b 100644 --- a/target/m68k/cpu.c +++ b/target/m68k/cpu.c @@ -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); + cpu_exec_realizefn(cs, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); diff --git a/target/m68k/helper.c b/target/m68k/helper.c index caae291..7e50ff5 100644 --- a/target/m68k/helper.c +++ b/target/m68k/helper.c @@ -156,26 +156,6 @@ static int m68k_fpu_gdb_set_reg(CPUM68KState *env, uint8_t *mem_buf, int n) return 0; } -M68kCPU *cpu_m68k_init(const char *cpu_model) -{ - M68kCPU *cpu; - CPUM68KState *env; - ObjectClass *oc; - - oc = cpu_class_by_name(TYPE_M68K_CPU, cpu_model); - if (oc == NULL) { - return NULL; - } - cpu = M68K_CPU(object_new(object_class_get_name(oc))); - env = &cpu->env; - - register_m68k_insns(env); - - object_property_set_bool(OBJECT(cpu), true, "realized", NULL); - - return cpu; -} - void m68k_cpu_init_gdb(M68kCPU *cpu) { CPUState *cs = CPU(cpu);
call register_m68k_insns() at realize time which makes cpu_m68k_init() typical object creation function. As result we can replace it with cpu_generic_init() which does the same job, reducing code duplication a bit. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- CC: Thomas Huth <huth@tuxfamily.org> CC: Laurent Vivier <laurent@vivier.eu> --- target/m68k/cpu.h | 3 +-- hw/m68k/an5206.c | 2 +- hw/m68k/mcf5208.c | 2 +- target/m68k/cpu.c | 2 ++ target/m68k/helper.c | 20 -------------------- 5 files changed, 5 insertions(+), 24 deletions(-)