Eduardo Habkost <ehabk...@redhat.com> writes: > On Wed, Jul 13, 2016 at 06:24:17PM -0400, Bandan Das wrote: >> Igor Mammedov <imamm...@redhat.com> writes: >> >> > CPU added with device_add help won't have APIC ID set, >> > so set it according to socket/core/thread ids provided >> > with device_add command. >> > >> > Signed-off-by: Igor Mammedov <imamm...@redhat.com> >> > --- >> > v3: >> > - use %u for printing topo ids >> > v2: >> > - add validity checks for socket-id/core-id/thread-id values >> > --- >> > hw/i386/pc.c | 44 +++++++++++++++++++++++++++++++++++++++++--- >> > 1 file changed, 41 insertions(+), 3 deletions(-) >> > >> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> > index 24231ca..29da2d4 100644 >> > --- a/hw/i386/pc.c >> > +++ b/hw/i386/pc.c >> > @@ -1763,14 +1763,52 @@ static void pc_cpu_pre_plug(HotplugHandler >> > *hotplug_dev, >> > DeviceState *dev, Error **errp) >> > { >> > int idx; >> > + CPUArchId *cpu_slot; >> > X86CPUTopoInfo topo; >> > X86CPU *cpu = X86_CPU(dev); >> > PCMachineState *pcms = PC_MACHINE(hotplug_dev); >> > - CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx); >> > >> > + /* if APIC ID is not set, set it based on socket/core/thread >> > properties */ >> > + if (cpu->apic_id == UNASSIGNED_APIC_ID) { >> > + int max_socket = (max_cpus - 1) / smp_threads / smp_cores; >> > + >> > + if (cpu->socket_id < 0) { >> > + error_setg(errp, "CPU socket-id is not set"); >> > + return; >> > + } else if (cpu->socket_id > max_socket) { >> > + error_setg(errp, "Invalid CPU socket-id: %u must be in range >> > 0:%u", >> > + cpu->socket_id, max_socket); >> > + return; >> > + } >> > + if (cpu->core_id < 0) { >> > + error_setg(errp, "CPU core-id is not set"); >> > + return; >> > + } else if (cpu->core_id > (smp_cores - 1)) { >> > + error_setg(errp, "Invalid CPU core-id: %u must be in range >> > 0:%u", >> > + cpu->core_id, smp_cores - 1); >> > + return; >> > + } >> > + if (cpu->thread_id < 0) { >> > + error_setg(errp, "CPU thread-id is not set"); >> > + return; >> > + } else if (cpu->thread_id > (smp_threads - 1)) { >> > + error_setg(errp, "Invalid CPU thread-id: %u must be in range >> > 0:%u", >> > + cpu->thread_id, smp_threads - 1); >> > + return; >> > + } >> >> Just curoious, when any of these values < 0, the only other values that they >> can take is -1, right ? > > No, the user might have set thread=-2 explicitly, and this is > where we validate the user-provided values. > >> I am wondering why decided to do the check differently >> for in the preceeding patch. > > Because on both cases we want thread_id <= -2 to generate an > error message. On patch 06/19, thread_id == -1 is one case where > the error message will be skipped (but thread_id == -2 will > generate an error). In this patch, all negative values should > generate an error.
Understood, this makes sense. Thanks. >> >> Bandan >> > + topo.pkg_id = cpu->socket_id; >> > + topo.core_id = cpu->core_id; >> > + topo.smt_id = cpu->thread_id; >> > + cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, >> > &topo); >> > + } >> > + >> > + cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx); >> > if (!cpu_slot) { >> > - error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32 >> > - "), valid range 0:%d", cpu->apic_id, >> > + x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, >> > &topo); >> > + error_setg(errp, "Invalid CPU[socket: %d, core: %d, thread: %d] >> > with" >> > + " APIC ID (%" PRIu32 "), valid index range 0:%d", >> > + topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id, >> > pcms->possible_cpus->len - 1); >> > return; >> > }