Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[KONRAD Frédéric]

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,  <address@hidden> wrote:
> > > > > From: KONRAD Frederic <address@hidden>
> > > > >
> > > > > Create virtio-blk which extends virtio-device, so it can be
> > > > > connected on
> > > > > virtio-bus.
> > > > >
> > > > > Signed-off-by: KONRAD Frederic <address@hidden>
> > > > > ---
> > > > >    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