Re: [Qemu-devel] [PATCH v2 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Igor Mammedov]

On Tue, 10 Mar 2015 18:57:35 -0300
Eduardo Habkost <address@hidden> wrote:

> Instead of passing icc_bridge from the PC initialization code to
> cpu_x86_create(), make the PC initialization code attach the CPU to
> icc_bridge.
> 
> The only difference here is that icc_bridge attachment will now be done
> after x86_cpu_parse_featurestr() is called. But this shouldn't make any
> difference, as property setters shouldn't depend on icc_bridge.
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
With comment below fixed
Reviewed-by: Igor Mammedov <address@hidden>

> ---
> Changes v1 -> v2:
>  * Keep existing check for NULL icc_bridge and error reporting, instead
>    of assing assert(icc_bridge)
> ---
>  hw/i386/pc.c      | 13 +++++++++++--
>  target-i386/cpu.c | 14 ++------------
>  target-i386/cpu.h |  3 +--
>  3 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b5b2aad..a26e0ec 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -992,18 +992,27 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
> level)
>  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
>                            DeviceState *icc_bridge, Error **errp)
>  {
> -    X86CPU *cpu;
> +    X86CPU *cpu = NULL;
>      Error *local_err = NULL;
>  
> -    cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
> +    if (icc_bridge == NULL) {
turn it into assert(icc_bridge != NULL)
that should never be NULL or we have a codding error somewhere in code.

> +        error_setg(&local_err, "Invalid icc-bridge value");
> +        goto out;
> +    }
> +
> +    cpu = cpu_x86_create(cpu_model, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          return NULL;
>      }
>  
> +    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
> +    object_unref(OBJECT(cpu));
> +
>      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));
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 50907d0..097924c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2076,8 +2076,7 @@ static void x86_cpu_load_def(X86CPU *cpu, 
> X86CPUDefinition *def, Error **errp)
>  
>  }
>  
> -X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
> -                       Error **errp)
> +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>  {
>      X86CPU *cpu = NULL;
>      X86CPUClass *xcc;
> @@ -2108,15 +2107,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, 
> DeviceState *icc_bridge,
>  
>      cpu = X86_CPU(object_new(object_class_get_name(oc)));
>  
> -#ifndef CONFIG_USER_ONLY
> -    if (icc_bridge == NULL) {
> -        error_setg(&error, "Invalid icc-bridge value");
> -        goto out;
> -    }
> -    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
> -    object_unref(OBJECT(cpu));
> -#endif
> -
>      x86_cpu_parse_featurestr(CPU(cpu), features, &error);
>      if (error) {
>          goto out;
> @@ -2139,7 +2129,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>      Error *error = NULL;
>      X86CPU *cpu;
>  
> -    cpu = cpu_x86_create(cpu_model, NULL, &error);
> +    cpu = cpu_x86_create(cpu_model, &error);
>      if (error) {
>          goto out;
>      }
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 0638d24..8d748bd 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -982,8 +982,7 @@ typedef struct CPUX86State {
>  #include "cpu-qom.h"
>  
>  X86CPU *cpu_x86_init(const char *cpu_model);
> -X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
> -                       Error **errp);
> +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
>  int cpu_x86_exec(CPUX86State *s);
>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>  void x86_cpudef_setup(void);