On Mon, Jun 06, 2016 at 05:16:52PM +0200, Igor Mammedov wrote: > considering that features are converted to > global properties and global properties are > automatically applied to every new instance > of created CPU (at object_new() time), there > is no point in parsing cpu_model string every > time a CPU created. > So move parsing outside CPU creation loop and > do it only once. > Parsing also should be done before any CPU is > created so that features would affect the first > CPU a well. > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > --- > hw/i386/pc.c | 37 ++++++++++++++++++++++++++++--------- > target-i386/cpu.c | 44 -------------------------------------------- > target-i386/cpu.h | 1 - > 3 files changed, 28 insertions(+), 54 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index c48322b..0331e6d 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1041,21 +1041,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int > level) > } > } > > -static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > +static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id, > Error **errp) > { > X86CPU *cpu = NULL; > Error *local_err = NULL; > > - cpu = cpu_x86_create(cpu_model, &local_err); > - if (local_err != NULL) { > - goto out; > - } > + cpu = X86_CPU(object_new(typename));
Nice. :) > > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > > -out: > if (local_err) { > error_propagate(errp, local_err); > object_unref(OBJECT(cpu)); > @@ -1067,7 +1063,8 @@ out: > void pc_hot_add_cpu(const int64_t id, Error **errp) > { > X86CPU *cpu; > - MachineState *machine = MACHINE(qdev_get_machine()); > + ObjectClass *oc; > + PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); > int64_t apic_id = x86_cpu_apic_id_from_index(id); > Error *local_err = NULL; > > @@ -1095,7 +1092,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) > return; > } > > - cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err); > + assert(pcms->possible_cpus->cpus[0].cpu); /* BSP is always present */ > + oc = OBJECT_CLASS(CPU_GET_CLASS(pcms->possible_cpus->cpus[0].cpu)); The same pattern will probably repeat in other machines. I wouldn't mind adding a new MachineState::cpu_type field, as we already have MachineState::cpu_model. MachineState::cpu_model could eventually go away if we move all parse_features() calls to generic code. > + cpu = pc_new_cpu(object_class_get_name(oc), apic_id, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > @@ -1106,6 +1105,10 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) > void pc_cpus_init(PCMachineState *pcms) > { > int i, j; > + CPUClass *cc; > + ObjectClass *oc; > + const char *typename; > + gchar **model_pieces; > X86CPU *cpu = NULL; > MachineState *machine = MACHINE(pcms); > > @@ -1118,6 +1121,22 @@ void pc_cpus_init(PCMachineState *pcms) > #endif > } > > + model_pieces = g_strsplit(machine->cpu_model, ",", 2); > + if (!model_pieces[0]) { > + error_report("Invalid/empty CPU model name"); > + exit(1); > + } > + > + oc = cpu_class_by_name(TYPE_X86_CPU, model_pieces[0]); > + if (oc == NULL) { > + error_report("Unable to find CPU definition: %s", model_pieces[0]); > + exit(1); > + } > + typename = object_class_get_name(oc); > + cc = CPU_CLASS(oc); > + cc->parse_features(typename, model_pieces[1], &error_fatal); > + g_strfreev(model_pieces); Can we move this to a generic function to be reused by other machines? > + > /* Calculates the limit to CPU APIC ID values > * > * Limit for the APIC ID value, so that all > @@ -1148,7 +1167,7 @@ void pc_cpus_init(PCMachineState *pcms) > } > > if (i < smp_cpus) { > - cpu = pc_new_cpu(machine->cpu_model, > x86_cpu_apic_id_from_index(i), > + cpu = pc_new_cpu(typename, x86_cpu_apic_id_from_index(i), > &error_fatal); > pcms->possible_cpus->cpus[i].cpu = CPU(cpu); > object_unref(OBJECT(cpu)); > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 43b22e6..c633579 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2211,50 +2211,6 @@ static void x86_cpu_load_def(X86CPU *cpu, > X86CPUDefinition *def, Error **errp) > > } > > -X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) > -{ > - X86CPU *cpu = NULL; > - ObjectClass *oc; > - CPUClass *cc; > - gchar **model_pieces; > - char *name, *features; > - Error *error = NULL; > - const char *typename; > - > - model_pieces = g_strsplit(cpu_model, ",", 2); > - if (!model_pieces[0]) { > - error_setg(&error, "Invalid/empty CPU model name"); > - goto out; > - } > - name = model_pieces[0]; > - features = model_pieces[1]; > - > - oc = x86_cpu_class_by_name(name); > - if (oc == NULL) { > - error_setg(&error, "Unable to find CPU definition: %s", name); > - goto out; > - } > - cc = CPU_CLASS(oc); > - typename = object_class_get_name(oc); > - > - cc->parse_features(typename, features, &error); > - cpu = X86_CPU(object_new(typename)); > - if (error) { > - goto out; > - } > - > -out: > - if (error != NULL) { > - error_propagate(errp, error); > - if (cpu) { > - object_unref(OBJECT(cpu)); > - cpu = NULL; > - } > - } > - g_strfreev(model_pieces); > - return cpu; > -} Nice. :) > - > X86CPU *cpu_x86_init(const char *cpu_model) > { > return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model)); > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index a257c53..a9a3b87 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1227,7 +1227,6 @@ void x86_cpu_exec_enter(CPUState *cpu); > void x86_cpu_exec_exit(CPUState *cpu); > > X86CPU *cpu_x86_init(const char *cpu_model); > -X86CPU *cpu_x86_create(const char *cpu_model, Error **errp); > int cpu_x86_exec(CPUState *cpu); > void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf); > int cpu_x86_support_mca_broadcast(CPUX86State *env); > -- > 1.8.3.1 > -- Eduardo