[Qemu-devel] [PATCH 0/3] s390: ipl device, cpu reset handler and cpu model support

[Qemu-devel] [PATCH 0/3] s390: ipl device, cpu reset handler and cpu model support

[Jens Freimann]

Hi Alex,

here's a reworked version of the ipl-device patch, the cpu reset handler
and the patch to query cpu definitions via QMP.

Viktors cpu query patch now has an #ifdef CONFIG_KVM  and the square brackets around
"host" in the text output were removed.  It provides the basic funtionality we need and will 
be followed by further patches in the future. 

Patch 1: Creates a new ipl device and moves ipl code from s390_virtio.c
Patch 2: Adds a cpu reset handler to 
Patch 3  Allow to query cpu types via commandline -? argument as well
         as via qmp

Christian Borntraeger (1):
  s390: Move IPL code into a separate device

Jens Freimann (1):
  s390: Add CPU reset handler

Viktor Mihajlovski (1):
  S390: Enable -cpu help and QMP query-cpu-definitions

 hw/s390-virtio.c       | 104 +++++----------------------------
 hw/s390x/Makefile.objs |   1 +
 hw/s390x/ipl.c         | 153 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-s390x/cpu.c     |  55 +++++++++++++++++-
 target-s390x/cpu.h     |   3 +
 target-s390x/kvm.c     |   9 ++-
 6 files changed, 233 insertions(+), 92 deletions(-)
 create mode 100644 hw/s390x/ipl.c

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/3] s390: Move IPL code into a separate device

[Jens Freimann]

From: Christian Borntraeger <borntraeger@de.ibm.com>

Lets move the code to setup IPL for external kernel
or via the zipl rom into a separate file. This allows to

- define a reboot handler, setting up the PSW appropriately
- enhance the boot code to IPL disks that contain a bootmap that
  was created with zipl under LPAR or z/VM (future patch)
- reuse that code for several machines (e.g. virtio-ccw and virtio-s390)
- allow different machines to provide different defaults

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
v1 -> v2:
 * get rid of ipl.h
 * move defines to ipl.c and make s390_ipl_cpu static

---
 hw/s390-virtio.c       |  98 ++++---------------------------
 hw/s390x/Makefile.objs |   1 +
 hw/s390x/ipl.c         | 153 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+), 88 deletions(-)
 create mode 100644 hw/s390x/ipl.c

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index ca1bb09..a350430 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -25,7 +25,6 @@
 #include "boards.h"
 #include "monitor.h"
 #include "loader.h"
-#include "elf.h"
 #include "hw/virtio.h"
 #include "hw/sysbus.h"
 #include "kvm.h"
@@ -48,17 +47,6 @@
 #define KVM_S390_VIRTIO_RESET           1
 #define KVM_S390_VIRTIO_SET_STATUS      2
 
-#define KERN_IMAGE_START                0x010000UL
-#define KERN_PARM_AREA                  0x010480UL
-#define INITRD_START                    0x800000UL
-#define INITRD_PARM_START               0x010408UL
-#define INITRD_PARM_SIZE                0x010410UL
-#define PARMFILE_START                  0x001000UL
-
-#define ZIPL_START			0x009000UL
-#define ZIPL_LOAD_ADDR			0x009000UL
-#define ZIPL_FILENAME			"s390-zipl.rom"
-
 #define MAX_BLK_DEVS                    10
 
 static VirtIOS390Bus *s390_bus;
@@ -156,15 +144,10 @@ static void s390_init(QEMUMachineInitArgs *args)
 {
     ram_addr_t my_ram_size = args->ram_size;
     const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
     CPUS390XState *env = NULL;
+    DeviceState *dev;
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
-    ram_addr_t kernel_size = 0;
-    ram_addr_t initrd_offset;
-    ram_addr_t initrd_size = 0;
     int shift = 0;
     uint8_t *storage_keys;
     void *virtio_region;
@@ -185,6 +168,15 @@ static void s390_init(QEMUMachineInitArgs *args)
     /* get a BUS */
     s390_bus = s390_virtio_bus_init(&my_ram_size);
     s390_sclp_init();
+    dev  = qdev_create(NULL, "s390-ipl");
+    if (args->kernel_filename) {
+        qdev_prop_set_string(dev, "kernel", args->kernel_filename);
+    }
+    if (args->initrd_filename) {
+        qdev_prop_set_string(dev, "initrd", args->initrd_filename);
+    }
+    qdev_prop_set_string(dev, "cmdline", args->kernel_cmdline);
+    qdev_init_nofail(dev);
 
     /* allocate RAM */
     memory_region_init_ram(ram, "s390.ram", my_ram_size);
@@ -225,76 +217,6 @@ static void s390_init(QEMUMachineInitArgs *args)
         tmp_env->storage_keys = storage_keys;
     }
 
