Re: [qemu-s390x] [PATCH v1] numa: s390x has no NUMA

[Christian Borntraeger]

On 02/23/2018 06:36 PM, David Hildenbrand wrote:
> Right now it is possible to crash QEMU for s390x by providing e.g.
>     -numa node,nodeid=0,cpus=0-1
> 
> Problem is, that numa.c uses mc->cpu_index_to_instance_props as an
> indicator whether NUMA is supported by a machine type. We don't
> implement NUMA on s390x (and that concept also doesn't really exist).
> We need mc->cpu_index_to_instance_props for query-cpus.

Looks like we assert because of 
machine->possible_cpus == 0.

Later during boot this is created in s390_possible_cpu_arch_ids. (via 
s390_init_cpus). What we (in the future) actually could provide is a 
cpu topology.

So something like this also fixes the bug

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index fd5bfcdaa5..d981335ca9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "cpu.h"
+#include "sysemu/numa.h"
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
 #include "hw/s390x/s390-virtio-hcall.h"
@@ -393,11 +394,20 @@ static void 
s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *machine,
                                                      unsigned cpu_index)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+    /* make sure possible_cpu are intialized */
+    mc->possible_cpu_arch_ids(machine);
     g_assert(machine->possible_cpus && cpu_index < 
machine->possible_cpus->len);
 
     return machine->possible_cpus->cpus[cpu_index].props;
 }
 
+static int64_t s390_get_default_cpu_node_id(const MachineState *ms, int idx)
+{
+    return idx / smp_cpus % nb_numa_nodes;
+}
+
 static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
 {
     int i;
@@ -473,6 +483,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void 
*data)
     mc->get_hotplug_handler = s390_get_hotplug_handler;
     mc->cpu_index_to_instance_props = s390_cpu_index_to_props;
     mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
+    mc->get_default_cpu_node_id = s390_get_default_cpu_node_id;
     /* it is overridden with 'host' cpu *in kvm_arch_init* */
     mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
     hc->plug = s390_machine_device_plug;


and it would allow us to extend things later on. On the other hand, my fix does 
not
implement anything so your fix is "more correct".

> 
> So let's fix this case.
> 
> qemu-system-s390x: -numa node,nodeid=0,cpus=0-1: NUMA is not supported by
>                    this machine-type
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  numa.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/numa.c b/numa.c
> index 7e0e789b02..3b9be613d9 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -80,10 +80,16 @@ static void parse_numa_node(MachineState *ms, 
> NumaNodeOptions *node,
>          return;
>      }
> 
> +#ifdef TARGET_S390X
> +    /* s390x provides cpu_index_to_instance_props but has no NUMA */
> +    error_report("NUMA is not supported by this machine-type");
> +    exit(1);
> +#else
>      if (!mc->cpu_index_to_instance_props) {
>          error_report("NUMA is not supported by this machine-type");
>          exit(1);
>      }
> +#endif
>      for (cpus = node->cpus; cpus; cpus = cpus->next) {
>          CpuInstanceProperties props;
>          if (cpus->value >= max_cpus) {
>