| Message ID | 20180223173657.29125-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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?
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 :)
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.
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) {
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(+)