-    /* One CPU has to run */
-    s390_add_running_cpu(env);
-
-    if (kernel_filename) {
-
-        kernel_size = load_elf(kernel_filename, NULL, NULL, NULL, NULL,
-                               NULL, 1, ELF_MACHINE, 0);
-        if (kernel_size == -1UL) {
-            kernel_size = load_image_targphys(kernel_filename, 0, ram_size);
-        }
-        if (kernel_size == -1UL) {
-            fprintf(stderr, "qemu: could not load kernel '%s'\n",
-                    kernel_filename);
-            exit(1);
-        }
-        /*
-         * we can not rely on the ELF entry point, since up to 3.2 this
-         * value was 0x800 (the SALIPL loader) and it wont work. For
-         * all (Linux) cases 0x10000 (KERN_IMAGE_START) should be fine.
-         */
-        env->psw.addr = KERN_IMAGE_START;
-        env->psw.mask = 0x0000000180000000ULL;
-    } else {
-        ram_addr_t bios_size = 0;
-        char *bios_filename;
-
-        /* Load zipl bootloader */
-        if (bios_name == NULL) {
-            bios_name = ZIPL_FILENAME;
-        }
-
-        bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-        bios_size = load_image_targphys(bios_filename, ZIPL_LOAD_ADDR, 4096);
-        g_free(bios_filename);
-
-        if ((long)bios_size < 0) {
-            hw_error("could not load bootloader '%s'\n", bios_name);
-        }
-
-        if (bios_size > 4096) {
-            hw_error("stage1 bootloader is > 4k\n");
-        }
-
-        env->psw.addr = ZIPL_START;
-        env->psw.mask = 0x0000000180000000ULL;
-    }
-
-    if (initrd_filename) {
-        initrd_offset = INITRD_START;
-        while (kernel_size + 0x100000 > initrd_offset) {
-            initrd_offset += 0x100000;
-        }
-        initrd_size = load_image_targphys(initrd_filename, initrd_offset,
-                                          ram_size - initrd_offset);
-        if (initrd_size == -1UL) {
-            fprintf(stderr, "qemu: could not load initrd '%s'\n",
-                    initrd_filename);
-            exit(1);
-        }
-
-        /* we have to overwrite values in the kernel image, which are "rom" */
-        stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
-        stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
-    }
-
-    if (rom_ptr(KERN_PARM_AREA)) {
-        /* we have to overwrite values in the kernel image, which are "rom" */
-        memcpy(rom_ptr(KERN_PARM_AREA), kernel_cmdline,
-               strlen(kernel_cmdline) + 1);
-    }
 
     /* Create VirtIO network adapters */
     for(i = 0; i < nb_nics; i++) {
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 096dfcd..4a5a5d8 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -4,3 +4,4 @@ obj-y := $(addprefix ../,$(obj-y))
 obj-y += sclp.o
 obj-y += event-facility.o
 obj-y += sclpquiesce.o sclpconsole.o
+obj-y += ipl.o
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
new file mode 100644
index 0000000..945a9ba
--- /dev/null
+++ b/hw/s390x/ipl.c
@@ -0,0 +1,153 @@
+/*
+ * bootloader support
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Christian Borntraeger <borntraeger@de.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at your
+ * option) any later version.  See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <sysemu.h>
+#include "cpu.h"
+#include "elf.h"
+#include "hw/loader.h"
+#include "hw/sysbus.h"
+
+#define KERN_IMAGE_START                0x010000UL
+#define KERN_PARM_AREA                  0x010480UL
+#define INITRD_START                    0x800000UL
+#define INITRD_PARM_START               0x010408UL
+#define INITRD_PARM_SIZE                0x010410UL
+#define PARMFILE_START                  0x001000UL
+#define ZIPL_FILENAME                   "s390-zipl.rom"
+#define ZIPL_IMAGE_START                0x009000UL
+#define IPL_PSW_MASK                    0x0000000180000000ULL
+
+typedef struct {
+    SysBusDevice dev;
+    char *kernel;
+    char *initrd;
+    char *cmdline;
+} S390IPLState;
+
+static void s390_ipl_cpu(uint64_t pswaddr)
+{
+    CPUS390XState *env = qemu_get_cpu(0);
+    env->psw.addr = pswaddr;
+    env->psw.mask = IPL_PSW_MASK;
+    s390_add_running_cpu(env);
+}
+
+static int s390_ipl_init(SysBusDevice *dev)
+{
+    S390IPLState *ipl = DO_UPCAST(S390IPLState, dev, dev);
+    ram_addr_t kernel_size = 0;
+
+    if (!ipl->kernel) {
+        ram_addr_t bios_size = 0;
+        char *bios_filename;
+
+        /* Load zipl bootloader */
+        if (bios_name == NULL) {
+            bios_name = ZIPL_FILENAME;
+        }
+
+        bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+        bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START, 4096);
+        g_free(bios_filename);
+
+        if ((long)bios_size < 0) {
+            hw_error("could not load bootloader '%s'\n", bios_name);
+        }
+
+        if (bios_size > 4096) {
+            hw_error("stage1 bootloader is > 4k\n");
+        }
+        return 0;
+    } else {
+        kernel_size = load_elf(ipl->kernel, NULL, NULL, NULL, NULL,
+                               NULL, 1, ELF_MACHINE, 0);
+        if (kernel_size == -1UL) {
+            kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
+        }
+        if (kernel_size == -1UL) {
+            fprintf(stderr, "could not load kernel '%s'\n", ipl->kernel);
+            return -1;
+        }
+        /* we have to overwrite values in the kernel image, which are "rom" */
+        strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
+    }
+    if (ipl->initrd) {
+        ram_addr_t initrd_offset, initrd_size;
+
+        initrd_offset = INITRD_START;
+        while (kernel_size + 0x100000 > initrd_offset) {
+            initrd_offset += 0x100000;
+        }
+        initrd_size = load_image_targphys(ipl->initrd, initrd_offset,
+                                          ram_size - initrd_offset);
+        if (initrd_size == -1UL) {
+            fprintf(stderr, "qemu: could not load initrd '%s'\n", ipl->initrd);
+            exit(1);
+        }
+
+        /* we have to overwrite values in the kernel image, which are "rom" */
+        stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
+        stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
+    }
+
+    return 0;
+}
+
+static Property s390_ipl_properties[] = {
+    DEFINE_PROP_STRING("kernel", S390IPLState, kernel),
+    DEFINE_PROP_STRING("initrd", S390IPLState, initrd),
+    DEFINE_PROP_STRING("cmdline", S390IPLState, cmdline),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void s390_ipl_reset(DeviceState *dev)
+{
+    S390IPLState *ipl = DO_UPCAST(S390IPLState, dev.qdev, dev);
+
+    if (ipl->kernel) {
+        /*
+         * we can not rely on the ELF entry point, since up to 3.2 this
+         * value was 0x800 (the SALIPL loader) and it wont work. For
+         * all (Linux) cases 0x10000 (KERN_IMAGE_START) should be fine.
+         */
+        return s390_ipl_cpu(KERN_IMAGE_START);
+    } else {
+        return s390_ipl_cpu(ZIPL_IMAGE_START);
+    }
+}
+
+static void s390_ipl_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = s390_ipl_init;
+    dc->props = s390_ipl_properties;
+    dc->reset = s390_ipl_reset;
+    dc->no_user = 1;
+}
+
+static TypeInfo s390_ipl_info = {
+    .class_init = s390_ipl_class_init,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .name  = "s390-ipl",
+    .instance_size  = sizeof(S390IPLState),
+};
+
+static void s390_register_ipl(void)
+{
+    type_register_static(&s390_ipl_info);
+}
+
+type_init(s390_register_ipl)
+
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/3] s390: Add CPU reset handler

[Jens Freimann]

Add a CPU reset handler to have all CPUs in a PoP compliant
state.

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>

---
v2 -> v3: 
* explain in comment which code sets cpu 0 to running during IPL

v1 -> v2:
* move setting of control registers and psa to s390_cpu_reset
  and call it from the new s390_machine_cpu_reset_cb()
  This makes it more similar to how it is done on x86
* in s390_cpu_reset() set env->halted state of cpu after
  the memset. This is needed to keep our s390_cpu_running
  counter in sync when s390_cpu_reset is called via the
  qemu_devices_reset path
* set env->halted state in s390_cpu_initfn to 1 to avoid
  decrementing the cpu counter during first reset
---
 target-s390x/cpu.c | 32 ++++++++++++++++++++++++++++++--
 target-s390x/kvm.c |  9 ++++++++-
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 619b202..75d4036 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2009 Ulrich Hecht
  * Copyright (c) 2011 Alexander Graf
  * Copyright (c) 2012 SUSE LINUX Products GmbH
+ * Copyright (c) 2012 IBM Corp.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -18,9 +19,13 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see
  * <http://www.gnu.org/licenses/lgpl-2.1.html>
+ * Contributions after 2012-12-11 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ *
  */
 
 #include "cpu.h"
+#include "hw/hw.h"
 #include "qemu-common.h"
 #include "qemu-timer.h"
 
@@ -37,12 +42,30 @@ static void s390_cpu_reset(CPUState *s)
         log_cpu_state(env, 0);
     }
 
-    scc->parent_reset(s);
+    s390_del_running_cpu(env);
 
+    scc->parent_reset(s);
     memset(env, 0, offsetof(CPUS390XState, breakpoints));
+
+    /* architectured initial values for CR 0 and 14 */
+    env->cregs[0] = 0xE0UL;
+    env->cregs[14] = 0xC2000000UL;
+    /* set to z/Architecture mode */
+    env->psw.mask = 0x0000000180000000ULL;
+    env->psa = 0;
+    /* set halted to 1 to make sure we can add the cpu in
+     * s390_ipl_cpu code, where env->halted is set back to 0
+     * after incrementing the cpu counter */
+    env->halted = 1;
     /* FIXME: reset vector? */
     tlb_flush(env, 1);
-    s390_add_running_cpu(env);
+}
+
+static void s390_cpu_machine_reset_cb(void *opaque)
+{
+    S390CPU *cpu = opaque;
+
+    cpu_reset(CPU(cpu));
 }
 
 static void s390_cpu_initfn(Object *obj)
@@ -66,7 +89,12 @@ static void s390_cpu_initfn(Object *obj)
     env->cpu_num = cpu_num++;
     env->ext_index = -1;
 
+    /* set env->halted state to 1 to avoid decrementing the running
+     * cpu counter in s390_cpu_reset to a negative number at 
+     * initial ipl */
+    env->halted = 1;
     cpu_reset(CPU(cpu));
+    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 }
 
 static void s390_cpu_class_init(ObjectClass *oc, void *data)
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 94de764..fda9f1f 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -85,7 +85,14 @@ int kvm_arch_init_vcpu(CPUS390XState *env)
 
 void kvm_arch_reset_vcpu(CPUS390XState *env)
 {
-    /* FIXME: add code to reset vcpu. */
+   /* The initial reset call is needed here to reset in-kernel
+    * vcpu data that we can't access directly from QEMU
+    * (i.e. with older kernels which don't support sync_regs/ONE_REG).
+    * Before this ioctl cpu_synchronize_state() is called in common kvm
+    * code (kvm-all) */
+    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {
+        perror("Can't reset vcpu\n");
+    }
 }
 
 int kvm_arch_put_registers(CPUS390XState *env, int level)
-- 
1.7.12.4

[Qemu-devel] [PATCH 3/3] S390: Enable -cpu help and QMP query-cpu-definitions

[Jens Freimann]

From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>

This enables qemu -cpu help to return a list of supported CPU models
on s390 and also to query for cpu definitions in the monitor.
Initially only cpu model = host is returned. This needs to be reworked
into a full-fledged CPU model handling later on.
This change is needed to allow libvirt exploiters (like OpenStack)
to specify a CPU model.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
v1 -> v2:
* only print output with CONFIG_KVM

---
 hw/s390-virtio.c   |  6 +++++-
 target-s390x/cpu.c | 23 +++++++++++++++++++++++
 target-s390x/cpu.h |  3 +++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index a350430..60fde26 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -2,6 +2,7 @@
  * QEMU S390 virtio target
  *
  * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ * Copyright IBM Corp 2012
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -13,7 +14,10 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Lesser General Public License for more details.
  *
- * You should have received a copy of the GNU Lesser General Public
+ * Contributions after 2012-10-29 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ *
+ * You should have received a copy of the GNU (Lesser) General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 75d4036..adca789 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -28,8 +28,31 @@
 #include "hw/hw.h"
 #include "qemu-common.h"
 #include "qemu-timer.h"
+#include "arch_init.h"
 
 
+/* generate CPU information for cpu -? */
+void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
+{
+#ifdef CONFIG_KVM
+    (*cpu_fprintf)(f, "s390 %16s\n", "host");
+#endif
+}
+
+CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
+{
+    CpuDefinitionInfoList *entry;
+    CpuDefinitionInfo *info;
+
+    info = g_malloc0(sizeof(*info));
+    info->name = g_strdup("host");
+
+    entry = g_malloc0(sizeof(*entry));
+    entry->value = info;
+
+    return entry;
+}
+
 /* CPUClass::reset() */
 static void s390_cpu_reset(CPUState *s)
 {
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 0f9a1f7..3513976 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -350,6 +350,9 @@ static inline void cpu_set_tls(CPUS390XState *env, target_ulong newtls)
 #define cpu_gen_code cpu_s390x_gen_code
 #define cpu_signal_handler cpu_s390x_signal_handler
 
+void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
+#define cpu_list s390_cpu_list
+
 #include "exec-all.h"
 
 #ifdef CONFIG_USER_ONLY
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2/3] s390: Add CPU reset handler

[Andreas Färber]

Am 14.12.2012 17:46, schrieb Jens Freimann:
> Add a CPU reset handler to have all CPUs in a PoP compliant
> state.
> 
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>

The logic looks okay now. Some comments inline.

> ---
> v2 -> v3: 
> * explain in comment which code sets cpu 0 to running during IPL
> 
> v1 -> v2:
> * move setting of control registers and psa to s390_cpu_reset
>   and call it from the new s390_machine_cpu_reset_cb()
>   This makes it more similar to how it is done on x86
> * in s390_cpu_reset() set env->halted state of cpu after
>   the memset. This is needed to keep our s390_cpu_running
>   counter in sync when s390_cpu_reset is called via the
>   qemu_devices_reset path
> * set env->halted state in s390_cpu_initfn to 1 to avoid
>   decrementing the cpu counter during first reset
> ---
>  target-s390x/cpu.c | 32 ++++++++++++++++++++++++++++++--
>  target-s390x/kvm.c |  9 ++++++++-
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 619b202..75d4036 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -4,6 +4,7 @@
>   * Copyright (c) 2009 Ulrich Hecht
>   * Copyright (c) 2011 Alexander Graf
>   * Copyright (c) 2012 SUSE LINUX Products GmbH
> + * Copyright (c) 2012 IBM Corp.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -18,9 +19,13 @@
>   * You should have received a copy of the GNU Lesser General Public
>   * License along with this library; if not, see
>   * <http://www.gnu.org/licenses/lgpl-2.1.html>
> + * Contributions after 2012-12-11 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + *
>   */
>  
>  #include "cpu.h"
> +#include "hw/hw.h"
>  #include "qemu-common.h"
>  #include "qemu-timer.h"
>  
> @@ -37,12 +42,30 @@ static void s390_cpu_reset(CPUState *s)
>          log_cpu_state(env, 0);
>      }
>  
> -    scc->parent_reset(s);
> +    s390_del_running_cpu(env);
>  
> +    scc->parent_reset(s);

If this gets respun, a white line separating the parent reset from the
local reset would be nice. :)

>      memset(env, 0, offsetof(CPUS390XState, breakpoints));
> +
> +    /* architectured initial values for CR 0 and 14 */
> +    env->cregs[0] = 0xE0UL;
> +    env->cregs[14] = 0xC2000000UL;
> +    /* set to z/Architecture mode */
> +    env->psw.mask = 0x0000000180000000ULL;
> +    env->psa = 0;
> +    /* set halted to 1 to make sure we can add the cpu in
> +     * s390_ipl_cpu code, where env->halted is set back to 0
> +     * after incrementing the cpu counter */
> +    env->halted = 1;
>      /* FIXME: reset vector? */

Do the above added cregs/psw/psa reset values resolve this FIXME? Or
does that refer to something different?

>      tlb_flush(env, 1);
> -    s390_add_running_cpu(env);
> +}
> +
> +static void s390_cpu_machine_reset_cb(void *opaque)
> +{
> +    S390CPU *cpu = opaque;
> +
> +    cpu_reset(CPU(cpu));
>  }
>  
>  static void s390_cpu_initfn(Object *obj)
> @@ -66,7 +89,12 @@ static void s390_cpu_initfn(Object *obj)
>      env->cpu_num = cpu_num++;
>      env->ext_index = -1;
>  
> +    /* set env->halted state to 1 to avoid decrementing the running
> +     * cpu counter in s390_cpu_reset to a negative number at 
> +     * initial ipl */
> +    env->halted = 1;
>      cpu_reset(CPU(cpu));
> +    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);

Since we register the reset handler in instance_init, we should
unregister it in a instance_finalize callback (uninitfn?). Since we do
not hot-unplug s390 CPUs yet to my knowledge, that could be done in a
follow-up.

(For x86 it it registered in the provisional realize function and not
unregistered lacking a matching unrealization mechanism today; elsewhere
reset registration is done in the machine.)

>  }
>  
>  static void s390_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 94de764..fda9f1f 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -85,7 +85,14 @@ int kvm_arch_init_vcpu(CPUS390XState *env)
>  
>  void kvm_arch_reset_vcpu(CPUS390XState *env)
>  {

Note to Alex: In my upcoming KVM CPUState series, this argument type
changes to CPUState ...

> -    /* FIXME: add code to reset vcpu. */
> +   /* The initial reset call is needed here to reset in-kernel
> +    * vcpu data that we can't access directly from QEMU
> +    * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> +    * Before this ioctl cpu_synchronize_state() is called in common kvm
> +    * code (kvm-all) */
> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {

... but so does the argument for kvm_vcpu_ioctl(), so merging becomes a
trivial env -> cpu change.

> +        perror("Can't reset vcpu\n");
> +    }
>  }
>  
>  int kvm_arch_put_registers(CPUS390XState *env, int level)

Andreas

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

Re: [Qemu-devel] [PATCH 3/3] S390: Enable -cpu help and QMP query-cpu-definitions

[Andreas Färber]

Am 14.12.2012 17:46, schrieb Jens Freimann:
> From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> 
> This enables qemu -cpu help to return a list of supported CPU models
> on s390 and also to query for cpu definitions in the monitor.
> Initially only cpu model = host is returned. This needs to be reworked
> into a full-fledged CPU model handling later on.
> This change is needed to allow libvirt exploiters (like OpenStack)
> to specify a CPU model.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

I guess we can live with this solution for now,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Can you follow-up with a strcmp(cpu_model, "host") check for
!kvm_enabled() in cpu_s390x_init() please?

Thanks,
Andreas

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

Re: [Qemu-devel] [PATCH 1/3] s390: Move IPL code into a separate device

[Andreas Färber]

Am 14.12.2012 17:46, schrieb Jens Freimann:
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Lets move the code to setup IPL for external kernel
> or via the zipl rom into a separate file. This allows to
> 
> - define a reboot handler, setting up the PSW appropriately

Careful with the ordering then: Since patch 2/3 adds another reset
handler in the CPU instance_init, the ipl device must be created after
the CPU - I'm guessing this is the case here but will also need to be
assured in the ccw machine.

> - enhance the boot code to IPL disks that contain a bootmap that
>   was created with zipl under LPAR or z/VM (future patch)
> - reuse that code for several machines (e.g. virtio-ccw and virtio-s390)
> - allow different machines to provide different defaults
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> ---
> v1 -> v2:
>  * get rid of ipl.h
>  * move defines to ipl.c and make s390_ipl_cpu static
> 
> ---
>  hw/s390-virtio.c       |  98 ++++---------------------------
>  hw/s390x/Makefile.objs |   1 +
>  hw/s390x/ipl.c         | 153 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 164 insertions(+), 88 deletions(-)
>  create mode 100644 hw/s390x/ipl.c
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index ca1bb09..a350430 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
[...]
> @@ -185,6 +168,15 @@ static void s390_init(QEMUMachineInitArgs *args)
>      /* get a BUS */
>      s390_bus = s390_virtio_bus_init(&my_ram_size);
>      s390_sclp_init();
> +    dev  = qdev_create(NULL, "s390-ipl");
> +    if (args->kernel_filename) {
> +        qdev_prop_set_string(dev, "kernel", args->kernel_filename);
> +    }
> +    if (args->initrd_filename) {
> +        qdev_prop_set_string(dev, "initrd", args->initrd_filename);
> +    }
> +    qdev_prop_set_string(dev, "cmdline", args->kernel_cmdline);

Why NULL checks for 2 out of 3 string properties?

> +    qdev_init_nofail(dev);
>  
>      /* allocate RAM */
>      memory_region_init_ram(ram, "s390.ram", my_ram_size);
[...]
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> new file mode 100644
> index 0000000..945a9ba
> --- /dev/null
> +++ b/hw/s390x/ipl.c

Nice location. :)

> @@ -0,0 +1,153 @@
> +/*
> + * bootloader support
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Christian Borntraeger <borntraeger@de.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
> + * option) any later version.  See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <sysemu.h>

"sysemu.h"?

> +#include "cpu.h"
> +#include "elf.h"
> +#include "hw/loader.h"
> +#include "hw/sysbus.h"
> +
> +#define KERN_IMAGE_START                0x010000UL
> +#define KERN_PARM_AREA                  0x010480UL
> +#define INITRD_START                    0x800000UL
> +#define INITRD_PARM_START               0x010408UL
> +#define INITRD_PARM_SIZE                0x010410UL
> +#define PARMFILE_START                  0x001000UL
> +#define ZIPL_FILENAME                   "s390-zipl.rom"
> +#define ZIPL_IMAGE_START                0x009000UL
> +#define IPL_PSW_MASK                    0x0000000180000000ULL
> +
> +typedef struct {

Anonymous structs are discouraged (not sure where that makes a
difference, maybe gdb?), i.e. typedef struct S390IPLState {

> +    SysBusDevice dev;

Please adopt the following QOM convention:

SysBusDevice parent_obj; // this field is then referenced nowhere
// white line; in header files /*< private/public >*/ gtk-doc annotation
...
> +    char *kernel;
> +    char *initrd;
> +    char *cmdline;
> +} S390IPLState;

I read that you got rid of an ipl.h; since you are using this device
from a machine that seems okay - if used from another object, header
files are encouraged. Or if memory address constants are to be shared
with a qtest test case (don't think that makes sense for a bootloader).

> +
> +static void s390_ipl_cpu(uint64_t pswaddr)
> +{
> +    CPUS390XState *env = qemu_get_cpu(0);
> +    env->psw.addr = pswaddr;
> +    env->psw.mask = IPL_PSW_MASK;
> +    s390_add_running_cpu(env);
> +}
> +
> +static int s390_ipl_init(SysBusDevice *dev)
> +{
> +    S390IPLState *ipl = DO_UPCAST(S390IPLState, dev, dev);

Please use a QOM cast macro S390_IPL(dev) instead of DO_UPCAST().

You'll find many examples in
https://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02746.html

> +    ram_addr_t kernel_size = 0;
> +
> +    if (!ipl->kernel) {
> +        ram_addr_t bios_size = 0;
> +        char *bios_filename;
> +
> +        /* Load zipl bootloader */
> +        if (bios_name == NULL) {
> +            bios_name = ZIPL_FILENAME;
> +        }
> +
> +        bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +        bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START, 4096);
> +        g_free(bios_filename);
> +
> +        if ((long)bios_size < 0) {
> +            hw_error("could not load bootloader '%s'\n", bios_name);
> +        }
> +
> +        if (bios_size > 4096) {
> +            hw_error("stage1 bootloader is > 4k\n");
> +        }
> +        return 0;
> +    } else {
> +        kernel_size = load_elf(ipl->kernel, NULL, NULL, NULL, NULL,
> +                               NULL, 1, ELF_MACHINE, 0);
> +        if (kernel_size == -1UL) {
> +            kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
> +        }
> +        if (kernel_size == -1UL) {
> +            fprintf(stderr, "could not load kernel '%s'\n", ipl->kernel);
> +            return -1;
> +        }
> +        /* we have to overwrite values in the kernel image, which are "rom" */
> +        strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> +    }
> +    if (ipl->initrd) {
> +        ram_addr_t initrd_offset, initrd_size;
> +
> +        initrd_offset = INITRD_START;
> +        while (kernel_size + 0x100000 > initrd_offset) {
> +            initrd_offset += 0x100000;
> +        }
> +        initrd_size = load_image_targphys(ipl->initrd, initrd_offset,
> +                                          ram_size - initrd_offset);
> +        if (initrd_size == -1UL) {
> +            fprintf(stderr, "qemu: could not load initrd '%s'\n", ipl->initrd);
> +            exit(1);
> +        }
> +
> +        /* we have to overwrite values in the kernel image, which are "rom" */
> +        stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
> +        stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
> +    }
> +
> +    return 0;
> +}
> +
> +static Property s390_ipl_properties[] = {
> +    DEFINE_PROP_STRING("kernel", S390IPLState, kernel),
> +    DEFINE_PROP_STRING("initrd", S390IPLState, initrd),
> +    DEFINE_PROP_STRING("cmdline", S390IPLState, cmdline),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void s390_ipl_reset(DeviceState *dev)
> +{
> +    S390IPLState *ipl = DO_UPCAST(S390IPLState, dev.qdev, dev);

Ditto.

> +
> +    if (ipl->kernel) {
> +        /*
> +         * we can not rely on the ELF entry point, since up to 3.2 this
> +         * value was 0x800 (the SALIPL loader) and it wont work. For
> +         * all (Linux) cases 0x10000 (KERN_IMAGE_START) should be fine.
> +         */
> +        return s390_ipl_cpu(KERN_IMAGE_START);
> +    } else {
> +        return s390_ipl_cpu(ZIPL_IMAGE_START);
> +    }
> +}
> +
> +static void s390_ipl_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = s390_ipl_init;
> +    dc->props = s390_ipl_properties;
> +    dc->reset = s390_ipl_reset;
> +    dc->no_user = 1;
> +}
> +
> +static TypeInfo s390_ipl_info = {

static const

> +    .class_init = s390_ipl_class_init,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .name  = "s390-ipl",
> +    .instance_size  = sizeof(S390IPLState),
> +};
> +
> +static void s390_register_ipl(void)

s390_ipl_register_types?

> +{
> +    type_register_static(&s390_ipl_info);
> +}
> +
> +type_init(s390_register_ipl)
> +

Trailing white line.

Can't fully judge the IPL logic but the code movement looks sensible.

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

Re: [Qemu-devel] [PATCH 1/3] s390: Move IPL code into a separate device

[Christian Borntraeger]

On 16/12/12 17:26, Andreas Färber wrote:
> Am 14.12.2012 17:46, schrieb Jens Freimann:
>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>>
>> Lets move the code to setup IPL for external kernel
>> or via the zipl rom into a separate file. This allows to
>>
>> - define a reboot handler, setting up the PSW appropriately
> 
> Careful with the ordering then: Since patch 2/3 adds another reset
> handler in the CPU instance_init, the ipl device must be created after
> the CPU - I'm guessing this is the case here but will also need to be
> assured in the ccw machine.
> 
>> - enhance the boot code to IPL disks that contain a bootmap that
>>   was created with zipl under LPAR or z/VM (future patch)
>> - reuse that code for several machines (e.g. virtio-ccw and virtio-s390)
>> - allow different machines to provide different defaults
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> ---
>> v1 -> v2:
>>  * get rid of ipl.h
>>  * move defines to ipl.c and make s390_ipl_cpu static
>>
>> ---
>>  hw/s390-virtio.c       |  98 ++++---------------------------
>>  hw/s390x/Makefile.objs |   1 +
>>  hw/s390x/ipl.c         | 153 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 164 insertions(+), 88 deletions(-)
>>  create mode 100644 hw/s390x/ipl.c
>>
>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>> index ca1bb09..a350430 100644
>> --- a/hw/s390-virtio.c
>> +++ b/hw/s390-virtio.c
> [...]
>> @@ -185,6 +168,15 @@ static void s390_init(QEMUMachineInitArgs *args)
>>      /* get a BUS */
>>      s390_bus = s390_virtio_bus_init(&my_ram_size);
>>      s390_sclp_init();
>> +    dev  = qdev_create(NULL, "s390-ipl");
>> +    if (args->kernel_filename) {
>> +        qdev_prop_set_string(dev, "kernel", args->kernel_filename);
>> +    }
>> +    if (args->initrd_filename) {
>> +        qdev_prop_set_string(dev, "initrd", args->initrd_filename);
>> +    }
>> +    qdev_prop_set_string(dev, "cmdline", args->kernel_cmdline);
> 
> Why NULL checks for 2 out of 3 string properties?

cmdline is always a valid string, (never NULL), but kernel and initrd can
be NULL, which kills qdev_prop_set_string.


>> + * Authors:
>> + *  Christian Borntraeger <borntraeger@de.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
>> + * option) any later version.  See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include <sysemu.h>
> 
> "sysemu.h"?

bios_name. 
I could use another property which is modified/set by the machine init.

> 
>> +#include "cpu.h"
>> +#include "elf.h"
>> +#include "hw/loader.h"
>> +#include "hw/sysbus.h"
>> +
>> +#define KERN_IMAGE_START                0x010000UL
>> +#define KERN_PARM_AREA                  0x010480UL
>> +#define INITRD_START                    0x800000UL
>> +#define INITRD_PARM_START               0x010408UL
>> +#define INITRD_PARM_SIZE                0x010410UL
>> +#define PARMFILE_START                  0x001000UL
>> +#define ZIPL_FILENAME                   "s390-zipl.rom"
>> +#define ZIPL_IMAGE_START                0x009000UL
>> +#define IPL_PSW_MASK                    0x0000000180000000ULL
>> +
>> +typedef struct {
> 
> Anonymous structs are discouraged (not sure where that makes a
> difference, maybe gdb?), i.e. typedef struct S390IPLState {
> 
>> +    SysBusDevice dev;
> 
> Please adopt the following QOM convention:
> 
> SysBusDevice parent_obj; // this field is then referenced nowhere


ok

>> +
>> +static void s390_ipl_cpu(uint64_t pswaddr)
>> +{
>> +    CPUS390XState *env = qemu_get_cpu(0);
>> +    env->psw.addr = pswaddr;
>> +    env->psw.mask = IPL_PSW_MASK;
>> +    s390_add_running_cpu(env);
>> +}
>> +
>> +static int s390_ipl_init(SysBusDevice *dev)
>> +{
>> +    S390IPLState *ipl = DO_UPCAST(S390IPLState, dev, dev);
> 
> Please use a QOM cast macro S390_IPL(dev) instead of DO_UPCAST().
> 
> You'll find many examples in
> https://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02746.html

OK.

[..]
> 
>> +static TypeInfo s390_ipl_info = {
> 
> static const

ok
> 
>> +    .class_init = s390_ipl_class_init,
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .name  = "s390-ipl",
>> +    .instance_size  = sizeof(S390IPLState),
>> +};
>> +
>> +static void s390_register_ipl(void)
> 
> s390_ipl_register_types?

makes sense.
> 
>> +{
>> +    type_register_static(&s390_ipl_info);
>> +}
>> +
>> +type_init(s390_register_ipl)
>> +
> 
> Trailing white line.
ok

Re: [Qemu-devel] [PATCH 2/3] s390: Add CPU reset handler

[Jens Freimann]

On Sun, Dec 16, 2012 at 04:30:21PM +0100, Andreas Färber wrote:
> Am 14.12.2012 17:46, schrieb Jens Freimann:
> > Add a CPU reset handler to have all CPUs in a PoP compliant
> > state.
> > 
> > Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> 
> The logic looks okay now. Some comments inline.
> 
> > ---
> > v2 -> v3: 
> > * explain in comment which code sets cpu 0 to running during IPL
> > 
> > v1 -> v2:
> > * move setting of control registers and psa to s390_cpu_reset
> >   and call it from the new s390_machine_cpu_reset_cb()
> >   This makes it more similar to how it is done on x86
> > * in s390_cpu_reset() set env->halted state of cpu after
> >   the memset. This is needed to keep our s390_cpu_running
> >   counter in sync when s390_cpu_reset is called via the
> >   qemu_devices_reset path
> > * set env->halted state in s390_cpu_initfn to 1 to avoid
> >   decrementing the cpu counter during first reset
> > ---
> >  target-s390x/cpu.c | 32 ++++++++++++++++++++++++++++++--
> >  target-s390x/kvm.c |  9 ++++++++-
> >  2 files changed, 38 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> > index 619b202..75d4036 100644
> > --- a/target-s390x/cpu.c
> > +++ b/target-s390x/cpu.c
> > @@ -4,6 +4,7 @@
> >   * Copyright (c) 2009 Ulrich Hecht
> >   * Copyright (c) 2011 Alexander Graf
> >   * Copyright (c) 2012 SUSE LINUX Products GmbH
> > + * Copyright (c) 2012 IBM Corp.
> >   *
> >   * This library is free software; you can redistribute it and/or
> >   * modify it under the terms of the GNU Lesser General Public
> > @@ -18,9 +19,13 @@
> >   * You should have received a copy of the GNU Lesser General Public
> >   * License along with this library; if not, see
> >   * <http://www.gnu.org/licenses/lgpl-2.1.html>
> > + * Contributions after 2012-12-11 are licensed under the terms of the
> > + * GNU GPL, version 2 or (at your option) any later version.
> > + *
> >   */
> >  
> >  #include "cpu.h"
> > +#include "hw/hw.h"
> >  #include "qemu-common.h"
> >  #include "qemu-timer.h"
> >  
> > @@ -37,12 +42,30 @@ static void s390_cpu_reset(CPUState *s)
> >          log_cpu_state(env, 0);
> >      }
> >  
> > -    scc->parent_reset(s);
> > +    s390_del_running_cpu(env);
> >  
> > +    scc->parent_reset(s);
> 
> If this gets respun, a white line separating the parent reset from the
> local reset would be nice. :)

Ok

> >      memset(env, 0, offsetof(CPUS390XState, breakpoints));
> > +
> > +    /* architectured initial values for CR 0 and 14 */
> > +    env->cregs[0] = 0xE0UL;
> > +    env->cregs[14] = 0xC2000000UL;
> > +    /* set to z/Architecture mode */
> > +    env->psw.mask = 0x0000000180000000ULL;
> > +    env->psa = 0;
> > +    /* set halted to 1 to make sure we can add the cpu in
> > +     * s390_ipl_cpu code, where env->halted is set back to 0
> > +     * after incrementing the cpu counter */
> > +    env->halted = 1;
> >      /* FIXME: reset vector? */
> 
> Do the above added cregs/psw/psa reset values resolve this FIXME? Or
> does that refer to something different?

Yes, together with the ipl device this fixme is resolved.

> >      tlb_flush(env, 1);
> > -    s390_add_running_cpu(env);
> > +}
> > +
> > +static void s390_cpu_machine_reset_cb(void *opaque)
> > +{
> > +    S390CPU *cpu = opaque;
> > +
> > +    cpu_reset(CPU(cpu));
> >  }
> >  
> >  static void s390_cpu_initfn(Object *obj)
> > @@ -66,7 +89,12 @@ static void s390_cpu_initfn(Object *obj)
> >      env->cpu_num = cpu_num++;
> >      env->ext_index = -1;
> >  
> > +    /* set env->halted state to 1 to avoid decrementing the running
> > +     * cpu counter in s390_cpu_reset to a negative number at 
> > +     * initial ipl */
> > +    env->halted = 1;
> >      cpu_reset(CPU(cpu));
> > +    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> 
> Since we register the reset handler in instance_init, we should
> unregister it in a instance_finalize callback (uninitfn?). Since we do
> not hot-unplug s390 CPUs yet to my knowledge, that could be done in a
> follow-up.
> 
> (For x86 it it registered in the provisional realize function and not
> unregistered lacking a matching unrealization mechanism today; elsewhere
> reset registration is done in the machine.)

