Re: [Qemu-devel] [PATCH 1/2] s390/ipl: Fix boot order

[Andreas Färber]

Hi,

Am 17.06.2013 14:29, schrieb Christian Borntraeger:
> The latest ipl code adoptions collided with some of the virtio

"adaptions"?

> refactoring rework. This resulted in always booting the first
> disk. Lets fix booting from a given ID.

"Let's"?

> The new code also checks for command lines without bootindex to
> avoid random behaviour when accessing dev_st (==0).
> 
> Signed-off-by: Christian Borntraeger <address@hidden>
> CC: address@hidden
> ---
>  hw/s390x/ipl.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0aeb003..8b25b1c 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -156,13 +156,15 @@ static void s390_ipl_reset(DeviceState *dev)
>      if (!ipl->kernel) {
>          /* booting firmware, tell what device to boot from */
>          DeviceState *dev_st = get_boot_device(0);
> -        VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
> -                OBJECT(&(dev_st->parent_obj)), "virtio-blk-ccw");

This should've never accessed parent_obj but simply use OBJECT(dev_st).

I would expect object_dynamic_cast() to return NULL on NULL input then,
but it seems the issue is rather that dev_st will be the VirtioDevice
and not the VirtIOS390Device.

> -
> -        if (ccw_dev) {
> -            env->regs[7] = ccw_dev->sch->cssid << 24 |
> -                           ccw_dev->sch->ssid << 16 |
> -                           ccw_dev->sch->devno;
> +        if (dev_st) {
> +            VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) 
> object_dynamic_cast(
> +                    OBJECT((dev_st->parent_obj.parent)), "virtio-blk-ccw");

This is worse and equivalent to OBJECT(OBJECT(dev_st)->parent), with the
outer cast superfluous and parent being an Object-private field
according to include/qom/object.h.

IIRC we had once suggested to introduce an object_get_parent() accessor,
but Anthony was against it for some reason...? CC'ing.

Instead, I believe it would be permissible to access the device's bus,
which in turn has a pointer to its parent device:

OBJECT(qdev_get_parent_bus(dev_st)->parent)

> +
> +            if (ccw_dev) {
> +                env->regs[7] = ccw_dev->sch->cssid << 24 |
> +                               ccw_dev->sch->ssid << 16 |
> +                               ccw_dev->sch->devno;
> +            }

Previously, env->regs[7] would've been assigned -1 for !ccw_dev.
Functional change intentional?

Cheers,
Andreas

>          } else {
>              env->regs[7] = -1;
>          }

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg