qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/3] s390: ipl device and cpu reset handler
@ 2012-12-07 13:55 Jens Freimann
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line Jens Freimann
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Jens Freimann @ 2012-12-07 13:55 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Heinz Graalfs, qemu-devel, Christian Borntraeger, Jens Freimann,
	Cornelia Huck, Einar Lueck

Hi Alex,

some s390 patches dealing with ipl device, CPU reset and a bugfix: 

Patch 1 fixes a bug that overwrites the standard kernel command line
Patch 2 is an initial patch towards the previously discussed ipl device.
        More patches will follow soon.
Patch 3 adds a cpu reset handler. With this patch and the ipl device 
        we can reboot on s390

Christian Borntraeger (2):
  s390: Fix empty kernel command line
  s390: Move IPL code into a separate device

Jens Freimann (1):
  s390: add cpu reset handler

 hw/s390-virtio.c       |   32 +++----------------
 hw/s390x/Makefile.objs |    1 +
 hw/s390x/ipl.c         |   80 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/ipl.h         |   34 ++++++++++++++++++++
 target-s390x/cpu.c     |   25 +++++++++++++++
 target-s390x/kvm.c     |   10 +++++-
 6 files changed, 154 insertions(+), 28 deletions(-)
 create mode 100644 hw/s390x/ipl.c
 create mode 100644 hw/s390x/ipl.h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line
  2012-12-07 13:55 [Qemu-devel] [PATCH 0/3] s390: ipl device and cpu reset handler Jens Freimann
@ 2012-12-07 13:55 ` Jens Freimann
  2012-12-11 10:34   ` Alexander Graf
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device Jens Freimann
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 3/3] s390: add cpu reset handler Jens Freimann
  2 siblings, 1 reply; 15+ messages in thread
From: Jens Freimann @ 2012-12-07 13:55 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Heinz Graalfs, qemu-devel, Christian Borntraeger, Jens Freimann,
	Cornelia Huck, Einar Lueck

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

Since commit 967c0da73a7b0da186baba6632301d83644a570c
    vl.c: Avoid segfault when started with no arguments

the user can specify a kernel without a command line. Lets not
overwrite the default command line with \0 in that case.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 hw/s390-virtio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index ca1bb09..d77871a 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
         stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
     }
 
-    if (rom_ptr(KERN_PARM_AREA)) {
+    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {
         /* we have to overwrite values in the kernel image, which are "rom" */
         memcpy(rom_ptr(KERN_PARM_AREA), kernel_cmdline,
                strlen(kernel_cmdline) + 1);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device
  2012-12-07 13:55 [Qemu-devel] [PATCH 0/3] s390: ipl device and cpu reset handler Jens Freimann
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line Jens Freimann
@ 2012-12-07 13:55 ` Jens Freimann
  2012-12-07 15:14   ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Christian Borntraeger
  2012-12-11 10:38   ` [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device Alexander Graf
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 3/3] s390: add cpu reset handler Jens Freimann
  2 siblings, 2 replies; 15+ messages in thread
From: Jens Freimann @ 2012-12-07 13:55 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Heinz Graalfs, qemu-devel, Christian Borntraeger, Jens Freimann,
	Cornelia Huck, Einar Lueck

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 (see additional patch)
  and thus disabling the bios code that only works for specifically
  prepared disks.
- reuse that code for several machines (e.g. virtio-ccw and virtio-s390)

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 hw/s390-virtio.c       |   30 ++---------------
 hw/s390x/Makefile.objs |    1 +
 hw/s390x/ipl.c         |   80 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/ipl.h         |   34 ++++++++++++++++++++
 4 files changed, 119 insertions(+), 26 deletions(-)
 create mode 100644 hw/s390x/ipl.c
 create mode 100644 hw/s390x/ipl.h

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index d77871a..10a5fd7 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -33,6 +33,7 @@
 
 #include "hw/s390-virtio-bus.h"
 #include "hw/s390x/sclp.h"
+#include "hw/s390x/ipl.h"
 
 //#define DEBUG_S390
 
@@ -48,17 +49,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;
@@ -185,6 +175,7 @@ static void s390_init(QEMUMachineInitArgs *args)
     /* get a BUS */
     s390_bus = s390_virtio_bus_init(&my_ram_size);
     s390_sclp_init();
+    s390_ipl_init();
 
     /* allocate RAM */
     memory_region_init_ram(ram, "s390.ram", my_ram_size);
@@ -225,11 +216,7 @@ 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) {
@@ -240,13 +227,6 @@ static void s390_init(QEMUMachineInitArgs *args)
                     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;
@@ -257,7 +237,7 @@ static void s390_init(QEMUMachineInitArgs *args)
         }
 
         bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-        bios_size = load_image_targphys(bios_filename, ZIPL_LOAD_ADDR, 4096);
+        bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START, 4096);
         g_free(bios_filename);
 
         if ((long)bios_size < 0) {
@@ -267,9 +247,6 @@ static void s390_init(QEMUMachineInitArgs *args)
         if (bios_size > 4096) {
             hw_error("stage1 bootloader is > 4k\n");
         }
-
-        env->psw.addr = ZIPL_START;
-        env->psw.mask = 0x0000000180000000ULL;
     }
 
     if (initrd_filename) {
@@ -352,3 +329,4 @@ static void s390_machine_init(void)
 }
 
 machine_init(s390_machine_init);
+
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..1736fca
--- /dev/null
+++ b/hw/s390x/ipl.c
@@ -0,0 +1,80 @@
+/*
+ * 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 "hw/loader.h"
+#include "hw/sysbus.h"
+#include "hw/s390x/ipl.h"
+
+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_default_init(SysBusDevice *dev)
+{
+    return 0;
+}
+
+static Property s390_ipl_properties[] = {
+        DEFINE_PROP_END_OF_LIST(),
+};
+
+static void s390_ipl_reset(DeviceState *dev)
+{
+    if (rom_ptr(KERN_IMAGE_START)) {
+        /*
+         * 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);
+    }
+    return s390_ipl_cpu(ZIPL_IMAGE_START);
+}
+
+static void s390_ipl_default_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = s390_ipl_default_init;
+    dc->props = s390_ipl_properties;
+    dc->reset = s390_ipl_reset;
+    dc->no_user = 1;
+}
+
+static TypeInfo s390_ipl_default_info = {
+    .class_init = s390_ipl_default_class_init,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .name  = "s390-ipl-default",
+};
+
+static void s390_register_ipl(void)
+{
+    type_register_static(&s390_ipl_default_info);
+}
+
+type_init(s390_register_ipl)
+
+void s390_ipl_init(void)
+{
+    DeviceState *dev  = qdev_create(NULL, "s390-ipl-default");
+    qdev_init_nofail(dev);
+}
+
+
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
new file mode 100644
index 0000000..214abbc
--- /dev/null
+++ b/hw/s390x/ipl.h
@@ -0,0 +1,34 @@
+/*
+ * ipl 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.
+ *
+ */
+
+
+#ifndef S390_IPL_H
+#define S390_IPL_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
+
+/* starts the first cpu with the given address and a default psw mask */
+void s390_ipl_cpu(uint64_t pswaddr);
+
+/* Initialize the ipl code */
+void s390_ipl_init(void);
+
+#endif //S390_IPL_H
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH 3/3] s390: add cpu reset handler
  2012-12-07 13:55 [Qemu-devel] [PATCH 0/3] s390: ipl device and cpu reset handler Jens Freimann
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line Jens Freimann
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device Jens Freimann
@ 2012-12-07 13:55 ` Jens Freimann
  2012-12-07 19:07   ` Andreas Färber
  2 siblings, 1 reply; 15+ messages in thread
