From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52111) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkH31-0002sf-HQ for qemu-devel@nongnu.org; Sun, 16 Dec 2012 11:26:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TkH2z-0001DS-OX for qemu-devel@nongnu.org; Sun, 16 Dec 2012 11:26:39 -0500 Received: from cantor2.suse.de ([195.135.220.15]:51125 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkH2z-0001DO-Bj for qemu-devel@nongnu.org; Sun, 16 Dec 2012 11:26:37 -0500 Message-ID: <50CDF630.3070500@suse.de> Date: Sun, 16 Dec 2012 17:26:24 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= 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> In-Reply-To: <1355503606-54131-2-git-send-email-jfrei@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable 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: Jens Freimann , Christian Borntraeger Cc: Cornelia Huck , Heinz Graalfs , Einar Lueck , Alexander Graf , qemu-devel Am 14.12.2012 17:46, schrieb Jens Freimann: > From: Christian Borntraeger >=20 > Lets move the code to setup IPL for external kernel > or via the zipl rom into a separate file. This allows to >=20 > - 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 >=20 > 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 >=20 > --- > 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 >=20 > 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 =3D s390_virtio_bus_init(&my_ram_size); > s390_sclp_init(); > + dev =3D 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); > =20 > /* 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 > + * > + * 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 d= irectory. > + * > + */ > + > +#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 { 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 =3D qemu_get_cpu(0); > + env->psw.addr =3D pswaddr; > + env->psw.mask =3D IPL_PSW_MASK; > + s390_add_running_cpu(env); > +} > + > +static int s390_ipl_init(SysBusDevice *dev) > +{ > + S390IPLState *ipl =3D 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 =3D 0; > + > + if (!ipl->kernel) { > + ram_addr_t bios_size =3D 0; > + char *bios_filename; > + > + /* Load zipl bootloader */ > + if (bios_name =3D=3D NULL) { > + bios_name =3D ZIPL_FILENAME; > + } > + > + bios_filename =3D qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_nam= e); > + bios_size =3D load_image_targphys(bios_filename, ZIPL_IMAGE_ST= ART, 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 =3D load_elf(ipl->kernel, NULL, NULL, NULL, NULL, > + NULL, 1, ELF_MACHINE, 0); > + if (kernel_size =3D=3D -1UL) { > + kernel_size =3D load_image_targphys(ipl->kernel, 0, ram_si= ze); > + } > + if (kernel_size =3D=3D -1UL) { > + fprintf(stderr, "could not load kernel '%s'\n", ipl->kerne= l); > + 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 =3D INITRD_START; > + while (kernel_size + 0x100000 > initrd_offset) { > + initrd_offset +=3D 0x100000; > + } > + initrd_size =3D load_image_targphys(ipl->initrd, initrd_offset= , > + ram_size - initrd_offset); > + if (initrd_size =3D=3D -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[] =3D { > + 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 =3D DO_UPCAST(S390IPLState, dev.qdev, dev); Ditto. > + > + if (ipl->kernel) { > + /* > + * we can not rely on the ELF entry point, since up to 3.2 thi= s > + * 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 =3D DEVICE_CLASS(klass); > + SysBusDeviceClass *k =3D SYS_BUS_DEVICE_CLASS(klass); > + > + k->init =3D s390_ipl_init; > + dc->props =3D s390_ipl_properties; > + dc->reset =3D s390_ipl_reset; > + dc->no_user =3D 1; > +} > + > +static TypeInfo s390_ipl_info =3D { static const > + .class_init =3D s390_ipl_class_init, > + .parent =3D TYPE_SYS_BUS_DEVICE, > + .name =3D "s390-ipl", > + .instance_size =3D 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg