[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [Qemu-devel] [PATCH 1/2] s390/ipl: Fix boot order
From: |
Andreas Färber |
Subject: |
Re: [Qemu-stable] [Qemu-devel] [PATCH 1/2] s390/ipl: Fix boot order |
Date: |
Mon, 17 Jun 2013 23:54:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130510 Thunderbird/17.0.6 |
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