Patchwork [v6,2/8] virtio-blk: add the virtio-blk device.

login
register
mail settings
Submitter fred.konrad@greensocs.com
Date March 12, 2013, 9:22 a.m.
Message ID <1363080131-16427-3-git-send-email-fred.konrad@greensocs.com>
Download mbox | patch
Permalink /patch/226865/
State New
Headers show

Comments

fred.konrad@greensocs.com - March 12, 2013, 9:22 a.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 | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 hw/virtio-blk.h | 28 +++++++++++++++++
 hw/virtio-pci.c | 11 +------
 3 files changed, 122 insertions(+), 15 deletions(-)
Peter Maydell - March 12, 2013, 2:28 p.m.
On 12 March 2013 09:22,  <fred.konrad@greensocs.com> wrote:
>  /* The ID for virtio_block */
> @@ -130,4 +134,28 @@ typedef struct VirtIOBlock {
>  #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
>          DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
>
> +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)                          \
> +        DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0, false),
> +#else
> +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
> +#endif /* CONFIG_VIRTIO_BLK_DATA_PLANE */
> +
> +#ifdef __linux__
> +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)                       \
> +        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
> +#else
> +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
> +#endif /* __linux__ */
> +
> +#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
> +        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
> +        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
> +        DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
> +        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
> +        DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)                       \
> +        DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
> +
> +void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
> +
>  #endif
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 39c1966..9ed0228 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -1084,19 +1084,10 @@ static void virtio_rng_exit_pci(PCIDevice *pci_dev)
>
>  static Property virtio_blk_properties[] = {
>      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> -    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
> -    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOPCIProxy, blk.conf),
> -    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
> -#ifdef __linux__
> -    DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
> -#endif
> -    DEFINE_PROP_BIT("config-wce", VirtIOPCIProxy, blk.config_wce, 0, true),
>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> -#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> -    DEFINE_PROP_BIT("x-data-plane", VirtIOPCIProxy, blk.data_plane, 0, false),
> -#endif
>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
> +    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOPCIProxy, blk)
>      DEFINE_PROP_END_OF_LIST(),

You need to tweak your macro definitions so that the user can
put a comma after DEFINE_VIRTIO_BLK_PROPERTIES() [compare
DEFINE_BLOCK_PROPERTIES and DEFINE_BLOCK_CHS_PROPERTIES].
Otherwise it's going to get confusing and somebody's going
to add one without noticing that that gets you an extra
empty properties array element.

>  };
>
> --
> 1.7.11.7
>

-- PMM
fred.konrad@greensocs.com - March 12, 2013, 2:37 p.m.
On 12/03/2013 15:28, Peter Maydell wrote:
> On 12 March 2013 09:22,  <fred.konrad@greensocs.com> wrote:
>>   /* The ID for virtio_block */
>> @@ -130,4 +134,28 @@ typedef struct VirtIOBlock {
>>   #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
>>           DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
>>
>> +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>> +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)                          \
>> +        DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0, false),
>> +#else
>> +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
>> +#endif /* CONFIG_VIRTIO_BLK_DATA_PLANE */
>> +
>> +#ifdef __linux__
>> +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)                       \
>> +        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
>> +#else
>> +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
>> +#endif /* __linux__ */
>> +
>> +#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
>> +        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
>> +        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
>> +        DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
>> +        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
>> +        DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)                       \
>> +        DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
>> +
>> +void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
>> +
>>   #endif
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 39c1966..9ed0228 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -1084,19 +1084,10 @@ static void virtio_rng_exit_pci(PCIDevice *pci_dev)
>>
>>   static Property virtio_blk_properties[] = {
>>       DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>> -    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
>> -    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOPCIProxy, blk.conf),
>> -    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
>> -#ifdef __linux__
>> -    DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
>> -#endif
>> -    DEFINE_PROP_BIT("config-wce", VirtIOPCIProxy, blk.config_wce, 0, true),
>>       DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>> -#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>> -    DEFINE_PROP_BIT("x-data-plane", VirtIOPCIProxy, blk.data_plane, 0, false),
>> -#endif
>>       DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>>       DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
>> +    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOPCIProxy, blk)
>>       DEFINE_PROP_END_OF_LIST(),
> You need to tweak your macro definitions so that the user can
> put a comma after DEFINE_VIRTIO_BLK_PROPERTIES() [compare
> DEFINE_BLOCK_PROPERTIES and DEFINE_BLOCK_CHS_PROPERTIES].
> Otherwise it's going to get confusing and somebody's going
> to add one without noticing that that gets you an extra
> empty properties array element.
Do you have any idea for that?

