cpu_s390x_init() is used only *-user targets indirectly
via cpu_init() macro and has a hack to assign ids to created
cpus (I'm not sure if 'id' really matters to *-user emulation).

So to on safe side, instead of having custom wrapper to do numbering
replace it with cpu_generic_init() and use S390CPUClass::next_cpu_id
which could serve the same purpose as static variable and move cpu->id
initialization to s390_cpu_initfn for CONFIG_USER_ONLY use-case.

PS:
ifdef is ugly but it allows us to hide s390x detail that isn't
set by *-user targets and reuse generic cpu creation utility
for btoh machine and user emulation.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
perhaps cpu->id isn't need by CONFIG_USER_ONLY but I'd leave to it
s390x maintainers to deal with it.

CC: Richard Henderson <rth@twiddle.net>
CC: Alexander Graf <agraf@suse.de>
CC: Cornelia Huck <cohuck@redhat.com>
CC: Thomas Huth <thuth@redhat.com>
---
 target/s390x/cpu.h    |  3 +--
 target/s390x/cpu.c    |  7 +++++++
 target/s390x/helper.c | 14 --------------
 3 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index bdb9bdb..b6d25c6 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -477,7 +477,6 @@ static inline bool get_per_in_range(CPUS390XState *env, uint64_t addr)
 void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
 #endif
 
-S390CPU *cpu_s390x_init(const char *cpu_model);
 S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
 S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
 void s390x_translate_init(void);
@@ -641,7 +640,7 @@ static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
 
 extern void subsystem_reset(void);
 
-#define cpu_init(model) CPU(cpu_s390x_init(model))
+#define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
 #define cpu_signal_handler cpu_s390x_signal_handler
 
 void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index accef03..e40a26f 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -303,6 +303,13 @@ static void s390_cpu_initfn(Object *obj)
         inited = true;
         s390x_translate_init();
     }
+
+#if defined(CONFIG_USER_ONLY)
+    {
+        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
+        cpu->id = scc->next_cpu_id;
+    }
+#endif
 }
 
 static void s390_cpu_finalize(Object *obj)
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index aef09e1..632b030 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -129,20 +129,6 @@ out:
     return cpu;
 }
 
-S390CPU *cpu_s390x_init(const char *cpu_model)
-{
-    Error *err = NULL;
-    S390CPU *cpu;
-    /* Use to track CPU ID for linux-user only */
-    static int64_t next_cpu_id;
-
-    cpu = s390x_new_cpu(cpu_model, next_cpu_id++, &err);
-    if (err) {
-        error_report_err(err);
-    }
-    return cpu;
-}
-
 #if defined(CONFIG_USER_ONLY)
 
 void s390_cpu_do_interrupt(CPUState *cs)
-- 
2.7.4

On Fri, 14 Jul 2017 15:52:02 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> cpu_s390x_init() is used only *-user targets indirectly
> via cpu_init() macro and has a hack to assign ids to created
> cpus (I'm not sure if 'id' really matters to *-user emulation).
> 
> So to on safe side, instead of having custom wrapper to do numbering
> replace it with cpu_generic_init() and use S390CPUClass::next_cpu_id
> which could serve the same purpose as static variable and move cpu->id
> initialization to s390_cpu_initfn for CONFIG_USER_ONLY use-case.
> 
> PS:
> ifdef is ugly but it allows us to hide s390x detail that isn't
> set by *-user targets and reuse generic cpu creation utility
> for btoh machine and user emulation.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> perhaps cpu->id isn't need by CONFIG_USER_ONLY but I'd leave to it
> s390x maintainers to deal with it.
> 
> CC: Richard Henderson <rth@twiddle.net>
> CC: Alexander Graf <agraf@suse.de>
> CC: Cornelia Huck <cohuck@redhat.com>
> CC: Thomas Huth <thuth@redhat.com>
> ---
>  target/s390x/cpu.h    |  3 +--
>  target/s390x/cpu.c    |  7 +++++++
>  target/s390x/helper.c | 14 --------------
>  3 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index bdb9bdb..b6d25c6 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -477,7 +477,6 @@ static inline bool get_per_in_range(CPUS390XState *env, uint64_t addr)
>  void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
>  #endif
>  
> -S390CPU *cpu_s390x_init(const char *cpu_model);
>  S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
>  S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
>  void s390x_translate_init(void);
> @@ -641,7 +640,7 @@ static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
>  
>  extern void subsystem_reset(void);
>  
> -#define cpu_init(model) CPU(cpu_s390x_init(model))
> +#define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
>  #define cpu_signal_handler cpu_s390x_signal_handler
>  
>  void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index accef03..e40a26f 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -303,6 +303,13 @@ static void s390_cpu_initfn(Object *obj)
>          inited = true;
>          s390x_translate_init();
>      }
> +
> +#if defined(CONFIG_USER_ONLY)
> +    {
> +        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
> +        cpu->id = scc->next_cpu_id;
> +    }
> +#endif

