| Message ID | 1457040633-30951-7-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.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
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 <mjrosato@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); > + 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, &err); > 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, &err) returns with the next available cpu_index which can be compared against max_cpus directly. Regards, Bharata.
> > cpu_exec_init(cs, &err); > > 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, &err) 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
On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote: > > > > cpu_exec_init(cs, &err); > > > 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, &err) 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.
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, &err); > > > > 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, &err) 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.
> On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote: > > > > > > cpu_exec_init(cs, &err); > > > > 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, &err) 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
On Fri, 4 Mar 2016 12:50:05 +0100 David Hildenbrand <dahi@linux.vnet.ibm.com> wrote: > > On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote: > > > > > > > > cpu_exec_init(cs, &err); > > > > > 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, &err) 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 >
> > 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
On Mon, 7 Mar 2016 11:02:11 +0100 David Hildenbrand <dahi@linux.vnet.ibm.com> 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 > >
On Mon, 7 Mar 2016 11:12:14 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > On Mon, 7 Mar 2016 11:02:11 +0100 > David Hildenbrand <dahi@linux.vnet.ibm.com> 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 :)
On Mon, 7 Mar 2016 12:49:27 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Mon, 7 Mar 2016 11:12:14 +0100 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Mon, 7 Mar 2016 11:02:11 +0100 > > David Hildenbrand <dahi@linux.vnet.ibm.com> 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 :) >
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); + 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, &err); 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, &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); #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; + #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) +{ + S390CPU *cpu = NULL; + 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; }
Check for and propogate errors during s390 cpu creation. Signed-off-by: Matthew Rosato <mjrosato@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(-)