Ok, I'll keep that in mind and send a followup patch.

Thanks!
Jens

> 
> >  }
> >  
> >  static void s390_cpu_class_init(ObjectClass *oc, void *data)
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 94de764..fda9f1f 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -85,7 +85,14 @@ int kvm_arch_init_vcpu(CPUS390XState *env)
> >  
> >  void kvm_arch_reset_vcpu(CPUS390XState *env)
> >  {
> 
> Note to Alex: In my upcoming KVM CPUState series, this argument type
> changes to CPUState ...
> 
> > -    /* FIXME: add code to reset vcpu. */
> > +   /* The initial reset call is needed here to reset in-kernel
> > +    * vcpu data that we can't access directly from QEMU
> > +    * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> > +    * Before this ioctl cpu_synchronize_state() is called in common kvm
> > +    * code (kvm-all) */
> > +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {
> 
> ... but so does the argument for kvm_vcpu_ioctl(), so merging becomes a
> trivial env -> cpu change.
> 
> > +        perror("Can't reset vcpu\n");
> > +    }
> >  }
> >  
> >  int kvm_arch_put_registers(CPUS390XState *env, int level)
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 

-- 
Mit freundlichen Grüßen / Kind regards 

Jens Freimann 

-- 
IBM Linux Technology Center / Boeblingen lab
IBM Systems &Technology Group, Systems Software Development
-------------------------------------------------------------
IBM Deutschland
Schoenaicher Str 220
71032 Boeblingen
Phone: +49-7031-16 x1122
E-Mail: jfrei@de.ibm.com
-------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [Qemu-devel] [PATCH 3/3] S390: Enable -cpu help and QMP query-cpu-definitions

[Alexander Graf]

On 14.12.2012, at 17:46, Jens Freimann wrote:

> From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> 
> This enables qemu -cpu help to return a list of supported CPU models
> on s390 and also to query for cpu definitions in the monitor.
> Initially only cpu model = host is returned. This needs to be reworked
> into a full-fledged CPU model handling later on.
> This change is needed to allow libvirt exploiters (like OpenStack)
> to specify a CPU model.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> v1 -> v2:
> * only print output with CONFIG_KVM
> 
> ---
> hw/s390-virtio.c   |  6 +++++-
> target-s390x/cpu.c | 23 +++++++++++++++++++++++
> target-s390x/cpu.h |  3 +++
> 3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index a350430..60fde26 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -2,6 +2,7 @@
>  * QEMU S390 virtio target
>  *
>  * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> + * Copyright IBM Corp 2012
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
> @@ -13,7 +14,10 @@
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>  * Lesser General Public License for more details.
>  *
> - * You should have received a copy of the GNU Lesser General Public
> + * Contributions after 2012-10-29 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU (Lesser) General Public
>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>  */
> 
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 75d4036..adca789 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -28,8 +28,31 @@
> #include "hw/hw.h"
> #include "qemu-common.h"
> #include "qemu-timer.h"
> +#include "arch_init.h"
> 
> 
> +/* generate CPU information for cpu -? */
> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> +{
> +#ifdef CONFIG_KVM
> +    (*cpu_fprintf)(f, "s390 %16s\n", "host");
> +#endif
> +}
> +

static const struct CpuDefinitionInfo cpu_info[] = {
#ifdef CONFIG_KVM
    {
        .name = "host",
    },
#endif
};

static const struct CPUDefinitionInfoList cpu_entry = {
    value = cpu_info,
};

> +CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
> +{
> +    CpuDefinitionInfoList *entry;
> +    CpuDefinitionInfo *info;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup("host");
> +
> +    entry = g_malloc0(sizeof(*entry));
> +    entry->value = info;

return &entry;

(completely untested, don't you think the above would work? The code as is looks quite leaky.)

> +
> +    return entry;
> +}
> +
> /* CPUClass::reset() */
> static void s390_cpu_reset(CPUState *s)
> {
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 0f9a1f7..3513976 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -350,6 +350,9 @@ static inline void cpu_set_tls(CPUS390XState *env, target_ulong newtls)
> #define cpu_gen_code cpu_s390x_gen_code
> #define cpu_signal_handler cpu_s390x_signal_handler
> 
> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> +#define cpu_list s390_cpu_list
> +
> #include "exec-all.h"
> 
> #ifdef CONFIG_USER_ONLY
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 2/3] s390: Add CPU reset handler

[Alexander Graf]

On 14.12.2012, at 17:46, Jens Freimann wrote:

> Add a CPU reset handler to have all CPUs in a PoP compliant
> state.
> 
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> 
> ---
> v2 -> v3: 
> * explain in comment which code sets cpu 0 to running during IPL
> 
> v1 -> v2:
> * move setting of control registers and psa to s390_cpu_reset
>  and call it from the new s390_machine_cpu_reset_cb()
>  This makes it more similar to how it is done on x86
> * in s390_cpu_reset() set env->halted state of cpu after
>  the memset. This is needed to keep our s390_cpu_running
>  counter in sync when s390_cpu_reset is called via the
>  qemu_devices_reset path
> * set env->halted state in s390_cpu_initfn to 1 to avoid
>  decrementing the cpu counter during first reset
> ---
> target-s390x/cpu.c | 32 ++++++++++++++++++++++++++++++--
> target-s390x/kvm.c |  9 ++++++++-
> 2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 619b202..75d4036 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -4,6 +4,7 @@
>  * Copyright (c) 2009 Ulrich Hecht
>  * Copyright (c) 2011 Alexander Graf
>  * Copyright (c) 2012 SUSE LINUX Products GmbH
> + * Copyright (c) 2012 IBM Corp.
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
> @@ -18,9 +19,13 @@
>  * You should have received a copy of the GNU Lesser General Public
>  * License along with this library; if not, see
>  * <http://www.gnu.org/licenses/lgpl-2.1.html>
> + * Contributions after 2012-12-11 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + *
>  */
> 
> #include "cpu.h"
> +#include "hw/hw.h"
> #include "qemu-common.h"
> #include "qemu-timer.h"
> 
> @@ -37,12 +42,30 @@ static void s390_cpu_reset(CPUState *s)
>         log_cpu_state(env, 0);
>     }
> 
> -    scc->parent_reset(s);
> +    s390_del_running_cpu(env);
> 
> +    scc->parent_reset(s);
>     memset(env, 0, offsetof(CPUS390XState, breakpoints));

