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

[Igor Mammedov <imammedo@redhat.com>]

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


-- 
Regards,
  Igor