Re: [Qemu-devel] [PATCH 1/3] s390: Move IPL code into a separate device

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 16/12/12 17:26, Andreas Färber wrote:
> Am 14.12.2012 17:46, schrieb Jens Freimann:
>> 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
> 
> 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 <borntraeger@de.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@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 <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>
> 
> "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