| Message ID | 1493816238-33120-4-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 03, 2017 at 02:56:57PM +0200, Igor Mammedov wrote: > for now precalculate and store mp_afinity in possible_cpus > as ARM cpus don't have socket/core/thread-id properties yet. > In follow patches possible_cpus will be used for storing > and setting NUMA node mapping and replace legacy bitmap > based numa_info[node_id].node_cpu/numa_get_node_for_cpu() > > For the lack of better idea, this patch cannibalizes > possible_cpus.cpus[x].props.thread_id so that > *_cpu_index_to_props() callback could return addressable > by props CPU which will be used by machine_set_cpu_numa_node() > in follow up patches to assign a CPU to node. But > cannibalizing is fine for now as that thread_id isn't exposed > to users (no hotpluggable_cpus callback support for ARM yet) > and it will be used only internally until 'device_add cpu' > is supported where we can decide on which properties to use. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v2: > (Drew) > - discarding result of possible_cpu_arch_ids() makes > call not obvious and is confusing. Instead assign > possible_cpu_arch_ids() result to local var and use > it instead of direct access to machine->possible_cpus > field, as it's done in pc.c > --- > hw/arm/virt.c | 40 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 37 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 61ae437..e2c5626 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1221,6 +1221,8 @@ static void machvirt_init(MachineState *machine) > { > VirtMachineState *vms = VIRT_MACHINE(machine); > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine); > + MachineClass *mc = MACHINE_GET_CLASS(machine); > + const CPUArchIdList *possible_cpus; > qemu_irq pic[NUM_IRQS]; > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *secure_sysmem = NULL; > @@ -1344,10 +1346,16 @@ static void machvirt_init(MachineState *machine) > exit(1); > } > > - for (n = 0; n < smp_cpus; n++) { > - Object *cpuobj = object_new(typename); > + possible_cpus = mc->possible_cpu_arch_ids(machine); > + for (n = 0; n < possible_cpus->len; n++) { > + Object *cpuobj; > > - object_property_set_int(cpuobj, virt_cpu_mp_affinity(vms, n), > + if (n >= smp_cpus) { > + break; > + } Why the break instead of just looping 'n < smp_cpus' like x86 does? Is there some future work where looping up to possible_cpus->len (aka max_cpus) is what we'll eventually want? If so, then we need a TODO comment here. If not, then we should clean this up by removing the break. Thanks, drew > + > + cpuobj = object_new(typename); > + object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id, > "mp-affinity", NULL); > > if (!vms->secure) { > @@ -1527,6 +1535,31 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) > } > } > > +static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) > +{ > + int n; > + VirtMachineState *vms = VIRT_MACHINE(ms); > + > + if (ms->possible_cpus) { > + assert(ms->possible_cpus->len == max_cpus); > + return ms->possible_cpus; > + } > + > + ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) + > + sizeof(CPUArchId) * max_cpus); > + ms->possible_cpus->len = max_cpus; > + for (n = 0; n < ms->possible_cpus->len; n++) { > + ms->possible_cpus->cpus[n].arch_id = > + virt_cpu_mp_affinity(vms, n); > + ms->possible_cpus->cpus[n].props.has_thread_id = true; > + ms->possible_cpus->cpus[n].props.thread_id = n; > + > + /* TODO: add 'has_node/node' here to describe > + to which node core belongs */ > + } > + return ms->possible_cpus; > +} > + > static void virt_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -1543,6 +1576,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) > mc->pci_allow_0_address = true; > /* We know we will never create a pre-ARMv7 CPU which needs 1K pages */ > mc->minimum_page_bits = 12; > + mc->possible_cpu_arch_ids = virt_possible_cpu_arch_ids; > } > > static const TypeInfo virt_machine_info = { > -- > 2.7.4 >
On Thu, 4 May 2017 11:38:22 +0200 Andrew Jones <drjones@redhat.com> wrote: > On Wed, May 03, 2017 at 02:56:57PM +0200, Igor Mammedov wrote: > > for now precalculate and store mp_afinity in possible_cpus > > as ARM cpus don't have socket/core/thread-id properties yet. > > In follow patches possible_cpus will be used for storing > > and setting NUMA node mapping and replace legacy bitmap > > based numa_info[node_id].node_cpu/numa_get_node_for_cpu() > > > > For the lack of better idea, this patch cannibalizes > > possible_cpus.cpus[x].props.thread_id so that > > *_cpu_index_to_props() callback could return addressable > > by props CPU which will be used by machine_set_cpu_numa_node() > > in follow up patches to assign a CPU to node. But > > cannibalizing is fine for now as that thread_id isn't exposed > > to users (no hotpluggable_cpus callback support for ARM yet) > > and it will be used only internally until 'device_add cpu' > > is supported where we can decide on which properties to use. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > v2: > > (Drew) > > - discarding result of possible_cpu_arch_ids() makes > > call not obvious and is confusing. Instead assign > > possible_cpu_arch_ids() result to local var and use > > it instead of direct access to machine->possible_cpus > > field, as it's done in pc.c > > --- > > hw/arm/virt.c | 40 +++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 37 insertions(+), 3 deletions(-) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 61ae437..e2c5626 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1221,6 +1221,8 @@ static void machvirt_init(MachineState *machine) > > { > > VirtMachineState *vms = VIRT_MACHINE(machine); > > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine); > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > + const CPUArchIdList *possible_cpus; > > qemu_irq pic[NUM_IRQS]; > > MemoryRegion *sysmem = get_system_memory(); > > MemoryRegion *secure_sysmem = NULL; > > @@ -1344,10 +1346,16 @@ static void machvirt_init(MachineState *machine) > > exit(1); > > } > > > > - for (n = 0; n < smp_cpus; n++) { > > - Object *cpuobj = object_new(typename); > > + possible_cpus = mc->possible_cpu_arch_ids(machine); > > + for (n = 0; n < possible_cpus->len; n++) { > > + Object *cpuobj; > > > > - object_property_set_int(cpuobj, virt_cpu_mp_affinity(vms, n), > > + if (n >= smp_cpus) { > > + break; > > + } > > Why the break instead of just looping 'n < smp_cpus' like x86 does? Is > there some future work where looping up to possible_cpus->len (aka > max_cpus) is what we'll eventually want? If so, then we need a TODO > comment here. If not, then we should clean this up by removing the break. There is no plans to loop here upto possible_cpus->len. It seemed to me more consistent/safer to use index limited by possible_cpus->len to index possible_cpus->cpus[n] array than index limited by smp_cpus though the former currently is always less than smp_cpus. If you prefer 'n < smp_cpus' loop, then I can switch to it. > > Thanks, > drew > > > + > > + cpuobj = object_new(typename); > > + object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id, > > "mp-affinity", NULL); > > > > if (!vms->secure) { > > @@ -1527,6 +1535,31 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) > > } > > } > > > > +static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) > > +{ > > + int n; > > + VirtMachineState *vms = VIRT_MACHINE(ms); > > + > > + if (ms->possible_cpus) { > > + assert(ms->possible_cpus->len == max_cpus); > > + return ms->possible_cpus; > > + } > > + > > + ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) + > > + sizeof(CPUArchId) * max_cpus); > > + ms->possible_cpus->len = max_cpus; > > + for (n = 0; n < ms->possible_cpus->len; n++) { > > + ms->possible_cpus->cpus[n].arch_id = > > + virt_cpu_mp_affinity(vms, n); > > + ms->possible_cpus->cpus[n].props.has_thread_id = true; > > + ms->possible_cpus->cpus[n].props.thread_id = n; > > + > > + /* TODO: add 'has_node/node' here to describe > > + to which node core belongs */ > > + } > > + return ms->possible_cpus; > > +} > > + > > static void virt_machine_class_init(ObjectClass *oc, void *data) > > { > > MachineClass *mc = MACHINE_CLASS(oc); > > @@ -1543,6 +1576,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) > > mc->pci_allow_0_address = true; > > /* We know we will never create a pre-ARMv7 CPU which needs 1K pages */ > > mc->minimum_page_bits = 12; > > + mc->possible_cpu_arch_ids = virt_possible_cpu_arch_ids; > > } > > > > static const TypeInfo virt_machine_info = { > > -- > > 2.7.4 > > >
On Thu, May 04, 2017 at 02:55:09PM +0200, Igor Mammedov wrote: > On Thu, 4 May 2017 11:38:22 +0200 > Andrew Jones <drjones@redhat.com> wrote: > > > On Wed, May 03, 2017 at 02:56:57PM +0200, Igor Mammedov wrote: > > > for now precalculate and store mp_afinity in possible_cpus > > > as ARM cpus don't have socket/core/thread-id properties yet. > > > In follow patches possible_cpus will be used for storing > > > and setting NUMA node mapping and replace legacy bitmap > > > based numa_info[node_id].node_cpu/numa_get_node_for_cpu() > > > > > > For the lack of better idea, this patch cannibalizes > > > possible_cpus.cpus[x].props.thread_id so that > > > *_cpu_index_to_props() callback could return addressable > > > by props CPU which will be used by machine_set_cpu_numa_node() > > > in follow up patches to assign a CPU to node. But > > > cannibalizing is fine for now as that thread_id isn't exposed > > > to users (no hotpluggable_cpus callback support for ARM yet) > > > and it will be used only internally until 'device_add cpu' > > > is supported where we can decide on which properties to use. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > v2: > > > (Drew) > > > - discarding result of possible_cpu_arch_ids() makes > > > call not obvious and is confusing. Instead assign > > > possible_cpu_arch_ids() result to local var and use > > > it instead of direct access to machine->possible_cpus > > > field, as it's done in pc.c > > > --- > > > hw/arm/virt.c | 40 +++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 37 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > > index 61ae437..e2c5626 100644 > > > --- a/hw/arm/virt.c > > > +++ b/hw/arm/virt.c > > > @@ -1221,6 +1221,8 @@ static void machvirt_init(MachineState *machine) > > > { > > > VirtMachineState *vms = VIRT_MACHINE(machine); > > > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine); > > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > > + const CPUArchIdList *possible_cpus; > > > qemu_irq pic[NUM_IRQS]; > > > MemoryRegion *sysmem = get_system_memory(); > > > MemoryRegion *secure_sysmem = NULL; > > > @@ -1344,10 +1346,16 @@ static void machvirt_init(MachineState *machine) > > > exit(1); > > > } > > > > > > - for (n = 0; n < smp_cpus; n++) { > > > - Object *cpuobj = object_new(typename); > > > + possible_cpus = mc->possible_cpu_arch_ids(machine); > > > + for (n = 0; n < possible_cpus->len; n++) { > > > + Object *cpuobj; > > > > > > - object_property_set_int(cpuobj, virt_cpu_mp_affinity(vms, n), > > > + if (n >= smp_cpus) { > > > + break; > > > + } > > > > Why the break instead of just looping 'n < smp_cpus' like x86 does? Is > > there some future work where looping up to possible_cpus->len (aka > > max_cpus) is what we'll eventually want? If so, then we need a TODO > > comment here. If not, then we should clean this up by removing the break. > There is no plans to loop here upto possible_cpus->len. > > It seemed to me more consistent/safer to use index limited > by possible_cpus->len to index possible_cpus->cpus[n] array > than index limited by smp_cpus though the former currently is > always less than smp_cpus. ^ greater than or equal to > > If you prefer 'n < smp_cpus' loop, then I can switch to it. I just don't like the 'if (n >= smp_cpus) { break; }' - the whole thing would look much nicer without it. And, if there's a valid concern that possible_cpus->len can be < smp_cpus, then we should check it in x86 too. Anyway we can check both conditions in the 'for', which would look a bit more pleasing to me... for (n = 0; n < possible_cpus->len && n < smp_cpus; n++) { Object *cpuobj = object_new(typename); object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id, "mp-affinity", NULL); ... All that said, it's just a nit in the end, so Reviewed-by: Andrew Jones <drjones@redhat.com>
On Thu, 4 May 2017 15:16:02 +0200 Andrew Jones <drjones@redhat.com> wrote: > On Thu, May 04, 2017 at 02:55:09PM +0200, Igor Mammedov wrote: > > On Thu, 4 May 2017 11:38:22 +0200 > > Andrew Jones <drjones@redhat.com> wrote: > > > > > On Wed, May 03, 2017 at 02:56:57PM +0200, Igor Mammedov wrote: > > > > for now precalculate and store mp_afinity in possible_cpus > > > > as ARM cpus don't have socket/core/thread-id properties yet. > > > > In follow patches possible_cpus will be used for storing > > > > and setting NUMA node mapping and replace legacy bitmap > > > > based numa_info[node_id].node_cpu/numa_get_node_for_cpu() > > > > > > > > For the lack of better idea, this patch cannibalizes > > > > possible_cpus.cpus[x].props.thread_id so that > > > > *_cpu_index_to_props() callback could return addressable > > > > by props CPU which will be used by machine_set_cpu_numa_node() > > > > in follow up patches to assign a CPU to node. But > > > > cannibalizing is fine for now as that thread_id isn't exposed > > > > to users (no hotpluggable_cpus callback support for ARM yet) > > > > and it will be used only internally until 'device_add cpu' > > > > is supported where we can decide on which properties to use. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > v2: > > > > (Drew) > > > > - discarding result of possible_cpu_arch_ids() makes > > > > call not obvious and is confusing. Instead assign > > > > possible_cpu_arch_ids() result to local var and use > > > > it instead of direct access to machine->possible_cpus > > > > field, as it's done in pc.c > > > > --- > > > > hw/arm/virt.c | 40 +++++++++++++++++++++++++++++++++++++--- > > > > 1 file changed, 37 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > > > index 61ae437..e2c5626 100644 > > > > --- a/hw/arm/virt.c > > > > +++ b/hw/arm/virt.c > > > > @@ -1221,6 +1221,8 @@ static void machvirt_init(MachineState *machine) > > > > { > > > > VirtMachineState *vms = VIRT_MACHINE(machine); > > > > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine); > > > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > > > + const CPUArchIdList *possible_cpus; > > > > qemu_irq pic[NUM_IRQS]; > > > > MemoryRegion *sysmem = get_system_memory(); > > > > MemoryRegion *secure_sysmem = NULL; > > > > @@ -1344,10 +1346,16 @@ static void machvirt_init(MachineState *machine) > > > > exit(1); > > > > } > > > > > > > > - for (n = 0; n < smp_cpus; n++) { > > > > - Object *cpuobj = object_new(typename); > > > > + possible_cpus = mc->possible_cpu_arch_ids(machine); > > > > + for (n = 0; n < possible_cpus->len; n++) { > > > > + Object *cpuobj; > > > > > > > > - object_property_set_int(cpuobj, virt_cpu_mp_affinity(vms, n), > > > > + if (n >= smp_cpus) { > > > > + break; > > > > + } > > > > > > Why the break instead of just looping 'n < smp_cpus' like x86 does? Is > > > there some future work where looping up to possible_cpus->len (aka > > > max_cpus) is what we'll eventually want? If so, then we need a TODO > > > comment here. If not, then we should clean this up by removing the break. > > There is no plans to loop here upto possible_cpus->len. > > > > It seemed to me more consistent/safer to use index limited > > by possible_cpus->len to index possible_cpus->cpus[n] array > > than index limited by smp_cpus though the former currently is > > always less than smp_cpus. > ^ greater than or equal to > > > > If you prefer 'n < smp_cpus' loop, then I can switch to it. > > I just don't like the 'if (n >= smp_cpus) { break; }' - the whole thing > would look much nicer without it. And, if there's a valid concern that > possible_cpus->len can be < smp_cpus, then we should check it in x86 > too. Anyway we can check both conditions in the 'for', which would > look a bit more pleasing to me... > > for (n = 0; n < possible_cpus->len && n < smp_cpus; n++) { nice, I'll do it this way on respin. > Object *cpuobj = object_new(typename); > object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id, > "mp-affinity", NULL); > ... > > All that said, it's just a nit in the end, so > > Reviewed-by: Andrew Jones <drjones@redhat.com>
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 61ae437..e2c5626 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1221,6 +1221,8 @@ static void machvirt_init(MachineState *machine) { VirtMachineState *vms = VIRT_MACHINE(machine); VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine); + MachineClass *mc = MACHINE_GET_CLASS(machine); + const CPUArchIdList *possible_cpus; qemu_irq pic[NUM_IRQS]; MemoryRegion *sysmem = get_system_memory(); MemoryRegion *secure_sysmem = NULL; @@ -1344,10 +1346,16 @@ static void machvirt_init(MachineState *machine) exit(1); } - for (n = 0; n < smp_cpus; n++) { - Object *cpuobj = object_new(typename); + possible_cpus = mc->possible_cpu_arch_ids(machine); + for (n = 0; n < possible_cpus->len; n++) { + Object *cpuobj; - object_property_set_int(cpuobj, virt_cpu_mp_affinity(vms, n), + if (n >= smp_cpus) { + break; + } + + cpuobj = object_new(typename); + object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id, "mp-affinity", NULL); if (!vms->secure) { @@ -1527,6 +1535,31 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp) } } +static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) +{ + int n; + VirtMachineState *vms = VIRT_MACHINE(ms); + + if (ms->possible_cpus) { + assert(ms->possible_cpus->len == max_cpus); + return ms->possible_cpus; + } + + ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) + + sizeof(CPUArchId) * max_cpus); + ms->possible_cpus->len = max_cpus; + for (n = 0; n < ms->possible_cpus->len; n++) { + ms->possible_cpus->cpus[n].arch_id = + virt_cpu_mp_affinity(vms, n); + ms->possible_cpus->cpus[n].props.has_thread_id = true; + ms->possible_cpus->cpus[n].props.thread_id = n; + + /* TODO: add 'has_node/node' here to describe + to which node core belongs */ + } + return ms->possible_cpus; +} + static void virt_machine_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); @@ -1543,6 +1576,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) mc->pci_allow_0_address = true; /* We know we will never create a pre-ARMv7 CPU which needs 1K pages */ mc->minimum_page_bits = 12; + mc->possible_cpu_arch_ids = virt_possible_cpu_arch_ids; } static const TypeInfo virt_machine_info = {
for now precalculate and store mp_afinity in possible_cpus as ARM cpus don't have socket/core/thread-id properties yet. In follow patches possible_cpus will be used for storing and setting NUMA node mapping and replace legacy bitmap based numa_info[node_id].node_cpu/numa_get_node_for_cpu() For the lack of better idea, this patch cannibalizes possible_cpus.cpus[x].props.thread_id so that *_cpu_index_to_props() callback could return addressable by props CPU which will be used by machine_set_cpu_numa_node() in follow up patches to assign a CPU to node. But cannibalizing is fine for now as that thread_id isn't exposed to users (no hotpluggable_cpus callback support for ARM yet) and it will be used only internally until 'device_add cpu' is supported where we can decide on which properties to use. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v2: (Drew) - discarding result of possible_cpu_arch_ids() makes call not obvious and is confusing. Instead assign possible_cpu_arch_ids() result to local var and use it instead of direct access to machine->possible_cpus field, as it's done in pc.c --- hw/arm/virt.c | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-)