Patchwork [11/19] target-i386: introduce apic-id property

login
register
mail settings
Submitter Igor Mammedov
Date April 11, 2013, 2:51 p.m.
Message ID <1365691918-30594-12-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/235803/
State New
Headers show

Comments

Igor Mammedov - April 11, 2013, 2:51 p.m.
... 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(-)
Eduardo Habkost - April 11, 2013, 7:12 p.m.
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.

(And then icc-bridge could be responsible for ensuring APIC ID
uniqueness, not the CPU object itself)

> +        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
>
Igor Mammedov - April 12, 2013, 10:20 a.m.
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
>
Eduardo Habkost - April 12, 2013, 1:13 p.m.
On Fri, Apr 12, 2013 at 12:20:45PM +0200, Igor Mammedov wrote:
> 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

Well, if link location changed, we could change the cpu_exists()
implementation. We don't even need to hardcode the icc-bridge path, we
could simply follow appropriate links if we set them up.

My problem with cpu_exists() is that it is based on global state,
instead of simply using the links between bus/devices to find out if the
IDs are being allocated correctly. We could make it work like PCI: devfn
conflicts are solved simply by looking at the list of devices in the
specific PCIBus where the device is being added. See
do_pci_register_device().

That said, I am not against the current approach if we don't have the
necessary bus/links modelled yet. But if we are going to add icc-bridge,
we could benefit from it to implement cpu_exists().