Shouldn't parent_reset already do the memset?

> +
> +    /* architectured initial values for CR 0 and 14 */
> +    env->cregs[0] = 0xE0UL;
> +    env->cregs[14] = 0xC2000000UL;
> +    /* set to z/Architecture mode */
> +    env->psw.mask = 0x0000000180000000ULL;

While at it, please convert this into something that uses #define's to make things readable.


Alex

> +    env->psa = 0;
> +    /* set halted to 1 to make sure we can add the cpu in
> +     * s390_ipl_cpu code, where env->halted is set back to 0
> +     * after incrementing the cpu counter */
> +    env->halted = 1;
>     /* FIXME: reset vector? */
>     tlb_flush(env, 1);
> -    s390_add_running_cpu(env);
> +}
> +
> +static void s390_cpu_machine_reset_cb(void *opaque)
> +{
> +    S390CPU *cpu = opaque;
> +
> +    cpu_reset(CPU(cpu));
> }
> 
> static void s390_cpu_initfn(Object *obj)
> @@ -66,7 +89,12 @@ static void s390_cpu_initfn(Object *obj)
>     env->cpu_num = cpu_num++;
>     env->ext_index = -1;
> 
> +    /* set env->halted state to 1 to avoid decrementing the running
> +     * cpu counter in s390_cpu_reset to a negative number at 
> +     * initial ipl */
> +    env->halted = 1;
>     cpu_reset(CPU(cpu));
> +    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> }
> 
> static void s390_cpu_class_init(ObjectClass *oc, void *data)
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 94de764..fda9f1f 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -85,7 +85,14 @@ int kvm_arch_init_vcpu(CPUS390XState *env)
> 
> void kvm_arch_reset_vcpu(CPUS390XState *env)
> {
> -    /* FIXME: add code to reset vcpu. */
> +   /* The initial reset call is needed here to reset in-kernel
> +    * vcpu data that we can't access directly from QEMU
> +    * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> +    * Before this ioctl cpu_synchronize_state() is called in common kvm
> +    * code (kvm-all) */
> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {
> +        perror("Can't reset vcpu\n");
> +    }
> }
> 
> int kvm_arch_put_registers(CPUS390XState *env, int level)
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 2/3] s390: Add CPU reset handler

[Jens Freimann]

On Mon, Dec 17, 2012 at 03:49:41PM +0100, Alexander Graf wrote:
> 
> On 14.12.2012, at 17:46, Jens Freimann wrote:
> 
> > Add a CPU reset handler to have all CPUs in a PoP compliant
> > state.
> > 
> > Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> > 
> > ---
> > v2 -> v3: 
> > * explain in comment which code sets cpu 0 to running during IPL
> > 
> > v1 -> v2:
> > * move setting of control registers and psa to s390_cpu_reset
> >  and call it from the new s390_machine_cpu_reset_cb()
> >  This makes it more similar to how it is done on x86
> > * in s390_cpu_reset() set env->halted state of cpu after
> >  the memset. This is needed to keep our s390_cpu_running
> >  counter in sync when s390_cpu_reset is called via the
> >  qemu_devices_reset path
> > * set env->halted state in s390_cpu_initfn to 1 to avoid
> >  decrementing the cpu counter during first reset
> > ---
> > target-s390x/cpu.c | 32 ++++++++++++++++++++++++++++++--
> > target-s390x/kvm.c |  9 ++++++++-
> > 2 files changed, 38 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> > index 619b202..75d4036 100644
> > --- a/target-s390x/cpu.c
> > +++ b/target-s390x/cpu.c
> > @@ -4,6 +4,7 @@
> >  * Copyright (c) 2009 Ulrich Hecht
> >  * Copyright (c) 2011 Alexander Graf
> >  * Copyright (c) 2012 SUSE LINUX Products GmbH
> > + * Copyright (c) 2012 IBM Corp.
> >  *
> >  * This library is free software; you can redistribute it and/or
> >  * modify it under the terms of the GNU Lesser General Public
> > @@ -18,9 +19,13 @@
> >  * You should have received a copy of the GNU Lesser General Public
> >  * License along with this library; if not, see
> >  * <http://www.gnu.org/licenses/lgpl-2.1.html>
> > + * Contributions after 2012-12-11 are licensed under the terms of the
> > + * GNU GPL, version 2 or (at your option) any later version.
> > + *
> >  */
> > 
> > #include "cpu.h"
> > +#include "hw/hw.h"
> > #include "qemu-common.h"
> > #include "qemu-timer.h"
> > 
> > @@ -37,12 +42,30 @@ static void s390_cpu_reset(CPUState *s)
> >         log_cpu_state(env, 0);
> >     }
> > 
> > -    scc->parent_reset(s);
> > +    s390_del_running_cpu(env);
> > 
> > +    scc->parent_reset(s);
> >     memset(env, 0, offsetof(CPUS390XState, breakpoints));
> 
> Shouldn't parent_reset already do the memset?

parent_reset is cpu_common_reset() in qom/cpu.c and that
is an empty function as of now. It's what ARM and x86 do as well.

> > +
> > +    /* architectured initial values for CR 0 and 14 */
> > +    env->cregs[0] = 0xE0UL;
> > +    env->cregs[14] = 0xC2000000UL;
> > +    /* set to z/Architecture mode */
> > +    env->psw.mask = 0x0000000180000000ULL;
> 
> While at it, please convert this into something that uses #define's to make things readable.

ok

Jens

> 
> Alex
> 
> > +    env->psa = 0;
> > +    /* set halted to 1 to make sure we can add the cpu in
> > +     * s390_ipl_cpu code, where env->halted is set back to 0
> > +     * after incrementing the cpu counter */
> > +    env->halted = 1;
> >     /* FIXME: reset vector? */
> >     tlb_flush(env, 1);
> > -    s390_add_running_cpu(env);
> > +}
> > +
> > +static void s390_cpu_machine_reset_cb(void *opaque)
> > +{
> > +    S390CPU *cpu = opaque;
> > +
> > +    cpu_reset(CPU(cpu));
> > }
> > 
> > static void s390_cpu_initfn(Object *obj)
> > @@ -66,7 +89,12 @@ static void s390_cpu_initfn(Object *obj)
> >     env->cpu_num = cpu_num++;
> >     env->ext_index = -1;
> > 
> > +    /* set env->halted state to 1 to avoid decrementing the running
> > +     * cpu counter in s390_cpu_reset to a negative number at 
> > +     * initial ipl */
> > +    env->halted = 1;
> >     cpu_reset(CPU(cpu));
> > +    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> > }
> > 
> > static void s390_cpu_class_init(ObjectClass *oc, void *data)
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 94de764..fda9f1f 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -85,7 +85,14 @@ int kvm_arch_init_vcpu(CPUS390XState *env)
> > 
> > void kvm_arch_reset_vcpu(CPUS390XState *env)
> > {
> > -    /* FIXME: add code to reset vcpu. */
> > +   /* The initial reset call is needed here to reset in-kernel
> > +    * vcpu data that we can't access directly from QEMU
> > +    * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> > +    * Before this ioctl cpu_synchronize_state() is called in common kvm
> > +    * code (kvm-all) */
> > +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {
> > +        perror("Can't reset vcpu\n");
> > +    }
> > }
> > 
> > int kvm_arch_put_registers(CPUS390XState *env, int level)
> > -- 
> > 1.7.12.4
> > 
> 

-- 
Mit freundlichen Grüßen / Kind regards 

Jens Freimann 

-- 
IBM Linux Technology Center / Boeblingen lab
IBM Systems &Technology Group, Systems Software Development
-------------------------------------------------------------
IBM Deutschland
Schoenaicher Str 220
71032 Boeblingen
Phone: +49-7031-16 x1122
E-Mail: jfrei@de.ibm.com
-------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [Qemu-devel] [PATCH 2/3] s390: Add CPU reset handler

