From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53440) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UohO2-0003p3-BS for qemu-devel@nongnu.org; Mon, 17 Jun 2013 17:54:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UohO0-0003Ay-SA for qemu-devel@nongnu.org; Mon, 17 Jun 2013 17:54:54 -0400 Message-ID: <51BF85A8.9000202@suse.de> Date: Mon, 17 Jun 2013 23:54:48 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1371472182-10074-1-git-send-email-borntraeger@de.ibm.com> <1371472182-10074-2-git-send-email-borntraeger@de.ibm.com> In-Reply-To: <1371472182-10074-2-git-send-email-borntraeger@de.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] s390/ipl: Fix boot order List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Borntraeger Cc: qemu-stable@nongnu.org, qemu-devel , Dominik Dingel , Alexander Graf , Anthony Liguori , =?ISO-8859-15?Q?KONRAD_Fr=E9d=E9ric?= 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 (=3D=3D0). >=20 > Signed-off-by: Christian Borntraeger > CC: qemu-stable@nongnu.org > --- > hw/s390x/ipl.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) >=20 > 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 =3D get_boot_device(0); > - VirtioCcwDevice *ccw_dev =3D (VirtioCcwDevice *) object_dynami= c_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] =3D ccw_dev->sch->cssid << 24 | > - ccw_dev->sch->ssid << 16 | > - ccw_dev->sch->devno; > + if (dev_st) { > + VirtioCcwDevice *ccw_dev =3D (VirtioCcwDevice *) object_dy= namic_cast( > + OBJECT((dev_st->parent_obj.parent)), "virtio-blk-c= cw"); 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] =3D 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] =3D -1; > } --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg