Re: [Qemu-devel] [PATCH v3 6/7] cpu: add device_add foo-x86_64-cpu support

[Eduardo Habkost]

On Thu, Jan 29, 2015 at 03:46:03PM +0100, Igor Mammedov wrote:
[...]
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3406fe8..4347948 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1705,6 +1705,7 @@ static void x86_cpuid_set_apic_id(Object *obj, 
> > Visitor *v, void *opaque,
> >      const int64_t max = UINT32_MAX;
> >      Error *error = NULL;
> >      int64_t value;
> > +    X86CPUTopoInfo topo;
> >  
> >      if (dev->realized) {
> >          error_setg(errp, "Attempt to set property '%s' on '%s' after "
> > @@ -1724,6 +1725,19 @@ static void x86_cpuid_set_apic_id(Object *obj, 
> > Visitor *v, void *opaque,
> >          return;
> >      }
> >  
> > +    if (value > x86_cpu_apic_id_from_index(max_cpus - 1)) {
> > +        error_setg(errp, "CPU with APIC ID %" PRIi64
> > +                   " is more than MAX APIC ID limits", value);
> > +        return;
> > +    }
> > +
> > +    x86_topo_ids_from_apic_id(smp_cores, smp_threads, value, &topo);
> > +    if (topo.smt_id >= smp_threads || topo.core_id >= smp_cores) {
> If I recall correctly, APIC id also encodes socket and numa node it belongs 
> to,
> so why it's not checked here?

APIC ID doesn't encode NUMA node information, just the socket, core, and
thread IDs.

If I understand it coreectly, the check above is to ensure we are within
the limits configured in the command-line. There's a cores-per-socket
and threads-per-core option, but not an explicit limit for the number of
sockets. The limit for the number of sockets is implicit (it depends on
max_cpus), and we are already checking the value against max_cpus above.

This is related to a previous discussion about the semantics of the
"sockets" option. I always assumed the "sockets" option was about
non-hotplug VCPUs (smp_cpus = threads * cores * sockets) but Drew
recently sent some patches assuming the "sockets" option should include
sockets for CPU hotplug as well (max_cpus = threads * cores * sockets),
and it may make more sense. In either case, we need to define and
document those semantics more clearly to avoid confusion.

> 
> > +        error_setg(errp, "CPU with APIC ID %" PRIi64 " does not match "
> > +                   "topology configuration.", value);
> > +        return;
> > +    }
> > +
> >      if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
> >          error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
> >          return;
[...]
> > +#define APIC_ID_NOT_SET (~0U)
> > +
> >  static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> >  {
> > -    CPUX86State *env = &cpu->env;
> >      DeviceState *dev = DEVICE(cpu);
> >      APICCommonState *apic;
> >      const char *apic_type = "apic";
> > +    uint32_t apic_id;
> >  
> >      if (kvm_irqchip_in_kernel()) {
> >          apic_type = "kvm-apic";
> > @@ -2742,7 +2776,14 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error 
> > **errp)
> >  
> >      object_property_add_child(OBJECT(cpu), "apic",
> >                                OBJECT(cpu->apic_state), NULL);
> > -    qdev_prop_set_uint8(cpu->apic_state, "id", env->cpuid_apic_id);
> > +
> > +    apic_id = object_property_get_int(OBJECT(cpu), "apic-id", NULL);
> > +    if (apic_id == APIC_ID_NOT_SET) {
> Do we have in QOM a way to check if property was ever set?

I don't believe the QOM property model has any abstraction for unset
properties.

> 
> > +        apic_id = get_free_apic_id();
> > +        object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
> > +    }
> > +
> > +    qdev_prop_set_uint8(cpu->apic_state, "id", apic_id);

-- 
Eduardo