> 
> > 
> > (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.

That's why I believe the CPU has to calculate the APIC ID based on
signaling coming from other devices/buses, instead of allowing the APIC
ID to be directly specified in the device_add/-device options.  That's
how real CPUs work: as the CPU manufacturer doesn't know what will be
the package ID of the CPU, the APIC IDs are not hardcoded in the CPU;
they are calculated based on the CPU topology and some socket identifier
signal coming from the board.

Comparing it with PCI again: devfn of PCI devices can be specified
manually, but if it is not present you just need to look at the PCI bus
to find out the next available devfn.

Based on that, I thought that icc-bridge was going be the component
responsible for the APIC ID allocation logic, considering that it
already contains APIC-ID-based links to the CPU objects.


> 
> > 
> > > +        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
Igor Mammedov - April 12, 2013, 3:46 p.m.
On Fri, 12 Apr 2013 10:13:13 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Apr 12, 2013 at 12:20:45PM +0200, Igor Mammedov wrote:
> > 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
> 
> Well, if link location changed, we could change the cpu_exists()
> implementation. We don't even need to hardcode the icc-bridge path, we
> could simply follow appropriate links if we set them up.
> 
> My problem with cpu_exists() is that it is based on global state,
> instead of simply using the links between bus/devices to find out if the
> IDs are being allocated correctly. We could make it work like PCI: devfn
> conflicts are solved simply by looking at the list of devices in the
> specific PCIBus where the device is being added. See
> do_pci_register_device().
> 
> That said, I am not against the current approach if we don't have the
> necessary bus/links modelled yet. But if we are going to add icc-bridge,
> we could benefit from it to implement cpu_exists().
Well,check has to be done here anyway, regardless how cpu_exists() implemented.

BTW:
This discussion and below belongs more to 9/19 patch that implements
cpu_exists() than here.

> 
> > 
> > > 
> > > (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.
> 
> That's why I believe the CPU has to calculate the APIC ID based on
> signaling coming from other devices/buses, instead of allowing the APIC
You mean by board not by CPU, I suppose.

> ID to be directly specified in the device_add/-device options.  That's
> how real CPUs work: as the CPU manufacturer doesn't know what will be
> the package ID of the CPU, the APIC IDs are not hardcoded in the CPU;
> they are calculated based on the CPU topology and some socket identifier
> signal coming from the board.
that is why apic_id has been made a property, to be set from outside.

> 
> Comparing it with PCI again: devfn of PCI devices can be specified
> manually, but if it is not present you just need to look at the PCI bus
> to find out the next available devfn.
18/19 gives an option to lookup available APIC IDs in a target specific way.
If we come up with a better target agnostic way to lookup ID, we can always
add it on top of currently proposed implementation.

> Based on that, I thought that icc-bridge was going be the component
> responsible for the APIC ID allocation logic, considering that it
> already contains APIC-ID-based links to the CPU objects.
in 19/19 APIC IDs are allocated by board for each possible CPU, icc-bridge is
just convenient place for storing links now. If machine was QOM object, I'd
probably place it there.

> 
> 
> > 
> > > 
> > > > +        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
> 
> -- 
> Eduardo
Eduardo Habkost - April 12, 2013, 4:29 p.m.
On Fri, Apr 12, 2013 at 05:46:42PM +0200, Igor Mammedov wrote:
> On Fri, 12 Apr 2013 10:13:13 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > > > > +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
> > 
> > Well, if link location changed, we could change the cpu_exists()
> > implementation. We don't even need to hardcode the icc-bridge path, we
> > could simply follow appropriate links if we set them up.
> > 
> > My problem with cpu_exists() is that it is based on global state,
> > instead of simply using the links between bus/devices to find out if the
> > IDs are being allocated correctly. We could make it work like PCI: devfn
> > conflicts are solved simply by looking at the list of devices in the
> > specific PCIBus where the device is being added. See
> > do_pci_register_device().
> > 
> > That said, I am not against the current approach if we don't have the
> > necessary bus/links modelled yet. But if we are going to add icc-bridge,
> > we could benefit from it to implement cpu_exists().
> Well,check has to be done here anyway, regardless how cpu_exists() implemented.
> 
> BTW:
> This discussion and below belongs more to 9/19 patch that implements
> cpu_exists() than here.

OK.

> 
> > 
> > > 
> > > > 
> > > > (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.
> > 
> > That's why I believe the CPU has to calculate the APIC ID based on
> > signaling coming from other devices/buses, instead of allowing the APIC
> You mean by board not by CPU, I suppose.

Actually I am not 100% sure. For example: we could simply provide the
socket/core/thread IDs (or links to socket/core objects?) to the CPU
object, and the CPU object could calculate the APIC ID itself.

> 
> > ID to be directly specified in the device_add/-device options.  That's
> > how real CPUs work: as the CPU manufacturer doesn't know what will be
> > the package ID of the CPU, the APIC IDs are not hardcoded in the CPU;
> > they are calculated based on the CPU topology and some socket identifier
> > signal coming from the board.
> that is why apic_id has been made a property, to be set from outside.

True. I believe the conflict here is: we want other objects to set the
APIC ID (be it the board, or socket/core objects), but at the same time
it would be interesting to not expose the APIC ID outside QEMU. Being
too flexible regarding the APIC ID is more likely to cause problems
later.

That said, I don't mind having a "apic-id" property because it is easier
to simply expose it directly. But do you agree that: 1) we don't really
need to expose it to be set from outside QEMU; 2) we shouldn't require
it to be set from outside QEMU; 3) we should recommend users to not try
to fiddle it with?


> 
> > 
> > Comparing it with PCI again: devfn of PCI devices can be specified
> > manually, but if it is not present you just need to look at the PCI bus
> > to find out the next available devfn.
> 18/19 gives an option to lookup available APIC IDs in a target specific way.
> If we come up with a better target agnostic way to lookup ID, we can always
> add it on top of currently proposed implementation.
> 
> > Based on that, I thought that icc-bridge was going be the component
> > responsible for the APIC ID allocation logic, considering that it
> > already contains APIC-ID-based links to the CPU objects.
> in 19/19 APIC IDs are allocated by board for each possible CPU, icc-bridge is
> just convenient place for storing links now. If machine was QOM object, I'd
> probably place it there.

I was just wondering if we could solve both problems at the same time
time: if we have a convenient place to store APIC-ID-based links, we are
getting a more efficient APIC ID allocation/check system for free.
Especially considering how inefficient is the method used by
cpu_exists().

