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

[Eduardo Habkost]

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.

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