| Message ID | 20170830170601.15855-9-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | next round of s390x cleanups | expand |
On 30.08.2017 19:05, David Hildenbrand 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 are specified, so we don't raise people's > hope. Aurelien recently posted a patch to add that basic SIGP support: https://patchwork.kernel.org/patch/9717489/ I think it would make more sense to get that included instead. Thomas
On Wed, 30 Aug 2017 21:06:55 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 30.08.2017 19:05, David Hildenbrand 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 are specified, so we don't raise people's > > hope. > > Aurelien recently posted a patch to add that basic SIGP support: > > https://patchwork.kernel.org/patch/9717489/ > > I think it would make more sense to get that included instead. I'd look at it if it were reposted :)
On 30.08.2017 21:06, Thomas Huth wrote: > On 30.08.2017 19:05, David Hildenbrand 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 are specified, so we don't raise people's >> hope. > > Aurelien recently posted a patch to add that basic SIGP support: > > https://patchwork.kernel.org/patch/9717489/ > > I think it would make more sense to get that included instead. > > Thomas > Even then, it doesn't work reliably: - "*very rough* SMP support" - "this patch is nothing more than a way to determine what needs to be implemented" - "It should be rewritten from scratch before reaching in an acceptable state." Such broken feature should not be exposed to the user. Once we have properly fixed that we can enable and announce it "s390x now supports more than 1 VCPU under TCG". At this point, this is just wrong, even with this patch included. So I'd suggest including this now and reverting it once we have actual support. "so we don't raise people's hope."
On Wed, 30 Aug 2017 19:05:58 +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 are specified, so we don't raise people's > hope. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 7754e3eaf9..eff96808c4 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" > @@ -56,6 +57,11 @@ static void s390_init_cpus(MachineState *machine) > if (machine->cpu_model == NULL) { > machine->cpu_model = s390_default_cpu_model_name(); > } > + 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); Make this a #define, so we can just flip the switch when smp support is ready? > + exit(1); > + } > > ms->cpus = g_new0(S390CPU *, max_cpus); >
On 31.08.2017 16:41, Cornelia Huck wrote: > On Wed, 30 Aug 2017 19:05:58 +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 are specified, so we don't raise people's >> hope. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 7754e3eaf9..eff96808c4 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" >> @@ -56,6 +57,11 @@ static void s390_init_cpus(MachineState *machine) >> if (machine->cpu_model == NULL) { >> machine->cpu_model = s390_default_cpu_model_name(); >> } >> + 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); > > Make this a #define, so we can just flip the switch when smp support is > ready? As an alternative: yield a warning? > >> + exit(1); >> + } >> >> ms->cpus = g_new0(S390CPU *, max_cpus); >> >
On Thu, 31 Aug 2017 17:03:21 +0200 David Hildenbrand <david@redhat.com> wrote: > On 31.08.2017 16:41, Cornelia Huck wrote: > > On Wed, 30 Aug 2017 19:05:58 +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 are specified, so we don't raise people's > >> hope. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> hw/s390x/s390-virtio-ccw.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > >> index 7754e3eaf9..eff96808c4 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" > >> @@ -56,6 +57,11 @@ static void s390_init_cpus(MachineState *machine) > >> if (machine->cpu_model == NULL) { > >> machine->cpu_model = s390_default_cpu_model_name(); > >> } > >> + 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); > > > > Make this a #define, so we can just flip the switch when smp support is > > ready? > > As an alternative: yield a warning? If we know that this can't work, it makes sense to stop immediately, no? > > > > >> + exit(1); > >> + } > >> > >> ms->cpus = g_new0(S390CPU *, max_cpus); > >> > > > >
On 31.08.2017 17:07, Cornelia Huck wrote: > On Thu, 31 Aug 2017 17:03:21 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 31.08.2017 16:41, Cornelia Huck wrote: >>> On Wed, 30 Aug 2017 19:05:58 +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 are specified, so we don't raise people's >>>> hope. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index 7754e3eaf9..eff96808c4 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" >>>> @@ -56,6 +57,11 @@ static void s390_init_cpus(MachineState *machine) >>>> if (machine->cpu_model == NULL) { >>>> machine->cpu_model = s390_default_cpu_model_name(); >>>> } >>>> + 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); >>> >>> Make this a #define, so we can just flip the switch when smp support is >>> ready? >> >> As an alternative: yield a warning? > > If we know that this can't work, it makes sense to stop immediately, no? Hmm, like that than? diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 18ed0c57e3..dae848fa5f 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" @@ -56,6 +57,12 @@ static void s390_init_cpus(MachineState *machine) if (machine->cpu_model == NULL) { machine->cpu_model = s390_default_cpu_model_name(); } + if (tcg_enabled() && max_cpus > S390_TCG_MAX_CPUS) { + error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " + "supported by TCG (%d) on s390x", max_cpus, + S390_TCG_MAX_CPUS); + exit(1); + } ms->cpus = g_new0(S390CPU *, max_cpus); diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 7ed9103b33..4e1de2102d 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -210,6 +210,8 @@ static inline S390CPU *s390_env_get_cpu(CPUS390XState *env) #define ENV_OFFSET offsetof(S390CPU, env) +#define S390_TCG_MAX_CPUS 1 + #ifndef CONFIG_USER_ONLY extern const struct VMStateDescription vmstate_s390_cpu; #endif
On Thu, 31 Aug 2017 17:33:13 +0200 David Hildenbrand <david@redhat.com> wrote: > On 31.08.2017 17:07, Cornelia Huck wrote: > > If we know that this can't work, it makes sense to stop immediately, no? > > Hmm, like that than? > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 18ed0c57e3..dae848fa5f 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" > @@ -56,6 +57,12 @@ static void s390_init_cpus(MachineState *machine) > if (machine->cpu_model == NULL) { > machine->cpu_model = s390_default_cpu_model_name(); > } > + if (tcg_enabled() && max_cpus > S390_TCG_MAX_CPUS) { > + error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " > + "supported by TCG (%d) on s390x", max_cpus, > + S390_TCG_MAX_CPUS); > + exit(1); > + } > > ms->cpus = g_new0(S390CPU *, max_cpus); > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 7ed9103b33..4e1de2102d 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -210,6 +210,8 @@ static inline S390CPU > *s390_env_get_cpu(CPUS390XState *env) > > #define ENV_OFFSET offsetof(S390CPU, env) > > +#define S390_TCG_MAX_CPUS 1 > + > #ifndef CONFIG_USER_ONLY > extern const struct VMStateDescription vmstate_s390_cpu; > #endif > > Looks good!
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 7754e3eaf9..eff96808c4 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" @@ -56,6 +57,11 @@ static void s390_init_cpus(MachineState *machine) if (machine->cpu_model == NULL) { machine->cpu_model = s390_default_cpu_model_name(); } + 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); + } ms->cpus = g_new0(S390CPU *, max_cpus);
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 are specified, so we don't raise people's hope. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/s390-virtio-ccw.c | 6 ++++++ 1 file changed, 6 insertions(+)