It's a little tricky with the two conditions 
CONFIG_VIRTIO_BLK_DATA_PLANE and __linux__,

I can't have #define with an #ifdef inside..

Do you see what I mean?
>
>>   };
>>
>> --
>> 1.7.11.7
>>
> -- PMM
Peter Maydell - March 12, 2013, 2:42 p.m.
On 12 March 2013 14:37, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> On 12/03/2013 15:28, Peter Maydell wrote:
>>
>> On 12 March 2013 09:22,  <fred.konrad@greensocs.com> wrote:
>>>
>>>   /* The ID for virtio_block */
>>> @@ -130,4 +134,28 @@ typedef struct VirtIOBlock {
>>>   #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
>>>           DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
>>>
>>> +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>>> +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
>>> \
>>> +        DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0,
>>> false),
>>> +#else
>>> +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
>>> +#endif /* CONFIG_VIRTIO_BLK_DATA_PLANE */
>>> +
>>> +#ifdef __linux__
>>> +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
>>> \
>>> +        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
>>> +#else
>>> +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
>>> +#endif /* __linux__ */
>>> +
>>> +#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)
>>> \
>>> +        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),
>>> \
>>> +        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),
>>> \
>>> +        DEFINE_PROP_STRING("serial", _state, _field.serial),
>>> \
>>> +        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0,
>>> true),    \
>>> +        DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
>>> \
>>> +        DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
>>> +
>>> +void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
>>> +
>>>   #endif
>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>> index 39c1966..9ed0228 100644
>>> --- a/hw/virtio-pci.c
>>> +++ b/hw/virtio-pci.c
>>> @@ -1084,19 +1084,10 @@ static void virtio_rng_exit_pci(PCIDevice
>>> *pci_dev)
>>>
>>>   static Property virtio_blk_properties[] = {
>>>       DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>>> -    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
>>> -    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOPCIProxy, blk.conf),
>>> -    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
>>> -#ifdef __linux__
>>> -    DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
>>> -#endif
>>> -    DEFINE_PROP_BIT("config-wce", VirtIOPCIProxy, blk.config_wce, 0,
>>> true),
>>>       DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>>> VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>>> -#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>>> -    DEFINE_PROP_BIT("x-data-plane", VirtIOPCIProxy, blk.data_plane, 0,
>>> false),
>>> -#endif
>>>       DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>>>       DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
>>> +    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOPCIProxy, blk)
>>>       DEFINE_PROP_END_OF_LIST(),
>>
>> You need to tweak your macro definitions so that the user can
>> put a comma after DEFINE_VIRTIO_BLK_PROPERTIES() [compare
>> DEFINE_BLOCK_PROPERTIES and DEFINE_BLOCK_CHS_PROPERTIES].
>> Otherwise it's going to get confusing and somebody's going
>> to add one without noticing that that gets you an extra
>> empty properties array element.
>
> Do you have any idea for that?
>
> It's a little tricky with the two conditions CONFIG_VIRTIO_BLK_DATA_PLANE
> and __linux__,
>
> I can't have #define with an #ifdef inside..
>
> Do you see what I mean?

Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
ones that are expected to be used by other code, right? So you can
define them with commas (and name them something so it's obvious
they're not intended for wider use as property array elements),
and then just make sure your public-facing DEFINE_VIRTIO_BLK_PROPERTIES
doesn't end with a comma. (You can do that by putting the macros
that expand to maybe-comma-or-not at the front, not the end.)

