Am 14.12.2012 17:46, schrieb Jens Freimann:
> From: Christian Borntraeger <borntrae...@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 <borntrae...@de.ibm.com>
> Signed-off-by: Jens Freimann <jf...@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 <borntrae...@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