It looks like the problem with my approach is that the QOM APIC-ID-based
links are target-specific, but cpu_exists() is target-independent. If
both were target-specific or both were target-independent, we could
easily reuse the same mechanism for both, but that's not the case now.
liguang - April 15, 2013, 2:30 a.m.
在 2013-04-12五的 13:29 -0300,Eduardo Habkost写道:
> On Fri, Apr 12, 2013 at 05:46:42PM +0200, Igor Mammedov wrote:
> > On Fri, 12 Apr 2013 10:13:13 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...]
> > > > > > +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
> > > 
> > > Well, if link location changed, we could change the cpu_exists()
> > > implementation. We don't even need to hardcode the icc-bridge path, we
> > > could simply follow appropriate links if we set them up.
> > > 
> > > My problem with cpu_exists() is that it is based on global state,
> > > instead of simply using the links between bus/devices to find out if the
> > > IDs are being allocated correctly. We could make it work like PCI: devfn
> > > conflicts are solved simply by looking at the list of devices in the
> > > specific PCIBus where the device is being added. See
> > > do_pci_register_device().
> > > 
> > > That said, I am not against the current approach if we don't have the
> > > necessary bus/links modelled yet. But if we are going to add icc-bridge,
> > > we could benefit from it to implement cpu_exists().
> > Well,check has to be done here anyway, regardless how cpu_exists() implemented.
> > 
> > BTW:
> > This discussion and below belongs more to 9/19 patch that implements
> > cpu_exists() than here.
> 
> OK.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > (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.
> > > 
> > > That's why I believe the CPU has to calculate the APIC ID based on
> > > signaling coming from other devices/buses, instead of allowing the APIC
> > You mean by board not by CPU, I suppose.
> 
> Actually I am not 100% sure. For example: we could simply provide the
> socket/core/thread IDs (or links to socket/core objects?) to the CPU
> object, and the CPU object could calculate the APIC ID itself.
> 
> > 
> > > ID to be directly specified in the device_add/-device options.  That's
> > > how real CPUs work: as the CPU manufacturer doesn't know what will be
> > > the package ID of the CPU, the APIC IDs are not hardcoded in the CPU;
> > > they are calculated based on the CPU topology and some socket identifier
> > > signal coming from the board.
> > that is why apic_id has been made a property, to be set from outside.
> 
> True. I believe the conflict here is: we want other objects to set the
> APIC ID (be it the board, or socket/core objects), 

ideally, we really prefer that,
but, you know, QEMU machine implementation seems can't feed us enough
to let us decide CPU ID as real hardware board, and neither in future
for QEMU doesn't like to emulate many complex signals that connect
devices.
so, I think, this property is just as well as we can do,
maybe we can't expect too much :-).

> but at the same time
> it would be interesting to not expose the APIC ID outside QEMU. Being
> too flexible regarding the APIC ID is more likely to cause problems
> later.
> 
> That said, I don't mind having a "apic-id" property because it is easier
> to simply expose it directly. But do you agree that: 1) we don't really
> need to expose it to be set from outside QEMU; 2) we shouldn't require
> it to be set from outside QEMU; 3) we should recommend users to not try
> to fiddle it with?
> 
> 
> > 
> > > 
> > > Comparing it with PCI again: devfn of PCI devices can be specified
> > > manually, but if it is not present you just need to look at the PCI bus
> > > to find out the next available devfn.
> > 18/19 gives an option to lookup available APIC IDs in a target specific way.
> > If we come up with a better target agnostic way to lookup ID, we can always
> > add it on top of currently proposed implementation.
> > 
> > > Based on that, I thought that icc-bridge was going be the component
> > > responsible for the APIC ID allocation logic, considering that it
> > > already contains APIC-ID-based links to the CPU objects.
> > in 19/19 APIC IDs are allocated by board for each possible CPU, icc-bridge is
> > just convenient place for storing links now. If machine was QOM object, I'd
> > probably place it there.
> 
> I was just wondering if we could solve both problems at the same time
> time: if we have a convenient place to store APIC-ID-based links, we are
> getting a more efficient APIC ID allocation/check system for free.
> Especially considering how inefficient is the method used by
> cpu_exists().
> 
> It looks like the problem with my approach is that the QOM APIC-ID-based
> links are target-specific, but cpu_exists() is target-independent. If
> both were target-specific or both were target-independent, we could
> easily reuse the same mechanism for both, but that's not the case now.
>
Igor Mammedov - April 15, 2013, 1:37 p.m.
On Fri, 12 Apr 2013 13:29:40 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Apr 12, 2013 at 05:46:42PM +0200, Igor Mammedov wrote:
> > On Fri, 12 Apr 2013 10:13:13 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...]
> > > > > > +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
> > > 
> > > Well, if link location changed, we could change the cpu_exists()
> > > implementation. We don't even need to hardcode the icc-bridge path, we
> > > could simply follow appropriate links if we set them up.
> > > 
> > > My problem with cpu_exists() is that it is based on global state,
> > > instead of simply using the links between bus/devices to find out if the
> > > IDs are being allocated correctly. We could make it work like PCI: devfn
> > > conflicts are solved simply by looking at the list of devices in the
> > > specific PCIBus where the device is being added. See
> > > do_pci_register_device().
> > > 
> > > That said, I am not against the current approach if we don't have the
> > > necessary bus/links modelled yet. But if we are going to add icc-bridge,
> > > we could benefit from it to implement cpu_exists().
> > Well,check has to be done here anyway, regardless how cpu_exists()
> > implemented.
> > 
> > BTW:
> > This discussion and below belongs more to 9/19 patch that implements
> > cpu_exists() than here.
> 
> OK.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > (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.
> > > 
> > > That's why I believe the CPU has to calculate the APIC ID based on
> > > signaling coming from other devices/buses, instead of allowing the APIC
> > You mean by board not by CPU, I suppose.
> 
> Actually I am not 100% sure. For example: we could simply provide the
> socket/core/thread IDs (or links to socket/core objects?) to the CPU
> object, and the CPU object could calculate the APIC ID itself.
> 
> > 
> > > ID to be directly specified in the device_add/-device options.  That's
> > > how real CPUs work: as the CPU manufacturer doesn't know what will be
> > > the package ID of the CPU, the APIC IDs are not hardcoded in the CPU;
> > > they are calculated based on the CPU topology and some socket identifier
> > > signal coming from the board.
> > that is why apic_id has been made a property, to be set from outside.
> 
> True. I believe the conflict here is: we want other objects to set the
> APIC ID (be it the board, or socket/core objects), but at the same time
> it would be interesting to not expose the APIC ID outside QEMU. Being
> too flexible regarding the APIC ID is more likely to cause problems
> later.
> 
> That said, I don't mind having a "apic-id" property because it is easier
> to simply expose it directly. But do you agree that: 1) we don't really
> need to expose it to be set from outside QEMU; 2) we shouldn't require
> it to be set from outside QEMU; 3) we should recommend users to not try
> to fiddle it with?
Due to nature of per thread CPU hotplug, management will have to specify some
kind of ID to specify which CPU is being plugged. Management really
doesn't/shouldn't care what this ID is.

