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

["KONRAD Frédéric" <fred.konrad@greensocs.com>]

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