On Tue, Jul 01, 2014 at 01:50:06PM -0700, Nishanth Aravamudan wrote: > On 01.07.2014 [17:39:57 -0300], Eduardo Habkost wrote: > > On Tue, Jul 01, 2014 at 01:13:28PM -0700, Nishanth Aravamudan wrote: > > [...] > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index 12472c6..cdefafe 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1121,6 +1121,18 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t > > > below_4g_mem_size, > > > guest_info->ram_size = below_4g_mem_size + above_4g_mem_size; > > > guest_info->apic_id_limit = pc_apic_id_limit(max_cpus); > > > guest_info->apic_xrupt_override = kvm_allows_irq0_override(); > > > + /* No support for sparse NUMA node IDs yet: */ > > > + for (i = max_numa_nodeid - 1; i >= 0; i--) { > > > + /* Report large node IDs first, to make mistakes easier to spot > > > */ > > > + if (!numa_info[i].present) { > > > + error_report("numa: Node ID missing: %d", i); > > > + exit(EXIT_FAILURE); > > > + } > > > + } > > > + > > > + /* This must be always true if all nodes are present */ > > > + assert(num_numa_nodes == max_numa_nodeid); > > > + > > > > I wonder if there's a better place where we could put this check. > > Well, only i386 and ppc support NUMA, afaict. So I'm not sure where it > makes sense to put it. I guess we could have a flag that the > architectures set that indicates sparse NUMA support or not, and put > this in the generic code. > > Or do you mean putting this check somewhere else in the PC init code?
I mean somewhere else in the PC init code. But as today the code that calls pc_guest_info_init() and pc_memory_init() is duplicated in both pc_piix.c and pc_q35.c, this looks like the best place we have. > > > > guest_info->numa_nodes = num_numa_nodes; > > > guest_info->node_mem = g_malloc0(guest_info->numa_nodes * > > > sizeof *guest_info->node_mem); > > [...] > > > diff --git a/numa.c b/numa.c > > > index 5930df0..a689e52 100644 > > > --- a/numa.c > > > +++ b/numa.c > > [...] > > > @@ -225,9 +220,12 @@ void set_numa_nodes(void) > > > * must cope with this anyway, because there are BIOSes out > > > there in > > > * real machines which also use this scheme. > > > */ > > > - if (i == num_numa_nodes) { > > > - for (i = 0; i < max_cpus; i++) { > > > - set_bit(i, numa_info[i % num_numa_nodes].node_cpu); > > > + if (i == max_numa_nodeid) { > > > + for (i = 0, j = 0; i < max_cpus; i++) { > > > > Doesn't j need to be initialized to -1, here? > > Arrgh, sorry had been messing with your suggestion to use a while loop. > You're right, it needs to be -1 here. > > > Except for that, patch looks good to me. But I would be more comfortable > > with it if we had automated tests to help ensure we are not breaking > > compatibility of existing NUMA command-line conbinations with these > > changes. > > Is that the test target in the qemu source? Are there examples of any > such NUMA tests already? I use 'make check' to run them, they are in the tests/ directory. I am not aware of any NUMA-related test, but I see two possible ways of testing it: using qtest and asking for for the NUMA node info through the monitor, or a unit test for numa.c that simply calls numa_node_parse() and set_numa_nodes(), and then checks the result on numa_info[] directly. A third option may be using qtest and checking the resulting ACPI tables directly. It would cover even more code, but would be specific to PC. The tests won't be a requirement to me, but they would surely be welcome (and would have detected the j=0 mistake above). -- Eduardo