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