From: Jens Freimann @ 2012-12-07 13:55 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Heinz Graalfs, qemu-devel, Christian Borntraeger, Jens Freimann,
	Cornelia Huck, Einar Lueck

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

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 target-s390x/cpu.c |   25 +++++++++++++++++++++++++
 target-s390x/kvm.c |   10 +++++++++-
 2 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 619b202..a601380 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,12 +19,35 @@
  * 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-07 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"
 
+/* TODO: remove me, when reset over QOM tree is implemented */
+static void s390_cpu_machine_reset_cb(void *opaque)
+{
+    S390CPU *cpu = opaque;
+    CPUS390XState *env = &cpu->env;
+
+    memset(env->regs, 0, sizeof(env->regs));
+    memset(env->aregs, 0, sizeof(env->aregs));
+    memset(env->cregs, 0, sizeof(env->cregs));
+    memset(env->fregs, 0, sizeof(env->fregs));
+    /* architectured initial values for CR 0 and 14 */
+    env->cregs[0] = 0xE0UL;
+    env->cregs[14] = 0xC2000000UL;
+    env->psw.addr = 0;
+    /* set to z/Architecture mode */
+    env->psw.mask = 0x0000000180000000ULL;
+    env->psa = 0;
+    s390_del_running_cpu(env);
+}
 
 /* CPUClass::reset() */
 static void s390_cpu_reset(CPUState *s)
@@ -56,6 +80,7 @@ static void s390_cpu_initfn(Object *obj)
 
     cpu_exec_init(env);
 #if !defined(CONFIG_USER_ONLY)
