| Message ID | 1466693669-139967-8-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 23, 2016 at 04:54:25PM +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> > --- > hw/i386/pc.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 87352ae..63e9bb6 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1758,13 +1758,23 @@ 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(pcms, CPU(dev), &idx); > > + if (cpu->apic_id == 0xFFFFFFFF) { > + /* APIC ID not set, set it based on socket/core/thread properties */ > + X86CPUTopoInfo topo = { cpu->socket, cpu->core, cpu->thread }; > + cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo); > + } What happens if neither apic-id or socket/core/thread are set? apicid_from_topo_ids() needs the caller to ensure core_id < nr_cores && smt_id < nr_threads. Do you do that? > + > + cpu_slot = pc_find_cpu(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); The error message is a bit confusing: the interface is not based on CPU index anymore, but it still says "valid index range 0:%d". > return; > } > @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > * added CPUs so that query_hotpluggable_cpus would show correct values > */ > if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1) { > - X86CPUTopoInfo topo; > - > x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo); > cpu->thread = topo.smt_id; > cpu->core = topo.core_id; > -- > 1.8.3.1 >
On Thu, 23 Jun 2016 14:19:16 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jun 23, 2016 at 04:54:25PM +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> > > --- > > hw/i386/pc.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 87352ae..63e9bb6 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1758,13 +1758,23 @@ 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(pcms, CPU(dev), &idx); > > > > + if (cpu->apic_id == 0xFFFFFFFF) { > > + /* APIC ID not set, set it based on socket/core/thread > > properties */ > > + X86CPUTopoInfo topo = { cpu->socket, cpu->core, > > cpu->thread }; > > + cpu->apic_id = apicid_from_topo_ids(smp_cores, > > smp_threads, &topo); > > + } > > What happens if neither apic-id or socket/core/thread are set? > > apicid_from_topo_ids() needs the caller to ensure > core_id < nr_cores && smt_id < nr_threads. Do you do that? it will get wrong apic_id and error in "if (!cpu_slot)" will trigger, I can explicitly check for unset values report error saying that they must be set if you prefer. > > > + > > + cpu_slot = pc_find_cpu(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); > > The error message is a bit confusing: the interface is not based > on CPU index anymore, but it still says "valid index range 0:%d". it's still based on CPU index for cpu-add and -numa cpus, so until we get rid of index in there it still should be printed. > > > return; > > } > > @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler > > *hotplug_dev, > > * added CPUs so that query_hotpluggable_cpus would show > > correct values */ > > if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1) > > { > > - X86CPUTopoInfo topo; > > - > > x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, > > smp_threads, &topo); cpu->thread = topo.smt_id; > > cpu->core = topo.core_id; > > -- > > 1.8.3.1 > > >
On Thu, Jun 23, 2016 at 07:48:24PM +0200, Igor Mammedov wrote: > On Thu, 23 Jun 2016 14:19:16 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Jun 23, 2016 at 04:54:25PM +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> > > > --- > > > hw/i386/pc.c | 18 +++++++++++++----- > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index 87352ae..63e9bb6 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1758,13 +1758,23 @@ 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(pcms, CPU(dev), &idx); > > > > > > + if (cpu->apic_id == 0xFFFFFFFF) { > > > + /* APIC ID not set, set it based on socket/core/thread > > > properties */ > > > + X86CPUTopoInfo topo = { cpu->socket, cpu->core, > > > cpu->thread }; > > > + cpu->apic_id = apicid_from_topo_ids(smp_cores, > > > smp_threads, &topo); > > > + } > > > > What happens if neither apic-id or socket/core/thread are set? > > > > apicid_from_topo_ids() needs the caller to ensure > > core_id < nr_cores && smt_id < nr_threads. Do you do that? > it will get wrong apic_id and error in "if (!cpu_slot)" will trigger, > I can explicitly check for unset values report error saying that > they must be set if you prefer. Will it? If smp_cores=2 and smp_threads=2, socket=0,core=0,thread=3 will generate a valid APIC ID (corresponding to socket=0,core=1,thread=1). About unset/invalid values: apicid_from_topo_ids() documentation explicitly requires core_id < nr_cores && smt_id < nr_threads, and may generate a valid APIC ID if only some of the socket/core/thread values are set to -1. (Also, if you don't check for missing/invalid values explicitly you will generate a very confusing error message for the user). > > > > > > + > > > + cpu_slot = pc_find_cpu(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); > > > > The error message is a bit confusing: the interface is not based > > on CPU index anymore, but it still says "valid index range 0:%d". > it's still based on CPU index for cpu-add and -numa cpus, > so until we get rid of index in there it still should be printed. OK. > > > > > > return; > > > } > > > @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler > > > *hotplug_dev, > > > * added CPUs so that query_hotpluggable_cpus would show > > > correct values */ > > > if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1) > > > { > > > - X86CPUTopoInfo topo; > > > - > > > x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, > > > smp_threads, &topo); cpu->thread = topo.smt_id; > > > cpu->core = topo.core_id; > > > -- > > > 1.8.3.1 > > > > > >
On Thu, 23 Jun 2016 16:27:46 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jun 23, 2016 at 07:48:24PM +0200, Igor Mammedov wrote: > > On Thu, 23 Jun 2016 14:19:16 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Thu, Jun 23, 2016 at 04:54:25PM +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> > > > > --- > > > > hw/i386/pc.c | 18 +++++++++++++----- > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > index 87352ae..63e9bb6 100644 > > > > --- a/hw/i386/pc.c > > > > +++ b/hw/i386/pc.c > > > > @@ -1758,13 +1758,23 @@ 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(pcms, CPU(dev), &idx); > > > > > > > > + if (cpu->apic_id == 0xFFFFFFFF) { > > > > + /* APIC ID not set, set it based on socket/core/thread > > > > properties */ > > > > + X86CPUTopoInfo topo = { cpu->socket, cpu->core, > > > > cpu->thread }; > > > > + cpu->apic_id = apicid_from_topo_ids(smp_cores, > > > > smp_threads, &topo); > > > > + } > > > > > > What happens if neither apic-id or socket/core/thread are set? > > > > > > apicid_from_topo_ids() needs the caller to ensure > > > core_id < nr_cores && smt_id < nr_threads. Do you do that? > > it will get wrong apic_id and error in "if (!cpu_slot)" will > > trigger, I can explicitly check for unset values report error > > saying that they must be set if you prefer. > > Will it? If smp_cores=2 and smp_threads=2, > socket=0,core=0,thread=3 will generate a valid APIC ID > (corresponding to socket=0,core=1,thread=1). > > About unset/invalid values: apicid_from_topo_ids() documentation > explicitly requires core_id < nr_cores && smt_id < nr_threads, > and may generate a valid APIC ID if only some of the > socket/core/thread values are set to -1. > > (Also, if you don't check for missing/invalid values explicitly > you will generate a very confusing error message for the user). ok, I'll add explicit checks for unset/invalid values here as you suggested in previous patch. > > > > > > > > > > + > > > > + cpu_slot = pc_find_cpu(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); > > > > > > The error message is a bit confusing: the interface is not based > > > on CPU index anymore, but it still says "valid index range 0:%d". > > it's still based on CPU index for cpu-add and -numa cpus, > > so until we get rid of index in there it still should be printed. > > OK. > > > > > > > > > > return; > > > > } > > > > @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler > > > > *hotplug_dev, > > > > * added CPUs so that query_hotpluggable_cpus would show > > > > correct values */ > > > > if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == > > > > -1) { > > > > - X86CPUTopoInfo topo; > > > > - > > > > x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, > > > > smp_threads, &topo); cpu->thread = topo.smt_id; > > > > cpu->core = topo.core_id; > > > > -- > > > > 1.8.3.1 > > > > > > > > > >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 87352ae..63e9bb6 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1758,13 +1758,23 @@ 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(pcms, CPU(dev), &idx); + if (cpu->apic_id == 0xFFFFFFFF) { + /* APIC ID not set, set it based on socket/core/thread properties */ + X86CPUTopoInfo topo = { cpu->socket, cpu->core, cpu->thread }; + cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo); + } + + cpu_slot = pc_find_cpu(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; } @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, * added CPUs so that query_hotpluggable_cpus would show correct values */ if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1) { - X86CPUTopoInfo topo; - x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo); cpu->thread = topo.smt_id; cpu->core = topo.core_id;
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> --- hw/i386/pc.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)