For enabling early cpu to numa node configuration at runtime
qmp_query_hotpluggable_cpus() should provide a list of available
cpu slots at early stage, before machine_init() is called and
the 1st cpu is created, so that mgmt might be able to call it
and use output to set numa mapping.
Use MachineClass::possible_cpu_arch_ids() callback to set
cpu type info, along with the rest of possible cpu properties,
to let machine define which cpu type* will be used.
* for SPAPR it will be a spapr core type and for ARM/s390x/x86
a respective descendant of CPUClass.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/hw/boards.h | 2 ++
hw/arm/virt.c | 3 ++-
hw/core/machine.c | 12 ++++++------
hw/i386/pc.c | 4 +++-
hw/ppc/spapr.c | 13 ++++++++-----
hw/s390x/s390-virtio-ccw.c | 1 +
6 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 156e0a5..2c3e958 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
* CPUArchId:
* @arch_id - architecture-dependent CPU ID of present or possible CPU
* @cpu - pointer to corresponding CPU object if it's present on NULL otherwise
+ * @type - QOM class name of possible @cpu object
* @props - CPU object properties, initialized by board
* #vcpus_count - number of threads provided by @cpu object
*/
@@ -88,6 +89,7 @@ typedef struct {
int64_t vcpus_count;
CpuInstanceProperties props;
Object *cpu;
+ const char *type;
} CPUArchId;
/**
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9e18b41..88319db 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1357,7 +1357,7 @@ static void machvirt_init(MachineState *machine)
break;
}
- cpuobj = object_new(machine->cpu_type);
+ cpuobj = object_new(possible_cpus->cpus[n].type);
object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id,
"mp-affinity", NULL);
@@ -1573,6 +1573,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
sizeof(CPUArchId) * max_cpus);
ms->possible_cpus->len = max_cpus;
for (n = 0; n < ms->possible_cpus->len; n++) {
+ ms->possible_cpus->cpus[n].type = ms->cpu_type;
ms->possible_cpus->cpus[n].arch_id =
virt_cpu_mp_affinity(vms, n);
ms->possible_cpus->cpus[n].props.has_thread_id = true;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index f482211..1e1fca5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -363,18 +363,18 @@ static void machine_init_notify(Notifier *notifier, void *data)
HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
{
int i;
- Object *cpu;
HotpluggableCPUList *head = NULL;
- const char *cpu_type;
+ MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+ /* force board to initialize possible_cpus if it hasn't been done yet */
+ mc->possible_cpu_arch_ids(machine);
- cpu = machine->possible_cpus->cpus[0].cpu;
- assert(cpu); /* Boot cpu is always present */
- cpu_type = object_get_typename(cpu);
for (i = 0; i < machine->possible_cpus->len; i++) {
+ Object *cpu;
HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
- cpu_item->type = g_strdup(cpu_type);
+ cpu_item->type = g_strdup(machine->possible_cpus->cpus[i].type);
cpu_item->vcpus_count = machine->possible_cpus->cpus[i].vcpus_count;
cpu_item->props = g_memdup(&machine->possible_cpus->cpus[i].props,
sizeof(*cpu_item->props));
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8e307f7..99afb2f1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1147,7 +1147,8 @@ void pc_cpus_init(PCMachineState *pcms)
pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
possible_cpus = mc->possible_cpu_arch_ids(ms);
for (i = 0; i < smp_cpus; i++) {
- pc_new_cpu(ms->cpu_type, possible_cpus->cpus[i].arch_id, &error_fatal);
+ pc_new_cpu(possible_cpus->cpus[i].type, possible_cpus->cpus[i].arch_id,
+ &error_fatal);
}
}
@@ -2269,6 +2270,7 @@ static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
for (i = 0; i < ms->possible_cpus->len; i++) {
X86CPUTopoInfo topo;
+ ms->possible_cpus->cpus[i].type = ms->cpu_type;
ms->possible_cpus->cpus[i].vcpus_count = 1;
ms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 29de845..9f455e8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2129,11 +2129,6 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
int boot_cores_nr = smp_cpus / smp_threads;
int i;
- if (!type) {
- error_report("Unable to find sPAPR CPU Core definition");
- exit(1);
- }
-
possible_cpus = mc->possible_cpu_arch_ids(machine);
if (mc->has_hotpluggable_cpus) {
if (smp_cpus % smp_threads) {
@@ -3419,6 +3414,7 @@ static int64_t spapr_get_default_cpu_node_id(const MachineState *ms, int idx)
static const CPUArchIdList *spapr_possible_cpu_arch_ids(MachineState *machine)
{
int i;
+ const char *core_type;
int spapr_max_cores = max_cpus / smp_threads;
MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -3430,12 +3426,19 @@ static const CPUArchIdList *spapr_possible_cpu_arch_ids(MachineState *machine)
return machine->possible_cpus;
}
+ core_type = spapr_get_cpu_core_type(machine->cpu_type);
+ if (!core_type) {
+ error_report("Unable to find sPAPR CPU Core definition");
+ exit(1);
+ }
+
machine->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
sizeof(CPUArchId) * spapr_max_cores);
machine->possible_cpus->len = spapr_max_cores;
for (i = 0; i < machine->possible_cpus->len; i++) {
int core_id = i * smp_threads;
+ machine->possible_cpus->cpus[i].type = core_type;
machine->possible_cpus->cpus[i].vcpus_count = smp_threads;
machine->possible_cpus->cpus[i].arch_id = core_id;
machine->possible_cpus->cpus[i].props.has_core_id = true;
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f64db51..ae73fb6 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -385,6 +385,7 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
sizeof(CPUArchId) * max_cpus);
ms->possible_cpus->len = max_cpus;
for (i = 0; i < ms->possible_cpus->len; i++) {
+ ms->possible_cpus->cpus[i].type = ms->cpu_type;
ms->possible_cpus->cpus[i].vcpus_count = 1;
ms->possible_cpus->cpus[i].arch_id = i;
ms->possible_cpus->cpus[i].props.has_core_id = true;
--
2.7.4
For enabling early cpu to numa node configuration at runtime
qmp_query_hotpluggable_cpus() should provide a list of available
cpu slots at early stage, before machine_init() is called and
the 1st cpu is created, so that mgmt might be able to call it
and use output to set numa mapping.
Use MachineClass::possible_cpu_arch_ids() callback to set
cpu type info, along with the rest of possible cpu properties,
to let machine define which cpu type* will be used.
* for SPAPR it will be a spapr core type and for ARM/s390x/x86
a respective descendant of CPUClass.
Move parse_numa_opts() in vl.c after cpu_model is parsed into
cpu_type so that possible_cpu_arch_ids() would know which
cpu_type to use during layout initialization.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
- fix NULL dereference caused by not initialized
MachineState::cpu_type at the time parse_numa_opts()
were called
---
include/hw/boards.h | 2 ++
hw/arm/virt.c | 3 ++-
hw/core/machine.c | 12 ++++++------
hw/i386/pc.c | 4 +++-
hw/ppc/spapr.c | 13 ++++++++-----
hw/s390x/s390-virtio-ccw.c | 1 +
vl.c | 3 +--
7 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 191a5b3..fa21758 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
* CPUArchId:
* @arch_id - architecture-dependent CPU ID of present or possible CPU
* @cpu - pointer to corresponding CPU object if it's present on NULL otherwise
+ * @type - QOM class name of possible @cpu object
* @props - CPU object properties, initialized by board
* #vcpus_count - number of threads provided by @cpu object
*/
@@ -88,6 +89,7 @@ typedef struct {
int64_t vcpus_count;
CpuInstanceProperties props;
Object *cpu;
+ const char *type;
} CPUArchId;
/**
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9e18b41..88319db 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1357,7 +1357,7 @@ static void machvirt_init(MachineState *machine)
break;
}
- cpuobj = object_new(machine->cpu_type);
+ cpuobj = object_new(possible_cpus->cpus[n].type);
object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id,
"mp-affinity", NULL);
@@ -1573,6 +1573,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
sizeof(CPUArchId) * max_cpus);
ms->possible_cpus->len = max_cpus;
for (n = 0; n < ms->possible_cpus->len; n++) {
+ ms->possible_cpus->cpus[n].type = ms->cpu_type;
ms->possible_cpus->cpus[n].arch_id =
virt_cpu_mp_affinity(vms, n);
ms->possible_cpus->cpus[n].props.has_thread_id = true;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index df46275..42cea7c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -363,18 +363,18 @@ static void machine_init_notify(Notifier *notifier, void *data)
HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
{
int i;
- Object *cpu;
HotpluggableCPUList *head = NULL;
- const char *cpu_type;
+ MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+ /* force board to initialize possible_cpus if it hasn't been done yet */
+ mc->possible_cpu_arch_ids(machine);
- cpu = machine->possible_cpus->cpus[0].cpu;
- assert(cpu); /* Boot cpu is always present */
- cpu_type = object_get_typename(cpu);
for (i = 0; i < machine->possible_cpus->len; i++) {
+ Object *cpu;
HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
- cpu_item->type = g_strdup(cpu_type);
+ cpu_item->type = g_strdup(machine->possible_cpus->cpus[i].type);
cpu_item->vcpus_count = machine->possible_cpus->cpus[i].vcpus_count;
cpu_item->props = g_memdup(&machine->possible_cpus->cpus[i].props,
sizeof(*cpu_item->props));
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8e307f7..99afb2f1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1147,7 +1147,8 @@ void pc_cpus_init(PCMachineState *pcms)
pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
possible_cpus = mc->possible_cpu_arch_ids(ms);
for (i = 0; i < smp_cpus; i++) {
- pc_new_cpu(ms->cpu_type, possible_cpus->cpus[i].arch_id, &error_fatal);
+ pc_new_cpu(possible_cpus->cpus[i].type, possible_cpus->cpus[i].arch_id,
+ &error_fatal);
}
}
@@ -2269,6 +2270,7 @@ static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
for (i = 0; i < ms->possible_cpus->len; i++) {
X86CPUTopoInfo topo;
+ ms->possible_cpus->cpus[i].type = ms->cpu_type;
ms->possible_cpus->cpus[i].vcpus_count = 1;
ms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d682f01..9d63477 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2135,11 +2135,6 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
int boot_cores_nr = smp_cpus / smp_threads;
int i;
- if (!type) {
- error_report("Unable to find sPAPR CPU Core definition");
- exit(1);
- }
-
possible_cpus = mc->possible_cpu_arch_ids(machine);
if (mc->has_hotpluggable_cpus) {
if (smp_cpus % smp_threads) {
@@ -3438,6 +3433,7 @@ static int64_t spapr_get_default_cpu_node_id(const MachineState *ms, int idx)
static const CPUArchIdList *spapr_possible_cpu_arch_ids(MachineState *machine)
{
int i;
+ const char *core_type;
int spapr_max_cores = max_cpus / smp_threads;
MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -3449,12 +3445,19 @@ static const CPUArchIdList *spapr_possible_cpu_arch_ids(MachineState *machine)
return machine->possible_cpus;
}
+ core_type = spapr_get_cpu_core_type(machine->cpu_type);
+ if (!core_type) {
+ error_report("Unable to find sPAPR CPU Core definition");
+ exit(1);
+ }
+
machine->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
sizeof(CPUArchId) * spapr_max_cores);
machine->possible_cpus->len = spapr_max_cores;
for (i = 0; i < machine->possible_cpus->len; i++) {
int core_id = i * smp_threads;
+ machine->possible_cpus->cpus[i].type = core_type;
machine->possible_cpus->cpus[i].vcpus_count = smp_threads;
machine->possible_cpus->cpus[i].arch_id = core_id;
machine->possible_cpus->cpus[i].props.has_core_id = true;
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e593c71..75084a8 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -385,6 +385,7 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
sizeof(CPUArchId) * max_cpus);
ms->possible_cpus->len = max_cpus;
for (i = 0; i < ms->possible_cpus->len; i++) {
+ ms->possible_cpus->cpus[i].type = ms->cpu_type;
ms->possible_cpus->cpus[i].vcpus_count = 1;
ms->possible_cpus->cpus[i].arch_id = i;
ms->possible_cpus->cpus[i].props.has_core_id = true;
diff --git a/vl.c b/vl.c
index 0723835..598217a 100644
--- a/vl.c
+++ b/vl.c
@@ -4677,8 +4677,6 @@ int main(int argc, char **argv, char **envp)
default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
- parse_numa_opts(current_machine);
-
if (qemu_opts_foreach(qemu_find_opts("mon"),
mon_init_func, NULL, NULL)) {
exit(1);
@@ -4737,6 +4735,7 @@ int main(int argc, char **argv, char **envp)
cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model);
}
}
+ parse_numa_opts(current_machine);
machine_run_board_init(current_machine);
--
2.7.4
On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > For enabling early cpu to numa node configuration at runtime > qmp_query_hotpluggable_cpus() should provide a list of available > cpu slots at early stage, before machine_init() is called and > the 1st cpu is created, so that mgmt might be able to call it > and use output to set numa mapping. > Use MachineClass::possible_cpu_arch_ids() callback to set > cpu type info, along with the rest of possible cpu properties, > to let machine define which cpu type* will be used. > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > a respective descendant of CPUClass. > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > cpu_type so that possible_cpu_arch_ids() would know which > cpu_type to use during layout initialization. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > v2: > - fix NULL dereference caused by not initialized > MachineState::cpu_type at the time parse_numa_opts() > were called > --- > include/hw/boards.h | 2 ++ > hw/arm/virt.c | 3 ++- > hw/core/machine.c | 12 ++++++------ > hw/i386/pc.c | 4 +++- > hw/ppc/spapr.c | 13 ++++++++----- > hw/s390x/s390-virtio-ccw.c | 1 + > vl.c | 3 +-- > 7 files changed, 23 insertions(+), 15 deletions(-) > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 191a5b3..fa21758 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > * CPUArchId: > * @arch_id - architecture-dependent CPU ID of present or possible CPU I know this isn't really in scope for this patch, but is @arch_id here supposed to have meaning defined by the target, or by the machine? If it's the machime, it could do with a rename - "arch" means target to most people (thanks to Linux). If it's the target, it's kind of bogus, because it doesn't necessarily have a clear meaning per target - get_arch_id in CPUClass has the same problem, which is probably one reason it's basically only used by the x86 code at present. e.g. for target/ppc, what do we use? There's the PIR, which is in the CPU.. but only on some cpu models, not all. There will generally be some kind of master PIC id, but there are different PIC models on different boards. What goes in the devicetree? Well only some machines use devicetree, and they might define the cpu reg differently. Board designs will generally try to make some if not all of those possible values equal for simplicity, but there's still no real way of defining a sensible arch_id independent of machine / board. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, 19 Oct 2017 17:31:51 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > > For enabling early cpu to numa node configuration at runtime > > qmp_query_hotpluggable_cpus() should provide a list of available > > cpu slots at early stage, before machine_init() is called and > > the 1st cpu is created, so that mgmt might be able to call it > > and use output to set numa mapping. > > Use MachineClass::possible_cpu_arch_ids() callback to set > > cpu type info, along with the rest of possible cpu properties, > > to let machine define which cpu type* will be used. > > > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > > a respective descendant of CPUClass. > > > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > > cpu_type so that possible_cpu_arch_ids() would know which > > cpu_type to use during layout initialization. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > --- > > v2: > > - fix NULL dereference caused by not initialized > > MachineState::cpu_type at the time parse_numa_opts() > > were called > > --- > > include/hw/boards.h | 2 ++ > > hw/arm/virt.c | 3 ++- > > hw/core/machine.c | 12 ++++++------ > > hw/i386/pc.c | 4 +++- > > hw/ppc/spapr.c | 13 ++++++++----- > > hw/s390x/s390-virtio-ccw.c | 1 + > > vl.c | 3 +-- > > 7 files changed, 23 insertions(+), 15 deletions(-) > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 191a5b3..fa21758 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > * CPUArchId: > > * @arch_id - architecture-dependent CPU ID of present or possible CPU > > I know this isn't really in scope for this patch, but is @arch_id here > supposed to have meaning defined by the target, or by the machine? > > If it's the machime, it could do with a rename - "arch" means target > to most people (thanks to Linux). > > If it's the target, it's kind of bogus, because it doesn't necessarily > have a clear meaning per target - get_arch_id in CPUClass has the same > problem, which is probably one reason it's basically only used by the > x86 code at present. > > e.g. for target/ppc, what do we use? There's the PIR, which is in the > CPU.. but only on some cpu models, not all. There will generally be > some kind of master PIC id, but there are different PIC models on > different boards. What goes in the devicetree? Well only some > machines use devicetree, and they might define the cpu reg > differently. > > Board designs will generally try to make some if not all of those > possible values equal for simplicity, but there's still no real way of > defining a sensible arch_id independent of machine / board. I'd say arch_id is machine specific so far, it was introduced when we didn't have CpuInstanceProperties and at that time we considered only vcpus (threads) and doesn't really apply to spapr cores. In general we could do away with arch_id and use CpuInstanceProperties instead, but arch_id also serves aux purpose, it allows machine to pre-calculate(cache) apic-id/mpidr values in one place and then they are/(could be) used by arch in-depended code to build acpi tables. So if we drop arch_id we would need to introduce a machine hook, which would translate CpuInstanceProperties into current arch_id.
On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote: > On Thu, 19 Oct 2017 17:31:51 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > > > For enabling early cpu to numa node configuration at runtime > > > qmp_query_hotpluggable_cpus() should provide a list of available > > > cpu slots at early stage, before machine_init() is called and > > > the 1st cpu is created, so that mgmt might be able to call it > > > and use output to set numa mapping. > > > Use MachineClass::possible_cpu_arch_ids() callback to set > > > cpu type info, along with the rest of possible cpu properties, > > > to let machine define which cpu type* will be used. > > > > > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > > > a respective descendant of CPUClass. > > > > > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > > > cpu_type so that possible_cpu_arch_ids() would know which > > > cpu_type to use during layout initialization. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > > --- > > > v2: > > > - fix NULL dereference caused by not initialized > > > MachineState::cpu_type at the time parse_numa_opts() > > > were called > > > --- > > > include/hw/boards.h | 2 ++ > > > hw/arm/virt.c | 3 ++- > > > hw/core/machine.c | 12 ++++++------ > > > hw/i386/pc.c | 4 +++- > > > hw/ppc/spapr.c | 13 ++++++++----- > > > hw/s390x/s390-virtio-ccw.c | 1 + > > > vl.c | 3 +-- > > > 7 files changed, 23 insertions(+), 15 deletions(-) > > > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > index 191a5b3..fa21758 100644 > > > --- a/include/hw/boards.h > > > +++ b/include/hw/boards.h > > > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > > * CPUArchId: > > > * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > > I know this isn't really in scope for this patch, but is @arch_id here > > supposed to have meaning defined by the target, or by the machine? > > > > If it's the machime, it could do with a rename - "arch" means target > > to most people (thanks to Linux). > > > > If it's the target, it's kind of bogus, because it doesn't necessarily > > have a clear meaning per target - get_arch_id in CPUClass has the same > > problem, which is probably one reason it's basically only used by the > > x86 code at present. > > > > e.g. for target/ppc, what do we use? There's the PIR, which is in the > > CPU.. but only on some cpu models, not all. There will generally be > > some kind of master PIC id, but there are different PIC models on > > different boards. What goes in the devicetree? Well only some > > machines use devicetree, and they might define the cpu reg > > differently. > > > > Board designs will generally try to make some if not all of those > > possible values equal for simplicity, but there's still no real way of > > defining a sensible arch_id independent of machine / board. > I'd say arch_id is machine specific so far, it was introduced when we > didn't have CpuInstanceProperties and at that time we considered only > vcpus (threads) and doesn't really apply to spapr cores. > > In general we could do away with arch_id and use CpuInstanceProperties > instead, but arch_id also serves aux purpose, it allows machine to > pre-calculate(cache) apic-id/mpidr values in one place and then they > are/(could be) used by arch in-depended code to build acpi tables. > So if we drop arch_id we would need to introduce a machine hook, > which would translate CpuInstanceProperties into current arch_id. I think we need to do a better to job documenting where exactly we expect arch_id to be used and how, so people know what it's supposed to return. If the only place where it's useful now is ACPI code (is it?), should we rename it to something like get_acpi_id()? -- Eduardo
On Mon, 6 Nov 2017 16:02:16 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote: > > On Thu, 19 Oct 2017 17:31:51 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > > > > For enabling early cpu to numa node configuration at runtime > > > > qmp_query_hotpluggable_cpus() should provide a list of available > > > > cpu slots at early stage, before machine_init() is called and > > > > the 1st cpu is created, so that mgmt might be able to call it > > > > and use output to set numa mapping. > > > > Use MachineClass::possible_cpu_arch_ids() callback to set > > > > cpu type info, along with the rest of possible cpu properties, > > > > to let machine define which cpu type* will be used. > > > > > > > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > > > > a respective descendant of CPUClass. > > > > > > > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > > > > cpu_type so that possible_cpu_arch_ids() would know which > > > > cpu_type to use during layout initialization. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > --- > > > > v2: > > > > - fix NULL dereference caused by not initialized > > > > MachineState::cpu_type at the time parse_numa_opts() > > > > were called > > > > --- > > > > include/hw/boards.h | 2 ++ > > > > hw/arm/virt.c | 3 ++- > > > > hw/core/machine.c | 12 ++++++------ > > > > hw/i386/pc.c | 4 +++- > > > > hw/ppc/spapr.c | 13 ++++++++----- > > > > hw/s390x/s390-virtio-ccw.c | 1 + > > > > vl.c | 3 +-- > > > > 7 files changed, 23 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > > index 191a5b3..fa21758 100644 > > > > --- a/include/hw/boards.h > > > > +++ b/include/hw/boards.h > > > > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > > > * CPUArchId: > > > > * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > > > > I know this isn't really in scope for this patch, but is @arch_id here > > > supposed to have meaning defined by the target, or by the machine? > > > > > > If it's the machime, it could do with a rename - "arch" means target > > > to most people (thanks to Linux). > > > > > > If it's the target, it's kind of bogus, because it doesn't necessarily > > > have a clear meaning per target - get_arch_id in CPUClass has the same > > > problem, which is probably one reason it's basically only used by the > > > x86 code at present. > > > > > > e.g. for target/ppc, what do we use? There's the PIR, which is in the > > > CPU.. but only on some cpu models, not all. There will generally be > > > some kind of master PIC id, but there are different PIC models on > > > different boards. What goes in the devicetree? Well only some > > > machines use devicetree, and they might define the cpu reg > > > differently. > > > > > > Board designs will generally try to make some if not all of those > > > possible values equal for simplicity, but there's still no real way of > > > defining a sensible arch_id independent of machine / board. > > I'd say arch_id is machine specific so far, it was introduced when we > > didn't have CpuInstanceProperties and at that time we considered only > > vcpus (threads) and doesn't really apply to spapr cores. > > > > In general we could do away with arch_id and use CpuInstanceProperties > > instead, but arch_id also serves aux purpose, it allows machine to > > pre-calculate(cache) apic-id/mpidr values in one place and then they > > are/(could be) used by arch in-depended code to build acpi tables. > > So if we drop arch_id we would need to introduce a machine hook, > > which would translate CpuInstanceProperties into current arch_id. > > I think we need to do a better to job documenting where exactly > we expect arch_id to be used and how, so people know what it's > supposed to return. > > If the only place where it's useful now is ACPI code (is it?), > should we rename it to something like get_acpi_id()? It is also used in hw/s390x/sclp.c to fill out a control block, so acpi isn't the only user.
On Tue, Nov 07, 2017 at 04:04:04PM +0100, Cornelia Huck wrote: > On Mon, 6 Nov 2017 16:02:16 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote: > > > On Thu, 19 Oct 2017 17:31:51 +1100 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > > > > > For enabling early cpu to numa node configuration at runtime > > > > > qmp_query_hotpluggable_cpus() should provide a list of available > > > > > cpu slots at early stage, before machine_init() is called and > > > > > the 1st cpu is created, so that mgmt might be able to call it > > > > > and use output to set numa mapping. > > > > > Use MachineClass::possible_cpu_arch_ids() callback to set > > > > > cpu type info, along with the rest of possible cpu properties, > > > > > to let machine define which cpu type* will be used. > > > > > > > > > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > > > > > a respective descendant of CPUClass. > > > > > > > > > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > > > > > cpu_type so that possible_cpu_arch_ids() would know which > > > > > cpu_type to use during layout initialization. > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > --- > > > > > v2: > > > > > - fix NULL dereference caused by not initialized > > > > > MachineState::cpu_type at the time parse_numa_opts() > > > > > were called > > > > > --- > > > > > include/hw/boards.h | 2 ++ > > > > > hw/arm/virt.c | 3 ++- > > > > > hw/core/machine.c | 12 ++++++------ > > > > > hw/i386/pc.c | 4 +++- > > > > > hw/ppc/spapr.c | 13 ++++++++----- > > > > > hw/s390x/s390-virtio-ccw.c | 1 + > > > > > vl.c | 3 +-- > > > > > 7 files changed, 23 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > > > index 191a5b3..fa21758 100644 > > > > > --- a/include/hw/boards.h > > > > > +++ b/include/hw/boards.h > > > > > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > > > > * CPUArchId: > > > > > * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > > > > > > I know this isn't really in scope for this patch, but is @arch_id here > > > > supposed to have meaning defined by the target, or by the machine? > > > > > > > > If it's the machime, it could do with a rename - "arch" means target > > > > to most people (thanks to Linux). > > > > > > > > If it's the target, it's kind of bogus, because it doesn't necessarily > > > > have a clear meaning per target - get_arch_id in CPUClass has the same > > > > problem, which is probably one reason it's basically only used by the > > > > x86 code at present. > > > > > > > > e.g. for target/ppc, what do we use? There's the PIR, which is in the > > > > CPU.. but only on some cpu models, not all. There will generally be > > > > some kind of master PIC id, but there are different PIC models on > > > > different boards. What goes in the devicetree? Well only some > > > > machines use devicetree, and they might define the cpu reg > > > > differently. > > > > > > > > Board designs will generally try to make some if not all of those > > > > possible values equal for simplicity, but there's still no real way of > > > > defining a sensible arch_id independent of machine / board. > > > I'd say arch_id is machine specific so far, it was introduced when we > > > didn't have CpuInstanceProperties and at that time we considered only > > > vcpus (threads) and doesn't really apply to spapr cores. > > > > > > In general we could do away with arch_id and use CpuInstanceProperties > > > instead, but arch_id also serves aux purpose, it allows machine to > > > pre-calculate(cache) apic-id/mpidr values in one place and then they > > > are/(could be) used by arch in-depended code to build acpi tables. > > > So if we drop arch_id we would need to introduce a machine hook, > > > which would translate CpuInstanceProperties into current arch_id. > > > > I think we need to do a better to job documenting where exactly > > we expect arch_id to be used and how, so people know what it's > > supposed to return. > > > > If the only place where it's useful now is ACPI code (is it?), > > should we rename it to something like get_acpi_id()? > > It is also used in hw/s390x/sclp.c to fill out a control block, so acpi > isn't the only user. Yeah.. this is kind of bogus. The s390 use is in machine specific code, so it's basically just re-using the field for an unrelated usage to the x86/arm one (ACPI). If we can't assign a universal meaning to the field (even if the actual values are per-machine) - and I don't think we can - then I really don't think it belongs in CPUState. A machine hook which translates an ArchId to an acpi_id is the correct solution I believe. Or even an ACPIMachine interface (to be implemented by machines which do ACPI) which has a method to do this. Since both the assignment and use are in machine type specific code for s390, it can have its own field in the s390 specific cpu subclass. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, Nov 09, 2017 at 05:58:03PM +1100, David Gibson wrote: > On Tue, Nov 07, 2017 at 04:04:04PM +0100, Cornelia Huck wrote: > > On Mon, 6 Nov 2017 16:02:16 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote: > > > > On Thu, 19 Oct 2017 17:31:51 +1100 > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > > > > > > For enabling early cpu to numa node configuration at runtime > > > > > > qmp_query_hotpluggable_cpus() should provide a list of available > > > > > > cpu slots at early stage, before machine_init() is called and > > > > > > the 1st cpu is created, so that mgmt might be able to call it > > > > > > and use output to set numa mapping. > > > > > > Use MachineClass::possible_cpu_arch_ids() callback to set > > > > > > cpu type info, along with the rest of possible cpu properties, > > > > > > to let machine define which cpu type* will be used. > > > > > > > > > > > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > > > > > > a respective descendant of CPUClass. > > > > > > > > > > > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > > > > > > cpu_type so that possible_cpu_arch_ids() would know which > > > > > > cpu_type to use during layout initialization. > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > > > --- > > > > > > v2: > > > > > > - fix NULL dereference caused by not initialized > > > > > > MachineState::cpu_type at the time parse_numa_opts() > > > > > > were called > > > > > > --- > > > > > > include/hw/boards.h | 2 ++ > > > > > > hw/arm/virt.c | 3 ++- > > > > > > hw/core/machine.c | 12 ++++++------ > > > > > > hw/i386/pc.c | 4 +++- > > > > > > hw/ppc/spapr.c | 13 ++++++++----- > > > > > > hw/s390x/s390-virtio-ccw.c | 1 + > > > > > > vl.c | 3 +-- > > > > > > 7 files changed, 23 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > > > > index 191a5b3..fa21758 100644 > > > > > > --- a/include/hw/boards.h > > > > > > +++ b/include/hw/boards.h > > > > > > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > > > > > * CPUArchId: > > > > > > * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > > > > > > > > I know this isn't really in scope for this patch, but is @arch_id here > > > > > supposed to have meaning defined by the target, or by the machine? > > > > > > > > > > If it's the machime, it could do with a rename - "arch" means target > > > > > to most people (thanks to Linux). > > > > > > > > > > If it's the target, it's kind of bogus, because it doesn't necessarily > > > > > have a clear meaning per target - get_arch_id in CPUClass has the same > > > > > problem, which is probably one reason it's basically only used by the > > > > > x86 code at present. > > > > > > > > > > e.g. for target/ppc, what do we use? There's the PIR, which is in the > > > > > CPU.. but only on some cpu models, not all. There will generally be > > > > > some kind of master PIC id, but there are different PIC models on > > > > > different boards. What goes in the devicetree? Well only some > > > > > machines use devicetree, and they might define the cpu reg > > > > > differently. > > > > > > > > > > Board designs will generally try to make some if not all of those > > > > > possible values equal for simplicity, but there's still no real way of > > > > > defining a sensible arch_id independent of machine / board. > > > > I'd say arch_id is machine specific so far, it was introduced when we > > > > didn't have CpuInstanceProperties and at that time we considered only > > > > vcpus (threads) and doesn't really apply to spapr cores. > > > > > > > > In general we could do away with arch_id and use CpuInstanceProperties > > > > instead, but arch_id also serves aux purpose, it allows machine to > > > > pre-calculate(cache) apic-id/mpidr values in one place and then they > > > > are/(could be) used by arch in-depended code to build acpi tables. > > > > So if we drop arch_id we would need to introduce a machine hook, > > > > which would translate CpuInstanceProperties into current arch_id. > > > > > > I think we need to do a better to job documenting where exactly > > > we expect arch_id to be used and how, so people know what it's > > > supposed to return. > > > > > > If the only place where it's useful now is ACPI code (is it?), > > > should we rename it to something like get_acpi_id()? > > > > It is also used in hw/s390x/sclp.c to fill out a control block, so acpi > > isn't the only user. > > Yeah.. this is kind of bogus. The s390 use is in machine specific > code, so it's basically just re-using the field for an unrelated usage > to the x86/arm one (ACPI). > > If we can't assign a universal meaning to the field (even if the > actual values are per-machine) - and I don't think we can - then I > really don't think it belongs in CPUState. A machine hook which > translates an ArchId to an acpi_id is the correct solution I believe. > Or even an ACPIMachine interface (to be implemented by machines which > do ACPI) which has a method to do this. > > Since both the assignment and use are in machine type specific code > for s390, it can have its own field in the s390 specific cpu subclass. > I agree. This might require duplicating cpu_by_arch_id() and cpu_exists() into machine-specific code, but this doesn't sound too bad: there's only one user of cpu_by_arch_id() (that's x86-specific code living inside monitor.c), and one user of cpu_exists() (that's s390-specific code). (Maybe those users could be rewritten to use MachineState::possible_cpus, like pc_find_cpu_slot()). -- Eduardo
On Thu, 9 Nov 2017 18:02:35 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Nov 09, 2017 at 05:58:03PM +1100, David Gibson wrote: > > On Tue, Nov 07, 2017 at 04:04:04PM +0100, Cornelia Huck wrote: > > > On Mon, 6 Nov 2017 16:02:16 -0200 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote: > > > > > On Thu, 19 Oct 2017 17:31:51 +1100 > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > > > > > > > For enabling early cpu to numa node configuration at runtime > > > > > > > qmp_query_hotpluggable_cpus() should provide a list of available > > > > > > > cpu slots at early stage, before machine_init() is called and > > > > > > > the 1st cpu is created, so that mgmt might be able to call it > > > > > > > and use output to set numa mapping. > > > > > > > Use MachineClass::possible_cpu_arch_ids() callback to set > > > > > > > cpu type info, along with the rest of possible cpu properties, > > > > > > > to let machine define which cpu type* will be used. > > > > > > > > > > > > > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > > > > > > > a respective descendant of CPUClass. > > > > > > > > > > > > > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > > > > > > > cpu_type so that possible_cpu_arch_ids() would know which > > > > > > > cpu_type to use during layout initialization. > > > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > > > > > --- > > > > > > > v2: > > > > > > > - fix NULL dereference caused by not initialized > > > > > > > MachineState::cpu_type at the time parse_numa_opts() > > > > > > > were called > > > > > > > --- > > > > > > > include/hw/boards.h | 2 ++ > > > > > > > hw/arm/virt.c | 3 ++- > > > > > > > hw/core/machine.c | 12 ++++++------ > > > > > > > hw/i386/pc.c | 4 +++- > > > > > > > hw/ppc/spapr.c | 13 ++++++++----- > > > > > > > hw/s390x/s390-virtio-ccw.c | 1 + > > > > > > > vl.c | 3 +-- > > > > > > > 7 files changed, 23 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > > > > > index 191a5b3..fa21758 100644 > > > > > > > --- a/include/hw/boards.h > > > > > > > +++ b/include/hw/boards.h > > > > > > > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > > > > > > * CPUArchId: > > > > > > > * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > > > > > > > > > > I know this isn't really in scope for this patch, but is @arch_id here > > > > > > supposed to have meaning defined by the target, or by the machine? > > > > > > > > > > > > If it's the machime, it could do with a rename - "arch" means target > > > > > > to most people (thanks to Linux). > > > > > > > > > > > > If it's the target, it's kind of bogus, because it doesn't necessarily > > > > > > have a clear meaning per target - get_arch_id in CPUClass has the same > > > > > > problem, which is probably one reason it's basically only used by the > > > > > > x86 code at present. > > > > > > > > > > > > e.g. for target/ppc, what do we use? There's the PIR, which is in the > > > > > > CPU.. but only on some cpu models, not all. There will generally be > > > > > > some kind of master PIC id, but there are different PIC models on > > > > > > different boards. What goes in the devicetree? Well only some > > > > > > machines use devicetree, and they might define the cpu reg > > > > > > differently. > > > > > > > > > > > > Board designs will generally try to make some if not all of those > > > > > > possible values equal for simplicity, but there's still no real way of > > > > > > defining a sensible arch_id independent of machine / board. > > > > > I'd say arch_id is machine specific so far, it was introduced when we > > > > > didn't have CpuInstanceProperties and at that time we considered only > > > > > vcpus (threads) and doesn't really apply to spapr cores. > > > > > > > > > > In general we could do away with arch_id and use CpuInstanceProperties > > > > > instead, but arch_id also serves aux purpose, it allows machine to > > > > > pre-calculate(cache) apic-id/mpidr values in one place and then they > > > > > are/(could be) used by arch in-depended code to build acpi tables. > > > > > So if we drop arch_id we would need to introduce a machine hook, > > > > > which would translate CpuInstanceProperties into current arch_id. > > > > > > > > I think we need to do a better to job documenting where exactly > > > > we expect arch_id to be used and how, so people know what it's > > > > supposed to return. > > > > > > > > If the only place where it's useful now is ACPI code (is it?), > > > > should we rename it to something like get_acpi_id()? > > > > > > It is also used in hw/s390x/sclp.c to fill out a control block, so acpi > > > isn't the only user. > > > > Yeah.. this is kind of bogus. The s390 use is in machine specific > > code, so it's basically just re-using the field for an unrelated usage > > to the x86/arm one (ACPI). > > > > If we can't assign a universal meaning to the field (even if the > > actual values are per-machine) - and I don't think we can - then I > > really don't think it belongs in CPUState. A machine hook which > > translates an ArchId to an acpi_id is the correct solution I believe. > > Or even an ACPIMachine interface (to be implemented by machines which > > do ACPI) which has a method to do this. > > > > Since both the assignment and use are in machine type specific code > > for s390, it can have its own field in the s390 specific cpu subclass. > > > > I agree. This might require duplicating cpu_by_arch_id() and > cpu_exists() into machine-specific code, but this doesn't sound > too bad: there's only one user of cpu_by_arch_id() (that's > x86-specific code living inside monitor.c), and one user of > cpu_exists() (that's s390-specific code). > > (Maybe those users could be rewritten to use > MachineState::possible_cpus, like pc_find_cpu_slot()). David (H), does that sound workable to you?
On 10.11.2017 11:14, Cornelia Huck wrote:
> On Thu, 9 Nov 2017 18:02:35 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Thu, Nov 09, 2017 at 05:58:03PM +1100, David Gibson wrote:
>>> On Tue, Nov 07, 2017 at 04:04:04PM +0100, Cornelia Huck wrote:
>>>> On Mon, 6 Nov 2017 16:02:16 -0200
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>
>>>>> On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote:
>>>>>> On Thu, 19 Oct 2017 17:31:51 +1100
>>>>>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>>>
>>>>>>> On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote:
>>>>>>>> For enabling early cpu to numa node configuration at runtime
>>>>>>>> qmp_query_hotpluggable_cpus() should provide a list of available
>>>>>>>> cpu slots at early stage, before machine_init() is called and
>>>>>>>> the 1st cpu is created, so that mgmt might be able to call it
>>>>>>>> and use output to set numa mapping.
>>>>>>>> Use MachineClass::possible_cpu_arch_ids() callback to set
>>>>>>>> cpu type info, along with the rest of possible cpu properties,
>>>>>>>> to let machine define which cpu type* will be used.
>>>>>>>>
>>>>>>>> * for SPAPR it will be a spapr core type and for ARM/s390x/x86
>>>>>>>> a respective descendant of CPUClass.
>>>>>>>>
>>>>>>>> Move parse_numa_opts() in vl.c after cpu_model is parsed into
>>>>>>>> cpu_type so that possible_cpu_arch_ids() would know which
>>>>>>>> cpu_type to use during layout initialization.
>>>>>>>>
>>>>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>>>>
>>>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>> - fix NULL dereference caused by not initialized
>>>>>>>> MachineState::cpu_type at the time parse_numa_opts()
>>>>>>>> were called
>>>>>>>> ---
>>>>>>>> include/hw/boards.h | 2 ++
>>>>>>>> hw/arm/virt.c | 3 ++-
>>>>>>>> hw/core/machine.c | 12 ++++++------
>>>>>>>> hw/i386/pc.c | 4 +++-
>>>>>>>> hw/ppc/spapr.c | 13 ++++++++-----
>>>>>>>> hw/s390x/s390-virtio-ccw.c | 1 +
>>>>>>>> vl.c | 3 +--
>>>>>>>> 7 files changed, 23 insertions(+), 15 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>>>>>> index 191a5b3..fa21758 100644
>>>>>>>> --- a/include/hw/boards.h
>>>>>>>> +++ b/include/hw/boards.h
>>>>>>>> @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>>>>>>> * CPUArchId:
>>>>>>>> * @arch_id - architecture-dependent CPU ID of present or possible CPU
>>>>>>>
>>>>>>> I know this isn't really in scope for this patch, but is @arch_id here
>>>>>>> supposed to have meaning defined by the target, or by the machine?
>>>>>>>
>>>>>>> If it's the machime, it could do with a rename - "arch" means target
>>>>>>> to most people (thanks to Linux).
>>>>>>>
>>>>>>> If it's the target, it's kind of bogus, because it doesn't necessarily
>>>>>>> have a clear meaning per target - get_arch_id in CPUClass has the same
>>>>>>> problem, which is probably one reason it's basically only used by the
>>>>>>> x86 code at present.
>>>>>>>
>>>>>>> e.g. for target/ppc, what do we use? There's the PIR, which is in the
>>>>>>> CPU.. but only on some cpu models, not all. There will generally be
>>>>>>> some kind of master PIC id, but there are different PIC models on
>>>>>>> different boards. What goes in the devicetree? Well only some
>>>>>>> machines use devicetree, and they might define the cpu reg
>>>>>>> differently.
>>>>>>>
>>>>>>> Board designs will generally try to make some if not all of those
>>>>>>> possible values equal for simplicity, but there's still no real way of
>>>>>>> defining a sensible arch_id independent of machine / board.
>>>>>> I'd say arch_id is machine specific so far, it was introduced when we
>>>>>> didn't have CpuInstanceProperties and at that time we considered only
>>>>>> vcpus (threads) and doesn't really apply to spapr cores.
>>>>>>
>>>>>> In general we could do away with arch_id and use CpuInstanceProperties
>>>>>> instead, but arch_id also serves aux purpose, it allows machine to
>>>>>> pre-calculate(cache) apic-id/mpidr values in one place and then they
>>>>>> are/(could be) used by arch in-depended code to build acpi tables.
>>>>>> So if we drop arch_id we would need to introduce a machine hook,
>>>>>> which would translate CpuInstanceProperties into current arch_id.
>>>>>
>>>>> I think we need to do a better to job documenting where exactly
>>>>> we expect arch_id to be used and how, so people know what it's
>>>>> supposed to return.
>>>>>
>>>>> If the only place where it's useful now is ACPI code (is it?),
>>>>> should we rename it to something like get_acpi_id()?
>>>>
>>>> It is also used in hw/s390x/sclp.c to fill out a control block, so acpi
>>>> isn't the only user.
>>>
>>> Yeah.. this is kind of bogus. The s390 use is in machine specific
>>> code, so it's basically just re-using the field for an unrelated usage
>>> to the x86/arm one (ACPI).
as index == arch_id on s390x, that code could easily be changed to
something like:
@@ -45,7 +45,7 @@ static void prepare_cpu_entries(SCLPDevice *sclp,
CPUEntry *entry, int *count)
if (!ms->possible_cpus->cpus[i].cpu) {
continue;
}
- entry[*count].address = ms->possible_cpus->cpus[i].arch_id;
+ entry[*count].address = i;
entry[*count].type = 0;
memcpy(entry[*count].features, features, sizeof(features));
(*count)++;
arch_id just looked like the right thing to use (documentation issue
mentioned above)
>>>
>>> If we can't assign a universal meaning to the field (even if the
>>> actual values are per-machine) - and I don't think we can - then I
>>> really don't think it belongs in CPUState. A machine hook which
>>> translates an ArchId to an acpi_id is the correct solution I believe.
>>> Or even an ACPIMachine interface (to be implemented by machines which
>>> do ACPI) which has a method to do this.
>>>
>>> Since both the assignment and use are in machine type specific code
>>> for s390, it can have its own field in the s390 specific cpu subclass.
s390x doesn't need arch_id at all.
cs->cpu_index can be used.
>>>
>>
>> I agree. This might require duplicating cpu_by_arch_id() and
>> cpu_exists() into machine-specific code, but this doesn't sound
>> too bad: there's only one user of cpu_by_arch_id() (that's
>> x86-specific code living inside monitor.c), and one user of
>> cpu_exists() (that's s390-specific code).>>
>> (Maybe those users could be rewritten to use
>> MachineState::possible_cpus, like pc_find_cpu_slot()).
--
Thanks,
David / dhildenb
On Fri, Nov 10, 2017 at 01:34:42PM +0100, David Hildenbrand wrote:
> On 10.11.2017 11:14, Cornelia Huck wrote:
> > On Thu, 9 Nov 2017 18:02:35 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> >> On Thu, Nov 09, 2017 at 05:58:03PM +1100, David Gibson wrote:
> >>> On Tue, Nov 07, 2017 at 04:04:04PM +0100, Cornelia Huck wrote:
> >>>> On Mon, 6 Nov 2017 16:02:16 -0200
> >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>
> >>>>> On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote:
> >>>>>> On Thu, 19 Oct 2017 17:31:51 +1100
> >>>>>> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>>>>
> >>>>>>> On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote:
> >>>>>>>> For enabling early cpu to numa node configuration at runtime
> >>>>>>>> qmp_query_hotpluggable_cpus() should provide a list of available
> >>>>>>>> cpu slots at early stage, before machine_init() is called and
> >>>>>>>> the 1st cpu is created, so that mgmt might be able to call it
> >>>>>>>> and use output to set numa mapping.
> >>>>>>>> Use MachineClass::possible_cpu_arch_ids() callback to set
> >>>>>>>> cpu type info, along with the rest of possible cpu properties,
> >>>>>>>> to let machine define which cpu type* will be used.
> >>>>>>>>
> >>>>>>>> * for SPAPR it will be a spapr core type and for ARM/s390x/x86
> >>>>>>>> a respective descendant of CPUClass.
> >>>>>>>>
> >>>>>>>> Move parse_numa_opts() in vl.c after cpu_model is parsed into
> >>>>>>>> cpu_type so that possible_cpu_arch_ids() would know which
> >>>>>>>> cpu_type to use during layout initialization.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>>>>>>
> >>>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >>>>>>>
> >>>>>>>> ---
> >>>>>>>> v2:
> >>>>>>>> - fix NULL dereference caused by not initialized
> >>>>>>>> MachineState::cpu_type at the time parse_numa_opts()
> >>>>>>>> were called
> >>>>>>>> ---
> >>>>>>>> include/hw/boards.h | 2 ++
> >>>>>>>> hw/arm/virt.c | 3 ++-
> >>>>>>>> hw/core/machine.c | 12 ++++++------
> >>>>>>>> hw/i386/pc.c | 4 +++-
> >>>>>>>> hw/ppc/spapr.c | 13 ++++++++-----
> >>>>>>>> hw/s390x/s390-virtio-ccw.c | 1 +
> >>>>>>>> vl.c | 3 +--
> >>>>>>>> 7 files changed, 23 insertions(+), 15 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >>>>>>>> index 191a5b3..fa21758 100644
> >>>>>>>> --- a/include/hw/boards.h
> >>>>>>>> +++ b/include/hw/boards.h
> >>>>>>>> @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
> >>>>>>>> * CPUArchId:
> >>>>>>>> * @arch_id - architecture-dependent CPU ID of present or possible CPU
> >>>>>>>
> >>>>>>> I know this isn't really in scope for this patch, but is @arch_id here
> >>>>>>> supposed to have meaning defined by the target, or by the machine?
> >>>>>>>
> >>>>>>> If it's the machime, it could do with a rename - "arch" means target
> >>>>>>> to most people (thanks to Linux).
> >>>>>>>
> >>>>>>> If it's the target, it's kind of bogus, because it doesn't necessarily
> >>>>>>> have a clear meaning per target - get_arch_id in CPUClass has the same
> >>>>>>> problem, which is probably one reason it's basically only used by the
> >>>>>>> x86 code at present.
> >>>>>>>
> >>>>>>> e.g. for target/ppc, what do we use? There's the PIR, which is in the
> >>>>>>> CPU.. but only on some cpu models, not all. There will generally be
> >>>>>>> some kind of master PIC id, but there are different PIC models on
> >>>>>>> different boards. What goes in the devicetree? Well only some
> >>>>>>> machines use devicetree, and they might define the cpu reg
> >>>>>>> differently.
> >>>>>>>
> >>>>>>> Board designs will generally try to make some if not all of those
> >>>>>>> possible values equal for simplicity, but there's still no real way of
> >>>>>>> defining a sensible arch_id independent of machine / board.
> >>>>>> I'd say arch_id is machine specific so far, it was introduced when we
> >>>>>> didn't have CpuInstanceProperties and at that time we considered only
> >>>>>> vcpus (threads) and doesn't really apply to spapr cores.
> >>>>>>
> >>>>>> In general we could do away with arch_id and use CpuInstanceProperties
> >>>>>> instead, but arch_id also serves aux purpose, it allows machine to
> >>>>>> pre-calculate(cache) apic-id/mpidr values in one place and then they
> >>>>>> are/(could be) used by arch in-depended code to build acpi tables.
> >>>>>> So if we drop arch_id we would need to introduce a machine hook,
> >>>>>> which would translate CpuInstanceProperties into current arch_id.
> >>>>>
> >>>>> I think we need to do a better to job documenting where exactly
> >>>>> we expect arch_id to be used and how, so people know what it's
> >>>>> supposed to return.
> >>>>>
> >>>>> If the only place where it's useful now is ACPI code (is it?),
> >>>>> should we rename it to something like get_acpi_id()?
> >>>>
> >>>> It is also used in hw/s390x/sclp.c to fill out a control block, so acpi
> >>>> isn't the only user.
> >>>
> >>> Yeah.. this is kind of bogus. The s390 use is in machine specific
> >>> code, so it's basically just re-using the field for an unrelated usage
> >>> to the x86/arm one (ACPI).
>
> as index == arch_id on s390x, that code could easily be changed to
> something like:
>
> @@ -45,7 +45,7 @@ static void prepare_cpu_entries(SCLPDevice *sclp,
> CPUEntry *entry, int *count)
> if (!ms->possible_cpus->cpus[i].cpu) {
> continue;
> }
> - entry[*count].address = ms->possible_cpus->cpus[i].arch_id;
> + entry[*count].address = i;
What about decoupling it from the array index, by using:
entry[*count].address = ms->possible_cpus->cpus[i].props.core_id;
or:
entry[*count].address = S390_CPU(ms->possible_cpus->cpus[i].cpu)->core_id;
?
> entry[*count].type = 0;
> memcpy(entry[*count].features, features, sizeof(features));
> (*count)++;
>
> arch_id just looked like the right thing to use (documentation issue
> mentioned above)
>
>
> >>>
> >>> If we can't assign a universal meaning to the field (even if the
> >>> actual values are per-machine) - and I don't think we can - then I
> >>> really don't think it belongs in CPUState. A machine hook which
> >>> translates an ArchId to an acpi_id is the correct solution I believe.
> >>> Or even an ACPIMachine interface (to be implemented by machines which
> >>> do ACPI) which has a method to do this.
> >>>
> >>> Since both the assignment and use are in machine type specific code
> >>> for s390, it can have its own field in the s390 specific cpu subclass.
>
> s390x doesn't need arch_id at all.
>
> cs->cpu_index can be used.
What about the cpu_exists() check in s390_cpu_realizefn()? It
could be moved to a new s390_machine_device_pre_plug() method
that just checks ms->possible_cpus->cpus[cpu->env.core_id].cpu.
>
> >>>
> >>
> >> I agree. This might require duplicating cpu_by_arch_id() and
> >> cpu_exists() into machine-specific code, but this doesn't sound
> >> too bad: there's only one user of cpu_by_arch_id() (that's
> >> x86-specific code living inside monitor.c), and one user of
> >> cpu_exists() (that's s390-specific code).>>
> >> (Maybe those users could be rewritten to use
> >> MachineState::possible_cpus, like pc_find_cpu_slot()).
>
>
> --
>
> Thanks,
>
> David / dhildenb
--
Eduardo
On 10.11.2017 13:58, Eduardo Habkost wrote:
> On Fri, Nov 10, 2017 at 01:34:42PM +0100, David Hildenbrand wrote:
>> On 10.11.2017 11:14, Cornelia Huck wrote:
>>> On Thu, 9 Nov 2017 18:02:35 -0200
>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>
>>>> On Thu, Nov 09, 2017 at 05:58:03PM +1100, David Gibson wrote:
>>>>> On Tue, Nov 07, 2017 at 04:04:04PM +0100, Cornelia Huck wrote:
>>>>>> On Mon, 6 Nov 2017 16:02:16 -0200
>>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>>
>>>>>>> On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote:
>>>>>>>> On Thu, 19 Oct 2017 17:31:51 +1100
>>>>>>>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>>>>>
>>>>>>>>> On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote:
>>>>>>>>>> For enabling early cpu to numa node configuration at runtime
>>>>>>>>>> qmp_query_hotpluggable_cpus() should provide a list of available
>>>>>>>>>> cpu slots at early stage, before machine_init() is called and
>>>>>>>>>> the 1st cpu is created, so that mgmt might be able to call it
>>>>>>>>>> and use output to set numa mapping.
>>>>>>>>>> Use MachineClass::possible_cpu_arch_ids() callback to set
>>>>>>>>>> cpu type info, along with the rest of possible cpu properties,
>>>>>>>>>> to let machine define which cpu type* will be used.
>>>>>>>>>>
>>>>>>>>>> * for SPAPR it will be a spapr core type and for ARM/s390x/x86
>>>>>>>>>> a respective descendant of CPUClass.
>>>>>>>>>>
>>>>>>>>>> Move parse_numa_opts() in vl.c after cpu_model is parsed into
>>>>>>>>>> cpu_type so that possible_cpu_arch_ids() would know which
>>>>>>>>>> cpu_type to use during layout initialization.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>>>>>>
>>>>>>>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> v2:
>>>>>>>>>> - fix NULL dereference caused by not initialized
>>>>>>>>>> MachineState::cpu_type at the time parse_numa_opts()
>>>>>>>>>> were called
>>>>>>>>>> ---
>>>>>>>>>> include/hw/boards.h | 2 ++
>>>>>>>>>> hw/arm/virt.c | 3 ++-
>>>>>>>>>> hw/core/machine.c | 12 ++++++------
>>>>>>>>>> hw/i386/pc.c | 4 +++-
>>>>>>>>>> hw/ppc/spapr.c | 13 ++++++++-----
>>>>>>>>>> hw/s390x/s390-virtio-ccw.c | 1 +
>>>>>>>>>> vl.c | 3 +--
>>>>>>>>>> 7 files changed, 23 insertions(+), 15 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>>>>>>>> index 191a5b3..fa21758 100644
>>>>>>>>>> --- a/include/hw/boards.h
>>>>>>>>>> +++ b/include/hw/boards.h
>>>>>>>>>> @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>>>>>>>>> * CPUArchId:
>>>>>>>>>> * @arch_id - architecture-dependent CPU ID of present or possible CPU
>>>>>>>>>
>>>>>>>>> I know this isn't really in scope for this patch, but is @arch_id here
>>>>>>>>> supposed to have meaning defined by the target, or by the machine?
>>>>>>>>>
>>>>>>>>> If it's the machime, it could do with a rename - "arch" means target
>>>>>>>>> to most people (thanks to Linux).
>>>>>>>>>
>>>>>>>>> If it's the target, it's kind of bogus, because it doesn't necessarily
>>>>>>>>> have a clear meaning per target - get_arch_id in CPUClass has the same
>>>>>>>>> problem, which is probably one reason it's basically only used by the
>>>>>>>>> x86 code at present.
>>>>>>>>>
>>>>>>>>> e.g. for target/ppc, what do we use? There's the PIR, which is in the
>>>>>>>>> CPU.. but only on some cpu models, not all. There will generally be
>>>>>>>>> some kind of master PIC id, but there are different PIC models on
>>>>>>>>> different boards. What goes in the devicetree? Well only some
>>>>>>>>> machines use devicetree, and they might define the cpu reg
>>>>>>>>> differently.
>>>>>>>>>
>>>>>>>>> Board designs will generally try to make some if not all of those
>>>>>>>>> possible values equal for simplicity, but there's still no real way of
>>>>>>>>> defining a sensible arch_id independent of machine / board.
>>>>>>>> I'd say arch_id is machine specific so far, it was introduced when we
>>>>>>>> didn't have CpuInstanceProperties and at that time we considered only
>>>>>>>> vcpus (threads) and doesn't really apply to spapr cores.
>>>>>>>>
>>>>>>>> In general we could do away with arch_id and use CpuInstanceProperties
>>>>>>>> instead, but arch_id also serves aux purpose, it allows machine to
>>>>>>>> pre-calculate(cache) apic-id/mpidr values in one place and then they
>>>>>>>> are/(could be) used by arch in-depended code to build acpi tables.
>>>>>>>> So if we drop arch_id we would need to introduce a machine hook,
>>>>>>>> which would translate CpuInstanceProperties into current arch_id.
>>>>>>>
>>>>>>> I think we need to do a better to job documenting where exactly
>>>>>>> we expect arch_id to be used and how, so people know what it's
>>>>>>> supposed to return.
>>>>>>>
>>>>>>> If the only place where it's useful now is ACPI code (is it?),
>>>>>>> should we rename it to something like get_acpi_id()?
>>>>>>
>>>>>> It is also used in hw/s390x/sclp.c to fill out a control block, so acpi
>>>>>> isn't the only user.
>>>>>
>>>>> Yeah.. this is kind of bogus. The s390 use is in machine specific
>>>>> code, so it's basically just re-using the field for an unrelated usage
>>>>> to the x86/arm one (ACPI).
>>
>> as index == arch_id on s390x, that code could easily be changed to
>> something like:
>>
>> @@ -45,7 +45,7 @@ static void prepare_cpu_entries(SCLPDevice *sclp,
>> CPUEntry *entry, int *count)
>> if (!ms->possible_cpus->cpus[i].cpu) {
>> continue;
>> }
>> - entry[*count].address = ms->possible_cpus->cpus[i].arch_id;
>> + entry[*count].address = i;
>
> What about decoupling it from the array index, by using:
> entry[*count].address = ms->possible_cpus->cpus[i].props.core_id;
> or:
> entry[*count].address = S390_CPU(ms->possible_cpus->cpus[i].cpu)->core_id;
> ?
Yes, we could do that, but doesn't really matter for now. I would ACK
either :)
>
>
>> entry[*count].type = 0;
>> memcpy(entry[*count].features, features, sizeof(features));
>> (*count)++;
>>
>> arch_id just looked like the right thing to use (documentation issue
>> mentioned above)
>>
>>
>>>>>
>>>>> If we can't assign a universal meaning to the field (even if the
>>>>> actual values are per-machine) - and I don't think we can - then I
>>>>> really don't think it belongs in CPUState. A machine hook which
>>>>> translates an ArchId to an acpi_id is the correct solution I believe.
>>>>> Or even an ACPIMachine interface (to be implemented by machines which
>>>>> do ACPI) which has a method to do this.
>>>>>
>>>>> Since both the assignment and use are in machine type specific code
>>>>> for s390, it can have its own field in the s390 specific cpu subclass.
>>
>> s390x doesn't need arch_id at all.
>>
>> cs->cpu_index can be used.
>
> What about the cpu_exists() check in s390_cpu_realizefn()? It
> could be moved to a new s390_machine_device_pre_plug() method
> that just checks ms->possible_cpus->cpus[cpu->env.core_id].cpu.
>
I always hated that part (cpu_exists()). We can completely drop
cpu_exists() on s390x and simply add that check for pre plug as you
said, fine with me!
--
Thanks,
David / dhildenb
On Thu, 9 Nov 2017 18:02:35 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Nov 09, 2017 at 05:58:03PM +1100, David Gibson wrote: > > On Tue, Nov 07, 2017 at 04:04:04PM +0100, Cornelia Huck wrote: > > > On Mon, 6 Nov 2017 16:02:16 -0200 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote: > > > > > On Thu, 19 Oct 2017 17:31:51 +1100 > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > > > > > > > For enabling early cpu to numa node configuration at runtime > > > > > > > qmp_query_hotpluggable_cpus() should provide a list of available > > > > > > > cpu slots at early stage, before machine_init() is called and > > > > > > > the 1st cpu is created, so that mgmt might be able to call it > > > > > > > and use output to set numa mapping. > > > > > > > Use MachineClass::possible_cpu_arch_ids() callback to set > > > > > > > cpu type info, along with the rest of possible cpu properties, > > > > > > > to let machine define which cpu type* will be used. > > > > > > > > > > > > > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > > > > > > > a respective descendant of CPUClass. > > > > > > > > > > > > > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > > > > > > > cpu_type so that possible_cpu_arch_ids() would know which > > > > > > > cpu_type to use during layout initialization. > > > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > > > > > --- > > > > > > > v2: > > > > > > > - fix NULL dereference caused by not initialized > > > > > > > MachineState::cpu_type at the time parse_numa_opts() > > > > > > > were called > > > > > > > --- > > > > > > > include/hw/boards.h | 2 ++ > > > > > > > hw/arm/virt.c | 3 ++- > > > > > > > hw/core/machine.c | 12 ++++++------ > > > > > > > hw/i386/pc.c | 4 +++- > > > > > > > hw/ppc/spapr.c | 13 ++++++++----- > > > > > > > hw/s390x/s390-virtio-ccw.c | 1 + > > > > > > > vl.c | 3 +-- > > > > > > > 7 files changed, 23 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > > > > > index 191a5b3..fa21758 100644 > > > > > > > --- a/include/hw/boards.h > > > > > > > +++ b/include/hw/boards.h > > > > > > > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > > > > > > * CPUArchId: > > > > > > > * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > > > > > > > > > > I know this isn't really in scope for this patch, but is @arch_id here > > > > > > supposed to have meaning defined by the target, or by the machine? > > > > > > > > > > > > If it's the machime, it could do with a rename - "arch" means target > > > > > > to most people (thanks to Linux). > > > > > > > > > > > > If it's the target, it's kind of bogus, because it doesn't necessarily > > > > > > have a clear meaning per target - get_arch_id in CPUClass has the same > > > > > > problem, which is probably one reason it's basically only used by the > > > > > > x86 code at present. > > > > > > > > > > > > e.g. for target/ppc, what do we use? There's the PIR, which is in the > > > > > > CPU.. but only on some cpu models, not all. There will generally be > > > > > > some kind of master PIC id, but there are different PIC models on > > > > > > different boards. What goes in the devicetree? Well only some > > > > > > machines use devicetree, and they might define the cpu reg > > > > > > differently. > > > > > > > > > > > > Board designs will generally try to make some if not all of those > > > > > > possible values equal for simplicity, but there's still no real way of > > > > > > defining a sensible arch_id independent of machine / board. > > > > > I'd say arch_id is machine specific so far, it was introduced when we > > > > > didn't have CpuInstanceProperties and at that time we considered only > > > > > vcpus (threads) and doesn't really apply to spapr cores. > > > > > > > > > > In general we could do away with arch_id and use CpuInstanceProperties > > > > > instead, but arch_id also serves aux purpose, it allows machine to > > > > > pre-calculate(cache) apic-id/mpidr values in one place and then they > > > > > are/(could be) used by arch in-depended code to build acpi tables. > > > > > So if we drop arch_id we would need to introduce a machine hook, > > > > > which would translate CpuInstanceProperties into current arch_id. > > > > > > > > I think we need to do a better to job documenting where exactly > > > > we expect arch_id to be used and how, so people know what it's > > > > supposed to return. > > > > > > > > If the only place where it's useful now is ACPI code (is it?), > > > > should we rename it to something like get_acpi_id()? > > > > > > It is also used in hw/s390x/sclp.c to fill out a control block, so acpi > > > isn't the only user. > > > > Yeah.. this is kind of bogus. The s390 use is in machine specific > > code, so it's basically just re-using the field for an unrelated usage > > to the x86/arm one (ACPI). > > > > If we can't assign a universal meaning to the field (even if the > > actual values are per-machine) - and I don't think we can - then I > > really don't think it belongs in CPUState. A machine hook which > > translates an ArchId to an acpi_id is the correct solution I believe. > > Or even an ACPIMachine interface (to be implemented by machines which > > do ACPI) which has a method to do this. > > > > Since both the assignment and use are in machine type specific code > > for s390, it can have its own field in the s390 specific cpu subclass. > > > > I agree. This might require duplicating cpu_by_arch_id() and > cpu_exists() into machine-specific code, but this doesn't sound > too bad: there's only one user of cpu_by_arch_id() (that's > x86-specific code living inside monitor.c), and one user of > cpu_exists() (that's s390-specific code). > > (Maybe those users could be rewritten to use > MachineState::possible_cpus, like pc_find_cpu_slot()). I've added getting rid of arch_id from generic structure on my TODO list.
On Mon, Nov 06, 2017 at 04:02:16PM -0200, Eduardo Habkost wrote: > On Tue, Oct 31, 2017 at 03:01:14PM +0100, Igor Mammedov wrote: > > On Thu, 19 Oct 2017 17:31:51 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Wed, Oct 18, 2017 at 01:12:12PM +0200, Igor Mammedov wrote: > > > > For enabling early cpu to numa node configuration at runtime > > > > qmp_query_hotpluggable_cpus() should provide a list of available > > > > cpu slots at early stage, before machine_init() is called and > > > > the 1st cpu is created, so that mgmt might be able to call it > > > > and use output to set numa mapping. > > > > Use MachineClass::possible_cpu_arch_ids() callback to set > > > > cpu type info, along with the rest of possible cpu properties, > > > > to let machine define which cpu type* will be used. > > > > > > > > * for SPAPR it will be a spapr core type and for ARM/s390x/x86 > > > > a respective descendant of CPUClass. > > > > > > > > Move parse_numa_opts() in vl.c after cpu_model is parsed into > > > > cpu_type so that possible_cpu_arch_ids() would know which > > > > cpu_type to use during layout initialization. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > --- > > > > v2: > > > > - fix NULL dereference caused by not initialized > > > > MachineState::cpu_type at the time parse_numa_opts() > > > > were called > > > > --- > > > > include/hw/boards.h | 2 ++ > > > > hw/arm/virt.c | 3 ++- > > > > hw/core/machine.c | 12 ++++++------ > > > > hw/i386/pc.c | 4 +++- > > > > hw/ppc/spapr.c | 13 ++++++++----- > > > > hw/s390x/s390-virtio-ccw.c | 1 + > > > > vl.c | 3 +-- > > > > 7 files changed, 23 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > > index 191a5b3..fa21758 100644 > > > > --- a/include/hw/boards.h > > > > +++ b/include/hw/boards.h > > > > @@ -80,6 +80,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > > > > * CPUArchId: > > > > * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > > > > I know this isn't really in scope for this patch, but is @arch_id here > > > supposed to have meaning defined by the target, or by the machine? > > > > > > If it's the machime, it could do with a rename - "arch" means target > > > to most people (thanks to Linux). > > > > > > If it's the target, it's kind of bogus, because it doesn't necessarily > > > have a clear meaning per target - get_arch_id in CPUClass has the same > > > problem, which is probably one reason it's basically only used by the > > > x86 code at present. > > > > > > e.g. for target/ppc, what do we use? There's the PIR, which is in the > > > CPU.. but only on some cpu models, not all. There will generally be > > > some kind of master PIC id, but there are different PIC models on > > > different boards. What goes in the devicetree? Well only some > > > machines use devicetree, and they might define the cpu reg > > > differently. > > > > > > Board designs will generally try to make some if not all of those > > > possible values equal for simplicity, but there's still no real way of > > > defining a sensible arch_id independent of machine / board. > > I'd say arch_id is machine specific so far, it was introduced when we > > didn't have CpuInstanceProperties and at that time we considered only > > vcpus (threads) and doesn't really apply to spapr cores. > > > > In general we could do away with arch_id and use CpuInstanceProperties > > instead, but arch_id also serves aux purpose, it allows machine to > > pre-calculate(cache) apic-id/mpidr values in one place and then they > > are/(could be) used by arch in-depended code to build acpi tables. > > So if we drop arch_id we would need to introduce a machine hook, > > which would translate CpuInstanceProperties into current arch_id. > > I think we need to do a better to job documenting where exactly > we expect arch_id to be used and how, so people know what it's > supposed to return. The trouble with this is I think it's impossible - it doesn't have a well defined meaning. > If the only place where it's useful now is ACPI code (is it?), > should we rename it to something like get_acpi_id()? -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
© 2016 - 2026 Red Hat, Inc.