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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

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.