+    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
     qemu_get_timedate(&tm, 0);
     env->tod_offset = TOD_UNIX_EPOCH +
                       (time2tod(mktimegm(&tm)) * 1000000000ULL);
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 94de764..3969a49 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -85,7 +85,15 @@ 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.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [RFC/PATCH 0/2] ipl device followup
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device Jens Freimann
@ 2012-12-07 15:14   ` Christian Borntraeger
  2012-12-07 15:14     ` [Qemu-devel] [RFC/PATCH 1/2] s390: Add bootmap parsing to ipl device Christian Borntraeger
                       ` (2 more replies)
  2012-12-11 10:38   ` [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device Alexander Graf
  1 sibling, 3 replies; 15+ messages in thread
From: Christian Borntraeger @ 2012-12-07 15:14 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Heinz Graalfs, qemu-devel, Christian Borntraeger, Jens Freimann,
	Cornelia Huck, Einar Lueck

Alex,

here is were the IPL device code would move into. Some rough edges are still 
there, but it can ipl almost anything that was zipled under LPAR/VM.

Christian Borntraeger (2):
  s390: Add bootmap parsing to ipl device
  s390: enable ipl device for virtio-ccw

 hw/s390x/Makefile.objs |    2 +-
 hw/s390x/ipl-disk.c    |  504 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/ipl-disk.h    |  104 ++++++++++
 hw/s390x/ipl.c         |   51 ++++-
 4 files changed, 659 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/ipl-disk.c
 create mode 100644 hw/s390x/ipl-disk.h

-- 
1.7.10.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [RFC/PATCH 1/2] s390: Add bootmap parsing to ipl device
  2012-12-07 15:14   ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Christian Borntraeger
@ 2012-12-07 15:14     ` Christian Borntraeger
  2012-12-07 15:14     ` [Qemu-devel] [RFC/PATCH 2/2] s390: enable ipl device for virtio-ccw Christian Borntraeger
  2012-12-11 10:36     ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Alexander Graf
  2 siblings, 0 replies; 15+ messages in thread
From: Christian Borntraeger @ 2012-12-07 15:14 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Heinz Graalfs, qemu-devel, Christian Borntraeger, Jens Freimann,
	Cornelia Huck, Einar Lueck

The zipl bios code only works for specially prepared disks. This adds
parsing of the on disk bootmap of disks that are zipled with a zipl
under LPAR/zVM. Since all bootmaps are pretty similar the code is
written in a way to not only fcp bootmaps (which are architectured and
 also parsed by the firmware on real boxes) but also
- dasd bootmaps for eckd and fba, with new and old versions of zipl
- any kind of block size 512,1024,2048,4096
- bootmaps created with the SLES11 zipl (those understood by the "bios")

To make the behaviour consistent with old code, we have two ipl devices.
A default one (no_user) that just boots the kernel or the bios and one
that can be specified by the user with -device s390-ipl that also allows
to trigger the bootmap parsing.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/Makefile.objs |    2 +-
 hw/s390x/ipl-disk.c    |  499 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/ipl-disk.h    |  104 ++++++++++
 hw/s390x/ipl.c         |   51 ++++-
 4 files changed, 654 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/ipl-disk.c
 create mode 100644 hw/s390x/ipl-disk.h

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 4a5a5d8..a4a7c5a 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -4,4 +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
+obj-y += ipl.o ipl-disk.o
diff --git a/hw/s390x/ipl-disk.c b/hw/s390x/ipl-disk.c
new file mode 100644
index 0000000..1aab32b
--- /dev/null
+++ b/hw/s390x/ipl-disk.c
@@ -0,0 +1,499 @@
+/*
+ * disk ipl 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 <stdio.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <linux/fs.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include <sysemu.h>
+#include "hw/loader.h"
+#include "hw/s390-virtio-bus.h"
+#include "hw/sysbus.h"
+#include "hw/s390x/ipl.h"
+#include "hw/s390x/ipl-disk.h"
+
+
+typedef struct {
+    BlockDriverState *bs;
+    uint64_t (*blockno)(BlockPtr *blockptr);
+    uint64_t (*offset)(BlockPtr *blockptr);
+    uint64_t (*size)(BlockPtr *blockptr);
+    bool (*empty)(BlockPtr *blockptr);
+    BlockPtr *(*element)(BlockPtr *blockptr, int num);
+    uint32_t (*entries)(void);
+    uint32_t loadparm;
+    uint8_t heads;
+    uint8_t secs;
+    uint16_t blk_size;
+} Loader;
+
+/*
+ * We have one structure that is setup with the right callbacks for the
+ * detected type of boot loader
+ */
+static Loader loader;
+
+/* here are the FCP Callbacks */
+static uint64_t getblockno_fcp(BlockPtr *entry)
+{
+    return be64_to_cpu(entry->u.fcp.blockno);
+}
+
+static uint64_t getoffset_fcp(BlockPtr *entry)
+{
+    return getblockno_fcp(entry) * be16_to_cpu(entry->u.fcp.size);
+}
+
+static uint64_t getsize_fcp(BlockPtr *entry)
+{
+    return loader.blk_size * (be16_to_cpu(entry->u.fcp.blockct) + 1);
+}
+
+static bool getempty_fcp(BlockPtr *entry)
+{
+    return getblockno_fcp(entry) == 0UL;
+}
+
+static BlockPtr *getelement_fcp(BlockPtr *blockptr, int num)
+{
+     FCPBlockPtr *fcp = (FCPBlockPtr *) blockptr;
+
+     return (BlockPtr *) &fcp[num];
+}
+
+static uint32_t entries_fcp(void)
+{
+    return loader.blk_size / sizeof(FCPBlockPtr);
+};
+
+/* and here the callbacks for the new and old eckd map */
+static uint64_t getblockno_eckd(BlockPtr *entry)
+{
+    return 1UL * loader.secs * loader.heads * entry->u.eckd.cyls +
+           1UL * loader.secs * entry->u.eckd.heads +
+           1UL * entry->u.eckd.secs - 1UL;
+}
+
+static uint64_t getoffset_eckd(BlockPtr *entry)
+{
+    return getblockno_eckd(entry) * entry->u.eckd.block_size;
+}
+
+static uint64_t getsize_eckd(BlockPtr *entry)
+{
+    return loader.blk_size * (entry->u.eckd.count + 1);
+}
+
+static bool getempty_eckd(BlockPtr *entry)
+{
+    return getblockno_eckd(entry) == -1UL;
+}
+
+static BlockPtr *getelement_eckd(BlockPtr *blockptr, int num)
+{
+     ECKDBlockPtr *eckd = (ECKDBlockPtr *) blockptr;
+
+     return (BlockPtr *) &eckd[num];
+}
+
+static BlockPtr *getelement_neckd(BlockPtr *blockptr, int num)
+{
+     NECKDBlockPtr *neckd = (NECKDBlockPtr *) blockptr;
+
+     return (BlockPtr *) &neckd[num];
+}
+
+static uint32_t entries_eckd(void)
+{
+    return loader.blk_size / sizeof(ECKDBlockPtr);
+};
+
+static uint32_t entries_neckd(void)
+{
+    return loader.blk_size / sizeof(NECKDBlockPtr);
+};
+
+static bool magic_ok(void *tmp)
+{
+    return memcmp(tmp, "zIPL", 4) == 0 ? true : false;
+}
+
+static bool blk_size_ok(uint32_t blk_size)
+{
+    return blk_size == 512 || blk_size == 1024 || blk_size == 2048 ||
+           blk_size == 4096;
+}
+
+static uint64_t parse_segment_elements(BlockPtr *bptr,
+                                       uint64_t *address,
+                                       Loader *loader)
+{
+    unsigned d;
+    int len;
+    BlockPtr *block_entry;
+
+    for (d = 0; d < loader->entries() - 1; d++) {
+        if (*address > ram_size) {
+            error_report("s390-ipl: bootmap points to illegal address");
+            exit(1);
+        }
+        if (loader->empty(loader->element(bptr, d))) {
+            return 0;
+        }
+        len = bdrv_pread(loader->bs,
+                         loader->offset(loader->element(bptr, d)),
+                         (void *) (*address + qemu_get_ram_ptr(0)),
+                         loader->size(loader->element(bptr, d)));
+        if (len != loader->size(loader->element(bptr, d))) {
+            error_report("s390-ipl: error while parsing bootmap");
+            exit(1);
+        }
+        *address += len;
+    }
+
+    block_entry = loader->element(bptr, loader->entries() - 1);
+
+    return loader->blockno(block_entry);
+}
+
+static void parse_segment_table(uint64_t blockno, uint64_t address,
+                                Loader *loader)
+{
+    BlockPtr entries[loader->entries() + 1];
+
+    do {
+        bdrv_pread(loader->bs, blockno * loader->blk_size, entries,
+                   sizeof(entries));
+        blockno = parse_segment_elements(entries, &address, loader);
+    } while (blockno);
+}
+
+static uint64_t parse_program(BlockPtr *blockptr, Loader *loader)
+{
+    int ret;
+    uint64_t offset = loader->offset(blockptr);
+    ComponentHeader header;
+    ComponentEntry entry;
+
+    ret = bdrv_pread(loader->bs, offset, &header, sizeof(header));
+    if (ret != sizeof(header)) {
+        return -1;
+    }
+    if (!magic_ok(&header.magic)) {
+        return -1;
+    }
+
+    if (header.type != component_header_ipl) {
+        error_report("s390-ipl: no IPL header on bootdevice\n");
+        exit(1);
+    }
+
+    offset += sizeof(header);
+    ret = bdrv_pread(loader->bs, offset, &entry, sizeof(entry));
+    if (ret != sizeof(header)) {
+        return -1;
+    }
+
+    while (entry.component_type == component_load) {
+        parse_segment_table(loader->blockno(&entry.blkptr),
+                            entry.address.load_address, loader);
+        offset += sizeof(entry);
+        ret = bdrv_pread(loader->bs, offset, &entry, sizeof(entry));
+        if (ret != sizeof(header)) {
+            return -1;
+        }
+    }
+    if (entry.component_type == component_execute) {
+        return entry.address.load_address;
+    } else {
+        error_report("s390-ipl: no IPL address on bootmap");
+        exit(1);
+    }
+}
+
+static uint64_t parse_program_table(BlockPtr *blockptr,
+                                    Loader *loader)
+{
+    uint32_t n;
+    BlockPtr *block_entry;
+    BlockPtr entries[loader->entries()];
+
+    if (bdrv_pread(loader->bs, loader->offset(blockptr),
+                   entries, loader->blk_size) != loader->blk_size) {
+        return -1;
+    }
+
+    /* entry 0, holds the magic */
+    if (!magic_ok(&entries[0])) {
+        return -1;
+    }
+
+    /* Get the number of entries */
+    for (n = 1; n < loader->entries(); n++) {
+        if (loader->empty(loader->element(entries, n))) {
+            break;
+        }
+    }
+
+    /*
+     * on disk: 0 = magic, 1 = default, 2..n = entries
+     * on HMC: 0 = default, 1..m = entries
+     */
+    if (loader->loadparm >= n - 1) {
+        error_report("s390-ipl: Loadparm entry %d does not exist",
+                     loader->loadparm);
+        exit(1);
+    }
+
+    block_entry = loader->element(entries, loader->loadparm + 1);
+
+    return parse_program(block_entry, loader);
+}
+
+static uint64_t load_scsi_disk(BlockConf conf)
+{
+    FCPMbr fmbr;
+
+    bdrv_pread(conf.bs, 0, &fmbr, sizeof(fmbr));
+    if (magic_ok(&fmbr.magic) &&
+        blk_size_ok(be16_to_cpu(fmbr.blockptr.u.fcp.size))) {
+        loader.blk_size = be16_to_cpu(fmbr.blockptr.u.fcp.size);
+        return parse_program_table(&fmbr.blockptr, &loader);
+    }
+    return -1;
+}
+
+static uint64_t load_new_ldl_disk(BlockConf conf)
+{
+    NewECKDMbr nembr;
+
+    bdrv_pread(conf.bs, 112, &nembr, sizeof(nembr));
+    if (magic_ok(&nembr.magic) &&
+        blk_size_ok(be16_to_cpu(nembr.blockptr.u.neckd.block_size)) &&
+        nembr.dev_type == DEV_TYPE_ECKD) {
+        loader.blk_size = be16_to_cpu(nembr.blockptr.u.neckd.block_size);
+        return parse_program_table(&nembr.blockptr, &loader);
+    }
+    return -1;
+}
+
+static uint64_t parse_cdl(BlockConf conf, uint16_t blk_size)
+{
+    NewECKDMbr nembr;
+
+    loader.blk_size = blk_size;
+
+    /* new dasd bootmap for CDL*/
+    bdrv_pread(conf.bs, blk_size + 92, &nembr, sizeof(nembr));
+    if (magic_ok(&nembr.magic) &&
+        blk_size_ok(be16_to_cpu(nembr.blockptr.u.neckd.block_size)) &&
+        nembr.dev_type == DEV_TYPE_ECKD) {
+        return parse_program_table(&nembr.blockptr, &loader);
+    }
+
+    return -1;
+}
+
+static uint64_t load_new_cdl_disk(BlockConf conf)
+{
+    uint64_t address;
+
+    address = parse_cdl(conf, 4096);
+    if (address != -1) {
+        return address;
+    }
+    address = parse_cdl(conf, 2048);
+    if (address != -1) {
+        return address;
+    }
+    address = parse_cdl(conf, 1024);
+    if (address != -1) {
+        return address;
+    }
+    return parse_cdl(conf, 512);
+}
+
+static uint64_t parse_classic(BlockConf conf, uint16_t blk_size)
+{
+    int ret = -1;
+    ECKDMbr embr;
+
+    loader.blk_size = blk_size;
+
+    /* unfortunately there is no magic available */
+
+    /* classic cdl dasd bootmap */
+    bdrv_pread(conf.bs, blk_size + 4, &embr, sizeof(embr));
+    if (blk_size_ok(be16_to_cpu(embr.blockptr.block_size))) {
+        ret = parse_program_table((BlockPtr *) &embr.blockptr, &loader);
+    }
+
+    if (ret == -1) {
+        /* last chance, classic ldl dasd bootmap */
+        bdrv_pread(conf.bs, blk_size, &embr, sizeof(embr));
+        if (blk_size_ok(be16_to_cpu(embr.blockptr.block_size))) {
+            ret = parse_program_table((BlockPtr *) &embr.blockptr, &loader);
+        }
+    }
+    return ret;
+}
+
+static uint64_t load_classic_cdl_or_ldl_disk(BlockConf conf)
+{
+    uint64_t address;
+
+    address = parse_classic(conf, 4096);
+    if (address != -1) {
+        return address;
+    }
+    address = parse_classic(conf, 2048);
+    if (address != -1) {
+        return address;
+    }
+    address = parse_classic(conf, 1024);
+    if (address != -1) {
+        return address;
+    }
+    return parse_classic(conf, 512);
+}
+
+/*
+ * looks at the program tables written by the boot loader to load
+ * everything which is specified in the bootmap
+ */
+static unsigned long load_from_disk(BlockConf conf, uint32_t loadparm)
+{
+    uint64_t address;
+
+    loader.bs       = conf.bs;
+    loader.loadparm = loadparm;
+
+    /* try SCSI first */
+    loader.blockno  = getblockno_fcp;
+    loader.offset   = getoffset_fcp;
+    loader.size     = getsize_fcp;
+    loader.empty    = getempty_fcp;
+    loader.element  = getelement_fcp;
+    loader.entries  = entries_fcp;
+
+    address = load_scsi_disk(conf);
+    if (address != -1) {
+        return address & 0x7fffffff;
+    }
+
+    /* lets try several ECKD boot-loader types */
+    loader.heads    = conf.heads;
+    loader.secs     = conf.secs;
+    loader.blockno  = getblockno_eckd;
+    loader.offset   = getoffset_eckd;
+    loader.size     = getsize_eckd;
+    loader.empty    = getempty_eckd;
+    loader.element  = getelement_neckd;
+    loader.entries  = entries_neckd;
+
+    /* try new DASD LDL format */
+    address = load_new_ldl_disk(conf);
+    if (address != -1) {
+        return address & 0x7fffffff;
+    }
+
+    /* try new DASD CDL format with various block sizes */
+    address = load_new_cdl_disk(conf);
+    if (address != -1) {
+        return address & 0x7fffffff;
+    }
+
+    loader.element  = getelement_eckd;
+    loader.entries  = entries_eckd;
+
+    /* try classic CDL and LDL formats with various block sizes */
+    address = load_classic_cdl_or_ldl_disk(conf);
+    if (address != -1) {
+        return address & 0x7fffffff;
+    }
+    return -1;
+}
+
+static VirtIOBlkConf *getVirtIOBlkConf(DeviceState *dev, const char *id)
+{
+    VirtIOBlkConf *blk;
+
+    if (strcmp(dev->parent_bus->name, "s390-virtio") == 0) {
+        VirtIOS390Device *s390dev;
+        s390dev = DO_UPCAST(VirtIOS390Device, qdev, dev);
+        blk = &s390dev->blk;
+    } else {
+        error_report("s390-ipl: device '%s' is not a virtio device",
+                     id ? id : "0");
+        exit(1);
+    }
+
+    if (!blk->conf.bs) {
+        error_report("s390-ipl: device '%s' is not a block device",
+                     id ? id : "0");
+        exit(1);
+    }
+    return blk;
+}
+
+void s390_ipl_disk(const char *id, uint32_t loadparm)
+{
+    uint64_t addr = -1UL;
+    DeviceState *dev;
+    VirtIOBlkConf *blk;
+    DriveInfo *drive;
+
+    /* If no disk is specified, use the first one */
+    if (!id) {
+        /*
+         * libvirt and friends use if=none to create the device itself,
+         * standard command line without an if= will result in virtio.
+         * Lets search both types for a device
+         */
+        drive = drive_get_by_index(IF_NONE, 0);
+        if (!drive) {
+            drive = drive_get_by_index(IF_VIRTIO, 0);
+        }
+        if (drive) {
+            dev = bdrv_get_attached_dev(drive->bdrv);
+            if (!dev) {
+                error_report("s390-ipl: First drive has no attached device");
+                exit(1);
+            }
+        } else {
+            error_report("s390-ipl: No bootable disk found");
+            exit(1);
+        }
+    } else {
+        dev = qdev_find_recursive(sysbus_get_default(), id);
+        if (!dev) {
+            error_report("s390-ipl: Unable to find device '%s'", id);
+            exit(1);
+        }
+    }
+
+    blk = getVirtIOBlkConf(dev, id); /* or fail if no block device */
+
+    addr = load_from_disk(blk->conf, loadparm);
+    if (addr == -1)  {
+        error_report("s390-ipl: %s id '%s' does not contain a valid bootmap",
+                     qdev_fw_name(dev), id ? id : "0");
+        exit(1);
+    }
+
+    s390_ipl_cpu(addr);
+}
+
diff --git a/hw/s390x/ipl-disk.h b/hw/s390x/ipl-disk.h
new file mode 100644
index 0000000..9cab0cb
--- /dev/null
+++ b/hw/s390x/ipl-disk.h
@@ -0,0 +1,104 @@
+/*
+ * ipl 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.
+ *
+ */
+
+
+#ifndef S390_IPL_DISK_H
+#define S390_IPL_DISK_H
+
+#include "blockdev.h"
+#include "block_int.h"
+#include "cpu.h"
+#include "hw/s390x/ipl.h"
+
+typedef struct {
+    uint64_t blockno;
+    uint16_t size;
+    uint16_t blockct;
+    uint8_t reserved[4];
+} QEMU_PACKED FCPBlockPtr;
+
+typedef struct {
+    uint16_t cyls;
+    uint16_t heads;
+    uint8_t secs;
+    uint16_t block_size;
+    uint8_t count;
+    uint8_t reserved[8];
+} QEMU_PACKED NECKDBlockPtr;
+
+typedef struct {
+    uint16_t cyls;
+    uint16_t heads;
+    uint8_t secs;
+    uint16_t block_size;
+    uint8_t count;
+} QEMU_PACKED ECKDBlockPtr;
+
+
+typedef struct {
+    union {
+        NECKDBlockPtr neckd;
+        ECKDBlockPtr eckd;
+        FCPBlockPtr  fcp;
+    } u;
+} QEMU_PACKED BlockPtr;
+
+typedef struct {
+    BlockPtr blkptr;
+    uint8_t pad[7];
+    uint8_t component_type;
+    union {
+        uint64_t load_address;
+        uint64_t load_psw;
+    } address;
+} QEMU_PACKED ComponentEntry;
+
+typedef struct {
+    uint8_t magic[4];
+    uint8_t type;
+    uint8_t reserved[27];
+} QEMU_PACKED ComponentHeader;
+
+typedef struct {
+    ECKDBlockPtr blockptr;
+} QEMU_PACKED ECKDMbr;
+
+#define DEV_TYPE_ECKD    0x00
+#define DEV_TYPE_FBA     0x01
+
+typedef struct {
+    char    magic[4];
+    uint8_t version;
+    uint8_t bp_type;
+    uint8_t dev_type;
+    uint8_t flags;
+    BlockPtr blockptr;
+    uint8_t reserved[8];
+} QEMU_PACKED NewECKDMbr;
+
+typedef struct {
+    char magic[4];
+    uint32_t version_id;
+    uint8_t reserved[8];
+    BlockPtr blockptr;
+} QEMU_PACKED FCPMbr;
+
+#define component_execute 0x01
+#define component_load    0x02
+
+#define component_header_ipl 0x00
+
+/* IPLs the given device id */
+void s390_ipl_disk(const char *id, uint32_t loadparm);
+
+#endif //S390_IPL_DISK_H
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 86249cd..c2ef8b6 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -16,6 +16,7 @@
 #include "hw/loader.h"
 #include "hw/sysbus.h"
 #include "hw/s390x/ipl.h"
+#include "hw/s390x/ipl-disk.h"
 
 void s390_ipl_cpu(uint64_t pswaddr)
 {
@@ -25,17 +26,36 @@ void s390_ipl_cpu(uint64_t pswaddr)
     s390_add_running_cpu(env);
 }
 
+typedef struct {
+    SysBusDevice busdev;
+    uint32_t loadparm;
+    uint8_t use_bios;
+    char *iplid;
+} S390IPLState;
+
 static int s390_ipl_default_init(SysBusDevice *dev)
 {
     return 0;
 }
 
+static int s390_ipl_user_init(SysBusDevice *dev)
+{
+    dev->qdev.id = strdup("s390-ipl");
+    return 0;
+}
+
 static Property s390_ipl_properties[] = {
+        DEFINE_PROP_UINT32("loadparm", S390IPLState, loadparm, 0),
+        DEFINE_PROP_STRING("iplid", S390IPLState, iplid),
+        DEFINE_PROP_UINT8("use_bios", S390IPLState, use_bios, 0),
         DEFINE_PROP_END_OF_LIST(),
 };
 
 static void s390_ipl_reset(DeviceState *dev)
 {
+    S390IPLState *iplstate;
+    DeviceState *idev;
+
     if (rom_ptr(KERN_IMAGE_START)) {
         /*
          * we can not rely on the ELF entry point, since up to 3.2 this
@@ -44,7 +64,18 @@ static void s390_ipl_reset(DeviceState *dev)
          */
         return s390_ipl_cpu(KERN_IMAGE_START);
     }
-    return s390_ipl_cpu(ZIPL_IMAGE_START);
+
+    idev = qdev_find_recursive(sysbus_get_default(), "s390-ipl");
+    if (idev) {
+        iplstate = container_of(idev, S390IPLState, busdev.qdev);
+    } else {
+        iplstate = container_of(dev, S390IPLState, busdev.qdev);
+    }
+    if (iplstate->use_bios) {
+        return s390_ipl_cpu(ZIPL_IMAGE_START);
+    } else {
+        return s390_ipl_disk(iplstate->iplid, iplstate->loadparm);
+    }
 }
 
 static void s390_ipl_default_class_init(ObjectClass *klass, void *data)
@@ -58,15 +89,33 @@ static void s390_ipl_default_class_init(ObjectClass *klass, void *data)
     dc->no_user = 1;
 }
 
