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

[Igor Mammedov]

On Sun, 09 Jun 2013 03:11:35 +0200
Andreas Färber <address@hidden> wrote:

> 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.
Similar change was rejected before for i386 target and as temporary solution
target specific cpu_add hook was added. But do it needed for s390 at all?

if s390 doesn't support legacy -cpu [+-]foo-feature format then
CPU subclasses + properties should do the job, at least it's where we
heading with i386 target.

> 
> >      .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.
cpu_index in cpu_exec_init() is used for registering CPU's vmstate.
vmstate from cpu_exec_init() should be moved to realize time, probably to
common CPU class.

> 
> 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
>