Re: [Qemu-devel] [PATCH 04/15] target-i386: introduce apic-id property

[Eduardo Habkost]

On Thu, Apr 25, 2013 at 04:05:26PM +0200, Igor Mammedov wrote:
> The property is used from board level to set APIC ID for CPUs it
> creates. Do so in a new pc_new_cpu() helper, to be reused for hot-plug.
> 
> Signed-off-by: Igor Mammedov <address@hidden>

Reviewed-by: Eduardo Habkost <address@hidden>


> ---
> Note:
>   * pc_new_cpu() function will be reused later in CPU hot-plug hook.
> 
> v4:
>   * after switching to qemu_for_each_cpu() in cpu_exists(), first CPU
>     becomes visible to cpu_exists() early and setting property fails,
>     skip APIC ID check if value to be set is the same as the current.
>   * use error_propagate() in pc_new_cpu()
>   * return CPU from pc_new_cpu(). Moved from "move APIC to ICC bus"
>     to reduce its size.
> v3:
>   * user error_propagate() in property setter
> v2:
>   * use generic cpu_exists() instead of custom one
>   * make apic-id dynamic property, so it won't be possible to use it
>     as global property, since it's instance specific
> ---
>  hw/i386/pc.c      |   29 ++++++++++++++++++++++++++++-
>  target-i386/cpu.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 28fce0e..7c7dd86 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -890,9 +890,33 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
> level)
>      }
>  }
>  
> +static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error 
> **errp)
> +{
> +    X86CPU *cpu;
> +    Error *local_err = NULL;
> +
> +    cpu = cpu_x86_create(cpu_model, errp);
> +    if (!cpu) {
> +        return cpu;
> +    }
> +
> +    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> +
> +    if (local_err) {
> +        if (cpu != NULL) {
> +            object_unref(OBJECT(cpu));
> +            cpu = NULL;
> +        }
> +        error_propagate(errp, local_err);
> +    }
> +    return cpu;
> +}
> +
>  void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
> +    Error *error = NULL;
>  
>      /* init CPUs */
>      if (cpu_model == NULL) {
> @@ -904,7 +928,10 @@ void pc_cpus_init(const char *cpu_model)
>      }
>  
>      for (i = 0; i < smp_cpus; i++) {
> -        if (!cpu_x86_init(cpu_model)) {
> +        pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> +        if (error) {
> +            fprintf(stderr, "%s\n", error_get_pretty(error));
> +            error_free(error);
>              exit(1);
>          }
>      }
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index f34ba23..b0eb6ca 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1271,6 +1271,45 @@ static void x86_cpuid_set_tsc_freq(Object *obj, 
> Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> +static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
> +                                  const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    int64_t value = cpu->env.cpuid_apic_id;
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
> +                                  const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    const int64_t min = 0;
> +    const int64_t max = UINT32_MAX;
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +    if (value < min || value > max) {
> +        error_setg(&error, "Property %s.%s doesn't take value %" PRId64
> +                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> +                   object_get_typename(obj), name, value, min, max);
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
> +        error_setg(&error, "CPU with APIC ID %" PRIi64 " exists", value);
> +        error_propagate(errp, error);
> +        return;
> +    }
> +    cpu->env.cpuid_apic_id = value;
> +}
> +
>  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  {
>      x86_def_t *def;
> @@ -2259,6 +2298,9 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> +    object_property_add(obj, "apic-id", "int",
> +                        x86_cpuid_get_apic_id,
> +                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
>  
>      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
>  
> -- 
> 1.7.1
> 
> 

-- 
Eduardo