-- PMM
fred.konrad@greensocs.com - March 12, 2013, 3:08 p.m.
On 12/03/2013 15:42, Peter Maydell wrote:
> On 12 March 2013 14:37, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>> On 12/03/2013 15:28, Peter Maydell wrote:
>>> On 12 March 2013 09:22,  <fred.konrad@greensocs.com> wrote:
>>>>    /* The ID for virtio_block */
>>>> @@ -130,4 +134,28 @@ typedef struct VirtIOBlock {
>>>>    #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
>>>>            DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
>>>>
>>>> +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>>>> +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
>>>> \
>>>> +        DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0,
>>>> false),
>>>> +#else
>>>> +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
>>>> +#endif /* CONFIG_VIRTIO_BLK_DATA_PLANE */
>>>> +
>>>> +#ifdef __linux__
>>>> +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
>>>> \
>>>> +        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
>>>> +#else
>>>> +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
>>>> +#endif /* __linux__ */
>>>> +
>>>> +#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)
>>>> \
>>>> +        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),
>>>> \
>>>> +        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),
>>>> \
>>>> +        DEFINE_PROP_STRING("serial", _state, _field.serial),
>>>> \
>>>> +        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0,
>>>> true),    \
>>>> +        DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
>>>> \
>>>> +        DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
>>>> +
>>>> +void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
>>>> +
>>>>    #endif
>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>> index 39c1966..9ed0228 100644
>>>> --- a/hw/virtio-pci.c
>>>> +++ b/hw/virtio-pci.c
>>>> @@ -1084,19 +1084,10 @@ static void virtio_rng_exit_pci(PCIDevice
>>>> *pci_dev)
>>>>
>>>>    static Property virtio_blk_properties[] = {
>>>>        DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>>>> -    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
>>>> -    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOPCIProxy, blk.conf),
>>>> -    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
>>>> -#ifdef __linux__
>>>> -    DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
>>>> -#endif
>>>> -    DEFINE_PROP_BIT("config-wce", VirtIOPCIProxy, blk.config_wce, 0,
>>>> true),
>>>>        DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>>>> VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>>>> -#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>>>> -    DEFINE_PROP_BIT("x-data-plane", VirtIOPCIProxy, blk.data_plane, 0,
>>>> false),
>>>> -#endif
>>>>        DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>>>>        DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
>>>> +    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOPCIProxy, blk)
>>>>        DEFINE_PROP_END_OF_LIST(),
>>> You need to tweak your macro definitions so that the user can
>>> put a comma after DEFINE_VIRTIO_BLK_PROPERTIES() [compare
>>> DEFINE_BLOCK_PROPERTIES and DEFINE_BLOCK_CHS_PROPERTIES].
>>> Otherwise it's going to get confusing and somebody's going
>>> to add one without noticing that that gets you an extra
>>> empty properties array element.
>> Do you have any idea for that?
>>
>> It's a little tricky with the two conditions CONFIG_VIRTIO_BLK_DATA_PLANE
>> and __linux__,
>>
>> I can't have #define with an #ifdef inside..
>>
>> Do you see what I mean?
> Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
> and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
> ones that are expected to be used by other code, right? So you can
> define them with commas (and name them something so it's obvious
> they're not intended for wider use as property array elements),
> and then just make sure your public-facing DEFINE_VIRTIO_BLK_PROPERTIES
> doesn't end with a comma. (You can do that by putting the macros
> that expand to maybe-comma-or-not at the front, not the end.)
>
> -- PMM
ok, I can put a comment which say not to use them?

Thanks,

Fred
Peter Maydell - March 12, 2013, 3:12 p.m.
On 12 March 2013 15:08, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> On 12/03/2013 15:42, Peter Maydell wrote:
>>
>> Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
>> and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
>> ones that are expected to be used by other code, right? So you can
>> define them with commas (and name them something so it's obvious
>> they're not intended for wider use as property array elements),
>> and then just make sure your public-facing DEFINE_VIRTIO_BLK_PROPERTIES
>> doesn't end with a comma. (You can do that by putting the macros
>> that expand to maybe-comma-or-not at the front, not the end.)
>>
>> -- PMM
>
> ok, I can put a comment which say not to use them?

And suitable macro names (ie not ones which look like all
the other DEFINE_FOO_PROPERTIES ones). Alternatively since the
macro's only used once as far as I can see, you could just not
bother to abstract it out. The virtio-ccw blk properties still
just have inline #ifdefs for the scsi prop for instance.

-- PMM
fred.konrad@greensocs.com - March 12, 2013, 3:22 p.m.
On 12/03/2013 16:12, Peter Maydell wrote:
> On 12 March 2013 15:08, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>> On 12/03/2013 15:42, Peter Maydell wrote:
>>> Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
>>> and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
>>> ones that are expected to be used by other code, right? So you can
>>> define them with commas (and name them something so it's obvious
>>> they're not intended for wider use as property array elements),
>>> and then just make sure your public-facing DEFINE_VIRTIO_BLK_PROPERTIES
>>> doesn't end with a comma. (You can do that by putting the macros
>>> that expand to maybe-comma-or-not at the front, not the end.)
>>>
>>> -- PMM
>> ok, I can put a comment which say not to use them?
> And suitable macro names (ie not ones which look like all
> the other DEFINE_FOO_PROPERTIES ones). Alternatively since the
> macro's only used once as far as I can see, you could just not
> bother to abstract it out. The virtio-ccw blk properties still
> just have inline #ifdefs for the scsi prop for instance.
>
> -- PMM

The macro is used for virtio-blk device and virtio-blk-pci.
s390x devices don't use the same properties.
Cornelia Huck - March 12, 2013, 4:31 p.m.
On Tue, 12 Mar 2013 16:22:22 +0100
KONRAD Frédéric <fred.konrad@greensocs.com> wrote:

> On 12/03/2013 16:12, Peter Maydell wrote:
> > On 12 March 2013 15:08, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> >> On 12/03/2013 15:42, Peter Maydell wrote:
> >>> Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
> >>> and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
> >>> ones that are expected to be used by other code, right? So you can
> >>> define them with commas (and name them something so it's obvious
> >>> they're not intended for wider use as property array elements),
> >>> and then just make sure your public-facing DEFINE_VIRTIO_BLK_PROPERTIES
> >>> doesn't end with a comma. (You can do that by putting the macros
> >>> that expand to maybe-comma-or-not at the front, not the end.)
> >>>
> >>> -- PMM
> >> ok, I can put a comment which say not to use them?
> > And suitable macro names (ie not ones which look like all
> > the other DEFINE_FOO_PROPERTIES ones). Alternatively since the
> > macro's only used once as far as I can see, you could just not
> > bother to abstract it out. The virtio-ccw blk properties still
> > just have inline #ifdefs for the scsi prop for instance.
> >
> > -- PMM
> 
> The macro is used for virtio-blk device and virtio-blk-pci.
> s390x devices don't use the same properties.
> 

Looking at the s390 devices, the difference seems to be the following:

- CHS - missing on virtio-ccw, I'll do a patch.
- config_wce - missing on s390-virtio and virtio-ccw, should probably
  be added.
- x-data-plane - we plan to add this eventually to virtio-ccw, but not
  to s390-virtio. Could that be split out from the generic properties?
