Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Igor Mammedov]

On Mon, 7 Mar 2016 12:49:27 +0100
Cornelia Huck  wrote:

> On Mon, 7 Mar 2016 11:12:14 +0100
> Igor Mammedov  wrote:
> 
> > On Mon, 7 Mar 2016 11:02:11 +0100
> > David Hildenbrand  wrote:
> >   
> > > > > After all the discussions about
> > > > > -device-add s390-cpu,id=XX
> > > > > 
> > > > > As substitute/addition in the future for hotplug it is the 
> > > > > straightforward
> > > > > approach to allow setting the id as property. Nobody knows what crazy 
> > > > > new
> > > > > hotplug method we will come up with. But doing it the device way with 
> > > > > properties
> > > > > cannot be wrong. And the id is a fundamental concept of a vcpu 
> > > > > (cpu-add id=XX).  
> > > > with device_add 'id' is not a vcpu concept but and arbitrary user 
> > > > supplied string
> > > > property owned by Device. But since s390 matches current x86 thread 
> > > > based model it could be migrated to device_add the same way, for 
> > > > example:
> > > >   device_add s390-cpu,thread=XX
> > > 
> > > So should we name the property thread then?
> > > Looks like the id property is really special.
> > > 
> > > What do you suggest?  
> > I plan to add 'thread' property to x86-cpu, so you could the same for
> > s390 when the time for device_add comes there.  
> 
> So the conclusion is to simply deal with this later, right?
I'd say so.

> 
> If so, I'll just go ahead and apply v9 :)
> 

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Cornelia Huck]

On Mon, 7 Mar 2016 11:12:14 +0100
Igor Mammedov  wrote:

> On Mon, 7 Mar 2016 11:02:11 +0100
> David Hildenbrand  wrote:
> 
> > > > After all the discussions about
> > > > -device-add s390-cpu,id=XX
> > > > 
> > > > As substitute/addition in the future for hotplug it is the 
> > > > straightforward
> > > > approach to allow setting the id as property. Nobody knows what crazy 
> > > > new
> > > > hotplug method we will come up with. But doing it the device way with 
> > > > properties
> > > > cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add 
> > > > id=XX).
> > > with device_add 'id' is not a vcpu concept but and arbitrary user 
> > > supplied string
> > > property owned by Device. But since s390 matches current x86 thread based 
> > > model it could be migrated to device_add the same way, for example:
> > >   device_add s390-cpu,thread=XX  
> > 
> > So should we name the property thread then?
> > Looks like the id property is really special.
> > 
> > What do you suggest?
> I plan to add 'thread' property to x86-cpu, so you could the same for
> s390 when the time for device_add comes there.

So the conclusion is to simply deal with this later, right?

If so, I'll just go ahead and apply v9 :)

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Igor Mammedov]

On Mon, 7 Mar 2016 11:02:11 +0100
David Hildenbrand  wrote:

> > > After all the discussions about
> > > -device-add s390-cpu,id=XX
> > > 
> > > As substitute/addition in the future for hotplug it is the straightforward
> > > approach to allow setting the id as property. Nobody knows what crazy new
> > > hotplug method we will come up with. But doing it the device way with 
> > > properties
> > > cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add 
> > > id=XX).
> > with device_add 'id' is not a vcpu concept but and arbitrary user supplied 
> > string
> > property owned by Device. But since s390 matches current x86 thread based 
> > model it could be migrated to device_add the same way, for example:
> >   device_add s390-cpu,thread=XX  
> 
> So should we name the property thread then?
> Looks like the id property is really special.
> 
> What do you suggest?
I plan to add 'thread' property to x86-cpu, so you could the same for
s390 when the time for device_add comes there.

