Patchwork [RFC,v5,6/6] virtio-blk : Refactor virtio-blk.

login
register
mail settings
Submitter fred.konrad@greensocs.com
Date Dec. 4, 2012, 2:35 p.m.
Message ID <1354631742-4693-7-git-send-email-fred.konrad@greensocs.com>
Download mbox | patch
Permalink /patch/203680/
State New
Headers show

Comments

fred.konrad@greensocs.com - Dec. 4, 2012, 2:35 p.m.
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(-)
Peter Maydell - Dec. 5, 2012, 4:25 p.m.
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.

-- PMM
Andreas Färber - Dec. 5, 2012, 5:22 p.m.
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.
You can then rename vdev to parent_obj as a check that you caught all users.

Regards,
Andreas
fred.konrad@greensocs.com - Dec. 6, 2012, 9:11 a.m.
On 05/12/2012 17:25, Peter Maydell wrote:
> 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).
>
Ok, I think you're right, I'll check.

>>       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.
>
> -- PMM
Yes, sure, I put it in comment to explain, why I deleted it.

Thanks,

Fred.
Andreas Färber - Dec. 6, 2012, 9:18 a.m.
Am 06.12.2012 10:11, schrieb KONRAD Frédéric:
> On 05/12/2012 17:25, Peter Maydell wrote:
>> On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
>>> -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.
>>
>> -- PMM
> Yes, sure, I put it in comment to explain, why I deleted it.

Please don't comment out unneeded things. Instead, remove them
completely and put the explanation into the commit message. That helps
keep the patch small and avoids commented-out code bitrotting.

Andreas
fred.konrad@greensocs.com - Dec. 6, 2012, 9:21 a.m.
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);

?


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.
>
> Regards,
> Andreas
>

Thanks,

Fred.
fred.konrad@greensocs.com - Dec. 6, 2012, 9:23 a.m.
On 06/12/2012 10:18, Andreas Färber wrote:
> Am 06.12.2012 10:11, schrieb KONRAD Frédéric:
>> On 05/12/2012 17:25, Peter Maydell wrote:
>>> On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
>>>> -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.
>>>
>>> -- PMM
>> Yes, sure, I put it in comment to explain, why I deleted it.
> Please don't comment out unneeded things. Instead, remove them
> completely and put the explanation into the commit message. That helps
> keep the patch small and avoids commented-out code bitrotting.
>
> Andreas
>
Ok, no problem.
Andreas Färber - Dec. 6, 2012, 9:53 a.m.
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.

> 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*?

Andreas
fred.konrad@greensocs.com - Dec. 6, 2012, 10:10 a.m.
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
>
Peter Maydell - Dec. 6, 2012, 10:13 a.m.
On 6 December 2012 09:53, Andreas Färber <afaerber@suse.de> wrote:
> Am 06.12.2012 10:21, schrieb KONRAD Frédéric:
>> 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?

It can't just be a command line alias, or we will break migration.
It has to be a simple device that composes together the virtio-pci
and virtio-blk devices, plus legacy support for properties and
migration state, I think.

-- PMM
fred.konrad@greensocs.com - Dec. 6, 2012, 1:58 p.m.
On 06/12/2012 11:13, Peter Maydell wrote:
> On 6 December 2012 09:53, Andreas Färber <afaerber@suse.de> wrote:
>> Am 06.12.2012 10:21, schrieb KONRAD Frédéric:
>>> 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?
> It can't just be a command line alias, or we will break migration.
> It has to be a simple device that composes together the virtio-pci
> and virtio-blk devices, plus legacy support for properties and
> migration state, I think.
>
> -- PMM
Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not
breaking anything ?

Or do you have a better idea ?

Fred
Peter Maydell - Dec. 6, 2012, 2:21 p.m.
On 6 December 2012 13:58, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> On 06/12/2012 11:13, Peter Maydell wrote:
>> It can't just be a command line alias, or we will break migration.
>> It has to be a simple device that composes together the virtio-pci
>> and virtio-blk devices, plus legacy support for properties and
>> migration state, I think.

> Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not
> breaking anything ?

Not breaking things is a key part of the requirements here.
It's ok to say "I haven't converted virtio-net or the s390
transport in this patchset and therefore they are broken" as
an initial RFC (because we can look at how PCI/blk is done
and check it works before we expand the same thing out to
other transports/devices).  But you need to show how the
virtio-blk / virtio-pci refactoring works and leaves you with
a virtio-blk-pci that isn't broken (either at the end or at
any step along the way).

