Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

["Andreas Färber" <afaerber@suse.de>]

Am 16.02.2012 00:16, schrieb Igor Mammedov:
> Convert pc cpu to qdev device that is attached to icc bus, later
> hot-plug ability of icc bus will allow to implement cpu hot-plug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

This conflicts with CPU QOM'ification across targets (and no longer
applies due to type_init() introduction).

> ---
>  hw/pc.c              |   62 +++++++++++++++++++++++++++++++++++++++++++------
>  target-i386/cpu.h    |    1 +
>  target-i386/helper.c |   26 ++++++++++++++------
>  3 files changed, 73 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 33d8090..b8db5dc 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>      }
>  }
>  
> +typedef struct CPUPC {
> +    ICCBusDevice busdev;
> +    char *model;
> +    CPUState state;
> +} CPUPC;

I don't like this approach. For starters, using CPUState as field rather
than pointer seems a bad idea to me (CPUX86State would only be slightly
better). We should have a single code path through which we instantiate
CPUs, currently: cpu_$arch_init(const char *cpu_model)
With my series completed, this would return an X86CPU object.

Depending on our liking, we could either place some ICC / APIC /
whatever fields directly into that, or embed the X86CPU in an object
such as yours above as a link<X86CPU>. I do feel however that the model
string is misplaced there. Question is whether this ICC stuff is
actually part of the CPU or part of the CPU wiring on the mainboard - I
vaguely remember someone saying that this changed over time...? Having
both depending on CPU subclass might also be an option, but I'd rather
leave such decisions as a follow-up to the core QOM'ification.

Andreas

> +
>  static void pc_cpu_reset(void *opaque)
>  {
>      CPUState *env = opaque;
> @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque)
>      env->halted = !cpu_is_bsp(env);
>  }
>  
> -static CPUState *pc_new_cpu(const char *cpu_model)
> +static DeviceState *pc_new_cpu(const char *cpu_model)
>  {
> -    CPUState *env;
> +    DeviceState *dev;
> +    BusState *b;
>  
> -    env = cpu_init(cpu_model);
> -    if (!env) {
> -        fprintf(stderr, "Unable to find x86 CPU definition\n");
> -        exit(1);
> +    b = get_icc_bus();
> +    dev = qdev_create(b, "cpu-pc");
> +    if (!dev) {
> +        return NULL;
> +    }
> +    qdev_prop_set_string(dev, "model", g_strdup(cpu_model));
> +    if (qdev_init(dev) < 0) {
> +        return NULL;
> +    }
> +    return dev;
> +}
> +
> +static int cpu_device_init(ICCBusDevice *dev)
> +{
> +    CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
> +    CPUState *env = &cpu->state;
> +
> +    if (cpu_x86_init_inplace(env, cpu->model) < 0) {
> +        return -1;
>      }
> +
>      if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>          env->apic_state = apic_init(env, env->cpuid_apic_id);
>      }
> -    qemu_register_reset(pc_cpu_reset, env);
> +
> +    return 0;
> +}
> +
> +static void cpu_device_reset(DeviceState *dev) {
> +    CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev);
> +    CPUState *env = &cpu->state;
>      pc_cpu_reset(env);
> -    return env;
>  }
>  
> +static ICCBusDeviceInfo cpu_device_info = {
> +    .qdev.name = "cpu-pc",
> +    .qdev.size = sizeof(CPUPC),
> +    .qdev.reset = cpu_device_reset,
> +    .init = cpu_device_init,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_STRING("model", CPUPC, model),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void pc_register_devices(void)
> +{
> +    iccbus_register_devinfo(&cpu_device_info);
> +}
> +
> +device_init(pc_register_devices);
> +
>  void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 7e66bcf..830b65e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -775,6 +775,7 @@ typedef struct CPUX86State {
>  } CPUX86State;
>  
>  CPUX86State *cpu_x86_init(const char *cpu_model);
> +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model);
>  int cpu_x86_exec(CPUX86State *s);
>  void cpu_x86_close(CPUX86State *s);
>  void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg);
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 2586aff..df2f5ba 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
>      return 1;
>  }
>  
> -CPUX86State *cpu_x86_init(const char *cpu_model)
> +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model)
>  {
> -    CPUX86State *env;
>      static int inited;
>  
> -    env = g_malloc0(sizeof(CPUX86State));
> +    if (cpu_x86_register(env, cpu_model) < 0) {
> +        fprintf(stderr, "Unable to find x86 CPU definition\n");
> +        return -1;
> +    }
>      cpu_exec_init(env);
>      env->cpu_model_str = cpu_model;
>  
> @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>              cpu_set_debug_excp_handler(breakpoint_handler);
>  #endif
>      }
> -    if (cpu_x86_register(env, cpu_model) < 0) {
> -        cpu_x86_close(env);
> -        return NULL;
> -    }
>      env->cpuid_apic_id = env->cpu_index;
>      mce_init(env);
>  
>      qemu_init_vcpu(env);
>  
> -    return env;
> +    return 0;
>  }
>  
> +CPUX86State *cpu_x86_init(const char *cpu_model)
> +{
> +    CPUX86State *env;
> +
> +    env = g_malloc0(sizeof(CPUX86State));
> +    if (cpu_x86_init_inplace(env, cpu_model) < 0) {
> +        g_free(env);
> +        return NULL;
> +    }
> +
> +    return env;
> +}
>  #if !defined(CONFIG_USER_ONLY)
>  void do_cpu_init(CPUState *env)
>  {

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg