From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TgY8w-00072N-Se for qemu-devel@nongnu.org; Thu, 06 Dec 2012 04:53:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TgY8v-0002Jk-B4 for qemu-devel@nongnu.org; Thu, 06 Dec 2012 04:53:22 -0500 Received: from cantor2.suse.de ([195.135.220.15]:49840 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TgY8v-0002JY-1Y for qemu-devel@nongnu.org; Thu, 06 Dec 2012 04:53:21 -0500 Message-ID: <50C06B0C.7040901@suse.de> Date: Thu, 06 Dec 2012 10:53:16 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1354631742-4693-1-git-send-email-fred.konrad@greensocs.com> <1354631742-4693-7-git-send-email-fred.konrad@greensocs.com> <50BF82ED.4070205@suse.de> <50C063A5.6050101@greensocs.com> In-Reply-To: <50C063A5.6050101@greensocs.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?S09OUkFEIEZyw6lkw6lyaWM=?= Cc: Peter Maydell , aliguori@us.ibm.com, e.voevodin@samsung.com, mark.burton@greensocs.com, qemu-devel@nongnu.org, stefanha@redhat.com, cornelia.huck@de.ibm.com Am 06.12.2012 10:21, schrieb KONRAD Fr=C3=A9d=C3=A9ric: > On 05/12/2012 18:22, Andreas F=C3=A4rber wrote: >> Am 05.12.2012 17:25, schrieb Peter Maydell: >>> On 4 December 2012 14:35, wrote: >>>> From: KONRAD Frederic >>>> >>>> Create virtio-blk which extends virtio-device, so it can be >>>> connected on >>>> virtio-bus. >>>> >>>> Signed-off-by: KONRAD Frederic >>>> --- >>>> hw/virtio-blk.c | 170 >>>> ++++++++++++++++++++++++++++++++++++++++++++++++-------- >>>> hw/virtio-blk.h | 4 ++ >>>> 2 files changed, 150 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >>>> index e25cc96..ee1ea8b 100644 >>>> --- a/hw/virtio-blk.c >>>> +++ b/hw/virtio-blk.c >>>> @@ -21,24 +21,42 @@ >>>> #ifdef __linux__ >>>> # include >>>> #endif >>>> +#include "virtio-bus.h" >>>> >>>> +/* Take this structure as our device structure. */ >>>> typedef struct VirtIOBlock >>>> { >>>> + /* >>>> + * Adding parent_obj breaks to_virtio_blk cast function, >>>> + * and virtio_blk_init. >>>> + */ >>>> + DeviceState parent_obj; >>>> + /* >>>> + * We don't need that anymore, as we'll use QOM cast to get the >>>> + * VirtIODevice. Just temporary keep it, for not breaking >>>> functionality. >>>> + */ >>>> VirtIODevice vdev; >>> This doesn't make sense. After your previous patch, VirtIODevice >>> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice, >>> so there's nothing to do here. Adding this parent_obj field >>> here is just breaking things (it would make the VirtIOBlock >>> into a direct child of DeviceState, which isn't what we want). >>> >>>> BlockDriverState *bs; >>>> VirtQueue *vq; >>>> void *rq; >>>> QEMUBH *bh; >>>> BlockConf *conf; >>>> - VirtIOBlkConf *blk; >>>> + /* >>>> + * We can't use pointer with properties. >>>> + */ >>>> + VirtIOBlkConf blk; >>>> unsigned short sector_mask; >>>> DeviceState *qdev; >>>> } VirtIOBlock; >>>> >>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>>> -{ >>>> - return (VirtIOBlock *)vdev; >>>> -} >>>> +/* >>>> + * Use the QOM cast, so we don't need that anymore. >>>> + * >>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>>> + * { >>>> + * return (VirtIOBlock *)vdev; >>>> + * } >>>> + */ >>> If we don't need it, just delete it. >> Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by >> OBJECT_CHECK(), and replace all callers of to_virtio_blk() with >> VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you o= n. > Yes, that's why I comment to_virtio_blk(). >=20 > Isn't what I made in this patch with : >=20 > +#define TYPE_VIRTIO_BLK "virtio-blk" > +#define VIRTIO_BLK(obj) \ > + OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK) > + >=20 > and >=20 > - VirtIOBlock *s =3D to_virtio_blk(vdev); > + VirtIOBlock *s =3D VIRTIO_BLK(vdev); >=20 > ? Sorry. I expected to see the macros above the typedef above, but in the header is even better! :) VIRTIO_BLOCK vs. VIRTIO_BLK is just a style question. Further, I missed on brief sight that the to_* function was commented out, thought it was still being used. Didn't find enough time to review the series fully yet. > I agree with that, but, there is an issue : > The refactored VirtIOBlk is a device and seems to work, but the device > which use this VirtIOBlock > (eg virtio-blk-pci) are just allocating a structure ( in > virtio_common_init ). >=20 > That's why this patch is breaking virtio-blk-pci. Don't understand that part due to lack of virtio knowledge... Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should only be necessary as a command line option alias for backwards compatibility, no? Are you saying you can't make this switch and refactoring for virtio-blk *only*? Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg