| Message ID | 20170904154316.4148-12-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/04/2017 11:43 AM, 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 is specified, so we don't raise people's > hope. Make it a define, so we can easily bump it up later. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- Makes sense. Ran the described environment without this patch (errors) and again with this patch (graceful exit w/ message). Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > hw/s390x/s390-virtio-ccw.c | 7 +++++++ > target/s390x/cpu.h | 2 ++ > 2 files changed, 9 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index f67b4b5d58..f7ca20d77a 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" > @@ -55,6 +56,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); > + } > > cpu_states = g_new0(S390CPU *, max_cpus); > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 147aceba28..dca6aa9aae 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -209,6 +209,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 09/06/2017 11:16 AM, Matthew Rosato wrote: > On 09/04/2017 11:43 AM, 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 is specified, so we don't raise people's >> hope. Make it a define, so we can easily bump it up later. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- > > Makes sense. Ran the described environment without this patch (errors) > and again with this patch (graceful exit w/ message). > > Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> Can someone review http://patchwork.ozlabs.org/patch/760010/ which does at least start to add the SIGP support. Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N. I don't see the point of the define. r~
On 06.09.2017 23:20, Richard Henderson wrote: > On 09/06/2017 11:16 AM, Matthew Rosato wrote: >> On 09/04/2017 11:43 AM, 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 is specified, so we don't raise people's >>> hope. Make it a define, so we can easily bump it up later. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >> >> Makes sense. Ran the described environment without this patch (errors) >> and again with this patch (graceful exit w/ message). >> >> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > > Can someone review > > http://patchwork.ozlabs.org/patch/760010/ > Yes, we also discovered that patch during review of v1. > which does at least start to add the SIGP support. > > Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N. I don't > see the point of the define. > Conny requested a define. So it boils down to a) no define just as in v1 b) a define like S390_TCG_SMP_SUPPORTED What do you suggest? > > r~ >
On Thu, 7 Sep 2017 14:49:49 +0200 David Hildenbrand <david@redhat.com> wrote: > On 06.09.2017 23:20, Richard Henderson wrote: > > On 09/06/2017 11:16 AM, Matthew Rosato wrote: > >> On 09/04/2017 11:43 AM, 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 is specified, so we don't raise people's > >>> hope. Make it a define, so we can easily bump it up later. > >>> > >>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>> --- > >> > >> Makes sense. Ran the described environment without this patch (errors) > >> and again with this patch (graceful exit w/ message). > >> > >> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > > > > Can someone review > > > > http://patchwork.ozlabs.org/patch/760010/ > > > > Yes, we also discovered that patch during review of v1. > > > which does at least start to add the SIGP support. > > > > Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N. I don't > > see the point of the define. > > > > Conny requested a define. So it boils down to > > a) no define just as in v1 > b) a define like S390_TCG_SMP_SUPPORTED > > What do you suggest? FWIW, I'd prefer b). > > > > > r~ > > > >
On 06.09.2017 23:20, Richard Henderson wrote: > On 09/06/2017 11:16 AM, Matthew Rosato wrote: >> On 09/04/2017 11:43 AM, 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 is specified, so we don't raise people's >>> hope. Make it a define, so we can easily bump it up later. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >> >> Makes sense. Ran the described environment without this patch (errors) >> and again with this patch (graceful exit w/ message). >> >> Tested-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > > Can someone review > > http://patchwork.ozlabs.org/patch/760010/ > > which does at least start to add the SIGP support. FWIW, I started factoring out today KVM SIGP code to make it usable by TCG. I also started adding the missing SIGP instructions the kernel handles for KVM. I dropped the old TCG SIGP handling code and completely reuse the new SIGP code. I already got boot/reboot/shutdown properly running (implementing STOP and RESTART interrupts like KVM has). But its still quite hacky and there are is a bunch of stuff to clean up, especially: - external interrupt handling (the queue approach we have right now is no good for external calls and emergency signals) - floating interrupt support (io interrupts always going to CPU 0 is a hack) I think I can at least implement SIGP properly and fix the external call stuff. floating interrupts might require more thought. Aurelien, please tell me if you are currently still working on this, so we can coordinate. Thanks! > > Once tcg can bring up 2 cpus, I see no reason it couldn't bring up N. I don't > see the point of the define. > > > r~ >
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index f67b4b5d58..f7ca20d77a 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" @@ -55,6 +56,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); + } cpu_states = g_new0(S390CPU *, max_cpus); diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 147aceba28..dca6aa9aae 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -209,6 +209,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
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. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/s390-virtio-ccw.c | 7 +++++++ target/s390x/cpu.h | 2 ++ 2 files changed, 9 insertions(+)