On Wed, Jul 02, 2014 at 02:02:14PM -0700, Nishanth Aravamudan wrote: > On 02.07.2014 [15:21:38 -0300], Eduardo Habkost wrote: > > 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. > > Ok, so if I send out another revision with the fixed j initialization > below, is there anything else in my changes that you would like fixed?
I don't see any additional issues. > [...] > > > > 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. > > Got it, thanks. > > > 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. > > Do you have a preference for which of these to do? The one we find to be easier. :) An unit test may require untangling numa.o dependencies. qtest will probably require parsing the "info numa" output. A qtest case would cover more code (not just numa.c, but command-ilne handling on vl.c, and monitor code). > > > 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. > > I'm not comfortable saying I can get to this, as I still don't really > know the ACPI code, but I can put it on my todo list, at least. > > > The tests won't be a requirement to me, but they would surely be welcome > > (and would have detected the j=0 mistake above). > > I think it makes sense to put this in now, as it would have caught the > original issue(s) with sparse node numbering as well. > > Thanks, > Nish > -- Eduardo