| Message ID | 20130314082535.64c08ea5@gondolin |
|---|---|
| State | New |
| Headers | show |
On 14/03/2013 08:25, Cornelia Huck wrote: > On Wed, 13 Mar 2013 16:32:31 +0100 > KONRAD Frédéric <fred.konrad@greensocs.com> wrote: > >> 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? > I currently have the following two patches sitting in my pending queue > (git://github.com/cohuck/qemu virtio-ccw-pending); I'll probably submit > them once my current pull request is through. > > On top of this, s390-virtio and virtio-ccw could use the generic macro > for the virtio-blk properties from the start if x-data-plane is split > out (I can add it to virtio-ccw once we support it). > Ok good, And what about config-wce property for virtio-blk-s390x? It is missing too. Fred
On Thu, 14 Mar 2013 09:37:54 +0100 KONRAD Frédéric <fred.konrad@greensocs.com> wrote: > On 14/03/2013 08:25, Cornelia Huck wrote: > > On Wed, 13 Mar 2013 16:32:31 +0100 > > KONRAD Frédéric <fred.konrad@greensocs.com> wrote: > > > >> 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? > > I currently have the following two patches sitting in my pending queue > > (git://github.com/cohuck/qemu virtio-ccw-pending); I'll probably submit > > them once my current pull request is through. > > > > On top of this, s390-virtio and virtio-ccw could use the generic macro > > for the virtio-blk properties from the start if x-data-plane is split > > out (I can add it to virtio-ccw once we support it). > > > Ok good, > > And what about config-wce property for virtio-blk-s390x? It is missing too. See the second patch :)
On 14/03/2013 09:42, Cornelia Huck wrote: > On Thu, 14 Mar 2013 09:37:54 +0100 > KONRAD Frédéric <fred.konrad@greensocs.com> wrote: > >> On 14/03/2013 08:25, Cornelia Huck wrote: >>> On Wed, 13 Mar 2013 16:32:31 +0100 >>> KONRAD Frédéric <fred.konrad@greensocs.com> wrote: >>> >>>> 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? >>> I currently have the following two patches sitting in my pending queue >>> (git://github.com/cohuck/qemu virtio-ccw-pending); I'll probably submit >>> them once my current pull request is through. >>> >>> On top of this, s390-virtio and virtio-ccw could use the generic macro >>> for the virtio-blk properties from the start if x-data-plane is split >>> out (I can add it to virtio-ccw once we support it). >>> >> Ok good, >> >> And what about config-wce property for virtio-blk-s390x? It is missing too. > See the second patch :) > > > good, sorry for the noise :). Thanks, Fred
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index d4361f6..70aba41 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -755,6 +755,7 @@ static const TypeInfo virtio_ccw_net = { static Property virtio_ccw_blk_properties[] = { DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id), DEFINE_BLOCK_PROPERTIES(VirtioCcwDevice, blk.conf), + DEFINE_BLOCK_CHS_PROPERTIES(VirtioCcwDevice, blk.conf), DEFINE_PROP_STRING("serial", VirtioCcwDevice, blk.serial), #ifdef __linux__ DEFINE_PROP_BIT("scsi", VirtioCcwDevice, blk.scsi, 0, true), -- 1.7.9.5 From 9e60f80b0ca9943ce3e6d11630c3b5a8bf0c3cbd Mon Sep 17 00:00:00 2001 From: Cornelia Huck <cornelia.huck@de.ibm.com> Date: Wed, 13 Mar 2013 15:20:07 +0100 Subject: [PATCH 2/2] s390-virtio, virtio-ccw: Add config_wce for virtio-blk. There's no reason why we wouldn't want to make the cache mode configurable. Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> --- hw/s390x/s390-virtio-bus.c | 1 + hw/s390x/virtio-ccw.c | 1 + 2 files changed, 2 insertions(+) diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c index d9b7f83..18f1292 100644 --- a/hw/s390x/s390-virtio-bus.c +++ b/hw/s390x/s390-virtio-bus.c @@ -434,6 +434,7 @@ static Property s390_virtio_blk_properties[] = { #ifdef __linux__ DEFINE_PROP_BIT("scsi", VirtIOS390Device, blk.scsi, 0, true), #endif + DEFINE_PROP_BIT("config-wce", VirtIOS390Device, blk.config_wce, 0, true), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 70aba41..5795bdd 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -761,6 +761,7 @@ static Property virtio_ccw_blk_properties[] = { DEFINE_PROP_BIT("scsi", VirtioCcwDevice, blk.scsi, 0, true), #endif DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]), + DEFINE_PROP_BIT("config-wce", VirtioCcwDevice, blk.config_wce, 0, true), DEFINE_PROP_END_OF_LIST(), };