| Message ID | 1467786055-85835-8-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 06, 2016 at 08:20:43AM +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> > --- > v3: > - use %u for printing topo ids You didn't change the "Invalid CPU" error message. > 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; > + } > + > + 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" You are still printing the X86CPUTopoInfo fields as signed, here. With this changed to use %u too: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > + " APIC ID (%" PRIu32 "), valid index range 0:%d", I thought you were going to change it to print APIC ID as hex. (You can keep my Reviewed-by on either case) > + topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id, > pcms->possible_cpus->len - 1); > return; > } > -- > 2.7.0 >
On Mon, 11 Jul 2016 23:48:27 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Jul 06, 2016 at 08:20:43AM +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> > > --- > > v3: > > - use %u for printing topo ids > > You didn't change the "Invalid CPU" error message. > > > 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; > > + } > > + > > + 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" > > You are still printing the X86CPUTopoInfo fields as signed, here. > > With this changed to use %u too: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> thanks, fixed in v4 > > > > + " APIC ID (%" PRIu32 "), valid index range 0:%d", > > I thought you were going to change it to print APIC ID as hex. done, I'm sorry for missing it here. > > (You can keep my Reviewed-by on either case) > > > + topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id, > > pcms->possible_cpus->len - 1); > > return; > > } > > -- > > 2.7.0 > > >
On Mon, 11 Jul 2016 23:48:27 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Jul 06, 2016 at 08:20:43AM +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> [...] > > > > + " APIC ID (%" PRIu32 "), valid index range 0:%d", > > I thought you were going to change it to print APIC ID as hex. Since unsigned variant for APIC ID is already in x86-next, lets continue with that, and I'll do a patch on top of series that changes printed APIC ID to hex consistently all over the three.
Igor Mammedov <imammedo@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 <imammedo@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 ? I am wondering why decided to do the check differently for in the preceeding patch. 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; > }
On Wed, Jul 13, 2016 at 06:24:17PM -0400, Bandan Das wrote: > Igor Mammedov <imammedo@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 <imammedo@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. > > 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; > > }
Eduardo Habkost <ehabkost@redhat.com> writes: > On Wed, Jul 13, 2016 at 06:24:17PM -0400, Bandan Das wrote: >> Igor Mammedov <imammedo@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 <imammedo@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; >> > }
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; + } + + 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> --- 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(-)