| Message ID | 1363080131-16427-3-git-send-email-fred.konrad@greensocs.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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.
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?
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
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
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(), };