[Qemu-devel] [PATCH v1] numa: s390x has no NUMA

[Qemu-devel] [PATCH v1] numa: s390x has no NUMA

[David Hildenbrand]

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.

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 <david@redhat.com>
---
 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) {
-- 
2.14.3

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

[Claudio Imbrenda]

On Fri, 23 Feb 2018 18:36:57 +0100
David Hildenbrand <david@redhat.com> 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.
> 
> 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 <david@redhat.com>
> ---
>  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) {

seems straightforward

Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>

Re: [Qemu-devel] [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 <david@redhat.com>
> ---
>  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) {
> 

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

[David Hildenbrand]

On 26.02.2018 10:20, Christian Borntraeger wrote:
> 
> 
> 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

Yes, but I decided to not go this way because we don't support NUMA as
of now. -numa has to bail out (just as it did before I implemented
proper query-cpus support).

What you propose is something for future support - one we have cpu
topology information exposed.


-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Fri, 23 Feb 2018 18:36:57 +0100
David Hildenbrand <david@redhat.com> 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.

Is existence of cpu_index_to_instance_probs the correct indicator for
numa, then?

OTOH, your patch is straightforward...

> 
> 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 <david@redhat.com>
> ---
>  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) {

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

[David Hildenbrand]

On 26.02.2018 11:19, Cornelia Huck wrote:
> On Fri, 23 Feb 2018 18:36:57 +0100
> David Hildenbrand <david@redhat.com> 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.
> 
> Is existence of cpu_index_to_instance_probs the correct indicator for
> numa, then?
> 
> OTOH, your patch is straightforward...

Maybe it is get_default_cpu_node_id as Christian discovered?


-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Mon, 26 Feb 2018 11:28:26 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 26.02.2018 11:19, Cornelia Huck wrote:
> > On Fri, 23 Feb 2018 18:36:57 +0100
> > David Hildenbrand <david@redhat.com> 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.  
> > 
> > Is existence of cpu_index_to_instance_probs the correct indicator for
> > numa, then?
> > 
> > OTOH, your patch is straightforward...  
> 
> Maybe it is get_default_cpu_node_id as Christian discovered?

Yes, that seems like a better candidate for checking.

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

[Christian Borntraeger]

On 02/26/2018 11:35 AM, Cornelia Huck wrote:
> On Mon, 26 Feb 2018 11:28:26 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 26.02.2018 11:19, Cornelia Huck wrote:
>>> On Fri, 23 Feb 2018 18:36:57 +0100
>>> David Hildenbrand <david@redhat.com> 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.  
>>>
>>> Is existence of cpu_index_to_instance_probs the correct indicator for
>>> numa, then?
>>>
>>> OTOH, your patch is straightforward...  
>>
>> Maybe it is get_default_cpu_node_id as Christian discovered?
> 
> Yes, that seems like a better candidate for checking.

Agreed. 
As everybody else calls possible_cpu_arch_ids  in cpu_index_to_props
I am asking myself if we should do that as well anyway?

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

[David Hildenbrand]

On 26.02.2018 12:07, Christian Borntraeger wrote:
> 
> 
> On 02/26/2018 11:35 AM, Cornelia Huck wrote:
>> On Mon, 26 Feb 2018 11:28:26 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 26.02.2018 11:19, Cornelia Huck wrote:
>>>> On Fri, 23 Feb 2018 18:36:57 +0100
>>>> David Hildenbrand <david@redhat.com> 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.  
>>>>
>>>> Is existence of cpu_index_to_instance_probs the correct indicator for
>>>> numa, then?
>>>>
>>>> OTOH, your patch is straightforward...  
>>>
>>> Maybe it is get_default_cpu_node_id as Christian discovered?
>>
>> Yes, that seems like a better candidate for checking.
> 
> Agreed. 
> As everybody else calls possible_cpu_arch_ids  in cpu_index_to_props
> I am asking myself if we should do that as well anyway?
> 

Well, it found a BUG :)

-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Mon, 26 Feb 2018 12:07:43 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02/26/2018 11:35 AM, Cornelia Huck wrote:
> > On Mon, 26 Feb 2018 11:28:26 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 26.02.2018 11:19, Cornelia Huck wrote:  
> >>> On Fri, 23 Feb 2018 18:36:57 +0100
> >>> David Hildenbrand <david@redhat.com> 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.    
> >>>
> >>> Is existence of cpu_index_to_instance_probs the correct indicator for
> >>> numa, then?
> >>>
> >>> OTOH, your patch is straightforward...    
> >>
> >> Maybe it is get_default_cpu_node_id as Christian discovered?  
> > 
> > Yes, that seems like a better candidate for checking.  
> 
> Agreed. 
> As everybody else calls possible_cpu_arch_ids  in cpu_index_to_props
> I am asking myself if we should do that as well anyway?
> 

Making the behaviour consistent with other archs sounds like a good
idea.