Re: [Qemu-arm] [Qemu-devel] [PATCH 10/10] pc: parse cpu features only once

[Igor Mammedov]

On Wed, 8 Jun 2016 14:03:15 -0300
Eduardo Habkost <address@hidden> wrote:

> 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 <address@hidden>
> > ---
> >  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.
All of above should be done as one step i.e.
 add cpu_type + drop cpu_model
When we are ready to call parsing from generic code but not now.

For calling parsing from generic place, the only blocker is sparc
target. 
It needs to be converted to CPU subtypes + features=>properties,
like hat you've done with x86. 

> 
> > +    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?
It could be generalized and reduce similar site in virt-arm 
to 1 function. I'll do it on top of this series.

> > +
> >      /* 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
> > 
>