> 
> >   
> > > 
> > > So I'd like to avoid reworking everything again, to realize later that we
> > > want it as a property and rewriting it once again.
> > for s390, the thing about not rewriting everything once again could be
> > replacing places where cpu_index is used with CpuClass.arch_id().
> > arch_id() defaults to cpu_index but you can later override it with
> > your own id (whatever s390 uses for identifying cpus on baremetal)
> > so switching to device_add won't break anything.  
> 
> Okay, this way we could get rid of cpu_index later.
> 
> David
> 
> 

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> > After all the discussions about
> > -device-add s390-cpu,id=XX
> > 
> > As substitute/addition in the future for hotplug it is the straightforward
> > approach to allow setting the id as property. Nobody knows what crazy new
> > hotplug method we will come up with. But doing it the device way with 
> > properties
> > cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add 
> > id=XX).  
> with device_add 'id' is not a vcpu concept but and arbitrary user supplied 
> string
> property owned by Device. But since s390 matches current x86 thread based 
> model it could be migrated to device_add the same way, for example:
>   device_add s390-cpu,thread=XX

So should we name the property thread then?
Looks like the id property is really special.

What do you suggest?

> 
> > 
> > So I'd like to avoid reworking everything again, to realize later that we
> > want it as a property and rewriting it once again.  
> for s390, the thing about not rewriting everything once again could be
> replacing places where cpu_index is used with CpuClass.arch_id().
> arch_id() defaults to cpu_index but you can later override it with
> your own id (whatever s390 uses for identifying cpus on baremetal)
> so switching to device_add won't break anything.

Okay, this way we could get rid of cpu_index later.

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Igor Mammedov]

On Fri, 4 Mar 2016 12:50:05 +0100
David Hildenbrand  wrote:

> > On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:  
> > > 
> > > > >  cpu_exec_init(cs, );
> > > > >  if (err != NULL) {
> > > > >  error_propagate(errp, err);
> > > > >  return;
> > > > >  }
> > > > > +scc->next_cpu_id = cs->cpu_index + 1;  
> > > > 
> > > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of 
> > > > arch_id
> > > > for you. If it is just going to be monotonically increasing like 
> > > > cs->cpu_index,
> > > > couldn't you just use cs->cpu_index instead of introducing additional 
> > > > IDs ?
> > > > 
> > > > Note that cpu_exec_init(cs, ) returns with the next available 
> > > > cpu_index
> > > > which can be compared against max_cpus directly.
> > > > 
> > > > Regards,
> > > > Bharata.
> > > 
> > > I don't think that we should mix the id setting and cpu_index for now.
> > > 
> > > We can't simply set cpu_index before the device is realized. That logic
> > > belongs to cpu_exec_init().
> > 
> > Yes, I see that, but apart from the following obvious uses of the id
> > property from realizefn, are there other uses ?
> > 
> > 1 Checking against max_cpus
> >   cpu_index can be used for this.
> > 
> > 2 Checking if cpu with such an id exists
> >   cpu_exec_init() would never return with an in-use index. Hence cpu_index
> >   can be used here too given that you don't define ->get_arch_id()
> > 
> > 3 Checking if the id is the next expected one
> >   cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
> >   always have the next available id to be used in cpu_index.
> > 
> > Did I miss any other use other than these which makes it necessary to have
> > an explicit id property here ?  
> 
> After all the discussions about
> -device-add s390-cpu,id=XX
> 
> As substitute/addition in the future for hotplug it is the straightforward
> approach to allow setting the id as property. Nobody knows what crazy new
> hotplug method we will come up with. But doing it the device way with 
> properties
> cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add 
> id=XX).
with device_add 'id' is not a vcpu concept but and arbitrary user supplied 
string
property owned by Device. But since s390 matches current x86 thread based model 
it could be migrated to device_add the same way, for example:
  device_add s390-cpu,thread=XX

> 
> So I'd like to avoid reworking everything again, to realize later that we
> want it as a property and rewriting it once again.
for s390, the thing about not rewriting everything once again could be
replacing places where cpu_index is used with CpuClass.arch_id().
arch_id() defaults to cpu_index but you can later override it with
your own id (whatever s390 uses for identifying cpus on baremetal)
so switching to device_add won't break anything.


> 
> David
> 

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> >   
> > > >  cpu_exec_init(cs, );
> > > >  if (err != NULL) {
> > > >  error_propagate(errp, err);
> > > >  return;
> > > >  }
> > > > +scc->next_cpu_id = cs->cpu_index + 1;
> > > 
> > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of 
> > > arch_id
> > > for you. If it is just going to be monotonically increasing like 
> > > cs->cpu_index,
> > > couldn't you just use cs->cpu_index instead of introducing additional IDs 
> > > ?
> > > 
> > > Note that cpu_exec_init(cs, ) returns with the next available 
> > > cpu_index
> > > which can be compared against max_cpus directly.
> > > 
> > > Regards,
> > > Bharata.  
> > 
> > I don't think that we should mix the id setting and cpu_index for now.
> > 
> > We can't simply set cpu_index before the device is realized. That logic
> > belongs to cpu_exec_init().  
> 
> Yes, I see that, but apart from the following obvious uses of the id
> property from realizefn, are there other uses ?
> 
> 1 Checking against max_cpus
>   cpu_index can be used for this.
> 
> 2 Checking if cpu with such an id exists
>   cpu_exec_init() would never return with an in-use index. Hence cpu_index
>   can be used here too given that you don't define ->get_arch_id()
> 
> 3 Checking if the id is the next expected one
>   cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
>   always have the next available id to be used in cpu_index.
> 
> Did I miss any other use other than these which makes it necessary to have
> an explicit id property here ?

After all the discussions about
-device-add s390-cpu,id=XX

As substitute/addition in the future for hotplug it is the straightforward
approach to allow setting the id as property. Nobody knows what crazy new
hotplug method we will come up with. But doing it the device way with properties
cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).

So I'd like to avoid reworking everything again, to realize later that we
want it as a property and rewriting it once again.

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Bharata B Rao]

On Fri, Mar 04, 2016 at 05:01:29PM +0530, Bharata B Rao wrote:
> On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> > 
> > > >  cpu_exec_init(cs, );
> > > >  if (err != NULL) {
> > > >  error_propagate(errp, err);
> > > >  return;
> > > >  }
> > > > +scc->next_cpu_id = cs->cpu_index + 1;  
> > > 
> > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of 
> > > arch_id
> > > for you. If it is just going to be monotonically increasing like 
> > > cs->cpu_index,
> > > couldn't you just use cs->cpu_index instead of introducing additional IDs 
> > > ?
> > > 
> > > Note that cpu_exec_init(cs, ) returns with the next available 
> > > cpu_index
> > > which can be compared against max_cpus directly.
> > > 
> > > Regards,
> > > Bharata.
> > 
> > I don't think that we should mix the id setting and cpu_index for now.
> > 
> > We can't simply set cpu_index before the device is realized. That logic
> > belongs to cpu_exec_init().
> 
> Yes, I see that, but apart from the following obvious uses of the id
> property from realizefn, are there other uses ?
> 
> 1 Checking against max_cpus
>   cpu_index can be used for this.
> 
> 2 Checking if cpu with such an id exists
>   cpu_exec_init() would never return with an in-use index. Hence cpu_index
>   can be used here too given that you don't define ->get_arch_id()
> 
> 3 Checking if the id is the next expected one
>   cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
>   always have the next available id to be used in cpu_index.
> 
> Did I miss any other use other than these which makes it necessary to have
> an explicit id property here ?

Oops, Sorry this is not device_add, but cpu-add that gets the id from the
user. So ignore the above :(

> 
> Regards,
> Bharata.

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Bharata B Rao]

On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> 
> > >  cpu_exec_init(cs, );
> > >  if (err != NULL) {
> > >  error_propagate(errp, err);
> > >  return;
> > >  }
> > > +scc->next_cpu_id = cs->cpu_index + 1;  
> > 
> > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
> > for you. If it is just going to be monotonically increasing like 
> > cs->cpu_index,
> > couldn't you just use cs->cpu_index instead of introducing additional IDs ?
> > 
> > Note that cpu_exec_init(cs, ) returns with the next available cpu_index
> > which can be compared against max_cpus directly.
> > 
> > Regards,
> > Bharata.
> 
> I don't think that we should mix the id setting and cpu_index for now.
> 
> We can't simply set cpu_index before the device is realized. That logic
> belongs to cpu_exec_init().

Yes, I see that, but apart from the following obvious uses of the id
property from realizefn, are there other uses ?

1 Checking against max_cpus
  cpu_index can be used for this.

