Re: [Qemu-devel] [PATCH 11/19] target-i386: introduce apic-id property

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 11 Apr 2013 16:12:33 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Apr 11, 2013 at 04:51:50PM +0200, Igor Mammedov wrote:
> > ... and use it from board level to set APIC ID for CPUs
> > it creates.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > Note:
> >   * pc_new_cpu() function will be reused later in CPU hot-plug hook.
> > 
> > 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      | 25 ++++++++++++++++++++++++-
> >  target-i386/cpu.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 8d75b34..3adf294 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -869,9 +869,29 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> >      }
> >  }
> >  
> > +static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
> > +{
> > +    X86CPU *cpu;
> > +
> > +    cpu = cpu_x86_create(cpu_model, errp);
> > +    if (!cpu) {
> > +        return;
> > +    }
> > +
> > +    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
> > +    object_property_set_bool(OBJECT(cpu), true, "realized", errp);
> > +
> > +    if (error_is_set(errp)) {
> > +        if (cpu != NULL) {
> > +            object_unref(OBJECT(cpu));
> > +        }
> > +    }
> > +}
> > +
> >  void pc_cpus_init(const char *cpu_model)
> >  {
> >      int i;
> > +    Error *error = NULL;
> >  
> >      /* init CPUs */
> >      if (cpu_model == NULL) {
> > @@ -883,7 +903,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 9cca031..4ddc18a 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1272,6 +1272,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 (cpu_exists(value)) {
> > +        error_setg(&error, "CPU with APIC ID %" PRIi64 " exists", value);
> > +        error_propagate(errp, error);
> 
> What about implementing this check after implementing the
> /machine/icc-bridge links? Then we can simply check if
> "/machine/icc-bridge/cpus[ID]" is already set.
because it's more generic and won't break even if link location changes

> 
> (And then icc-bridge could be responsible for ensuring APIC ID
> uniqueness, not the CPU object itself)
Yep, with cpu-add id could be/is verified before setting property, but if
one considers device_add or fictional -device cpuxxx on cmd line, then won't
be an external check for this. This check takes care about these use cases.

> 
> > +        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.8.2
> > 
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor