From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38439) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UG2Xs-0000TQ-Kb for qemu-devel@nongnu.org; Thu, 14 Mar 2013 03:25:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UG2Xp-0003TN-Ck for qemu-devel@nongnu.org; Thu, 14 Mar 2013 03:25:48 -0400 Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:54723) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UG2Xp-0003Se-1n for qemu-devel@nongnu.org; Thu, 14 Mar 2013 03:25:45 -0400 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 14 Mar 2013 07:23:43 -0000 Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by b06cxnps3074.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2E7PTkb50855978 for ; Thu, 14 Mar 2013 07:25:29 GMT Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2E7PbQv014586 for ; Thu, 14 Mar 2013 01:25:38 -0600 Date: Thu, 14 Mar 2013 08:25:35 +0100 From: Cornelia Huck Message-ID: <20130314082535.64c08ea5@gondolin> In-Reply-To: <51409C0F.4000007@greensocs.com> References: <1363080131-16427-1-git-send-email-fred.konrad@greensocs.com> <1363080131-16427-3-git-send-email-fred.konrad@greensocs.com> <513F3DAF.8050405@greensocs.com> <513F44E7.2020306@greensocs.com> <513F482E.1000106@greensocs.com> <20130312173102.3e86ef76@gondolin> <514037CA.1010607@greensocs.com> <51409C0F.4000007@greensocs.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 2/8] virtio-blk: add the virtio-blk device. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: KONRAD =?UTF-8?B?RnLDqWTDqXJpYw==?= Cc: Kevin Wolf , Peter Maydell , aliguori@us.ibm.com, mst@redhat.com, mark.burton@greensocs.com, qemu-devel@nongnu.org, Stefan Hajnoczi , afaerber@suse.de On Wed, 13 Mar 2013 16:32:31 +0100 KONRAD Fr=C3=A9d=C3=A9ric wrote: > On 13/03/2013 09:24, KONRAD Fr=C3=A9d=C3=A9ric wrote: > > On 12/03/2013 17:31, Cornelia Huck wrote: > >> On Tue, 12 Mar 2013 16:22:22 +0100 > >> KONRAD Fr=C3=A9d=C3=A9ric wrote: > >> > >>> On 12/03/2013 16:12, Peter Maydell wrote: > >>>> On 12 March 2013 15:08, KONRAD Fr=C3=A9d=C3=A9ric =20 > >>>> 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=20 > >>>>>> 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=20 > > 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). =46rom 763c1a8ff61faaef5b488072cc9965bd29f8a1fd Mon Sep 17 00:00:00 2001 From: Cornelia Huck 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 --- 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 =3D { static Property virtio_ccw_blk_properties[] =3D { 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), --=20 1.7.9.5 =46rom 9e60f80b0ca9943ce3e6d11630c3b5a8bf0c3cbd Mon Sep 17 00:00:00 2001 From: Cornelia Huck 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 --- 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[] =3D { #ifdef __linux__ DEFINE_PROP_BIT("scsi", VirtIOS390Device, blk.scsi, 0, true), #endif + DEFINE_PROP_BIT("config-wce", VirtIOS390Device, blk.config_wce, 0, tru= e), DEFINE_PROP_END_OF_LIST(), }; =20 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[] =3D { 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(), }; =20 --=20 1.7.9.5