Re: [Qemu-devel] [PATCH 6/8] [PATCH RFC v3] s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug

[Andreas Färber]

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <address@hidden>
> 
>     s390_new_cpu is created to encapsulate the creation of a new QOM S390CPU
>     object given a cpuid and a model string.
> 
>     All actual cpu initialization code is moved from boot time specific 
> functions to
>     s390_cpu_initfn (qom init routine) or to s390_new_cpu. This is done to 
> allow us
>     to use the same basic code path for a cpu created at boot time and one 
> created
>     during a hotplug operation.

Intentionally indented?

> 
> Signed-off-by: Jason J. Herne <address@hidden>
> ---
>  hw/s390x/s390-virtio.c |   25 ++++++++++++-------------
>  target-s390x/cpu.c     |    4 ++--
>  target-s390x/cpu.h     |    1 +
>  target-s390x/helper.c  |   12 ++++++++++++
>  4 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 5ad9cf3..103f32e 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -56,11 +56,16 @@ static S390CPU **ipi_states;
>  
>  void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state)
>  {
> -    ipi_states[cpu_addr] = state;
> +    if (cpu_addr < max_cpus) {
> +        ipi_states[cpu_addr] = state;
> +    }
>  }
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> +    if (cpu_addr >= max_cpus) {
> +        return NULL;
> +    }
>      return ipi_states[cpu_addr];
>  }
>  
> @@ -197,19 +202,13 @@ void s390_init_cpus(const char *cpu_model)
>          cpu_model = "host";
>      }
>  
> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> -
> -    for (i = 0; i < smp_cpus; i++) {
> -        S390CPU *cpu;
> -        CPUState *cs;
> +    ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
>  
> -        cpu = cpu_s390x_init(cpu_model);
> -        cs = CPU(cpu);
> -
> -        ipi_states[i] = cpu;
> -        cs->halted = 1;
> -        cpu->env.exception_index = EXCP_HLT;
> -        cpu->env.storage_keys = s390_get_storage_keys();
> +    for (i = 0; i < max_cpus; i++) {
> +        ipi_states[i] = NULL;

Using g_malloc0() above would hopefully be more efficient and would
allow to leave the loop untouched for easier review.

> +        if (i < smp_cpus) {
> +            s390_new_cpu(cpu_model, i);
> +        }
>      }
>  }
>  
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 6be6c08..c90a91c 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -116,7 +116,6 @@ static void s390_cpu_initfn(Object *obj)
>      S390CPU *cpu = S390_CPU(obj);
>      CPUS390XState *env = &cpu->env;
>      static bool inited;
> -    static int cpu_num = 0;
>  #if !defined(CONFIG_USER_ONLY)
>      struct tm tm;
>  #endif
> @@ -135,8 +134,9 @@ static void s390_cpu_initfn(Object *obj)
>       * cpu counter in s390_cpu_reset to a negative number at
>       * initial ipl */
>      cs->halted = 1;
> +    cpu->env.exception_index = EXCP_HLT;
> +    env->storage_keys = s390_get_storage_keys();

4/8?

>  #endif
> -    env->cpu_num = cpu_num++;
>      env->ext_index = -1;
>  
>      if (tcg_enabled() && !inited) {
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 62eb810..0f68dd0 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -315,6 +315,7 @@ static inline int get_ilen(uint8_t opc)
>  #endif
>  
>  S390CPU *cpu_s390x_init(const char *cpu_model);
> +S390CPU *s390_new_cpu(const char *cpu_model, int64_t cpuid);
>  void s390x_translate_init(void);
>  int cpu_s390x_exec(CPUS390XState *s);
>  
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index 61abfd7..a39b148 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -70,6 +70,18 @@ void s390x_cpu_timer(void *opaque)
>  }
>  #endif
>  
> +S390CPU *s390_new_cpu(const char *cpu_model, int64_t cpuid)

Like I said on IRC, I'm not so fond of copying x86 workarounds here...
x86 does not have a fully QOM'ified CPU, s390x does.

> +{
> +    S390CPU *cpu;
> +
> +    cpu = cpu_s390x_init(cpu_model);
> +    cpu->env.cpu_num = cpuid;

linux-user never calls s390_new_cpu(), so it will change behavior in
always having cpu_num of 0. I guess we can live with that but such a
change needs to be mentioned in the commit message at least.

Why is this moved to after CPU init? Can't we just override the field if
need be? Either Jens or Christian said that we would not want to fill up
holes in ipi_tables to have the CPU address be always unique; which
would mean that it would always be counting as before. if we need to
tweak it, we should add a property to be able to set it from command
line and QMP.

This affects migration btw: We would need to migrate the current or next
CPU address since the last CPU might've been hot-unplugged so that next
CPU address != last non-NULL ipi_states[] slot plus one.

> +#if !defined(CONFIG_USER_ONLY)
> +    s390_cpu_set_ipistate(cpuid, cpu);
> +#endif

...leaving only this then. Why not do this from the CPU realizefn so
that errors actually can be propagated? If cpuid >= max_cpus the above
will silently do nothing.

In that case we don't need this function any longer.

> +    return cpu;
> +}
> +
>  S390CPU *cpu_s390x_init(const char *cpu_model)
>  {
>      S390CPU *cpu;

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg