[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu cre
From: |
David Hildenbrand |
Subject: |
Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation |
Date: |
Fri, 4 Mar 2016 09:01:26 +0100 |
> Check for and propogate errors during s390 cpu creation.
>
> Signed-off-by: Matthew Rosato <address@hidden>
> ---
> 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
- [Qemu-devel] [PATCH v8 3/7] s390x/cpu: Get rid of side effects when creating a vcpu, (continued)
- [Qemu-devel] [PATCH v8 3/7] s390x/cpu: Get rid of side effects when creating a vcpu, Matthew Rosato, 2016/03/03
- [Qemu-devel] [PATCH v8 4/7] s390x/cpu: Tolerate max_cpus, Matthew Rosato, 2016/03/03
- [Qemu-devel] [PATCH v8 5/7] s390x/cpu: Add CPU property links, Matthew Rosato, 2016/03/03
- [Qemu-devel] [PATCH v8 7/7] s390x/cpu: Allow hotplug of CPUs, Matthew Rosato, 2016/03/03
- [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, Matthew Rosato, 2016/03/03
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation,
David Hildenbrand <=
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, Bharata B Rao, 2016/03/04
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, David Hildenbrand, 2016/03/04
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, Bharata B Rao, 2016/03/04
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, Bharata B Rao, 2016/03/04
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, David Hildenbrand, 2016/03/04
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, Igor Mammedov, 2016/03/04
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, David Hildenbrand, 2016/03/07
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, Igor Mammedov, 2016/03/07
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, Cornelia Huck, 2016/03/07
- Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation, Igor Mammedov, 2016/03/07