> Check for and propogate errors during s390 cpu creation. > > Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com> > --- > 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, &error_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 = &cpu->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, &err); > 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, &value, &err); > + 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(&tm, 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; I think be best use this into the function cpu_s390x_init() then, as this is only needed to keep linux-user working. > + > #ifdef DEBUG_S390 > #ifdef DEBUG_S390_STDOUT > #define DPRINTF(fmt, ...) \ > @@ -65,14 +68,49 @@ void s390x_cpu_timer(void *opaque) > } > #endif > > -S390CPU *cpu_s390x_init(const char *cpu_model) > +S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp) > { > S390CPU *cpu; > > cpu = S390_CPU(object_new(TYPE_S390_CPU)); > > - object_property_set_bool(OBJECT(cpu), true, "realized", NULL); > + return cpu; > +} > + > +S390CPU *s390_new_cpu(const char *cpu_model, int64_t id, Error **errp) s/s390_/s390x_/ ? > +{ > + S390CPU *cpu = NULL; No need to initialize cpu. > + Error *err = NULL; > + > + cpu = cpu_s390x_create(cpu_model, &err); > + if (err != NULL) { > + goto out; > + } > + > + object_property_set_int(OBJECT(cpu), id, "id", &err); > + if (err != NULL) { > + goto out; > + } > + object_property_set_bool(OBJECT(cpu), true, "realized", &err); > > +out: > + if (err) { > + error_propagate(errp, err); > + object_unref(OBJECT(cpu)); > + cpu = NULL; > + } > + return cpu; > +} > + > +S390CPU *cpu_s390x_init(const char *cpu_model) > +{ > + Error *err = NULL; > + S390CPU *cpu; > + > + cpu = s390_new_cpu(cpu_model, next_cpu_id++, &err); > + if (err) { > + error_report_err(err); > + } > return cpu; > } > This looks like the right way for me. Only minor nits. David