Re: [Qemu-devel] [PATCH 8/8] [PATCH RFC v2] s390-qemu: cpu hotplug - Treat S390 cpus as devices

[Andreas Färber]

Am 07.06.2013 19:28, schrieb Jason J. Herne:
> From: "Jason J. Herne" <address@hidden>
> 
> Modify cpu initialization and QOM routines associated with s390-cpu such that
> all cpus on S390 are now created via the QOM device creation code path.
> 
> Signed-off-by: Jason J. Herne <address@hidden>
> ---
>  hw/s390x/s390-virtio-ccw.c |   15 ++++++++++-----
>  hw/s390x/s390-virtio.c     |   25 +++++--------------------
>  hw/s390x/s390-virtio.h     |    2 +-
>  include/qapi/qmp/qerror.h  |    3 +++
>  qdev-monitor.c             |   17 +++++++++++++++++
>  target-s390x/cpu.c         |   24 ++++++++++++++++++++++--
>  6 files changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 70bd858..141adce 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -95,12 +95,8 @@ static void ccw_init(QEMUMachineInitArgs *args)
>      /* allocate storage keys */
>      s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
>  
> -    /* init CPUs */
> -    s390_init_cpus(args->cpu_model);
> +    s390_init_ipi_states();
>  
> -    if (kvm_enabled()) {
> -        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> -    }
>      /*
>       * Create virtual css and set it as default so that non mcss-e
>       * enabled guests only see virtio devices.
> @@ -112,11 +108,20 @@ static void ccw_init(QEMUMachineInitArgs *args)
>      s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw");
>  }
>  
> +static void ccw_post_cpu_init(void)
> +{
> +    if (kvm_enabled()) {
> +        kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> +    }
> +}

Am I understanding correctly that all this is about differentiating one
call between the ccw and legacy machines?

Isn't there a machine-init-done Notifier that the ccw machine init could
register for?

What if CPU 0 were hot-unplugged? Would the capability need to be
re-enabled or will this remain a one-time task?

> +
>  static QEMUMachine ccw_machine = {
>      .name = "s390-ccw-virtio",
>      .alias = "s390-ccw",
>      .desc = "VirtIO-ccw based S390 machine",
> +    .cpu_device_str = "s390-cpu",

TYPE_S390_CPU would be safer than hardcoding, if we need to do this.

>      .init = ccw_init,
> +    .post_cpu_init = ccw_post_cpu_init,
>      .block_default_type = IF_VIRTIO,
>      .no_cdrom = 1,
>      .no_floppy = 1,
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 4af2d86..069a187 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -201,31 +201,17 @@ void s390_init_ipl_dev(const char *kernel_filename,
>      qdev_init_nofail(dev);
>  }
>  
> -void s390_init_cpus(const char *cpu_model)
> +void s390_init_ipi_states(void)
>  {
>      int i;
>  
> -    if (cpu_model == NULL) {
> -        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_p();
> +    for (i = 0; i < max_cpus; i++) {
> +        ipi_states[i] = NULL;
>      }
>  }
>  
> -

Whitespace change intentional?

>  void s390_create_virtio_net(BusState *bus, const char *name)
>  {
>      int i;
> @@ -296,8 +282,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>      /* allocate storage keys */
>      s390_set_storage_keys_p(g_malloc0(my_ram_size / TARGET_PAGE_SIZE));
>  
> -    /* init CPUs */
> -    s390_init_cpus(args->cpu_model);
> +    s390_init_ipi_states();
>  
>      /* Create VirtIO network adapters */
>      s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");

So effectively you're ripping out support for -cpu arguments and
assuming that s390-cpu will stay the only available type - when we were
actually just waiting for you guys to sort out how you want your models
to be named, which I believe you wanted to coordinate with Linux?

I still don't understand why you want to deviate from all other
architectures here. -smp N is supposed to create N times -cpu, not N
times QEMUMachine::cpu_device_str.

> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index c1cb042..7b1ef9f 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -20,7 +20,7 @@
>  typedef int (*s390_virtio_fn)(const uint64_t *args);
>  void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
>  
> -void s390_init_cpus(const char *cpu_model);
> +void s390_init_ipi_states(void);
>  void s390_init_ipl_dev(const char *kernel_filename,
>                         const char *kernel_cmdline,
>                         const char *initrd_filename,
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 6c0a18d..6627dc4 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -162,6 +162,9 @@ void assert_no_error(Error *err);
>  #define QERR_KVM_MISSING_CAP \
>      ERROR_CLASS_K_V_M_MISSING_CAP, "Using KVM without %s, %s unavailable"
>  
> +#define QERR_MAX_CPUS \
> +    ERROR_CLASS_GENERIC_ERROR, "The maximum number of cpus has already been 
> created for this guest"

Don't add QERR_* constants, use error_setg().

> +
>  #define QERR_MIGRATION_ACTIVE \
>      ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
>  
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index e54dbc2..a4adeb8 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -23,6 +23,9 @@
>  #include "monitor/qdev.h"
>  #include "qmp-commands.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/boards.h"
> +#include "sysemu/cpus.h"
>  #include "qemu/config-file.h"
>  
>  /*
> @@ -442,6 +445,14 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          return NULL;
>      }
>  
> +    if (driver && current_machine &&
> +        strcmp(driver, current_machine->cpu_device_str) == 0) {
> +        if (smp_cpus == max_cpus) {
> +            qerror_report(QERR_MAX_CPUS);

As mentioned on 7/8, this should best be handled on QOM realize level.
That way we get the check consistently and can pass on the error.

Also this hunk seems misplaced in this patch, not being s390-only code.

> +            return NULL;
> +        }
> +    }
> +
>      k = DEVICE_CLASS(obj);
>  
>      /* find bus */
> @@ -498,6 +509,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>          return NULL;
>      }
> +
> +    if (driver && current_machine &&
> +        strcmp(driver, current_machine->cpu_device_str) == 0) {
> +        resume_all_vcpus();
> +    }