+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_user_init;
+    dc->props = s390_ipl_properties;
+}
+
 static TypeInfo s390_ipl_default_info = {
     .class_init = s390_ipl_default_class_init,
     .parent = TYPE_SYS_BUS_DEVICE,
     .name  = "s390-ipl-default",
+    .instance_size  = sizeof(S390IPLState),
+};
+
+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_default_info);
+    type_register_static(&s390_ipl_info);
 }
 
 type_init(s390_register_ipl)
-- 
1.7.10.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [RFC/PATCH 2/2] s390: enable ipl device for virtio-ccw
  2012-12-07 15:14   ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Christian Borntraeger
  2012-12-07 15:14     ` [Qemu-devel] [RFC/PATCH 1/2] s390: Add bootmap parsing to ipl device Christian Borntraeger
@ 2012-12-07 15:14     ` Christian Borntraeger
  2012-12-11 10:36     ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Alexander Graf
  2 siblings, 0 replies; 15+ messages in thread
From: Christian Borntraeger @ 2012-12-07 15:14 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Heinz Graalfs, qemu-devel, Christian Borntraeger, Jens Freimann,
	Cornelia Huck, Einar Lueck

In case of virtio-ccw we also want to get a disk by its
device id.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/ipl-disk.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/s390x/ipl-disk.c b/hw/s390x/ipl-disk.c
index 1aab32b..6a83d9c 100644
--- a/hw/s390x/ipl-disk.c
+++ b/hw/s390x/ipl-disk.c
@@ -21,6 +21,7 @@
 #include <sysemu.h>
 #include "hw/loader.h"
 #include "hw/s390-virtio-bus.h"
+#include "hw/s390x/virtio-ccw.h"
 #include "hw/sysbus.h"
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/ipl-disk.h"
@@ -435,6 +436,10 @@ static VirtIOBlkConf *getVirtIOBlkConf(DeviceState *dev, const char *id)
         VirtIOS390Device *s390dev;
         s390dev = DO_UPCAST(VirtIOS390Device, qdev, dev);
         blk = &s390dev->blk;
+    } else if (strcmp(dev->parent_bus->name, "virtio-ccw") == 0) {
+        VirtioCcwData *ccwdev;
+        ccwdev = DO_UPCAST(VirtioCcwData, qdev, dev);
+        blk = &ccwdev->blk;
     } else {
         error_report("s390-ipl: device '%s' is not a virtio device",
                      id ? id : "0");
-- 
1.7.10.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] s390: add cpu reset handler
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 3/3] s390: add cpu reset handler Jens Freimann
@ 2012-12-07 19:07   ` Andreas Färber
  2012-12-08 17:32     ` Jens Freimann
  0 siblings, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2012-12-07 19:07 UTC (permalink / raw)
  To: Jens Freimann
  Cc: Heinz Graalfs, Alexander Graf, qemu-devel, Christian Borntraeger,
	Cornelia Huck, Einar Lueck

Am 07.12.2012 14:55, 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>
> ---
>  target-s390x/cpu.c |   25 +++++++++++++++++++++++++
>  target-s390x/kvm.c |   10 +++++++++-
>  2 files changed, 34 insertions(+), 1 deletions(-)
> 
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 619b202..a601380 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,12 +19,35 @@
>   * 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-07 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"
>  
> +/* TODO: remove me, when reset over QOM tree is implemented */
> +static void s390_cpu_machine_reset_cb(void *opaque)
> +{
> +    S390CPU *cpu = opaque;
> +    CPUS390XState *env = &cpu->env;
> +
> +    memset(env->regs, 0, sizeof(env->regs));
> +    memset(env->aregs, 0, sizeof(env->aregs));
> +    memset(env->cregs, 0, sizeof(env->cregs));
> +    memset(env->fregs, 0, sizeof(env->fregs));
> +    /* architectured initial values for CR 0 and 14 */
> +    env->cregs[0] = 0xE0UL;
> +    env->cregs[14] = 0xC2000000UL;
> +    env->psw.addr = 0;
> +    /* set to z/Architecture mode */
> +    env->psw.mask = 0x0000000180000000ULL;
> +    env->psa = 0;
> +    s390_del_running_cpu(env);
> +}

What's the connection between lack of reset over QOM tree and not
calling the existing s390_cpu_reset() from s390_cpu_machine_reset_cb()?
Are some CPUS390XState fields maybe misplaced?

Regards,
Andreas

>  
>  /* CPUClass::reset() */
>  static void s390_cpu_reset(CPUState *s)
> @@ -56,6 +80,7 @@ static void s390_cpu_initfn(Object *obj)
>  
>      cpu_exec_init(env);
>  #if !defined(CONFIG_USER_ONLY)
> +    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>      qemu_get_timedate(&tm, 0);
>      env->tod_offset = TOD_UNIX_EPOCH +
>                        (time2tod(mktimegm(&tm)) * 1000000000ULL);
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 94de764..3969a49 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -85,7 +85,15 @@ 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)
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] s390: add cpu reset handler
  2012-12-07 19:07   ` Andreas Färber
@ 2012-12-08 17:32     ` Jens Freimann
  0 siblings, 0 replies; 15+ messages in thread
From: Jens Freimann @ 2012-12-08 17:32 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Heinz Graalfs, Alexander Graf, qemu-devel, Christian Borntraeger,
	Cornelia Huck, Einar Lueck

On Fri, Dec 07, 2012 at 08:07:45PM +0100, Andreas Färber wrote:
> Am 07.12.2012 14:55, 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>
> > ---
> >  target-s390x/cpu.c |   25 +++++++++++++++++++++++++
> >  target-s390x/kvm.c |   10 +++++++++-
> >  2 files changed, 34 insertions(+), 1 deletions(-)
> > 
> > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> > index 619b202..a601380 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,12 +19,35 @@
> >   * 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-07 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"
> >  
> > +/* TODO: remove me, when reset over QOM tree is implemented */
> > +static void s390_cpu_machine_reset_cb(void *opaque)
> > +{
> > +    S390CPU *cpu = opaque;
> > +    CPUS390XState *env = &cpu->env;
> > +
> > +    memset(env->regs, 0, sizeof(env->regs));
> > +    memset(env->aregs, 0, sizeof(env->aregs));
> > +    memset(env->cregs, 0, sizeof(env->cregs));
> > +    memset(env->fregs, 0, sizeof(env->fregs));
> > +    /* architectured initial values for CR 0 and 14 */
> > +    env->cregs[0] = 0xE0UL;
> > +    env->cregs[14] = 0xC2000000UL;
> > +    env->psw.addr = 0;
> > +    /* set to z/Architecture mode */
> > +    env->psw.mask = 0x0000000180000000ULL;
> > +    env->psa = 0;
> > +    s390_del_running_cpu(env);
> > +}
> 
> What's the connection between lack of reset over QOM tree and not
> calling the existing s390_cpu_reset() from s390_cpu_machine_reset_cb()?
> Are some CPUS390XState fields maybe misplaced?

Thanks for the review!

Calling cpu_reset from s390_cpu_machine_reset() has an undesired
side-effect. By setting env->halted to zero we decrement our running cpu
counter too often. The way it is implemented in this patch, it works in all cases,
i.e. guest-triggered reboot, system_reset qmp command as well as system halt.

But I agree that it's not obvious why we do it this way. The running_cpus
counter complicates things. Maybe I can come up with a nicer solution...


Jens