> 
> > 
> > > 
> > > Comparing it with PCI again: devfn of PCI devices can be specified
> > > manually, but if it is not present you just need to look at the PCI bus
> > > to find out the next available devfn.
> > 18/19 gives an option to lookup available APIC IDs in a target specific
> > way. If we come up with a better target agnostic way to lookup ID, we can
> > always add it on top of currently proposed implementation.
> > 
> > > Based on that, I thought that icc-bridge was going be the component
> > > responsible for the APIC ID allocation logic, considering that it
> > > already contains APIC-ID-based links to the CPU objects.
> > in 19/19 APIC IDs are allocated by board for each possible CPU,
> > icc-bridge is just convenient place for storing links now. If machine was
> > QOM object, I'd probably place it there.
> 
> I was just wondering if we could solve both problems at the same time
> time: if we have a convenient place to store APIC-ID-based links, we are
> getting a more efficient APIC ID allocation/check system for free.
> Especially considering how inefficient is the method used by
> cpu_exists().
Like I said, when we come up with target-independent links interface,
we can easily switch implementation of cpu_exists(). Right now inefficiency
here isn't issue since it's used on slow path only.

> 
> It looks like the problem with my approach is that the QOM APIC-ID-based
> links are target-specific, but cpu_exists() is target-independent. If
> both were target-specific or both were target-independent, we could
> easily reuse the same mechanism for both, but that's not the case now.
> 