fred.konrad@greensocs.com - March 13, 2013, 8:24 a.m.
On 12/03/2013 17:31, Cornelia Huck wrote:
> On Tue, 12 Mar 2013 16:22:22 +0100
> KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>
>> On 12/03/2013 16:12, Peter Maydell wrote:
>>> On 12 March 2013 15:08, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>>>> On 12/03/2013 15:42, Peter Maydell wrote:
>>>>> Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
>>>>> and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
>>>>> ones that are expected to be used by other code, right? So you can
>>>>> define them with commas (and name them something so it's obvious
>>>>> they're not intended for wider use as property array elements),
>>>>> and then just make sure your public-facing DEFINE_VIRTIO_BLK_PROPERTIES
>>>>> doesn't end with a comma. (You can do that by putting the macros
>>>>> that expand to maybe-comma-or-not at the front, not the end.)
>>>>>
>>>>> -- PMM
>>>> ok, I can put a comment which say not to use them?
>>> And suitable macro names (ie not ones which look like all
>>> the other DEFINE_FOO_PROPERTIES ones). Alternatively since the
>>> macro's only used once as far as I can see, you could just not
>>> bother to abstract it out. The virtio-ccw blk properties still
>>> just have inline #ifdefs for the scsi prop for instance.
>>>
>>> -- PMM
>> The macro is used for virtio-blk device and virtio-blk-pci.
>> s390x devices don't use the same properties.
>>
> Looking at the s390 devices, the difference seems to be the following:
>
> - CHS - missing on virtio-ccw, I'll do a patch.
> - config_wce - missing on s390-virtio and virtio-ccw, should probably
>    be added.
> - x-data-plane - we plan to add this eventually to virtio-ccw, but not
>    to s390-virtio. Could that be split out from the generic properties?
>
ok, so what I can do is:

- split up x-data-plane property (so it will be only in virtio-pci.c).
- fix this comma thing.

Then when you put these two missing properties you can just replace all 
of them
  with the macro.

Is that ok for everybody? Peter? Stefan?

Thanks for replies,

Fred
fred.konrad@greensocs.com - March 13, 2013, 3:32 p.m.
On 13/03/2013 09:24, KONRAD Frédéric wrote:
> On 12/03/2013 17:31, Cornelia Huck wrote:
>> On Tue, 12 Mar 2013 16:22:22 +0100
>> KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>>
>>> On 12/03/2013 16:12, Peter Maydell wrote:
>>>> On 12 March 2013 15:08, KONRAD Frédéric <fred.konrad@greensocs.com> 
>>>> wrote:
>>>>> On 12/03/2013 15:42, Peter Maydell wrote:
>>>>>> Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
>>>>>> and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
>>>>>> ones that are expected to be used by other code, right? So you can
>>>>>> define them with commas (and name them something so it's obvious
>>>>>> they're not intended for wider use as property array elements),
>>>>>> and then just make sure your public-facing 
>>>>>> DEFINE_VIRTIO_BLK_PROPERTIES
>>>>>> doesn't end with a comma. (You can do that by putting the macros
>>>>>> that expand to maybe-comma-or-not at the front, not the end.)
>>>>>>
>>>>>> -- PMM
>>>>> ok, I can put a comment which say not to use them?
>>>> And suitable macro names (ie not ones which look like all
>>>> the other DEFINE_FOO_PROPERTIES ones). Alternatively since the
>>>> macro's only used once as far as I can see, you could just not
>>>> bother to abstract it out. The virtio-ccw blk properties still
>>>> just have inline #ifdefs for the scsi prop for instance.
>>>>
>>>> -- PMM
>>> The macro is used for virtio-blk device and virtio-blk-pci.
>>> s390x devices don't use the same properties.
>>>
>> Looking at the s390 devices, the difference seems to be the following:
>>
>> - CHS - missing on virtio-ccw, I'll do a patch.
>> - config_wce - missing on s390-virtio and virtio-ccw, should probably
>>    be added.
>> - x-data-plane - we plan to add this eventually to virtio-ccw, but not
>>    to s390-virtio. Could that be split out from the generic properties?
>>
> ok, so what I can do is:
>
> - split up x-data-plane property (so it will be only in virtio-pci.c).
> - fix this comma thing.
>
> Then when you put these two missing properties you can just replace 
> all of them
>  with the macro.
>
> Is that ok for everybody? Peter? Stefan?
>
Any other suggestion?

Thanks,

Fred

Patch

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 908c316..a0d0679 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -21,7 +21,11 @@ 
 #ifdef __linux__
 # include <scsi/sg.h>
 #endif
+#include "hw/virtio-bus.h"
 