[Andreas Färber]

Am 17.12.2012 15:49, schrieb Alexander Graf:
> 
> On 14.12.2012, at 17:46, Jens Freimann wrote:
> 
>> @@ -37,12 +42,30 @@ static void s390_cpu_reset(CPUState *s)
>>         log_cpu_state(env, 0);
>>     }
>>
>> -    scc->parent_reset(s);
>> +    s390_del_running_cpu(env);
>>
>> +    scc->parent_reset(s);
>>     memset(env, 0, offsetof(CPUS390XState, breakpoints));
> 
> Shouldn't parent_reset already do the memset?

No, because "env" location and size are specific to S390CPU.

And yes, it is ugly boilerplate code, but it cannot be solved with my
CPU_COMMON field movements alone (which partially add explicit reset
code based on the field location), there's quite a large number of
per-target fields that get reset that way, some intentionally, some
accidentally. ;-)

Andreas

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

Re: [Qemu-devel] [PATCH 2/3] s390: Add CPU reset handler

[Alexander Graf]

On 17.12.2012, at 18:21, Andreas Färber wrote:

> Am 17.12.2012 15:49, schrieb Alexander Graf:
>> 
>> On 14.12.2012, at 17:46, Jens Freimann wrote:
>> 
>>> @@ -37,12 +42,30 @@ static void s390_cpu_reset(CPUState *s)
>>>        log_cpu_state(env, 0);
>>>    }
>>> 
>>> -    scc->parent_reset(s);
>>> +    s390_del_running_cpu(env);
>>> 
>>> +    scc->parent_reset(s);
>>>    memset(env, 0, offsetof(CPUS390XState, breakpoints));
>> 
>> Shouldn't parent_reset already do the memset?
> 
> No, because "env" location and size are specific to S390CPU.
> 
> And yes, it is ugly boilerplate code, but it cannot be solved with my
> CPU_COMMON field movements alone (which partially add explicit reset
> code based on the field location), there's quite a large number of
> per-target fields that get reset that way, some intentionally, some
> accidentally. ;-)

I see :)


Alex

Re: [Qemu-devel] [PATCH 3/3] S390: Enable -cpu help and QMP query-cpu-definitions

[Andreas Färber]

Am 17.12.2012 15:47, schrieb Alexander Graf:
> 
> On 14.12.2012, at 17:46, Jens Freimann wrote:
> 
>> From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
>>
>> This enables qemu -cpu help to return a list of supported CPU models
>> on s390 and also to query for cpu definitions in the monitor.
>> Initially only cpu model = host is returned. This needs to be reworked
>> into a full-fledged CPU model handling later on.
>> This change is needed to allow libvirt exploiters (like OpenStack)
>> to specify a CPU model.
>>
>> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>> v1 -> v2:
>> * only print output with CONFIG_KVM
>>
>> ---
>> hw/s390-virtio.c   |  6 +++++-
>> target-s390x/cpu.c | 23 +++++++++++++++++++++++
>> target-s390x/cpu.h |  3 +++
>> 3 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>> index a350430..60fde26 100644
>> --- a/hw/s390-virtio.c
>> +++ b/hw/s390-virtio.c
>> @@ -2,6 +2,7 @@
>>  * QEMU S390 virtio target
>>  *
>>  * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
>> + * Copyright IBM Corp 2012
>>  *
>>  * This library is free software; you can redistribute it and/or
>>  * modify it under the terms of the GNU Lesser General Public
>> @@ -13,7 +14,10 @@
>>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>  * Lesser General Public License for more details.
>>  *
>> - * You should have received a copy of the GNU Lesser General Public
>> + * Contributions after 2012-10-29 are licensed under the terms of the
>> + * GNU GPL, version 2 or (at your option) any later version.
>> + *
>> + * You should have received a copy of the GNU (Lesser) General Public
>>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>  */
>>
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 75d4036..adca789 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -28,8 +28,31 @@
>> #include "hw/hw.h"
>> #include "qemu-common.h"
>> #include "qemu-timer.h"
>> +#include "arch_init.h"
>>
>>
>> +/* generate CPU information for cpu -? */
>> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>> +{
>> +#ifdef CONFIG_KVM
>> +    (*cpu_fprintf)(f, "s390 %16s\n", "host");
>> +#endif
>> +}
>> +
> 
> static const struct CpuDefinitionInfo cpu_info[] = {
> #ifdef CONFIG_KVM
>     {
>         .name = "host",
>     },
> #endif
> };
> 
> static const struct CPUDefinitionInfoList cpu_entry = {
>     value = cpu_info,
> };
> 
>> +CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>> +{
>> +    CpuDefinitionInfoList *entry;
>> +    CpuDefinitionInfo *info;
>> +
>> +    info = g_malloc0(sizeof(*info));
>> +    info->name = g_strdup("host");
>> +
>> +    entry = g_malloc0(sizeof(*entry));
>> +    entry->value = info;
> 
> return &entry;
> 
> (completely untested, don't you think the above would work? The code as is looks quite leaky.)

target-i386 does it the same way (well, not hardcoding "host"
obviously), so if there's leaks they need to be solved in generic code.

Mid-term we want to generate this list on the fly from CPU subclasses,
so I don't see the utility of starting a static model list for QMP only.
Given that subclasses are really easy to introduce, we might as well do
that. We could leave s390-cpu non-abstract as fallback for the non-host
cpu_models plus one host-s390-cpu for -cpu host; only thing to keep in
mind then is that the base class is not automatically filtered out, as
relied on for other targets.

Andreas

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

[Qemu-devel] [PATCH 0/3] s390: ipl device, cpu reset handler and cpu model support

[Jens Freimann]

Hi Alex,

here's a reworked version of the ipl-device patch, the cpu reset handler
and the patch to query cpu definitions via QMP.

Patch 1: Creates a new ipl device and moves ipl code from s390_virtio.c
Patch 2: Adds a cpu reset handler to 
Patch 3  Allow to query cpu types via commandline -? argument as well
         as via qmp


Christian Borntraeger (1):
  s390: Move IPL code into a separate device

Jens Freimann (1):
  s390: Add CPU reset handler

Viktor Mihajlovski (1):
  S390: Enable -cpu help and QMP query-cpu-definitions

 hw/s390-virtio.c       | 104 +++++------------------------
 hw/s390x/Makefile.objs |   1 +
 hw/s390x/ipl.c         | 174 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-s390x/cpu.c     |  58 ++++++++++++++++-
 target-s390x/cpu.h     |   3 +
 target-s390x/kvm.c     |   9 ++-
 6 files changed, 257 insertions(+), 92 deletions(-)
 create mode 100644 hw/s390x/ipl.c

-- 
1.7.12.4

[Qemu-devel] [PATCH 0/3] s390: ipl device, cpu reset handler and cpu model support

[Jens Freimann]

Hi Alex,

here's a reworked version of the ipl-device patch, the cpu reset handler
and a new patch to query cpu definitions via QMP.

Patch 1: Creates a new ipl device and moves ipl code from s390_virtio.c
Patch 2: Adds a cpu reset handler to 
Patch 3  Allow to query cpu types via commandline -? argument as well
         as via qmp


Christian Borntraeger (1):
  s390: Move IPL code into a separate device

Jens Freimann (1):
  s390: Add CPU reset handler

Viktor Mihajlovski (1):
  S390: Enable -cpu help and QMP query-cpu-definitions

 hw/s390-virtio.c       | 106 ++++++------------------------------
 hw/s390x/Makefile.objs |   1 +
 hw/s390x/ipl.c         | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/ipl.h         |  31 +++++++++++
 target-s390x/cpu.c     |  52 +++++++++++++++++-
 target-s390x/cpu.h     |   3 ++
 target-s390x/kvm.c     |   9 +++-
 7 files changed, 254 insertions(+), 92 deletions(-)
 create mode 100644 hw/s390x/ipl.c
 create mode 100644 hw/s390x/ipl.h

-- 
1.7.12.4