cpu_exists() is target-independent but it look-ups CPU by target specific ID,
that target provides via get_arch_id() hook. So for target-i386 it will be
APIC ID.
Eduardo Habkost - April 15, 2013, 1:45 p.m.
On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote:
[...]
> > > > ID to be directly specified in the device_add/-device options.  That's
> > > > how real CPUs work: as the CPU manufacturer doesn't know what will be
> > > > the package ID of the CPU, the APIC IDs are not hardcoded in the CPU;
> > > > they are calculated based on the CPU topology and some socket identifier
> > > > signal coming from the board.
> > > that is why apic_id has been made a property, to be set from outside.
> > 
> > True. I believe the conflict here is: we want other objects to set the
> > APIC ID (be it the board, or socket/core objects), but at the same time
> > it would be interesting to not expose the APIC ID outside QEMU. Being
> > too flexible regarding the APIC ID is more likely to cause problems
> > later.
> > 
> > That said, I don't mind having a "apic-id" property because it is easier
> > to simply expose it directly. But do you agree that: 1) we don't really
> > need to expose it to be set from outside QEMU; 2) we shouldn't require
> > it to be set from outside QEMU; 3) we should recommend users to not try
> > to fiddle it with?
> Due to nature of per thread CPU hotplug, management will have to specify some
> kind of ID to specify which CPU is being plugged. Management really
> doesn't/shouldn't care what this ID is.

