On Sun, 09 Jun 2013 03:11:35 +0200
Andreas Färber <afaer...@suse.de> wrote:
> Am 07.06.2013 19:28, schrieb Jason J. Herne:
> > From: "Jason J. Herne" <jjhe...@us.ibm.com>
> >
> > 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 <jjhe...@us.ibm.com>
> > ---
> > 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
>