What about the null machine, which calls cpu_init() as well? Am I
missing something?

>  }
>  
>  static void s390_cpu_finalize(Object *obj)
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index aef09e1..632b030 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -129,20 +129,6 @@ out:
>      return cpu;
>  }
>  
> -S390CPU *cpu_s390x_init(const char *cpu_model)
> -{
> -    Error *err = NULL;
> -    S390CPU *cpu;
> -    /* Use to track CPU ID for linux-user only */
> -    static int64_t next_cpu_id;
> -
> -    cpu = s390x_new_cpu(cpu_model, next_cpu_id++, &err);
> -    if (err) {
> -        error_report_err(err);
> -    }
> -    return cpu;
> -}
> -
>  #if defined(CONFIG_USER_ONLY)
>  
>  void s390_cpu_do_interrupt(CPUState *cs)

On Tue, 18 Jul 2017 14:30:09 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 14 Jul 2017 15:52:02 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > cpu_s390x_init() is used only *-user targets indirectly
> > via cpu_init() macro and has a hack to assign ids to created
> > cpus (I'm not sure if 'id' really matters to *-user emulation).
> > 
> > So to on safe side, instead of having custom wrapper to do numbering
> > replace it with cpu_generic_init() and use S390CPUClass::next_cpu_id
> > which could serve the same purpose as static variable and move cpu->id
> > initialization to s390_cpu_initfn for CONFIG_USER_ONLY use-case.
> > 
> > PS:
> > ifdef is ugly but it allows us to hide s390x detail that isn't
> > set by *-user targets and reuse generic cpu creation utility
> > for btoh machine and user emulation.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > perhaps cpu->id isn't need by CONFIG_USER_ONLY but I'd leave to it
> > s390x maintainers to deal with it.
> > 
> > CC: Richard Henderson <rth@twiddle.net>
> > CC: Alexander Graf <agraf@suse.de>
> > CC: Cornelia Huck <cohuck@redhat.com>
> > CC: Thomas Huth <thuth@redhat.com>
> > ---
> >  target/s390x/cpu.h    |  3 +--
> >  target/s390x/cpu.c    |  7 +++++++
> >  target/s390x/helper.c | 14 --------------
> >  3 files changed, 8 insertions(+), 16 deletions(-)
> > 
> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index bdb9bdb..b6d25c6 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
> > @@ -477,7 +477,6 @@ static inline bool get_per_in_range(CPUS390XState *env, uint64_t addr)
> >  void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
> >  #endif
> >  
> > -S390CPU *cpu_s390x_init(const char *cpu_model);
> >  S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
> >  S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
> >  void s390x_translate_init(void);
> > @@ -641,7 +640,7 @@ static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
> >  
> >  extern void subsystem_reset(void);
> >  
> > -#define cpu_init(model) CPU(cpu_s390x_init(model))
> > +#define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
> >  #define cpu_signal_handler cpu_s390x_signal_handler
> >  
> >  void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> > index accef03..e40a26f 100644
> > --- a/target/s390x/cpu.c
> > +++ b/target/s390x/cpu.c
> > @@ -303,6 +303,13 @@ static void s390_cpu_initfn(Object *obj)
> >          inited = true;
> >          s390x_translate_init();
> >      }
> > +
> > +#if defined(CONFIG_USER_ONLY)
> > +    {
> > +        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
> > +        cpu->id = scc->next_cpu_id;
> > +    }
> > +#endif  
> 
> What about the null machine, which calls cpu_init() as well? Am I
> missing something?
it creates only 1 cpu so for it following will be true:
 cpu->id == 0 and also scc->next_cpu_id == 0 at s390_cpu_initfn() time
then at realize() time scc->next_cpu_id++ happens and no more cpus could
be created in case of null machine.

Considering no -smp support for null-machine, we shouldn't care about
multiple instantiations with cpu_init() there. 
If we would ever start caring about -smp there, then we should
explicitly create cpus with properly set properties like other boards
do.


> 
> >  }
> >  
> >  static void s390_cpu_finalize(Object *obj)
> > diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> > index aef09e1..632b030 100644
> > --- a/target/s390x/helper.c
> > +++ b/target/s390x/helper.c
> > @@ -129,20 +129,6 @@ out:
> >      return cpu;
> >  }
> >  
> > -S390CPU *cpu_s390x_init(const char *cpu_model)
> > -{
> > -    Error *err = NULL;
> > -    S390CPU *cpu;
> > -    /* Use to track CPU ID for linux-user only */
> > -    static int64_t next_cpu_id;
> > -
> > -    cpu = s390x_new_cpu(cpu_model, next_cpu_id++, &err);
> > -    if (err) {
> > -        error_report_err(err);
> > -    }
> > -    return cpu;
> > -}
> > -
> >  #if defined(CONFIG_USER_ONLY)
> >  
> >  void s390_cpu_do_interrupt(CPUState *cs)  
> 

On Tue, 18 Jul 2017 15:17:21 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 18 Jul 2017 14:30:09 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, 14 Jul 2017 15:52:02 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > cpu_s390x_init() is used only *-user targets indirectly
> > > via cpu_init() macro and has a hack to assign ids to created
> > > cpus (I'm not sure if 'id' really matters to *-user emulation).
> > > 
> > > So to on safe side, instead of having custom wrapper to do numbering
> > > replace it with cpu_generic_init() and use S390CPUClass::next_cpu_id
> > > which could serve the same purpose as static variable and move cpu->id
> > > initialization to s390_cpu_initfn for CONFIG_USER_ONLY use-case.
> > > 
> > > PS:
> > > ifdef is ugly but it allows us to hide s390x detail that isn't
> > > set by *-user targets and reuse generic cpu creation utility
> > > for btoh machine and user emulation.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > perhaps cpu->id isn't need by CONFIG_USER_ONLY but I'd leave to it
> > > s390x maintainers to deal with it.
> > > 
> > > CC: Richard Henderson <rth@twiddle.net>
> > > CC: Alexander Graf <agraf@suse.de>
> > > CC: Cornelia Huck <cohuck@redhat.com>
> > > CC: Thomas Huth <thuth@redhat.com>
> > > ---
> > >  target/s390x/cpu.h    |  3 +--
> > >  target/s390x/cpu.c    |  7 +++++++
> > >  target/s390x/helper.c | 14 --------------
> > >  3 files changed, 8 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > > index bdb9bdb..b6d25c6 100644
> > > --- a/target/s390x/cpu.h
> > > +++ b/target/s390x/cpu.h
> > > @@ -477,7 +477,6 @@ static inline bool get_per_in_range(CPUS390XState *env, uint64_t addr)
> > >  void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
> > >  #endif
> > >  
> > > -S390CPU *cpu_s390x_init(const char *cpu_model);
> > >  S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
> > >  S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
> > >  void s390x_translate_init(void);
> > > @@ -641,7 +640,7 @@ static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
> > >  
> > >  extern void subsystem_reset(void);
> > >  
> > > -#define cpu_init(model) CPU(cpu_s390x_init(model))
> > > +#define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
> > >  #define cpu_signal_handler cpu_s390x_signal_handler
> > >  
> > >  void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> > > index accef03..e40a26f 100644
> > > --- a/target/s390x/cpu.c
> > > +++ b/target/s390x/cpu.c
> > > @@ -303,6 +303,13 @@ static void s390_cpu_initfn(Object *obj)
> > >          inited = true;
> > >          s390x_translate_init();
> > >      }
> > > +
> > > +#if defined(CONFIG_USER_ONLY)
> > > +    {
> > > +        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
> > > +        cpu->id = scc->next_cpu_id;
> > > +    }
> > > +#endif    
> > 
> > What about the null machine, which calls cpu_init() as well? Am I
> > missing something?  
> it creates only 1 cpu so for it following will be true:
>  cpu->id == 0 and also scc->next_cpu_id == 0 at s390_cpu_initfn() time
> then at realize() time scc->next_cpu_id++ happens and no more cpus could
> be created in case of null machine.
> 
> Considering no -smp support for null-machine, we shouldn't care about
> multiple instantiations with cpu_init() there. 
> If we would ever start caring about -smp there, then we should
> explicitly create cpus with properly set properties like other boards
> do.
ping,

Cornelia,
considering my previous reply does this patch look fine to you?

> 
> >   
> > >  }
> > >  
> > >  static void s390_cpu_finalize(Object *obj)
> > > diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> > > index aef09e1..632b030 100644
> > > --- a/target/s390x/helper.c
> > > +++ b/target/s390x/helper.c
> > > @@ -129,20 +129,6 @@ out:
> > >      return cpu;
> > >  }
> > >  
> > > -S390CPU *cpu_s390x_init(const char *cpu_model)
> > > -{
> > > -    Error *err = NULL;
> > > -    S390CPU *cpu;
> > > -    /* Use to track CPU ID for linux-user only */
> > > -    static int64_t next_cpu_id;
> > > -
> > > -    cpu = s390x_new_cpu(cpu_model, next_cpu_id++, &err);
> > > -    if (err) {
> > > -        error_report_err(err);
> > > -    }
> > > -    return cpu;
> > > -}
> > > -
> > >  #if defined(CONFIG_USER_ONLY)
> > >  
> > >  void s390_cpu_do_interrupt(CPUState *cs)    
> >   
> 
> 

On Mon, 14 Aug 2017 10:03:48 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 18 Jul 2017 15:17:21 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Tue, 18 Jul 2017 14:30:09 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Fri, 14 Jul 2017 15:52:02 +0200
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > >     
> > > > cpu_s390x_init() is used only *-user targets indirectly
> > > > via cpu_init() macro and has a hack to assign ids to created
> > > > cpus (I'm not sure if 'id' really matters to *-user emulation).
> > > > 
> > > > So to on safe side, instead of having custom wrapper to do numbering
> > > > replace it with cpu_generic_init() and use S390CPUClass::next_cpu_id
> > > > which could serve the same purpose as static variable and move cpu->id
> > > > initialization to s390_cpu_initfn for CONFIG_USER_ONLY use-case.
> > > > 
> > > > PS:
> > > > ifdef is ugly but it allows us to hide s390x detail that isn't
> > > > set by *-user targets and reuse generic cpu creation utility
> > > > for btoh machine and user emulation.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > perhaps cpu->id isn't need by CONFIG_USER_ONLY but I'd leave to it
> > > > s390x maintainers to deal with it.
> > > > 
> > > > CC: Richard Henderson <rth@twiddle.net>
> > > > CC: Alexander Graf <agraf@suse.de>
> > > > CC: Cornelia Huck <cohuck@redhat.com>
> > > > CC: Thomas Huth <thuth@redhat.com>
> > > > ---
> > > >  target/s390x/cpu.h    |  3 +--
> > > >  target/s390x/cpu.c    |  7 +++++++
> > > >  target/s390x/helper.c | 14 --------------
> > > >  3 files changed, 8 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > > > index bdb9bdb..b6d25c6 100644
> > > > --- a/target/s390x/cpu.h
> > > > +++ b/target/s390x/cpu.h
> > > > @@ -477,7 +477,6 @@ static inline bool get_per_in_range(CPUS390XState *env, uint64_t addr)
> > > >  void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
> > > >  #endif
> > > >  
> > > > -S390CPU *cpu_s390x_init(const char *cpu_model);
> > > >  S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
> > > >  S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
> > > >  void s390x_translate_init(void);
> > > > @@ -641,7 +640,7 @@ static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
> > > >  
> > > >  extern void subsystem_reset(void);
> > > >  
> > > > -#define cpu_init(model) CPU(cpu_s390x_init(model))
> > > > +#define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
> > > >  #define cpu_signal_handler cpu_s390x_signal_handler
> > > >  
> > > >  void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> > > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> > > > index accef03..e40a26f 100644
> > > > --- a/target/s390x/cpu.c
> > > > +++ b/target/s390x/cpu.c
> > > > @@ -303,6 +303,13 @@ static void s390_cpu_initfn(Object *obj)
> > > >          inited = true;
> > > >          s390x_translate_init();
> > > >      }
> > > > +
> > > > +#if defined(CONFIG_USER_ONLY)
> > > > +    {
> > > > +        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
> > > > +        cpu->id = scc->next_cpu_id;
> > > > +    }
> > > > +#endif      
> > > 
> > > What about the null machine, which calls cpu_init() as well? Am I
> > > missing something?    
> > it creates only 1 cpu so for it following will be true:
> >  cpu->id == 0 and also scc->next_cpu_id == 0 at s390_cpu_initfn() time
> > then at realize() time scc->next_cpu_id++ happens and no more cpus could
> > be created in case of null machine.
> > 
> > Considering no -smp support for null-machine, we shouldn't care about
> > multiple instantiations with cpu_init() there. 
> > If we would ever start caring about -smp there, then we should
> > explicitly create cpus with properly set properties like other boards
> > do.  
> ping,
> 
> Cornelia,
> considering my previous reply does this patch look fine to you?