As long as management really doesn't/shouldn't care what the ID is,
exposing the APIC ID in the form of an opaque CPU identifier wouldn't be
a problem to me. I just wanted to make clarify if we agree that messing
with the APIC ID directly won't be recommended and that the "apic-id"
property will be for QEMU internal use only.
Igor Mammedov - April 15, 2013, 2:34 p.m.
On Mon, 15 Apr 2013 10:45:24 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote:
> [...]
> > > > > ID to be directly specified in the device_add/-device options.
> > > > > That's how real CPUs work: as the CPU manufacturer doesn't know
> > > > > what will be the package ID of the CPU, the APIC IDs are not
> > > > > hardcoded in the CPU; they are calculated based on the CPU topology
> > > > > and some socket identifier signal coming from the board.
> > > > that is why apic_id has been made a property, to be set from outside.
> > > 
> > > True. I believe the conflict here is: we want other objects to set the
> > > APIC ID (be it the board, or socket/core objects), but at the same time
> > > it would be interesting to not expose the APIC ID outside QEMU. Being
> > > too flexible regarding the APIC ID is more likely to cause problems
> > > later.
> > > 
> > > That said, I don't mind having a "apic-id" property because it is easier
> > > to simply expose it directly. But do you agree that: 1) we don't really
> > > need to expose it to be set from outside QEMU; 2) we shouldn't require
> > > it to be set from outside QEMU; 3) we should recommend users to not try
> > > to fiddle it with?
> > Due to nature of per thread CPU hotplug, management will have to specify
> > some kind of ID to specify which CPU is being plugged. Management really
> > doesn't/shouldn't care what this ID is.
> 
> As long as management really doesn't/shouldn't care what the ID is,
> exposing the APIC ID in the form of an opaque CPU identifier wouldn't be
> a problem to me. I just wanted to make clarify if we agree that messing
> with the APIC ID directly won't be recommended and that the "apic-id"
> property will be for QEMU internal use only.
On contrary, it's useful external feature, x86 guests see only APIC ID, since
it's the only ID they [should] know about. So guest aware mgmt could
definitely use apic_id propery to correlate CPU in guest with QEMU view of
them.
Eduardo Habkost - April 15, 2013, 2:49 p.m.
On Mon, Apr 15, 2013 at 04:34:41PM +0200, Igor Mammedov wrote:
> On Mon, 15 Apr 2013 10:45:24 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote:
> > [...]
> > > > > > ID to be directly specified in the device_add/-device options.
> > > > > > That's how real CPUs work: as the CPU manufacturer doesn't know
> > > > > > what will be the package ID of the CPU, the APIC IDs are not
> > > > > > hardcoded in the CPU; they are calculated based on the CPU topology
> > > > > > and some socket identifier signal coming from the board.
> > > > > that is why apic_id has been made a property, to be set from outside.
> > > > 
> > > > True. I believe the conflict here is: we want other objects to set the
> > > > APIC ID (be it the board, or socket/core objects), but at the same time
> > > > it would be interesting to not expose the APIC ID outside QEMU. Being
> > > > too flexible regarding the APIC ID is more likely to cause problems
> > > > later.
> > > > 
> > > > That said, I don't mind having a "apic-id" property because it is easier
> > > > to simply expose it directly. But do you agree that: 1) we don't really
> > > > need to expose it to be set from outside QEMU; 2) we shouldn't require
> > > > it to be set from outside QEMU; 3) we should recommend users to not try
> > > > to fiddle it with?
> > > Due to nature of per thread CPU hotplug, management will have to specify
> > > some kind of ID to specify which CPU is being plugged. Management really
> > > doesn't/shouldn't care what this ID is.
> > 
> > As long as management really doesn't/shouldn't care what the ID is,
> > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be
> > a problem to me. I just wanted to make clarify if we agree that messing
> > with the APIC ID directly won't be recommended and that the "apic-id"
> > property will be for QEMU internal use only.
> On contrary, it's useful external feature, x86 guests see only APIC ID, since
> it's the only ID they [should] know about. So guest aware mgmt could
> definitely use apic_id propery to correlate CPU in guest with QEMU view of
> them.

You're right, _reading_ the APIC ID is very useful. I am worried about
_setting_ it from external code.
Igor Mammedov - April 15, 2013, 8:27 p.m.
On Mon, 15 Apr 2013 11:49:40 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Apr 15, 2013 at 04:34:41PM +0200, Igor Mammedov wrote:
> > On Mon, 15 Apr 2013 10:45:24 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote:
> > > [...]
> > > > > > > ID to be directly specified in the device_add/-device options.
> > > > > > > That's how real CPUs work: as the CPU manufacturer doesn't know
> > > > > > > what will be the package ID of the CPU, the APIC IDs are not
> > > > > > > hardcoded in the CPU; they are calculated based on the CPU topology
> > > > > > > and some socket identifier signal coming from the board.
> > > > > > that is why apic_id has been made a property, to be set from outside.
> > > > > 
> > > > > True. I believe the conflict here is: we want other objects to set the
> > > > > APIC ID (be it the board, or socket/core objects), but at the same time
> > > > > it would be interesting to not expose the APIC ID outside QEMU. Being
> > > > > too flexible regarding the APIC ID is more likely to cause problems
> > > > > later.
> > > > > 
> > > > > That said, I don't mind having a "apic-id" property because it is easier
> > > > > to simply expose it directly. But do you agree that: 1) we don't really
> > > > > need to expose it to be set from outside QEMU; 2) we shouldn't require
> > > > > it to be set from outside QEMU; 3) we should recommend users to not try
> > > > > to fiddle it with?
> > > > Due to nature of per thread CPU hotplug, management will have to specify
> > > > some kind of ID to specify which CPU is being plugged. Management really
> > > > doesn't/shouldn't care what this ID is.
> > > 
> > > As long as management really doesn't/shouldn't care what the ID is,
> > > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be
> > > a problem to me. I just wanted to make clarify if we agree that messing
> > > with the APIC ID directly won't be recommended and that the "apic-id"
> > > property will be for QEMU internal use only.
> > On contrary, it's useful external feature, x86 guests see only APIC ID, since
> > it's the only ID they [should] know about. So guest aware mgmt could
> > definitely use apic_id propery to correlate CPU in guest with QEMU view of
> > them.
> 
> You're right, _reading_ the APIC ID is very useful. I am worried about
> _setting_ it from external code.

currently it's not possible since cpu-add doesn't allow to set any properties.

We will need setting it for device_add though.
By then, I guess some way to check that it's valid would be enough, otherwise
hot-plugged CPU will be out of scope of MADT and guest would ignore it or
through an error.

> 
> -- 
> Eduardo
>
Eduardo Habkost - April 15, 2013, 8:49 p.m.
On Mon, Apr 15, 2013 at 10:27:57PM +0200, Igor Mammedov wrote:
> On Mon, 15 Apr 2013 11:49:40 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Apr 15, 2013 at 04:34:41PM +0200, Igor Mammedov wrote:
> > > On Mon, 15 Apr 2013 10:45:24 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote:
> > > > [...]
> > > > > > > > ID to be directly specified in the device_add/-device options.
> > > > > > > > That's how real CPUs work: as the CPU manufacturer doesn't know
> > > > > > > > what will be the package ID of the CPU, the APIC IDs are not
> > > > > > > > hardcoded in the CPU; they are calculated based on the CPU topology
> > > > > > > > and some socket identifier signal coming from the board.
> > > > > > > that is why apic_id has been made a property, to be set from outside.
> > > > > > 
> > > > > > True. I believe the conflict here is: we want other objects to set the
> > > > > > APIC ID (be it the board, or socket/core objects), but at the same time
> > > > > > it would be interesting to not expose the APIC ID outside QEMU. Being
> > > > > > too flexible regarding the APIC ID is more likely to cause problems
> > > > > > later.
> > > > > > 
> > > > > > That said, I don't mind having a "apic-id" property because it is easier
> > > > > > to simply expose it directly. But do you agree that: 1) we don't really
> > > > > > need to expose it to be set from outside QEMU; 2) we shouldn't require
> > > > > > it to be set from outside QEMU; 3) we should recommend users to not try
> > > > > > to fiddle it with?
> > > > > Due to nature of per thread CPU hotplug, management will have to specify
> > > > > some kind of ID to specify which CPU is being plugged. Management really
> > > > > doesn't/shouldn't care what this ID is.
> > > > 
> > > > As long as management really doesn't/shouldn't care what the ID is,
> > > > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be
> > > > a problem to me. I just wanted to make clarify if we agree that messing
> > > > with the APIC ID directly won't be recommended and that the "apic-id"
> > > > property will be for QEMU internal use only.
> > > On contrary, it's useful external feature, x86 guests see only APIC ID, since
> > > it's the only ID they [should] know about. So guest aware mgmt could
> > > definitely use apic_id propery to correlate CPU in guest with QEMU view of
> > > them.
> > 
> > You're right, _reading_ the APIC ID is very useful. I am worried about
> > _setting_ it from external code.
> 
> currently it's not possible since cpu-add doesn't allow to set any properties.
> 
> We will need setting it for device_add though.

Not necessarily. That's why I am insisting on an interface based on
links/topology, not based on a raw "apic-id" property: instead of
setting apic-id directly, we could just require that the CPU be attached
to the right socket/core objects, and the APIC ID would be magically
calculated correctly.

Or we could just let the right socket/core/thread IDs to be set as
properties, and apic-id could be calculated based on that. There are
many ways to expose an abstraction that's simpler to use and less likely
to cause problems.


> By then, I guess some way to check that it's valid would be enough, otherwise
> hot-plugged CPU will be out of scope of MADT and guest would ignore it or
> through an error.
>
Igor Mammedov - April 15, 2013, 9:13 p.m.
On Mon, 15 Apr 2013 17:49:20 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Apr 15, 2013 at 10:27:57PM +0200, Igor Mammedov wrote:
> > On Mon, 15 Apr 2013 11:49:40 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Mon, Apr 15, 2013 at 04:34:41PM +0200, Igor Mammedov wrote:
> > > > On Mon, 15 Apr 2013 10:45:24 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > 
> > > > > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote:
> > > > > [...]
> > > > > > > > > ID to be directly specified in the device_add/-device options.
> > > > > > > > > That's how real CPUs work: as the CPU manufacturer doesn't know
> > > > > > > > > what will be the package ID of the CPU, the APIC IDs are not
> > > > > > > > > hardcoded in the CPU; they are calculated based on the CPU topology
> > > > > > > > > and some socket identifier signal coming from the board.
> > > > > > > > that is why apic_id has been made a property, to be set from outside.
> > > > > > > 
> > > > > > > True. I believe the conflict here is: we want other objects to set the
> > > > > > > APIC ID (be it the board, or socket/core objects), but at the same time
> > > > > > > it would be interesting to not expose the APIC ID outside QEMU. Being
> > > > > > > too flexible regarding the APIC ID is more likely to cause problems
> > > > > > > later.
> > > > > > > 
> > > > > > > That said, I don't mind having a "apic-id" property because it is easier
> > > > > > > to simply expose it directly. But do you agree that: 1) we don't really
> > > > > > > need to expose it to be set from outside QEMU; 2) we shouldn't require
> > > > > > > it to be set from outside QEMU; 3) we should recommend users to not try
> > > > > > > to fiddle it with?
> > > > > > Due to nature of per thread CPU hotplug, management will have to specify
> > > > > > some kind of ID to specify which CPU is being plugged. Management really
> > > > > > doesn't/shouldn't care what this ID is.
> > > > > 
> > > > > As long as management really doesn't/shouldn't care what the ID is,
> > > > > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be
> > > > > a problem to me. I just wanted to make clarify if we agree that messing
> > > > > with the APIC ID directly won't be recommended and that the "apic-id"
> > > > > property will be for QEMU internal use only.
> > > > On contrary, it's useful external feature, x86 guests see only APIC ID, since
> > > > it's the only ID they [should] know about. So guest aware mgmt could
> > > > definitely use apic_id propery to correlate CPU in guest with QEMU view of
> > > > them.
> > > 
> > > You're right, _reading_ the APIC ID is very useful. I am worried about
> > > _setting_ it from external code.
> > 
> > currently it's not possible since cpu-add doesn't allow to set any properties.
> > 
> > We will need setting it for device_add though.
> 
> Not necessarily. That's why I am insisting on an interface based on
> links/topology, not based on a raw "apic-id" property: instead of
> setting apic-id directly, we could just require that the CPU be attached
> to the right socket/core objects, and the APIC ID would be magically
> calculated correctly.
> 
> Or we could just let the right socket/core/thread IDs to be set as
> properties, and apic-id could be calculated based on that. There are
> many ways to expose an abstraction that's simpler to use and less likely
> to cause problems.

if we have tree like this:

/nodes/0../sockets/0../cores/0../threads/0..
user will have to parse all branches IDs to collect node/socket/core/thread IDs
before adding CPU.
If use APIC ID on thread level then user is required to get only it and no
magic would be required to calculate it, because board has already calculated
it in advance.

BTW:
it'd be nicer to refuse wrong id earlier at property setting time then
postponing it till realize when it would be possible to check that
node/socket/core/thread info provided earlier was correct.

> 
> 
> > By then, I guess some way to check that it's valid would be enough, otherwise
> > hot-plugged CPU will be out of scope of MADT and guest would ignore it or
> > through an error.
> > 
> 
> -- 
> Eduardo

Patch

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