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
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>
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) {
>
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
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) {
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
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.
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?
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
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.
© 2016 - 2025 Red Hat, Inc.