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

[David Hildenbrand]

> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato <address@hidden>
> ---
>  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