| Message ID | 1466784366-281935-8-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 24, 2016 at 06:05:55PM +0200, Igor Mammedov wrote: > 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 <imammedo@redhat.com> > --- > 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 eaa4b59..0a65caf 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; The calculation looks right, but I would love to move this to common code that takes care of SMP configuration and topology, instead of an inline calculation. We have been bitten by bugs related to ad hoc topology calculations before (see commit 7dfddd7). I think we can do that as a follow-up patch after Drew's series is included. > + [...] > + 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" topo.{pkg,core,smt}_id are unsigned. Treating them as signed can generate confusing error messages: (qemu) device_add qemu64-x86_64-cpu,apic-id=0xfffffffe Invalid CPU[socket: -2, core: 0, thread: 0] with APIC ID (4294967294), valid index range 0:3 Also, I wonder if showing APIC ID in hex wouldn't be more useful. > + " 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; > } > -- > 1.8.3.1 > >
On Mon, 4 Jul 2016 16:17:09 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Jun 24, 2016 at 06:05:55PM +0200, Igor Mammedov wrote: > > 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 <imammedo@redhat.com> > > --- > > 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 eaa4b59..0a65caf 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; > > The calculation looks right, but I would love to move this to > common code that takes care of SMP configuration and topology, > instead of an inline calculation. We have been bitten by bugs > related to ad hoc topology calculations before (see commit > 7dfddd7). > > I think we can do that as a follow-up patch after Drew's series is > included. Ok, I can do it on top of Drew's series as follow-up > > + > [...] > > + 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" > > topo.{pkg,core,smt}_id are unsigned. Treating them as signed can > generate confusing error messages: > > (qemu) device_add qemu64-x86_64-cpu,apic-id=0xfffffffe > Invalid CPU[socket: -2, core: 0, thread: 0] with APIC ID > (4294967294), valid index range 0:3 > > Also, I wonder if showing APIC ID in hex wouldn't be more useful. it would be easier to read, I'll change it to hex. > > > > + " 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; > > } > > -- > > 1.8.3.1 > > > > >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index eaa4b59..0a65caf 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: %d must be in range 0:%d", + 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: %d must be in range 0:%d", + 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: %d must be in range 0:%d", + cpu->thread_id, smp_threads - 1); + return; + } + + 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; }
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 <imammedo@redhat.com> --- v2: - add validity checks for socket-id/core-id/thread-id values --- hw/i386/pc.c | 44 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-)