Re: [Qemu-devel] [PATCH 7/8] [PATCH RFC v3] s390-qemu: cpu hotplug - Implement hot_add_cpu hook

[Andreas Färber]

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <address@hidden>
> 
> Implement hot_add_cpu for S390 to allow hot plugging of cpus.
> 
> Signed-off-by: Jason J. Herne <address@hidden>
> ---
>  hw/s390x/s390-virtio-ccw.c |    3 +++
>  target-s390x/cpu.c         |   32 ++++++++++++++++++++++++++++++++
>  target-s390x/cpu.h         |    2 ++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index b469960..30b6a48 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -117,6 +117,9 @@ static QEMUMachine ccw_machine = {
>      .alias = "s390-ccw",
>      .desc = "VirtIO-ccw based S390 machine",
>      .init = ccw_init,
> +#if !defined(CONFIG_USER_ONLY)
> +    .hot_add_cpu = ccw_hot_add_cpu,
> +#endif

I doubt this #ifdeffery is necessary here?

>      .block_default_type = IF_VIRTIO,
>      .no_cdrom = 1,
>      .no_floppy = 1,
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index c90a91c..60029d9 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -27,6 +27,8 @@
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  #include "hw/hw.h"
> +#include "hw/s390x/sclp.h"
> +#include "sysemu/sysemu.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
>  #endif
> @@ -154,6 +156,36 @@ static void s390_cpu_finalize(Object *obj)
>  #endif
>  }
>  
> +#if !defined(CONFIG_USER_ONLY)
> +void ccw_hot_add_cpu(const int64_t id, Error **errp)
> +{
> +    S390CPU *new_cpu;
> +    CPUState *cpu;
> +    const char *model_str;
> +    int cpu_count = 0;
> +
> +    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {

CPU_FOREACH(cpu) {

> +        cpu_count++;
> +    }
> +
> +    if (cpu_count == max_cpus) {
> +        error_setg(errp, "Maximum number of cpus already defined");
> +        return;
> +    }
> +
> +    if (id != cpu_count) {
> +        error_setg(errp, "Unable to add CPU: %" PRIi64
> +                   ", The next available id is %d", id, cpu_count);
> +        return;
> +    }

This logic seems wrong according to your colleagues. It should be
checking against the static cpu_num counter or not checking at all if we
want to allow explicit device_add s390-cpu,cpu-num=42.

> +
> +    model_str = s390_cpu_addr2state(0)->env.cpu_model_str;
> +    new_cpu = s390_new_cpu(model_str, id);

As announced, a patch in my large series finally sent out removes
cpu_model_str field. Since we don't have any for s390x, I suggest that
you use the QOM constructs so that device_add works as well, i.e.
new_cpu = object_new(TYPE_S390_CPU).

> +    object_property_set_bool(OBJECT(new_cpu), true, "realized", NULL);
> +    raise_irq_cpu_hotplug();

This would mean moving this line into the realizefn, conditional on
dev->hotplugged (and probably #ifndef CONFIG_USER_ONLY).

Regards,
Andreas

> +}
> +#endif
> +
>  static const VMStateDescription vmstate_s390_cpu = {
>      .name = "cpu",
>      .unmigratable = 1,
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 0f68dd0..711aad4 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -383,6 +383,8 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  void s390_add_running_cpu(S390CPU *cpu);
>  unsigned s390_del_running_cpu(S390CPU *cpu);
>  
> +void ccw_hot_add_cpu(const int64_t id, Error **errp);
> +
>  /* service interrupts are floating therefore we must not pass an cpustate */
>  void s390_sclp_extint(uint32_t parm);
>  
> 


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