On 16/12/12 17:26, Andreas Färber wrote:
> 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?
cmdline is always a valid string, (never NULL), but kernel and initrd can
be NULL, which kills qdev_prop_set_string.
>> + * 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"?
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