+/*
+ * Moving to QOM later in this series.
+ */
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
 {
     return (VirtIOBlock *)vdev;
@@ -620,9 +624,16 @@  static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
-VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
+void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
 {
-    VirtIOBlock *s;
+    VirtIOBlock *s = VIRTIO_BLK(dev);
+    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
+}
+
+static VirtIODevice *virtio_blk_common_init(DeviceState *dev,
+                                          VirtIOBlkConf *blk, VirtIOBlock **ps)
+{
+    VirtIOBlock *s = *ps;
     static int virtio_blk_id;
 
     if (!blk->conf.bs) {
@@ -639,9 +650,20 @@  VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
         return NULL;
     }
 
-    s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
-                                          sizeof(struct virtio_blk_config),
-                                          sizeof(VirtIOBlock));
+    /*
+     * We have two cases here: the old virtio-blk-pci device, and the
+     * refactored virtio-blk.
+     */
+    if (s == NULL) {
+        /* virtio-blk-pci */
+        s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
+                                              sizeof(struct virtio_blk_config),
+                                              sizeof(VirtIOBlock));
+    } else {
+        /* virtio-blk */
+        virtio_init(VIRTIO_DEVICE(s), "virtio-blk", VIRTIO_ID_BLOCK,
+                    sizeof(struct virtio_blk_config));
+    }
 
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.set_config = virtio_blk_set_config;
@@ -675,6 +697,12 @@  VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
     return &s->vdev;
 }
 
+VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
+{
+    VirtIOBlock *s = NULL;
+    return virtio_blk_common_init(dev, blk, &s);
+}
+
 void virtio_blk_exit(VirtIODevice *vdev)
 {
     VirtIOBlock *s = to_virtio_blk(vdev);
@@ -688,3 +716,63 @@  void virtio_blk_exit(VirtIODevice *vdev)
     blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
 }
+
+
+static int virtio_blk_device_init(VirtIODevice *vdev)
+{
+    DeviceState *qdev = DEVICE(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
+    VirtIOBlkConf *blk = &(s->blk);
+    if (virtio_blk_common_init(qdev, blk, &s) == NULL) {
+        return -1;
+    }
+    return 0;
+}
+
+static int virtio_blk_device_exit(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtIOBlock *s = VIRTIO_BLK(dev);
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+    virtio_blk_data_plane_destroy(s->dataplane);
+    s->dataplane = NULL;
+#endif
+    qemu_del_vm_change_state_handler(s->change);
+    unregister_savevm(s->qdev, "virtio-blk", s);
+    blockdev_mark_auto_del(s->bs);
+    virtio_common_cleanup(vdev);
+    return 0;
+}
+
+static Property virtio_blk_properties[] = {
+    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlock, blk)
+    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->exit = virtio_blk_device_exit;
+    dc->props = virtio_blk_properties;
+    vdc->init = virtio_blk_device_init;
+    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 b704d50..5479424 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -20,6 +20,10 @@ 
 #include "dataplane/virtio-blk.h"
 #endif
 
+#define TYPE_VIRTIO_BLK "virtio-blk"
+#define VIRTIO_BLK(obj) \
+        OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK)
+
 /* from Linux's linux/virtio_blk.h */
 
 /* The ID for virtio_block */
@@ -130,4 +134,28 @@  typedef struct VirtIOBlock {
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
 
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)                          \
+        DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0, false),
+#else
+#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
+#endif /* CONFIG_VIRTIO_BLK_DATA_PLANE */
+
+#ifdef __linux__
+#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)                       \
+        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
+#else
+#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
+#endif /* __linux__ */
+
+#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
+        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
+        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
+        DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
+        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
+        DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)                       \
+        DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
+
+void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
+
 #endif
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 39c1966..9ed0228 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -1084,19 +1084,10 @@  static void virtio_rng_exit_pci(PCIDevice *pci_dev)
 
 static Property virtio_blk_properties[] = {
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
-    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
-    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOPCIProxy, blk.conf),
-    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
-#ifdef __linux__
-    DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
-#endif
-    DEFINE_PROP_BIT("config-wce", VirtIOPCIProxy, blk.config_wce, 0, true),
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    DEFINE_PROP_BIT("x-data-plane", VirtIOPCIProxy, blk.data_plane, 0, false),
-#endif
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOPCIProxy, blk)
     DEFINE_PROP_END_OF_LIST(),
 };