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

[Eduardo Habkost]

On Fri, Apr 12, 2013 at 12:20:45PM +0200, Igor Mammedov wrote:
> On Thu, 11 Apr 2013 16:12:33 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > > Reviewed-by: Paolo Bonzini <address@hidden>
> > > ---
> > > 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

-- 
Eduardo