From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d31aA-0002Vg-K9 for qemu-devel@nongnu.org; Tue, 25 Apr 2017 10:36:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d31a4-0007gr-Qd for qemu-devel@nongnu.org; Tue, 25 Apr 2017 10:36:46 -0400 Date: Tue, 25 Apr 2017 16:36:20 +0200 From: Igor Mammedov Message-ID: <20170425163620.30f4ceb2@Igors-MacBook-Pro.local> In-Reply-To: <20170425142827.rezulct5cxetweyw@kamzik.brq.redhat.com> References: <1490189568-167621-1-git-send-email-imammedo@redhat.com> <1490189568-167621-4-git-send-email-imammedo@redhat.com> <20170425142827.rezulct5cxetweyw@kamzik.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.10 03/23] hw/arm/virt: use machine->possible_cpus for storing possible topology info List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Jones Cc: qemu-devel@nongnu.org, Peter Maydell , Eduardo Habkost , qemu-arm@nongnu.org, qemu-ppc@nongnu.org, Shannon Zhao , Paolo Bonzini , David Gibson On Tue, 25 Apr 2017 16:28:27 +0200 Andrew Jones wrote: > On Wed, Mar 22, 2017 at 02:32:28PM +0100, 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 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 > > --- > > hw/arm/virt.c | 39 ++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 36 insertions(+), 3 deletions(-) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 484754e..4de46b1 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1237,6 +1237,7 @@ static void machvirt_init(MachineState *machine) > > { > > VirtMachineState *vms = VIRT_MACHINE(machine); > > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine); > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > qemu_irq pic[NUM_IRQS]; > > MemoryRegion *sysmem = get_system_memory(); > > MemoryRegion *secure_sysmem = NULL; > > @@ -1360,10 +1361,16 @@ static void machvirt_init(MachineState *machine) > > exit(1); > > } > > > > - for (n = 0; n < smp_cpus; n++) { > > - Object *cpuobj = object_new(typename); > > + mc->possible_cpu_arch_ids(machine); > > Hmm, this is a bit too subtle for my taste. Making this call, which by > name implies it returns the possible CPU Arch-IDs (which indeed it does), > but without assigning its return value to anything, looks wrong. I see > the reason to do so, though, is because on first call it will do the > allocation. I'd prefer a wrapper > > static void virt_alloc_possible_cpus(MachineState *ms) > { > /* First invocation does allocation and initialization. */ > (void)MACHINE_GET_CLASS(ms)->possible_cpu_arch_ids(ms); > } Ok, I'll make it generic machine_alloc_possible_cpus(MachineState *ms) and use it for x86/ppc as well > > > + for (n = 0; n < machine->possible_cpus->len; n++) { > > + Object *cpuobj; > > > > - object_property_set_int(cpuobj, virt_idx2mp_affinity(vms, n), > > + if (n >= smp_cpus) { > > + break; > > + } > > + > > + cpuobj = object_new(typename); > > + object_property_set_int(cpuobj, machine->possible_cpus->cpus[n].arch_id, > > "mp-affinity", NULL); > > > > if (!vms->secure) { > > @@ -1543,6 +1550,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_idx2mp_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); > > @@ -1559,6 +1591,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 > > > > > > Thanks, > drew