Re: [Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[Cornelia Huck]

On Thu, 31 Aug 2017 17:33:13 +0200
David Hildenbrand  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!

Re: [Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

On 31.08.2017 17:07, Cornelia Huck wrote:
> On Thu, 31 Aug 2017 17:03:21 +0200
> David Hildenbrand  wrote:
> 
>> On 31.08.2017 16:41, Cornelia Huck wrote:
>>> On Wed, 30 Aug 2017 19:05:58 +0200
>>> 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.

 Signed-off-by: David Hildenbrand 
 ---
  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


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[Cornelia Huck]

On Thu, 31 Aug 2017 17:03:21 +0200
David Hildenbrand  wrote:

> On 31.08.2017 16:41, Cornelia Huck wrote:
> > On Wed, 30 Aug 2017 19:05:58 +0200
> > 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.
> >>
> >> Signed-off-by: David Hildenbrand 
> >> ---
> >>  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);
> >>
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

On 31.08.2017 16:41, Cornelia Huck wrote:
> On Wed, 30 Aug 2017 19:05:58 +0200
> 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.
>>
>> Signed-off-by: David Hildenbrand 
>> ---
>>  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);
>>  
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[Cornelia Huck]

On Wed, 30 Aug 2017 19:05:58 +0200
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.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  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);
>  

Re: [Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

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."

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[Cornelia Huck]

On Wed, 30 Aug 2017 21:06:55 +0200
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.

I'd look at it if it were reposted :)

Re: [Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[Thomas Huth]

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

[Qemu-devel] [PATCH v1 08/11] s390x: allow only 1 CPU with TCG

[David Hildenbrand]

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 
---
 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);
+exit(1);
+}
 
 ms->cpus = g_new0(S390CPU *, max_cpus);
 
-- 
2.13.5