Re: [Qemu-devel] [PATCH v2 1/5] S390: Add virtio-blk boot

[Alexander Graf]

On 29.04.2013, at 16:52, Dominik Dingel wrote:

> Check for a kernel IPL entry and load kernel image if one was specified.
> If no kernel image was supplied and no boot device was specified or the boot 
> device
> is not of type virtio-blk-ccw, print an error message and exit.
> 
> Signed-off-by: Christian Paro <address@hidden>
> Signed-off-by: Dominik Dingel <address@hidden>
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index ace5ff5..bcbba7c 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -16,6 +16,8 @@
> #include "elf.h"
> #include "hw/loader.h"
> #include "hw/sysbus.h"
> +#include "hw/s390x/virtio-ccw.h"
> +#include "hw/s390x/css.h"
> 
> #define KERN_IMAGE_START                0x010000UL
> #define KERN_PARM_AREA                  0x010480UL
> @@ -57,13 +59,22 @@ typedef struct S390IPLState {
> } S390IPLState;
> 
> 
> -static void s390_ipl_cpu(uint64_t pswaddr)
> +static void s390_ipl_cpu(uint64_t pswaddr, VirtioCcwDevice *dev)
> {
>     S390CPU *cpu = S390_CPU(qemu_get_cpu(0));
>     CPUS390XState *env = &cpu->env;
> 
>     env->psw.addr = pswaddr;
>     env->psw.mask = IPL_PSW_MASK;
> +
> +    if (dev) {
> +        env->regs[7] = dev->sch->cssid << 24 |
> +                       dev->sch->ssid << 16 |
> +                       dev->sch->devno;
> +    } else {
> +        env->regs[7] = -1;

R7 has nothing to do with the IPL. Please move it into the reset path.

> +    }
> +
>     s390_add_running_cpu(cpu);
> }
> 
> @@ -151,8 +162,17 @@ static Property s390_ipl_properties[] = {
> static void s390_ipl_reset(DeviceState *dev)
> {
>     S390IPLState *ipl = S390_IPL(dev);
> -
> -    s390_ipl_cpu(ipl->start_addr);
> +    
> +    if (ipl->kernel) {
> +        s390_ipl_cpu(ipl->start_addr, NULL);
> +    } else {
> +        /* is there a bootable device? */
> +        DeviceState *dev_st = get_boot_device(0);
> +        VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
> +                OBJECT(&(dev_st->parent_obj)), "virtio-blk-ccw");
> +       
> +        s390_ipl_cpu(ipl->start_addr, ccw_dev);
> +    }

Please move s390_ipl_cpu() out of the branches. How about writing it like this?

    if (!ipl->kernel) {
        /* booting firmware, tell it what device to boot off of */
        DeviceState *dev_st = get_boot_device(0);
        VirtioCcwDevice *ccw_dev = ...
        if (ccw_dev) {
            env->regs[7] = <devid calculation>;
        } else {
            env->regs[7] = -1;
        }
    }

    s390_ipl_cpu(ipl->start_addr);


Alex