2 Checking if cpu with such an id exists
  cpu_exec_init() would never return with an in-use index. Hence cpu_index
  can be used here too given that you don't define ->get_arch_id()

3 Checking if the id is the next expected one
  cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
  always have the next available id to be used in cpu_index.

Did I miss any other use other than these which makes it necessary to have
an explicit id property here ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> >  cpu_exec_init(cs, );
> >  if (err != NULL) {
> >  error_propagate(errp, err);
> >  return;
> >  }
> > +scc->next_cpu_id = cs->cpu_index + 1;  
> 
> It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
> for you. If it is just going to be monotonically increasing like 
> cs->cpu_index,
> couldn't you just use cs->cpu_index instead of introducing additional IDs ?
> 
> Note that cpu_exec_init(cs, ) returns with the next available cpu_index
> which can be compared against max_cpus directly.
> 
> Regards,
> Bharata.

I don't think that we should mix the id setting and cpu_index for now.

We can't simply set cpu_index before the device is realized. That logic
belongs to cpu_exec_init(). But I agree that cpu_index and id should always
match after a successful realize.

That id / cpu_index stuff can be cleaned up later. We should not treat cpu_index
as a property for now (by exposing it as "id").

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Bharata B Rao]

On Thu, Mar 03, 2016 at 04:30:32PM -0500, Matthew Rosato wrote:
> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato 
> ---
>  hw/s390x/s390-virtio.c |  2 +-
>  target-s390x/cpu-qom.h |  1 +
>  target-s390x/cpu.c | 56 
> +-
>  target-s390x/cpu.h |  2 ++
>  target-s390x/helper.c  | 42 +++--
>  5 files changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index f00d6b4..2ab7b94 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -116,7 +116,7 @@ void s390_init_cpus(MachineState *machine)
>  }
> 
>  for (i = 0; i < smp_cpus; i++) {
> -cpu_s390x_init(machine->cpu_model);
> +s390_new_cpu(machine->cpu_model, i, _fatal);
>  }
>  }
> 
> diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
> index 56d82f2..1c90933 100644
> --- a/target-s390x/cpu-qom.h
> +++ b/target-s390x/cpu-qom.h
> @@ -68,6 +68,7 @@ typedef struct S390CPU {
>  /*< public >*/
> 
>  CPUS390XState env;
> +int64_t id;
>  /* needed for live migration */
>  void *irqstate;
>  uint32_t irqstate_saved_size;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 76c8eaf..d1b7af9 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -30,8 +30,10 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "trace.h"
> +#include "qapi/visitor.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #endif
> 
>  #define CR0_RESET   0xE0UL
> @@ -199,16 +201,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  CPUS390XState *env = >env;
>  Error *err = NULL;
> 
> +#if !defined(CONFIG_USER_ONLY)
> +if (cpu->id >= max_cpus) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", max allowed: %d", cpu->id, max_cpus - 1);
> +return;
> +}
> +#endif
> +if (cpu_exists(cpu->id)) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", it already exists", cpu->id);
> +return;
> +}
> +if (cpu->id != scc->next_cpu_id) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", The next available id is %" PRIi64, cpu->id,
> +   scc->next_cpu_id);
> +return;
> +}
> +
>  cpu_exec_init(cs, );
>  if (err != NULL) {
>  error_propagate(errp, err);
>  return;
>  }
> +scc->next_cpu_id = cs->cpu_index + 1;

It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
for you. If it is just going to be monotonically increasing like cs->cpu_index,
couldn't you just use cs->cpu_index instead of introducing additional IDs ?

