From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TgYPU-00064r-D3 for qemu-devel@nongnu.org; Thu, 06 Dec 2012 05:10:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TgYPS-00072c-Rd for qemu-devel@nongnu.org; Thu, 06 Dec 2012 05:10:28 -0500 Received: from greensocs.com ([87.106.252.221]:49311 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TgYPS-00072O-Hs for qemu-devel@nongnu.org; Thu, 06 Dec 2012 05:10:26 -0500 Message-ID: <50C06EFD.7090701@greensocs.com> Date: Thu, 06 Dec 2012 11:10:05 +0100 From: =?UTF-8?B?S09OUkFEIEZyw6lkw6lyaWM=?= 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> <50C06B0C.7040901@suse.de> In-Reply-To: <50C06B0C.7040901@suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed 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?QW5kcmVhcyBGw6RyYmVy?= 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 On 06/12/2012 10:53, Andreas F=C3=A4rber wrote: > 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 th= e >>>>> + * 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 = on. >> Yes, that's why I comment to_virtio_blk(). >> >> Isn't what I made in this patch with : >> >> +#define TYPE_VIRTIO_BLK "virtio-blk" >> +#define VIRTIO_BLK(obj) \ >> + OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK) >> + >> >> and >> >> - VirtIOBlock *s =3D to_virtio_blk(vdev); >> + VirtIOBlock *s =3D VIRTIO_BLK(vdev); >> >> ? > 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. Sorry for that, I'll remove the comment. > >> 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 ). >> >> 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 wit= h > this patch VirtIOBlk is moving to that new bus and virtio-blk-pci shoul= d > 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*? I mean that all device which use virtio_blk_init will be broken as we=20 use the OBJECT_CHECK cast and virtio_blk_init currently call virtio_common_init which allocate a VirtIODevice structure ( and don't create any device ). The other virtio-* devices shouldn't be affected. I must test. You suggest an alias for backwards compatibility ? It must create a=20 virtio-pci and a virtio-blk. Is it possible ? Fred > > Andreas >