Re: [Qemu-devel] [PATCH v6 2/8] virtio-blk: add the virtio-blk device.

[Cornelia Huck <cornelia.huck@de.ibm.com>]

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).

From 763c1a8ff61faaef5b488072cc9965bd29f8a1fd Mon Sep 17 00:00:00 2001
From: Cornelia Huck <cornelia.huck@de.ibm.com>
Date: Wed, 13 Mar 2013 14:43:22 +0100
Subject: [PATCH 1/2] virtio-ccw: Add missing blk chs properties.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/virtio-ccw.c |    1 +
 1 file changed, 1 insertion(+)

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(),
 };
 
-- 
1.7.9.5