| Message ID | 20170911152150.12535-12-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x cleanups and CPU hotplug via device_add | expand |
On Mon, 11 Sep 2017 17:21:40 +0200 David Hildenbrand <david@redhat.com> wrote: > Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the > guest tries to bring these CPUs up but fails), because we don't support > multiple CPUs on s390x under TCG. > > Let's bail out if more than 1 is specified, so we don't raise people's > hope. Make it a define, so we can easily bump it up later. > > Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index f67b4b5d58..f1198b2745 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -23,6 +23,7 @@ > #include "hw/s390x/css.h" > #include "virtio-ccw.h" > #include "qemu/config-file.h" > +#include "qemu/error-report.h" > #include "s390-pci-bus.h" > #include "hw/s390x/storage-keys.h" > #include "hw/s390x/storage-attributes.h" > @@ -47,6 +48,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > return cpu_states[cpu_addr]; > } > > +/* #define S390_TCG_SMP_SUPPORT */ I'd drop define and ifdef for something that doesn't exists > static void s390_init_cpus(MachineState *machine) > { > int i; > @@ -55,6 +58,13 @@ static void s390_init_cpus(MachineState *machine) > if (machine->cpu_model == NULL) { > machine->cpu_model = s390_default_cpu_model_name(); > } > +#ifndef S390_TCG_SMP_SUPPORT > + if (tcg_enabled() && max_cpus > 1) { > + error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " > + "supported by TCG (1) on s390x", max_cpus); > + exit(1); > + } > +#endif > > cpu_states = g_new0(S390CPU *, max_cpus); >
On 12.09.2017 14:43, Igor Mammedov wrote: > On Mon, 11 Sep 2017 17:21:40 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the >> guest tries to bring these CPUs up but fails), because we don't support >> multiple CPUs on s390x under TCG. >> >> Let's bail out if more than 1 is specified, so we don't raise people's >> hope. Make it a define, so we can easily bump it up later. >> >> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index f67b4b5d58..f1198b2745 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -23,6 +23,7 @@ >> #include "hw/s390x/css.h" >> #include "virtio-ccw.h" >> #include "qemu/config-file.h" >> +#include "qemu/error-report.h" >> #include "s390-pci-bus.h" >> #include "hw/s390x/storage-keys.h" >> #include "hw/s390x/storage-attributes.h" >> @@ -47,6 +48,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) >> return cpu_states[cpu_addr]; >> } >> >> +/* #define S390_TCG_SMP_SUPPORT */ > I'd drop define and ifdef for something that doesn't exists Conny requested it as we might see some work on that area (supporting smp) soon. So as long as there are no other opinions, I'll stick to the current version. Thanks!
On Tue, 12 Sep 2017 17:42:35 +0200 David Hildenbrand <david@redhat.com> wrote: > On 12.09.2017 14:43, Igor Mammedov wrote: > > On Mon, 11 Sep 2017 17:21:40 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the > >> guest tries to bring these CPUs up but fails), because we don't support > >> multiple CPUs on s390x under TCG. > >> > >> Let's bail out if more than 1 is specified, so we don't raise people's > >> hope. Make it a define, so we can easily bump it up later. > >> > >> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > >> index f67b4b5d58..f1198b2745 100644 > >> --- a/hw/s390x/s390-virtio-ccw.c > >> +++ b/hw/s390x/s390-virtio-ccw.c > >> @@ -23,6 +23,7 @@ > >> #include "hw/s390x/css.h" > >> #include "virtio-ccw.h" > >> #include "qemu/config-file.h" > >> +#include "qemu/error-report.h" > >> #include "s390-pci-bus.h" > >> #include "hw/s390x/storage-keys.h" > >> #include "hw/s390x/storage-attributes.h" > >> @@ -47,6 +48,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > >> return cpu_states[cpu_addr]; > >> } > >> > >> +/* #define S390_TCG_SMP_SUPPORT */ > > I'd drop define and ifdef for something that doesn't exists > > Conny requested it as we might see some work on that area (supporting > smp) soon. So as long as there are no other opinions, I'll stick to the > current version. > > Thanks! > I've just removed a bunch of TODO cpu models in PPC with corresponding macro, my guess they were also introduced with similar intent but ended up as dead code. So it's better to add new code when it actually is needed, instead of just in case.
On 13.09.2017 09:19, Igor Mammedov wrote: > On Tue, 12 Sep 2017 17:42:35 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 12.09.2017 14:43, Igor Mammedov wrote: >>> On Mon, 11 Sep 2017 17:21:40 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the >>>> guest tries to bring these CPUs up but fails), because we don't support >>>> multiple CPUs on s390x under TCG. >>>> >>>> Let's bail out if more than 1 is specified, so we don't raise people's >>>> hope. Make it a define, so we can easily bump it up later. >>>> >>>> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index f67b4b5d58..f1198b2745 100644 >>>> --- a/hw/s390x/s390-virtio-ccw.c >>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>> @@ -23,6 +23,7 @@ >>>> #include "hw/s390x/css.h" >>>> #include "virtio-ccw.h" >>>> #include "qemu/config-file.h" >>>> +#include "qemu/error-report.h" >>>> #include "s390-pci-bus.h" >>>> #include "hw/s390x/storage-keys.h" >>>> #include "hw/s390x/storage-attributes.h" >>>> @@ -47,6 +48,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) >>>> return cpu_states[cpu_addr]; >>>> } >>>> >>>> +/* #define S390_TCG_SMP_SUPPORT */ >>> I'd drop define and ifdef for something that doesn't exists >> >> Conny requested it as we might see some work on that area (supporting >> smp) soon. So as long as there are no other opinions, I'll stick to the >> current version. >> >> Thanks! >> > I've just removed a bunch of TODO cpu models in PPC with > corresponding macro, my guess they were also introduced > with similar intent but ended up as dead code. > > So it's better to add new code when it actually is needed, > instead of just in case. > v1 of this patch had no ifdef at all. So I'll let Conny decide whether to keep it like this or whether to drop the ifdef again.
On Wed, 13 Sep 2017 14:10:53 +0200 David Hildenbrand <david@redhat.com> wrote: > On 13.09.2017 09:19, Igor Mammedov wrote: > > On Tue, 12 Sep 2017 17:42:35 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 12.09.2017 14:43, Igor Mammedov wrote: > >>> On Mon, 11 Sep 2017 17:21:40 +0200 > >>> David Hildenbrand <david@redhat.com> wrote: > >>> > >>>> Specifying more than 1 CPU (e.g. -smp 5) leads to SIGP errors (the > >>>> guest tries to bring these CPUs up but fails), because we don't support > >>>> multiple CPUs on s390x under TCG. > >>>> > >>>> Let's bail out if more than 1 is specified, so we don't raise people's > >>>> hope. Make it a define, so we can easily bump it up later. > >>>> > >>>> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>> --- > >>>> hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ > >>>> 1 file changed, 10 insertions(+) > >>>> > >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > >>>> index f67b4b5d58..f1198b2745 100644 > >>>> --- a/hw/s390x/s390-virtio-ccw.c > >>>> +++ b/hw/s390x/s390-virtio-ccw.c > >>>> @@ -23,6 +23,7 @@ > >>>> #include "hw/s390x/css.h" > >>>> #include "virtio-ccw.h" > >>>> #include "qemu/config-file.h" > >>>> +#include "qemu/error-report.h" > >>>> #include "s390-pci-bus.h" > >>>> #include "hw/s390x/storage-keys.h" > >>>> #include "hw/s390x/storage-attributes.h" > >>>> @@ -47,6 +48,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > >>>> return cpu_states[cpu_addr]; > >>>> } > >>>> > >>>> +/* #define S390_TCG_SMP_SUPPORT */ > >>> I'd drop define and ifdef for something that doesn't exists > >> > >> Conny requested it as we might see some work on that area (supporting > >> smp) soon. So as long as there are no other opinions, I'll stick to the > >> current version. > >> > >> Thanks! > >> > > I've just removed a bunch of TODO cpu models in PPC with > > corresponding macro, my guess they were also introduced > > with similar intent but ended up as dead code. > > > > So it's better to add new code when it actually is needed, > > instead of just in case. > > > v1 of this patch had no ifdef at all. So I'll let Conny decide whether > to keep it like this or whether to drop the ifdef again. You could add a comment? OTOH, this one hits immediately, so whoever implements smp for tcg will remember to change this :) So no real objection to leaving out the ifdef.
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index f67b4b5d58..f1198b2745 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -23,6 +23,7 @@ #include "hw/s390x/css.h" #include "virtio-ccw.h" #include "qemu/config-file.h" +#include "qemu/error-report.h" #include "s390-pci-bus.h" #include "hw/s390x/storage-keys.h" #include "hw/s390x/storage-attributes.h" @@ -47,6 +48,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) return cpu_states[cpu_addr]; } +/* #define S390_TCG_SMP_SUPPORT */ + static void s390_init_cpus(MachineState *machine) { int i; @@ -55,6 +58,13 @@ static void s390_init_cpus(MachineState *machine) if (machine->cpu_model == NULL) { machine->cpu_model = s390_default_cpu_model_name(); } +#ifndef S390_TCG_SMP_SUPPORT + if (tcg_enabled() && max_cpus > 1) { + error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " + "supported by TCG (1) on s390x", max_cpus); + exit(1); + } +#endif cpu_states = g_new0(S390CPU *, max_cpus);