-- PMM
fred.konrad@greensocs.com - Dec. 6, 2012, 2:48 p.m.
On 06/12/2012 15:21, Peter Maydell wrote:
> On 6 December 2012 13:58, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>> On 06/12/2012 11:13, Peter Maydell wrote:
>>> It can't just be a command line alias, or we will break migration.
>>> It has to be a simple device that composes together the virtio-pci
>>> and virtio-blk devices, plus legacy support for properties and
>>> migration state, I think.
>> Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not
>> breaking anything ?
> Not breaking things is a key part of the requirements here.
Agree with that.

> It's ok to say "I haven't converted virtio-net or the s390
> transport in this patchset and therefore they are broken" as
> an initial RFC (because we can look at how PCI/blk is done
> and check it works before we expand the same thing out to
> other transports/devices).  But you need to show how the
> virtio-blk / virtio-pci refactoring works and leaves you with
> a virtio-blk-pci that isn't broken (either at the end or at
> any step along the way).
And if virtio-blk-pci is broken can we refactor it in the same "patch" ?

>
> -- PMM
>

Patch

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;
     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;
+ * }
+ */
 
 typedef struct VirtIOBlockReq
 {
@@ -55,18 +73,20 @@  typedef struct VirtIOBlockReq
 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 {
     VirtIOBlock *s = req->dev;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
     trace_virtio_blk_req_complete(req, status);
 
     stb_p(&req->in->status, status);
     virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
-    virtio_notify(&s->vdev, s->vq);
+    virtio_notify(vdev, s->vq);
 }
 
 static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     bool is_read)
 {
-    BlockErrorAction action = bdrv_get_error_action(req->dev->bs, is_read, error);
+    BlockErrorAction action = bdrv_get_error_action(req->dev->bs, is_read,
+                                                    error);
     VirtIOBlock *s = req->dev;
 
     if (action == BDRV_ACTION_STOP) {
@@ -164,7 +184,7 @@  static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
      */
     req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
 
-    if (!req->dev->blk->scsi) {
+    if (!req->dev->blk.scsi) {
         status = VIRTIO_BLK_S_UNSUPP;
         goto fail;
     }
@@ -384,7 +404,7 @@  static void virtio_blk_handle_request(VirtIOBlockReq *req,
          * terminated by '\0' only when shorter than buffer.
          */
         strncpy(req->elem.in_sg[0].iov_base,
-                s->blk->serial ? s->blk->serial : "",
+                s->blk.serial ? s->blk.serial : "",
                 MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
         g_free(req);
@@ -401,7 +421,7 @@  static void virtio_blk_handle_request(VirtIOBlockReq *req,
 
 static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     VirtIOBlockReq *req;
     MultiReqBuffer mrb = {
         .num_writes = 0,
@@ -422,7 +442,7 @@  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
 static void virtio_blk_dma_restart_bh(void *opaque)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlock *s = VIRTIO_BLK(opaque);
     VirtIOBlockReq *req = s->rq;
     MultiReqBuffer mrb = {
         .num_writes = 0,
@@ -444,7 +464,7 @@  static void virtio_blk_dma_restart_bh(void *opaque)
 static void virtio_blk_dma_restart_cb(void *opaque, int running,
                                       RunState state)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlock *s = VIRTIO_BLK(opaque);
 
     if (!running)
         return;
@@ -468,7 +488,7 @@  static void virtio_blk_reset(VirtIODevice *vdev)
  */
 static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     struct virtio_blk_config blkcfg;
     uint64_t capacity;
     int blk_size = s->conf->logical_block_size;
@@ -507,7 +527,7 @@  static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
 static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     struct virtio_blk_config blkcfg;
 
     memcpy(&blkcfg, config, sizeof(blkcfg));
@@ -516,7 +536,7 @@  static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
 
 static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
 
     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
     features |= (1 << VIRTIO_BLK_F_GEOMETRY);
@@ -535,7 +555,7 @@  static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
 
 static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     uint32_t features;
 
     if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
@@ -548,10 +568,11 @@  static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 
 static void virtio_blk_save(QEMUFile *f, void *opaque)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlock *s = VIRTIO_BLK(opaque);
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
     VirtIOBlockReq *req = s->rq;
 
-    virtio_save(&s->vdev, f);
+    virtio_save(vdev, f);
     
     while (req) {
         qemu_put_sbyte(f, 1);
@@ -563,13 +584,14 @@  static void virtio_blk_save(QEMUFile *f, void *opaque)
 
 static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlock *s = VIRTIO_BLK(opaque);
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
     int ret;
 
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(&s->vdev, f);
+    ret = virtio_load(vdev, f);
     if (ret) {
         return ret;
     }
@@ -591,9 +613,9 @@  static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 
 static void virtio_blk_resize(void *opaque)
 {
-    VirtIOBlock *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
 
-    virtio_notify_config(&s->vdev);
+    virtio_notify_config(vdev);
 }
 
 static const BlockDevOps virtio_block_ops = {
@@ -630,7 +652,7 @@  VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
     s->vdev.reset = virtio_blk_reset;
     s->bs = blk->conf.bs;
     s->conf = &blk->conf;
-    s->blk = blk;
+    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
     s->rq = NULL;
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
@@ -651,8 +673,108 @@  VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
 
 void virtio_blk_exit(VirtIODevice *vdev)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     unregister_savevm(s->qdev, "virtio-blk", s);
     blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
 }
+
+static int virtio_device_init(DeviceState *qdev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
+    VirtIOBlock *s = VIRTIO_BLK(qdev);
+
+    VirtIOBlkConf *blk = &(s->blk);
+    static int virtio_blk_id;
+
+    if (!blk->conf.bs) {
+        error_report("drive property not set");
+        return -1;
+    }
+    if (!bdrv_is_inserted(blk->conf.bs)) {
+        error_report("Device needs media, but drive is empty");
+        return -1;
+    }
+
+    blkconf_serial(&blk->conf, &blk->serial);
+    if (blkconf_geometry(&blk->conf, NULL, 65535, 255, 255) < 0) {
+        return -1;
+    }
+
+    virtio_common_init_(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
+                                          sizeof(struct virtio_blk_config),
+                                          sizeof(VirtIOBlock));
+
+    vdev->get_config = virtio_blk_update_config;
+    vdev->set_config = virtio_blk_set_config;
+    vdev->get_features = virtio_blk_get_features;
+    vdev->set_status = virtio_blk_set_status;
+    vdev->reset = virtio_blk_reset;
+    s->bs = blk->conf.bs;
+    s->conf = &blk->conf;
+    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
+    s->rq = NULL;
+    s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
+
+    s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
+
+    qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
+    s->qdev = qdev;
+    register_savevm(qdev, "virtio-blk", virtio_blk_id++, 2,
+                    virtio_blk_save, virtio_blk_load, s);
+    bdrv_set_dev_ops(s->bs, &virtio_block_ops, s);
+    bdrv_set_buffer_alignment(s->bs, s->conf->logical_block_size);
+
+    bdrv_iostatus_enable(s->bs);
+    add_boot_device_path(s->conf->bootindex, qdev, "/disk@0,0");
+
+    virtio_bus_plug_device(vdev);
+
+    return 0;
+}
+
+static int virtio_device_exit(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    virtio_blk_exit(vdev);
+    return 0;
+}
+
+static Property virtio_blk_properties[] = {
+    DEFINE_BLOCK_PROPERTIES(VirtIOBlock, blk.conf),
+    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtIOBlock, blk.serial),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtIOBlock, blk.scsi, 0, true),
+#endif
+    DEFINE_PROP_BIT("config-wce", VirtIOBlock, blk.config_wce, 0, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_blk_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    dc->init = virtio_device_init;
+    dc->exit = virtio_device_exit;
+    dc->props = virtio_blk_properties;
+    vdc->get_config = virtio_blk_update_config;
+    vdc->set_config = virtio_blk_set_config;
+    vdc->get_features = virtio_blk_get_features;
+    vdc->set_status = virtio_blk_set_status;
+    vdc->reset = virtio_blk_reset;
+}
+
+static const TypeInfo virtio_device_info = {
+    .name = TYPE_VIRTIO_BLK,
+    .parent = TYPE_VIRTIO_DEVICE,
+    .instance_size = sizeof(VirtIOBlock),
+    .class_init = virtio_blk_class_init,
+};
+
+static void virtio_register_types(void)
+{
+    type_register_static(&virtio_device_info);
+}
+
+type_init(virtio_register_types)
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index f0740d0..a33336e 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -111,4 +111,8 @@  struct VirtIOBlkConf
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
         DEFINE_PROP_BIT("config-wce", _state, _field, VIRTIO_BLK_F_CONFIG_WCE, true)
 
+#define TYPE_VIRTIO_BLK "virtio-blk"
+#define VIRTIO_BLK(obj) \
+        OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK)
+
 #endif