From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38353) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkLZ7-0003Wz-Uz for qemu-devel@nongnu.org; Sun, 16 Dec 2012 16:16:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TkLZ6-00038k-GH for qemu-devel@nongnu.org; Sun, 16 Dec 2012 16:16:05 -0500 Received: from e06smtp18.uk.ibm.com ([195.75.94.114]:38128) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkLZ6-00038L-7Z for qemu-devel@nongnu.org; Sun, 16 Dec 2012 16:16:04 -0500 Received: from /spool/local by e06smtp18.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sun, 16 Dec 2012 21:15:36 -0000 Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps4075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qBGLFobv60620906 for ; Sun, 16 Dec 2012 21:15:50 GMT Received: from d06av09.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qBGLFvTP012200 for ; Sun, 16 Dec 2012 14:15:58 -0700 Message-ID: <50CE3A0C.3050800@de.ibm.com> Date: Sun, 16 Dec 2012 22:15:56 +0100 From: Christian Borntraeger MIME-Version: 1.0 References: <1355503606-54131-1-git-send-email-jfrei@linux.vnet.ibm.com> <1355503606-54131-2-git-send-email-jfrei@linux.vnet.ibm.com> <50CDF630.3070500@suse.de> In-Reply-To: <50CDF630.3070500@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 1/3] s390: Move IPL code into a separate device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Andreas_F=E4rber?= Cc: Heinz Graalfs , Alexander Graf , qemu-devel , Jens Freimann , Cornelia Huck , Einar Lueck On 16/12/12 17:26, Andreas Färber wrote: > Am 14.12.2012 17:46, schrieb Jens Freimann: >> From: Christian Borntraeger >> >> 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 >> Signed-off-by: Jens Freimann >> --- >> 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 >> + * >> + * 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"? 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