| Message ID | 1456866806-31466-6-git-send-email-mjrosato@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> Check for and propogate errors during s390 cpu creation. > > Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++ > hw/s390x/s390-virtio.c | 2 +- > hw/s390x/s390-virtio.h | 1 + > target-s390x/cpu-qom.h | 3 +++ > target-s390x/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-- > target-s390x/cpu.h | 1 + > target-s390x/helper.c | 31 ++++++++++++++++++++-- > 7 files changed, 128 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 3090e76..4886dbf 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size) > s390_skeys_init(); > } > > +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp) > +{ > + S390CPU *cpu = NULL; > + Error *local_err = NULL; Think the naming schema is "err" now. > + > + if (id >= max_cpus) { > + error_setg(errp, "Unable to add CPU: %" PRIi64 > + ", max allowed: %d", id, max_cpus - 1); > + goto out; Could we also move this check to the realize function? > + } > + > + cpu = cpu_s390x_create(machine->cpu_model, &local_err); > + if (local_err != NULL) { > + goto out; > + } > + > + object_property_set_int(OBJECT(cpu), id, "id", &local_err); We should add a check in between if (err) { goto out; } > + object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > + > +out: > + if (cpu != NULL) { > + object_unref(OBJECT(cpu)); Is the object_unref() here correct? I know that we have one reference from VCPU creation. Where does the second one come from (is it from the hotplug handler? then I'd prefer a comment here :D ) > + } > + if (local_err) { > + error_propagate(errp, local_err); > + cpu = NULL; > + } > + return cpu; > +} > + > static void ccw_init(MachineState *machine) > { > int ret; > diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c > index 6bd9803..6f71ab0 100644 > --- a/hw/s390x/s390-virtio.c > +++ b/hw/s390x/s390-virtio.c > @@ -118,7 +118,7 @@ void s390_init_cpus(MachineState *machine) > } > > for (i = 0; i < smp_cpus; i++) { > - cpu_s390x_init(machine->cpu_model); > + s390_new_cpu(machine, i, &error_fatal); > } > } > > diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h > index ffd014c..fbcfdb8 100644 > --- a/hw/s390x/s390-virtio.h > +++ b/hw/s390x/s390-virtio.h > @@ -29,4 +29,5 @@ void s390_create_virtio_net(BusState *bus, const char *name); > void s390_nmi(NMIState *n, int cpu_index, Error **errp); > void s390_machine_reset(void); > void s390_memory_init(ram_addr_t mem_size); > +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp); > #endif > diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h > index 029a44a..1c90933 100644 > --- a/target-s390x/cpu-qom.h > +++ b/target-s390x/cpu-qom.h > @@ -47,6 +47,8 @@ typedef struct S390CPUClass { > CPUClass parent_class; > /*< public >*/ > > + int64_t next_cpu_id; > + > DeviceRealize parent_realize; > void (*parent_reset)(CPUState *cpu); > void (*load_normal)(CPUState *cpu); > @@ -66,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 8dfd063..ec66ed6 100644 > --- a/target-s390x/cpu.c > +++ b/target-s390x/cpu.c > @@ -30,6 +30,7 @@ > #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" > #endif > @@ -197,11 +198,26 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) > S390CPUClass *scc = S390_CPU_GET_CLASS(dev); > S390CPU *cpu = S390_CPU(dev); > CPUS390XState *env = &cpu->env; > + Error *local_err = NULL; > + > + 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, &local_err); > + if (local_err != NULL) { > + error_propagate(errp, local_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 = cs->cpu_index; > + env->cpu_num = cpu->id; > s390_cpu_gdb_init(cs); > qemu_init_vcpu(cs); > #if !defined(CONFIG_USER_ONLY) > @@ -213,6 +229,49 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) > scc->parent_realize(dev, errp); > } > > +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + S390CPU *cpu = S390_CPU(obj); > + int64_t value = cpu->id; > + > + visit_type_int(v, name, &value, 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 *local_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, &local_err); > + if (local_err) { > + error_propagate(errp, local_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; > + } > + if ((value != cpu->id) && cpu_exists(value)) { > + error_setg(errp, "CPU with ID %" PRIi64 " exists", value); > + return; > + } > + cpu->id = value; > +} Just curious, what about using a simple object_property_set_int() and doing all the checks in realize() ? Then we could live without manual getter/setter (and without the realize check). > + > static void s390_cpu_initfn(Object *obj) > { > CPUState *cs = CPU(obj); > @@ -226,7 +285,8 @@ static void s390_cpu_initfn(Object *obj) > cs->env_ptr = env; > cs->halted = 1; > cs->exception_index = EXCP_HLT; > - cpu_exec_init(cs, &error_abort); > + object_property_add(OBJECT(cpu), "id", "int64_t", s390_cpu_get_id, > + s390_cpu_set_id, NULL, NULL, NULL); > #if !defined(CONFIG_USER_ONLY) > qemu_get_timedate(&tm, 0); > env->tod_offset = TOD_UNIX_EPOCH + > @@ -342,6 +402,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) > CPUClass *cc = CPU_CLASS(scc); > DeviceClass *dc = DEVICE_CLASS(oc); > > + scc->next_cpu_id = 0; > scc->parent_realize = dc->realize; > dc->realize = s390_cpu_realizefn; > > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h > index 6ae5699..2c7d6bd 100644 > --- a/target-s390x/cpu.h > +++ b/target-s390x/cpu.h > @@ -413,6 +413,7 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen); > #endif > > S390CPU *cpu_s390x_init(const char *cpu_model); > +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..e562cb7 100644 > --- a/target-s390x/helper.c > +++ b/target-s390x/helper.c > @@ -65,14 +65,41 @@ 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 *cpu_s390x_init(const char *cpu_model) > +{ > + Error *error = NULL; > + S390CPU *cpu; > + S390CPUClass *scc; > + int64_t id; > > + cpu = cpu_s390x_create(cpu_model, &error); > + if (error) { > + goto out; > + } > + > + scc = S390_CPU_GET_CLASS(cpu); > + id = scc->next_cpu_id; > + > + object_property_set_int(OBJECT(cpu), id, "id", &error); dito > + object_property_set_bool(OBJECT(cpu), true, "realized", &error); > + > + out: > + if (error) { > + error_report_err(error); > + if (cpu != NULL) { > + object_unref(OBJECT(cpu)); > + cpu = NULL; > + } > + } Can we make both error handling blocks (s390_cpu_add) look alike? > return cpu; > } > This looks much better to me! David
> Check for and propogate errors during s390 cpu creation. > > Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++ > hw/s390x/s390-virtio.c | 2 +- > hw/s390x/s390-virtio.h | 1 + > target-s390x/cpu-qom.h | 3 +++ > target-s390x/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-- > target-s390x/cpu.h | 1 + > target-s390x/helper.c | 31 ++++++++++++++++++++-- > 7 files changed, 128 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 3090e76..4886dbf 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size) > s390_skeys_init(); > } > > +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp) Just a thought, if not passing machine but the model string, we could make cpu_s390x_init() call s390_new_cpu(). But then s390_new_cpu() would have to be moved again. Not sure if this is really worth it. David
>> +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name, >> + void *opaque, Error **errp) >> +{ >> + S390CPU *cpu = S390_CPU(obj); >> + int64_t value = cpu->id; >> + >> + visit_type_int(v, name, &value, 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 *local_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, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_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; >> + } >> + if ((value != cpu->id) && cpu_exists(value)) { >> + error_setg(errp, "CPU with ID %" PRIi64 " exists", value); >> + return; >> + } >> + cpu->id = value; >> +} > > Just curious, what about using a simple > > object_property_set_int() and doing all the checks in realize() ? > > Then we could live without manual getter/setter (and without the realize check). > I think we still need at least a manual setter, even if you want to move the checks to realize. See something like object_property_add_uint64_ptr() -- It sets a boilerplate get routine, and no set routine -- I think this presumes you set your property upfront (at add time), never change it for the life of the object, but want to read it later. By comparison, S390CPU.id is set sometime after instance_init, based on input. So, we call object_property_set_int() to update it -- This just passes the provided int value to the setter routine associated with the property. If one doesn't exist, you get: qemu: Insufficient permission to perform this operation I think this is also why we want to check for dev->realized in the setter routine, to make sure the property is not being changed "too late" -- Once the cpu is realized, the ID is baked and can't be changed. Or did I misunderstand your idea here? Matt
On 03/02/2016 02:59 AM, David Hildenbrand wrote: >> Check for and propogate errors during s390 cpu creation. >> >> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++ >> hw/s390x/s390-virtio.c | 2 +- >> hw/s390x/s390-virtio.h | 1 + >> target-s390x/cpu-qom.h | 3 +++ >> target-s390x/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-- >> target-s390x/cpu.h | 1 + >> target-s390x/helper.c | 31 ++++++++++++++++++++-- >> 7 files changed, 128 insertions(+), 5 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 3090e76..4886dbf 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size) >> s390_skeys_init(); >> } >> >> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp) > > Just a thought, if not passing machine but the model string, we could make > > cpu_s390x_init() call s390_new_cpu(). > > But then s390_new_cpu() would have to be moved again. Not sure if this is > really worth it. > Additionally, next_cpu_id would have to be moved somewhere other than the S390CPUClass, as we need that value for input to s390_new_cpu() here, but won't have a cpu object to acquire it until after s390_new_cpu() returns. Going to leave cpu_s390x_init() calling cpu_s390x_create() unless someone voices a strong opinion. Matt
> >> +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name, > >> + void *opaque, Error **errp) > >> +{ > >> + S390CPU *cpu = S390_CPU(obj); > >> + int64_t value = cpu->id; > >> + > >> + visit_type_int(v, name, &value, 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 *local_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, &local_err); > >> + if (local_err) { > >> + error_propagate(errp, local_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; > >> + } > >> + if ((value != cpu->id) && cpu_exists(value)) { > >> + error_setg(errp, "CPU with ID %" PRIi64 " exists", value); > >> + return; > >> + } > >> + cpu->id = value; > >> +} > > > > Just curious, what about using a simple > > > > object_property_set_int() and doing all the checks in realize() ? > > > > Then we could live without manual getter/setter (and without the realize check). > > > > I think we still need at least a manual setter, even if you want to move > the checks to realize. > > See something like object_property_add_uint64_ptr() -- It sets a > boilerplate get routine, and no set routine -- I think this presumes you > set your property upfront (at add time), never change it for the life of > the object, but want to read it later. > By comparison, S390CPU.id is set sometime after instance_init, based on > input. > > So, we call object_property_set_int() to update it -- This just passes > the provided int value to the setter routine associated with the > property. If one doesn't exist, you get: > qemu: Insufficient permission to perform this operation > > I think this is also why we want to check for dev->realized in the > setter routine, to make sure the property is not being changed "too > late" -- Once the cpu is realized, the ID is baked and can't be changed. > > Or did I misunderstand your idea here? If we care about malicious users, wanting to set id's after realize that is true. But I am no QOM expert and don't know if that is a scenarios that has to be taken care of. But as I see similar code for other properties, I assume we are better off doing it also that way. David
>> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp) >> +{ >> + S390CPU *cpu = NULL; >> + Error *local_err = NULL; > > Think the naming schema is "err" now. > >> + >> + if (id >= max_cpus) { >> + error_setg(errp, "Unable to add CPU: %" PRIi64 >> + ", max allowed: %d", id, max_cpus - 1); >> + goto out; > > Could we also move this check to the realize function? > >> + } >> + >> + cpu = cpu_s390x_create(machine->cpu_model, &local_err); >> + if (local_err != NULL) { >> + goto out; >> + } >> + >> + object_property_set_int(OBJECT(cpu), id, "id", &local_err); > > We should add a check in between > > if (err) { > goto out; > } > >> + object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); >> + >> +out: >> + if (cpu != NULL) { >> + object_unref(OBJECT(cpu)); > > Is the object_unref() here correct? > I know that we have one reference from VCPU creation. Where does the second one > come from (is it from the hotplug handler? then I'd prefer a comment here :D ) > After some digging, I believe this unref is not necessary for s390 (bus-less) and I'm now questioning the i386 code that I used as a base... @Igor/Andreas: In i386, looks like the unrefs were due to the ref created when adding the cpu to the icc bus. Andreas moved the checks outside of pc_new_cpu and explains their purpose here: 0e3bd562 - pc: Ensure non-zero CPU ref count after attaching to ICC bus But then a subsequent patch removed the bus and left the unrefs: 46232aaa - cpu/apic: drop icc bus/bridge Should that patch not have also dropped the unrefs in pc_hot_add_cpu() and pc_cpus_init()? Matt
On Thu, 3 Mar 2016 12:52:17 -0500 Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote: > >> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp) > >> +{ > >> + S390CPU *cpu = NULL; > >> + Error *local_err = NULL; > > > > Think the naming schema is "err" now. > > > >> + > >> + if (id >= max_cpus) { > >> + error_setg(errp, "Unable to add CPU: %" PRIi64 > >> + ", max allowed: %d", id, max_cpus - 1); > >> + goto out; > > > > Could we also move this check to the realize function? > > > >> + } > >> + > >> + cpu = cpu_s390x_create(machine->cpu_model, &local_err); > >> + if (local_err != NULL) { > >> + goto out; > >> + } > >> + > >> + object_property_set_int(OBJECT(cpu), id, "id", &local_err); > > > > We should add a check in between > > > > if (err) { > > goto out; > > } > > > >> + object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > >> + > >> +out: > >> + if (cpu != NULL) { > >> + object_unref(OBJECT(cpu)); > > > > Is the object_unref() here correct? > > I know that we have one reference from VCPU creation. Where does the second one > > come from (is it from the hotplug handler? then I'd prefer a comment here :D ) > > > > After some digging, I believe this unref is not necessary for s390 > (bus-less) and I'm now questioning the i386 code that I used as a base... > > @Igor/Andreas: > > In i386, looks like the unrefs were due to the ref created when adding > the cpu to the icc bus. Andreas moved the checks outside of pc_new_cpu > and explains their purpose here: > 0e3bd562 - pc: Ensure non-zero CPU ref count after attaching to ICC bus > > But then a subsequent patch removed the bus and left the unrefs: > 46232aaa - cpu/apic: drop icc bus/bridge > > Should that patch not have also dropped the unrefs in pc_hot_add_cpu() > and pc_cpus_init()? nope, bus made it own ref, nref is needed here to avoid leaking object as device_realize() implicitly adds it to /machine/devices/unattached/ creating an extra ref along the way. > > Matt >
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 3090e76..4886dbf 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size) s390_skeys_init(); } +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp) +{ + S390CPU *cpu = NULL; + Error *local_err = NULL; + + if (id >= max_cpus) { + error_setg(errp, "Unable to add CPU: %" PRIi64 + ", max allowed: %d", id, max_cpus - 1); + goto out; + } + + cpu = cpu_s390x_create(machine->cpu_model, &local_err); + if (local_err != NULL) { + goto out; + } + + object_property_set_int(OBJECT(cpu), id, "id", &local_err); + object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); + +out: + if (cpu != NULL) { + object_unref(OBJECT(cpu)); + } + if (local_err) { + error_propagate(errp, local_err); + cpu = NULL; + } + return cpu; +} + static void ccw_init(MachineState *machine) { int ret; diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c index 6bd9803..6f71ab0 100644 --- a/hw/s390x/s390-virtio.c +++ b/hw/s390x/s390-virtio.c @@ -118,7 +118,7 @@ void s390_init_cpus(MachineState *machine) } for (i = 0; i < smp_cpus; i++) { - cpu_s390x_init(machine->cpu_model); + s390_new_cpu(machine, i, &error_fatal); } } diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h index ffd014c..fbcfdb8 100644 --- a/hw/s390x/s390-virtio.h +++ b/hw/s390x/s390-virtio.h @@ -29,4 +29,5 @@ void s390_create_virtio_net(BusState *bus, const char *name); void s390_nmi(NMIState *n, int cpu_index, Error **errp); void s390_machine_reset(void); void s390_memory_init(ram_addr_t mem_size); +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp); #endif diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h index 029a44a..1c90933 100644 --- a/target-s390x/cpu-qom.h +++ b/target-s390x/cpu-qom.h @@ -47,6 +47,8 @@ typedef struct S390CPUClass { CPUClass parent_class; /*< public >*/ + int64_t next_cpu_id; + DeviceRealize parent_realize; void (*parent_reset)(CPUState *cpu); void (*load_normal)(CPUState *cpu); @@ -66,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 8dfd063..ec66ed6 100644 --- a/target-s390x/cpu.c +++ b/target-s390x/cpu.c @@ -30,6 +30,7 @@ #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" #endif @@ -197,11 +198,26 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) S390CPUClass *scc = S390_CPU_GET_CLASS(dev); S390CPU *cpu = S390_CPU(dev); CPUS390XState *env = &cpu->env; + Error *local_err = NULL; + + 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, &local_err); + if (local_err != NULL) { + error_propagate(errp, local_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 = cs->cpu_index; + env->cpu_num = cpu->id; s390_cpu_gdb_init(cs); qemu_init_vcpu(cs); #if !defined(CONFIG_USER_ONLY) @@ -213,6 +229,49 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) scc->parent_realize(dev, errp); } +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + S390CPU *cpu = S390_CPU(obj); + int64_t value = cpu->id; + + visit_type_int(v, name, &value, 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 *local_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, &local_err); + if (local_err) { + error_propagate(errp, local_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; + } + if ((value != cpu->id) && cpu_exists(value)) { + error_setg(errp, "CPU with ID %" PRIi64 " exists", value); + return; + } + cpu->id = value; +} + static void s390_cpu_initfn(Object *obj) { CPUState *cs = CPU(obj); @@ -226,7 +285,8 @@ static void s390_cpu_initfn(Object *obj) cs->env_ptr = env; cs->halted = 1; cs->exception_index = EXCP_HLT; - cpu_exec_init(cs, &error_abort); + object_property_add(OBJECT(cpu), "id", "int64_t", s390_cpu_get_id, + s390_cpu_set_id, NULL, NULL, NULL); #if !defined(CONFIG_USER_ONLY) qemu_get_timedate(&tm, 0); env->tod_offset = TOD_UNIX_EPOCH + @@ -342,6 +402,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) CPUClass *cc = CPU_CLASS(scc); DeviceClass *dc = DEVICE_CLASS(oc); + scc->next_cpu_id = 0; scc->parent_realize = dc->realize; dc->realize = s390_cpu_realizefn; diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h index 6ae5699..2c7d6bd 100644 --- a/target-s390x/cpu.h +++ b/target-s390x/cpu.h @@ -413,6 +413,7 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen); #endif S390CPU *cpu_s390x_init(const char *cpu_model); +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..e562cb7 100644 --- a/target-s390x/helper.c +++ b/target-s390x/helper.c @@ -65,14 +65,41 @@ 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 *cpu_s390x_init(const char *cpu_model) +{ + Error *error = NULL; + S390CPU *cpu; + S390CPUClass *scc; + int64_t id; + cpu = cpu_s390x_create(cpu_model, &error); + if (error) { + goto out; + } + + scc = S390_CPU_GET_CLASS(cpu); + id = scc->next_cpu_id; + + object_property_set_int(OBJECT(cpu), id, "id", &error); + object_property_set_bool(OBJECT(cpu), true, "realized", &error); + + out: + if (error) { + error_report_err(error); + if (cpu != NULL) { + object_unref(OBJECT(cpu)); + cpu = NULL; + } + } return cpu; }
Check for and propogate errors during s390 cpu creation. Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++ hw/s390x/s390-virtio.c | 2 +- hw/s390x/s390-virtio.h | 1 + target-s390x/cpu-qom.h | 3 +++ target-s390x/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-- target-s390x/cpu.h | 1 + target-s390x/helper.c | 31 ++++++++++++++++++++-- 7 files changed, 128 insertions(+), 5 deletions(-)