Sorry, that one fell through the cracks.

We may want to revisit this later; but for now, it should be fine.

I assume you want to merge this as a series? In that case,

Acked-by: Cornelia Huck <cohuck@redhat.com>

On Mon, 14 Aug 2017 10:53:37 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, 14 Aug 2017 10:03:48 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Tue, 18 Jul 2017 15:17:21 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > On Tue, 18 Jul 2017 14:30:09 +0200
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >     
> > > > On Fri, 14 Jul 2017 15:52:02 +0200
> > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > >       
> > > > > cpu_s390x_init() is used only *-user targets indirectly
> > > > > via cpu_init() macro and has a hack to assign ids to created
> > > > > cpus (I'm not sure if 'id' really matters to *-user emulation).
> > > > > 
> > > > > So to on safe side, instead of having custom wrapper to do numbering
> > > > > replace it with cpu_generic_init() and use S390CPUClass::next_cpu_id
> > > > > which could serve the same purpose as static variable and move cpu->id
> > > > > initialization to s390_cpu_initfn for CONFIG_USER_ONLY use-case.
> > > > > 
> > > > > PS:
> > > > > ifdef is ugly but it allows us to hide s390x detail that isn't
> > > > > set by *-user targets and reuse generic cpu creation utility
> > > > > for btoh machine and user emulation.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > > perhaps cpu->id isn't need by CONFIG_USER_ONLY but I'd leave to it
> > > > > s390x maintainers to deal with it.
> > > > > 
> > > > > CC: Richard Henderson <rth@twiddle.net>
> > > > > CC: Alexander Graf <agraf@suse.de>
> > > > > CC: Cornelia Huck <cohuck@redhat.com>
> > > > > CC: Thomas Huth <thuth@redhat.com>
> > > > > ---
> > > > >  target/s390x/cpu.h    |  3 +--
> > > > >  target/s390x/cpu.c    |  7 +++++++
> > > > >  target/s390x/helper.c | 14 --------------
> > > > >  3 files changed, 8 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > > > > index bdb9bdb..b6d25c6 100644
> > > > > --- a/target/s390x/cpu.h
> > > > > +++ b/target/s390x/cpu.h
> > > > > @@ -477,7 +477,6 @@ static inline bool get_per_in_range(CPUS390XState *env, uint64_t addr)
> > > > >  void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
> > > > >  #endif
> > > > >  
> > > > > -S390CPU *cpu_s390x_init(const char *cpu_model);
> > > > >  S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp);
> > > > >  S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp);
> > > > >  void s390x_translate_init(void);
> > > > > @@ -641,7 +640,7 @@ static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
> > > > >  
> > > > >  extern void subsystem_reset(void);
> > > > >  
> > > > > -#define cpu_init(model) CPU(cpu_s390x_init(model))
> > > > > +#define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
> > > > >  #define cpu_signal_handler cpu_s390x_signal_handler
> > > > >  
> > > > >  void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> > > > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> > > > > index accef03..e40a26f 100644
> > > > > --- a/target/s390x/cpu.c
> > > > > +++ b/target/s390x/cpu.c
> > > > > @@ -303,6 +303,13 @@ static void s390_cpu_initfn(Object *obj)
> > > > >          inited = true;
> > > > >          s390x_translate_init();
> > > > >      }
> > > > > +
> > > > > +#if defined(CONFIG_USER_ONLY)
> > > > > +    {
> > > > > +        S390CPUClass *scc = S390_CPU_GET_CLASS(obj);
> > > > > +        cpu->id = scc->next_cpu_id;
> > > > > +    }
> > > > > +#endif        
> > > > 
> > > > What about the null machine, which calls cpu_init() as well? Am I
> > > > missing something?      
> > > it creates only 1 cpu so for it following will be true:
> > >  cpu->id == 0 and also scc->next_cpu_id == 0 at s390_cpu_initfn() time
> > > then at realize() time scc->next_cpu_id++ happens and no more cpus could
> > > be created in case of null machine.
> > > 
> > > Considering no -smp support for null-machine, we shouldn't care about
> > > multiple instantiations with cpu_init() there. 
> > > If we would ever start caring about -smp there, then we should
> > > explicitly create cpus with properly set properties like other boards
> > > do.    
> > ping,
> > 
> > Cornelia,
> > considering my previous reply does this patch look fine to you?  
> 
> Sorry, that one fell through the cracks.
> 
> We may want to revisit this later; but for now, it should be fine.
I didn't look at possibility of refactoring next_cpu_id usage as
it's out of scope of this series. But from my understanding, it's been
introduced to support legacy cpu_add interface and enforce sequential
hotplug of CPUs when device_add CPU infrastructure hasn't been ready
yet. Perhaps S390 gurus could now refactor CPU hotplug to device_add
interface and get rid of next_cpu_id in process.

> 
> I assume you want to merge this as a series? In that case,
> 
> Acked-by: Cornelia Huck <cohuck@redhat.com>
Thanks!

On Mon, 14 Aug 2017 11:24:07 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> I didn't look at possibility of refactoring next_cpu_id usage as
> it's out of scope of this series. But from my understanding, it's been
> introduced to support legacy cpu_add interface and enforce sequential
> hotplug of CPUs when device_add CPU infrastructure hasn't been ready
> yet. Perhaps S390 gurus could now refactor CPU hotplug to device_add
> interface and get rid of next_cpu_id in process.

Yup, much potential there. I won't mind if someone wants to look at
this.