On 06/12/2012 10:53, Andreas Färber wrote:
Am 06.12.2012 10:21, schrieb KONRAD Frédéric:
On 05/12/2012 18:22, Andreas Färber wrote:
Am 05.12.2012 17:25, schrieb Peter Maydell:
On 4 December 2012 14:35, <fred.kon...@greensocs.com> wrote:
From: KONRAD Frederic <fred.kon...@greensocs.com>
Create virtio-blk which extends virtio-device, so it can be
connected on
virtio-bus.
Signed-off-by: KONRAD Frederic <fred.kon...@greensocs.com>
---
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 <scsi/sg.h>
#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 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 = to_virtio_blk(vdev);
+ VirtIOBlock *s = 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 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*?
I mean that all device which use virtio_blk_init will be broken as we
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
virtio-pci and
a virtio-blk. Is it possible ?
Fred
Andreas