> Regards,
> Andreas
> 
> >  
> >  /* CPUClass::reset() */
> >  static void s390_cpu_reset(CPUState *s)
> > @@ -56,6 +80,7 @@ static void s390_cpu_initfn(Object *obj)
> >  
> >      cpu_exec_init(env);
> >  #if !defined(CONFIG_USER_ONLY)
> > +    qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> >      qemu_get_timedate(&tm, 0);
> >      env->tod_offset = TOD_UNIX_EPOCH +
> >                        (time2tod(mktimegm(&tm)) * 1000000000ULL);
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 94de764..3969a49 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -85,7 +85,15 @@ 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)
> > 
> 
> 
> -- 
> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line Jens Freimann
@ 2012-12-11 10:34   ` Alexander Graf
  2012-12-11 11:15     ` Christian Borntraeger
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Graf @ 2012-12-11 10:34 UTC (permalink / raw)
  To: Jens Freimann
  Cc: Cornelia Huck, Christian Borntraeger, Heinz Graalfs, qemu-devel,
	Einar Lueck


On 07.12.2012, at 14:55, Jens Freimann wrote:

> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Since commit 967c0da73a7b0da186baba6632301d83644a570c
>    vl.c: Avoid segfault when started with no arguments
> 
> the user can specify a kernel without a command line. Lets not
> overwrite the default command line with \0 in that case.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> ---
> hw/s390-virtio.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index ca1bb09..d77871a 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>         stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>     }
> 
> -    if (rom_ptr(KERN_PARM_AREA)) {
> +    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {

why strlen()? If no -append option was passed, kernel_cmdline should be NULL. If -append "" was passed, the user wants to command line to be overwritten with "\0".


Alex

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [RFC/PATCH 0/2] ipl device followup
  2012-12-07 15:14   ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Christian Borntraeger
  2012-12-07 15:14     ` [Qemu-devel] [RFC/PATCH 1/2] s390: Add bootmap parsing to ipl device Christian Borntraeger
  2012-12-07 15:14     ` [Qemu-devel] [RFC/PATCH 2/2] s390: enable ipl device for virtio-ccw Christian Borntraeger
@ 2012-12-11 10:36     ` Alexander Graf
  2 siblings, 0 replies; 15+ messages in thread
From: Alexander Graf @ 2012-12-11 10:36 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Cornelia Huck, Heinz Graalfs, qemu-devel, Jens Freimann,
	Einar Lueck


On 07.12.2012, at 16:14, Christian Borntraeger wrote:

> Alex,
> 
> here is were the IPL device code would move into. Some rough edges are still 
> there, but it can ipl almost anything that was zipled under LPAR/VM.

Please try and check how nicely this can be done if we would map a firmware blob into the guest address space. I still believe that we can handle all boot strapping cases from within guest context.

The great advantage would be that we get user interaction and consistent device state. And less code that runs under host privileges.


Alex

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device
  2012-12-07 13:55 ` [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device Jens Freimann
  2012-12-07 15:14   ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Christian Borntraeger
@ 2012-12-11 10:38   ` Alexander Graf
  1 sibling, 0 replies; 15+ messages in thread
From: Alexander Graf @ 2012-12-11 10:38 UTC (permalink / raw)
  To: Jens Freimann
  Cc: Cornelia Huck, Christian Borntraeger, Heinz Graalfs, qemu-devel,
	Einar Lueck


On 07.12.2012, at 14:55, Jens Freimann wrote:

> 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 (see additional patch)
>  and thus disabling the bios code that only works for specifically
>  prepared disks.
> - reuse that code for several machines (e.g. virtio-ccw and virtio-s390)
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>

Creating a separate device sounds like a good idea, but please do so consistently. Please move everything that relates to booting into the device, including -kernel.

Also, I don't think we need a "-default" ipl device. Just create a generic ipl device that controls booting.


Alex

> ---
> hw/s390-virtio.c       |   30 ++---------------
> hw/s390x/Makefile.objs |    1 +
> hw/s390x/ipl.c         |   80 ++++++++++++++++++++++++++++++++++++++++++++++++
> hw/s390x/ipl.h         |   34 ++++++++++++++++++++
> 4 files changed, 119 insertions(+), 26 deletions(-)
> create mode 100644 hw/s390x/ipl.c
> create mode 100644 hw/s390x/ipl.h
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index d77871a..10a5fd7 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -33,6 +33,7 @@
> 
> #include "hw/s390-virtio-bus.h"
> #include "hw/s390x/sclp.h"
> +#include "hw/s390x/ipl.h"
> 
> //#define DEBUG_S390
> 
> @@ -48,17 +49,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;
> @@ -185,6 +175,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>     /* get a BUS */
>     s390_bus = s390_virtio_bus_init(&my_ram_size);
>     s390_sclp_init();
> +    s390_ipl_init();
> 
>     /* allocate RAM */
>     memory_region_init_ram(ram, "s390.ram", my_ram_size);
> @@ -225,11 +216,7 @@ 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) {
> @@ -240,13 +227,6 @@ static void s390_init(QEMUMachineInitArgs *args)
>                     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;
> @@ -257,7 +237,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>         }
> 
>         bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> -        bios_size = load_image_targphys(bios_filename, ZIPL_LOAD_ADDR, 4096);
> +        bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START, 4096);
>         g_free(bios_filename);
> 
>         if ((long)bios_size < 0) {
> @@ -267,9 +247,6 @@ static void s390_init(QEMUMachineInitArgs *args)
>         if (bios_size > 4096) {
>             hw_error("stage1 bootloader is > 4k\n");
>         }
> -
> -        env->psw.addr = ZIPL_START;
> -        env->psw.mask = 0x0000000180000000ULL;
>     }
> 
>     if (initrd_filename) {
> @@ -352,3 +329,4 @@ static void s390_machine_init(void)
> }
> 
> machine_init(s390_machine_init);
> +
> 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..1736fca
> --- /dev/null
> +++ b/hw/s390x/ipl.c
> @@ -0,0 +1,80 @@
> +/*
> + * 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 "hw/loader.h"
> +#include "hw/sysbus.h"
> +#include "hw/s390x/ipl.h"
> +
> +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_default_init(SysBusDevice *dev)
> +{
> +    return 0;
> +}
> +
> +static Property s390_ipl_properties[] = {
> +        DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void s390_ipl_reset(DeviceState *dev)
> +{
> +    if (rom_ptr(KERN_IMAGE_START)) {
> +        /*
> +         * 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);
> +    }
> +    return s390_ipl_cpu(ZIPL_IMAGE_START);
> +}
> +
> +static void s390_ipl_default_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = s390_ipl_default_init;
> +    dc->props = s390_ipl_properties;
> +    dc->reset = s390_ipl_reset;
> +    dc->no_user = 1;
> +}
> +
> +static TypeInfo s390_ipl_default_info = {
> +    .class_init = s390_ipl_default_class_init,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .name  = "s390-ipl-default",
> +};
> +
> +static void s390_register_ipl(void)
> +{
> +    type_register_static(&s390_ipl_default_info);
> +}
> +
> +type_init(s390_register_ipl)
> +
> +void s390_ipl_init(void)
> +{
> +    DeviceState *dev  = qdev_create(NULL, "s390-ipl-default");
> +    qdev_init_nofail(dev);
> +}
> +
> +
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> new file mode 100644
> index 0000000..214abbc
> --- /dev/null
> +++ b/hw/s390x/ipl.h
> @@ -0,0 +1,34 @@
> +/*
> + * ipl 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.
> + *
> + */
> +
> +
> +#ifndef S390_IPL_H
> +#define S390_IPL_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
> +
> +/* starts the first cpu with the given address and a default psw mask */
> +void s390_ipl_cpu(uint64_t pswaddr);
> +
> +/* Initialize the ipl code */
> +void s390_ipl_init(void);
> +
> +#endif //S390_IPL_H
> -- 
> 1.7.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line
  2012-12-11 10:34   ` Alexander Graf
@ 2012-12-11 11:15     ` Christian Borntraeger
  2012-12-11 11:19       ` Alexander Graf
  0 siblings, 1 reply; 15+ messages in thread
From: Christian Borntraeger @ 2012-12-11 11:15 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Cornelia Huck, Jens Freimann, Heinz Graalfs, qemu-devel,
	Einar Lueck

On 11/12/12 11:34, Alexander Graf wrote:
> 
> On 07.12.2012, at 14:55, Jens Freimann wrote:
> 
>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>>
>> Since commit 967c0da73a7b0da186baba6632301d83644a570c
>>    vl.c: Avoid segfault when started with no arguments
>>
>> the user can specify a kernel without a command line. Lets not
>> overwrite the default command line with \0 in that case.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> ---
>> hw/s390-virtio.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>> index ca1bb09..d77871a 100644
>> --- a/hw/s390-virtio.c
>> +++ b/hw/s390-virtio.c
>> @@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>>         stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>>     }
>>
>> -    if (rom_ptr(KERN_PARM_AREA)) {
>> +    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {
> 
> why strlen()? If no -append option was passed, kernel_cmdline should be NULL. If -append "" was passed, the user wants to command line to be overwritten with "\0".

Nope. kernel_cmdline is always a valid pointer.

vl.c:

[..]
    if (!kernel_cmdline) {
        kernel_cmdline = "";
    }

[..]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line
  2012-12-11 11:15     ` Christian Borntraeger
@ 2012-12-11 11:19       ` Alexander Graf
  2012-12-11 11:26         ` Christian Borntraeger
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Graf @ 2012-12-11 11:19 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Cornelia Huck, Jens Freimann, Heinz Graalfs, qemu-devel,
	Einar Lueck


On 11.12.2012, at 12:15, Christian Borntraeger wrote:

> On 11/12/12 11:34, Alexander Graf wrote:
>> 
>> On 07.12.2012, at 14:55, Jens Freimann wrote:
>> 
>>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>>> 
>>> Since commit 967c0da73a7b0da186baba6632301d83644a570c
>>>   vl.c: Avoid segfault when started with no arguments
>>> 
>>> the user can specify a kernel without a command line. Lets not
>>> overwrite the default command line with \0 in that case.
>>> 
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>>> ---
>>> hw/s390-virtio.c |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>> 
>>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>>> index ca1bb09..d77871a 100644
>>> --- a/hw/s390-virtio.c
>>> +++ b/hw/s390-virtio.c
>>> @@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>>>        stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>>>    }
>>> 
>>> -    if (rom_ptr(KERN_PARM_AREA)) {
>>> +    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {
>> 
>> why strlen()? If no -append option was passed, kernel_cmdline should be NULL. If -append "" was passed, the user wants to command line to be overwritten with "\0".
> 
> Nope. kernel_cmdline is always a valid pointer.
> 
> vl.c:
> 
> [..]
>    if (!kernel_cmdline) {
>        kernel_cmdline = "";
>    }

Then that's on purpose. Either we change the default for everyone or not at all. But checking for "" only in the s390 machine sounds off.


Alex

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line
  2012-12-11 11:19       ` Alexander Graf
@ 2012-12-11 11:26         ` Christian Borntraeger
  0 siblings, 0 replies; 15+ messages in thread
From: Christian Borntraeger @ 2012-12-11 11:26 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Cornelia Huck, Jens Freimann, Heinz Graalfs, qemu-devel,
	Einar Lueck

On 11/12/12 12:19, Alexander Graf wrote:
> 
> On 11.12.2012, at 12:15, Christian Borntraeger wrote:
> 
>> On 11/12/12 11:34, Alexander Graf wrote:
>>>
>>> On 07.12.2012, at 14:55, Jens Freimann wrote:
>>>
>>>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>
>>>> Since commit 967c0da73a7b0da186baba6632301d83644a570c
>>>>   vl.c: Avoid segfault when started with no arguments
>>>>
>>>> the user can specify a kernel without a command line. Lets not
>>>> overwrite the default command line with \0 in that case.
>>>>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>>>> ---
>>>> hw/s390-virtio.c |    2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>>>> index ca1bb09..d77871a 100644
>>>> --- a/hw/s390-virtio.c
>>>> +++ b/hw/s390-virtio.c
>>>> @@ -290,7 +290,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>>>>        stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>>>>    }
>>>>
>>>> -    if (rom_ptr(KERN_PARM_AREA)) {
>>>> +    if (rom_ptr(KERN_PARM_AREA) && strlen(kernel_cmdline)) {
>>>
>>> why strlen()? If no -append option was passed, kernel_cmdline should be NULL. If -append "" was passed, the user wants to command line to be overwritten with "\0".
>>
>> Nope. kernel_cmdline is always a valid pointer.
>>
>> vl.c:
>>
>> [..]
>>    if (!kernel_cmdline) {
>>        kernel_cmdline = "";
>>    }
> 
> Then that's on purpose. Either we change the default for everyone or not at all. But checking for "" only in the s390 machine sounds off.

Ok, makes sense.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2012-12-11 11:26 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-07 13:55 [Qemu-devel] [PATCH 0/3] s390: ipl device and cpu reset handler Jens Freimann
2012-12-07 13:55 ` [Qemu-devel] [PATCH 1/3] s390: Fix empty kernel command line Jens Freimann
2012-12-11 10:34   ` Alexander Graf
2012-12-11 11:15     ` Christian Borntraeger
2012-12-11 11:19       ` Alexander Graf
2012-12-11 11:26         ` Christian Borntraeger
2012-12-07 13:55 ` [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device Jens Freimann
2012-12-07 15:14   ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Christian Borntraeger
2012-12-07 15:14     ` [Qemu-devel] [RFC/PATCH 1/2] s390: Add bootmap parsing to ipl device Christian Borntraeger
2012-12-07 15:14     ` [Qemu-devel] [RFC/PATCH 2/2] s390: enable ipl device for virtio-ccw Christian Borntraeger
2012-12-11 10:36     ` [Qemu-devel] [RFC/PATCH 0/2] ipl device followup Alexander Graf
2012-12-11 10:38   ` [Qemu-devel] [PATCH 2/3] s390: Move IPL code into a separate device Alexander Graf
2012-12-07 13:55 ` [Qemu-devel] [PATCH 3/3] s390: add cpu reset handler Jens Freimann
2012-12-07 19:07   ` Andreas Färber
2012-12-08 17:32     ` Jens Freimann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).