Note that cpu_exec_init(cs, ) returns with the next available cpu_index
which can be compared against max_cpus directly.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato 
> ---
>  hw/s390x/s390-virtio.c |  2 +-
>  target-s390x/cpu-qom.h |  1 +
>  target-s390x/cpu.c | 56 
> +-
>  target-s390x/cpu.h |  2 ++
>  target-s390x/helper.c  | 42 +++--
>  5 files changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index f00d6b4..2ab7b94 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -116,7 +116,7 @@ void s390_init_cpus(MachineState *machine)
>  }
> 
>  for (i = 0; i < smp_cpus; i++) {
> -cpu_s390x_init(machine->cpu_model);
> +s390_new_cpu(machine->cpu_model, i, _fatal);
>  }
>  }
> 
> diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
> index 56d82f2..1c90933 100644
> --- a/target-s390x/cpu-qom.h
> +++ b/target-s390x/cpu-qom.h
> @@ -68,6 +68,7 @@ typedef struct S390CPU {
>  /*< public >*/
> 
>  CPUS390XState env;
> +int64_t id;
>  /* needed for live migration */
>  void *irqstate;
>  uint32_t irqstate_saved_size;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 76c8eaf..d1b7af9 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -30,8 +30,10 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "trace.h"
> +#include "qapi/visitor.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #endif
> 
>  #define CR0_RESET   0xE0UL
> @@ -199,16 +201,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  CPUS390XState *env = >env;
>  Error *err = NULL;
> 
> +#if !defined(CONFIG_USER_ONLY)
> +if (cpu->id >= max_cpus) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", max allowed: %d", cpu->id, max_cpus - 1);

not sure if we should report this into err and then to an error_propgate().

> +return;
> +}
> +#endif
> +if (cpu_exists(cpu->id)) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", it already exists", cpu->id);

dito

> +return;
> +}
> +if (cpu->id != scc->next_cpu_id) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", The next available id is %" PRIi64, cpu->id,
> +   scc->next_cpu_id);

dito

> +return;
> +}
> +
>  cpu_exec_init(cs, );
>  if (err != NULL) {
>  error_propagate(errp, err);
>  return;
>  }
> +scc->next_cpu_id = cs->cpu_index + 1;

Why can't we simply do a scc->next_cpu_id++ as before and leave cs->cpu_index
logic out?

> 
>  #if !defined(CONFIG_USER_ONLY)
>  qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>  #endif
> -env->cpu_num = scc->next_cpu_id++;
> +env->cpu_num = cpu->id;
>  s390_cpu_gdb_init(cs);
>  qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> @@ -220,6 +242,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  scc->parent_realize(dev, errp);
>  }
> 
> +static void s390_cpu_set_id(Object *obj, Visitor *v, const char *name,
> +void *opaque, Error **errp)

s/s390_/s390x_/ ?

> +{
> +S390CPU *cpu = S390_CPU(obj);
> +DeviceState *dev = DEVICE(obj);
> +const int64_t min = 0;
> +const int64_t max = UINT32_MAX;
> +Error *err = NULL;
> +int64_t value;
> +
> +if (dev->realized) {
> +error_setg(errp, "Attempt to set property '%s' on '%s' after "
> +   "it was realized", name, object_get_typename(obj));
> +return;
> +}
> +
> +visit_type_int(v, name, , );
> +if (err) {
> +error_propagate(errp, err);
> +return;
> +}
> +if (value < min || value > max) {
> +error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> +   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> +   object_get_typename(obj), name, value, min, max);
> +return;
> +}
> +cpu->id = value;
> +}
> +
>  static void s390_cpu_initfn(Object *obj)
>  {
>  CPUState *cs = CPU(obj);
> @@ -233,6 +285,8 @@ static void s390_cpu_initfn(Object *obj)
>  cs->env_ptr = env;
>  cs->halted = 1;
>  cs->exception_index = EXCP_HLT;
> +object_property_add(OBJECT(cpu), "id", "int64_t", NULL, s390_cpu_set_id,
> +NULL, NULL, NULL);

Would it be helpful for introspection to also get the id of a cpu?


>  #if !defined(CONFIG_USER_ONLY)
>  qemu_get_timedate(, 0);
>  env->tod_offset = TOD_UNIX_EPOCH +
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 49c8415..6667cc0 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -413,6 +413,8 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t 
> code, uint32_t ilen);
>  #endif
> 
>  S390CPU *cpu_s390x_init(const 

[Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[Matthew Rosato]

Check for and propogate errors during s390 cpu creation.

Signed-off-by: Matthew Rosato 
---
 hw/s390x/s390-virtio.c |  2 +-
 target-s390x/cpu-qom.h |  1 +
 target-s390x/cpu.c | 56 +-
 target-s390x/cpu.h |  2 ++
 target-s390x/helper.c  | 42 +++--
 5 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index f00d6b4..2ab7b94 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -116,7 +116,7 @@ void s390_init_cpus(MachineState *machine)
 }
 
 for (i = 0; i < smp_cpus; i++) {
-cpu_s390x_init(machine->cpu_model);
+s390_new_cpu(machine->cpu_model, i, _fatal);
 }
 }
 
diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index 56d82f2..1c90933 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -68,6 +68,7 @@ typedef struct S390CPU {
 /*< public >*/
 
 CPUS390XState env;
+int64_t id;
 /* needed for live migration */
 void *irqstate;
 uint32_t irqstate_saved_size;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 76c8eaf..d1b7af9 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -30,8 +30,10 @@
 #include "qemu/error-report.h"
 #include "hw/hw.h"
 #include "trace.h"
+#include "qapi/visitor.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
+#include "sysemu/sysemu.h"
 #endif
 
 #define CR0_RESET   0xE0UL
@@ -199,16 +201,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
**errp)
 CPUS390XState *env = >env;
 Error *err = NULL;
 
+#if !defined(CONFIG_USER_ONLY)
+if (cpu->id >= max_cpus) {
+error_setg(errp, "Unable to add CPU: %" PRIi64
+   ", max allowed: %d", cpu->id, max_cpus - 1);
+return;
+}
+#endif
+if (cpu_exists(cpu->id)) {
+error_setg(errp, "Unable to add CPU: %" PRIi64
+   ", it already exists", cpu->id);
+return;
+}
+if (cpu->id != scc->next_cpu_id) {
+error_setg(errp, "Unable to add CPU: %" PRIi64
+   ", The next available id is %" PRIi64, cpu->id,
+   scc->next_cpu_id);
+return;
+}
+
 cpu_exec_init(cs, );
 if (err != NULL) {
 error_propagate(errp, err);
 return;
 }
+scc->next_cpu_id = cs->cpu_index + 1;
 
 #if !defined(CONFIG_USER_ONLY)
 qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
-env->cpu_num = scc->next_cpu_id++;
+env->cpu_num = cpu->id;
 s390_cpu_gdb_init(cs);
 qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -220,6 +242,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
**errp)
 scc->parent_realize(dev, errp);
 }
 
+static void s390_cpu_set_id(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+S390CPU *cpu = S390_CPU(obj);
+DeviceState *dev = DEVICE(obj);
+const int64_t min = 0;
+const int64_t max = UINT32_MAX;
+Error *err = NULL;
+int64_t value;
+
+if (dev->realized) {
+error_setg(errp, "Attempt to set property '%s' on '%s' after "
+   "it was realized", name, object_get_typename(obj));
+return;
+}
+
+visit_type_int(v, name, , );
+if (err) {
+error_propagate(errp, err);
+return;
+}
+if (value < min || value > max) {
+error_setg(errp, "Property %s.%s doesn't take value %" PRId64
+   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
+   object_get_typename(obj), name, value, min, max);
+return;
+}
+cpu->id = value;
+}
+
 static void s390_cpu_initfn(Object *obj)
 {
 CPUState *cs = CPU(obj);
@@ -233,6 +285,8 @@ static void s390_cpu_initfn(Object *obj)
 cs->env_ptr = env;
 cs->halted = 1;
 cs->exception_index = EXCP_HLT;
+object_property_add(OBJECT(cpu), "id", "int64_t", NULL, s390_cpu_set_id,
+NULL, NULL, NULL);
 #if !defined(CONFIG_USER_ONLY)
 qemu_get_timedate(, 0);
 env->tod_offset = TOD_UNIX_EPOCH +
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 49c8415..6667cc0 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -413,6 +413,8 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t 
code, uint32_t ilen);
 #endif
 
 S390CPU *cpu_s390x_init(const char *cpu_model);
+S390CPU *s390_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);
 int cpu_s390x_exec(CPUState *cpu);
 
diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 838bdd9..c48c816 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -30,6 +30,9 @@
 //#define DEBUG_S390
 //#define DEBUG_S390_STDOUT
 
+/* Use to track cpu ID for linux-user only */
+static int64_t next_cpu_id;
+
 #ifdef DEBUG_S390
 #ifdef