Ditto, generic change.

Hasn't this been obsoleted? Hot-added CPUs get resumed in
qom/cpu.c:cpu_common_realizefn() now. And CPUs added with -device should
be resumed together with machine-created CPUs from what I recall.
If something doesn't work as expected, please explicitly say so, then we
can fix it in its own patch and optionally backport it.

> +
>      qdev->opts = opts;
>      return qdev;
>  }
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 23fe51f..8b92c9c 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -29,6 +29,8 @@
>  #include "hw/hw.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/s390x/sclp.h"
>  #endif
>  
>  #define CR0_RESET       0xE0UL
> @@ -106,6 +108,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      cpu_reset(CPU(cpu));
>  
>      scc->parent_realize(dev, errp);
> +
> +    cpu_synchronize_post_init(CPU(dev));

This is already done as part of parent_realize above, please drop.

> +    raise_irq_cpu_hotplug();

[FTR: introduced in 3/8]

Shouldn't this be conditional on DeviceState::hotplugged, just like
cpu_synchronize_post_init() in qom/cpu.c?

>  }
>  
>  static void s390_cpu_initfn(Object *obj)
> @@ -113,8 +118,9 @@ static void s390_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      S390CPU *cpu = S390_CPU(obj);
>      CPUS390XState *env = &cpu->env;
> +    int cpu_num = s390_cpu_get_free_state_idx();
>      static bool inited;
> -    static int cpu_num = 0;
> +
>  #if !defined(CONFIG_USER_ONLY)
>      struct tm tm;
>  #endif
> @@ -134,13 +140,20 @@ static void s390_cpu_initfn(Object *obj)
>       * initial ipl */
>      cs->halted = 1;
>  #endif
> -    env->cpu_num = cpu_num++;
> +    s390_cpu_set_state(cpu_num, cpu);

This function name is rather confusing here, can you find a more precise
name?

In fact, can't we replace the ipi_states[] array with QOM link<S390CPU>
properties? All we would be doing here is adding or setting a link from
/machine/cpu[cpu_num] (or so) to this instance.

> +    cs->cpu_index = cpu_num;

This is after cpu_exec_init(), so fiddling with cpu_index should be OK.

But perhaps you should override CPUClass::get_arch_id to return cpu_num
directly, for cpu_exists() / cpu-add? If this is about hot-remove then
we may need a more generic solution. At least I don't see you changing
any cpu_index-dependent code here.

> +    env->cpu_num = cpu_num;
>      env->ext_index = -1;

> +    env->cpu_model_str = "host";

This is unneeded, cpu_model_str is only used for linux-user, where your
-smp twiddling does not apply.

> +    cpu->env.exception_index = EXCP_HLT;
> +    cpu->env.storage_keys = s390_get_storage_keys_p();

Moved from s390_init_cpus(), fine.

>  
>      if (tcg_enabled() && !inited) {
>          inited = true;
>          s390x_translate_init();
>      }
> +
> +    smp_cpus += 1;

Won't we need some form of locking?

If we fiddle with a global CPU counter, we should do so in qom/cpu.c,
not just in s390x code.

>  }
>  
>  static void s390_cpu_finalize(Object *obj)
> @@ -152,6 +165,12 @@ static void s390_cpu_finalize(Object *obj)
>  #endif
>  }
>  
> +static int s390_cpu_unplug(DeviceState *dev)
> +{
> +    fprintf(stderr, "Removal of CPU devices is not supported.\n");
> +    return -1;
> +}
> +
>  static const VMStateDescription vmstate_s390_cpu = {
>      .name = "cpu",
>      .unmigratable = 1,
> @@ -165,6 +184,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void 
> *data)
>  
>      scc->parent_realize = dc->realize;
>      dc->realize = s390_cpu_realizefn;
> +    dc->unplug = s390_cpu_unplug;
>  
>      scc->parent_reset = cc->reset;
>      cc->reset = s390_cpu_reset;

That we should do in generic code, too.

Plus I count 6x dc->unplug plus 4x k->unplug, so we should probably
enhance the function signature with an Error** to properly return the
error message, since printing it to stderr means it won't show up on the
monitor console. I'll prepare a patch.

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