[{"id":1768426,"web_url":"http://patchwork.ozlabs.org/comment/1768426/","msgid":"<20170914104739.74ca3c70.cohuck@redhat.com>","list_archive_url":null,"date":"2017-09-14T08:47:39","subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","submitter":{"id":71914,"url":"http://patchwork.ozlabs.org/api/people/71914/","name":"Cornelia Huck","email":"cohuck@redhat.com"},"content":"On Wed, 13 Sep 2017 13:50:28 +0200\nHalil Pasic wrote:\n\n> Replace direct access which implicitly assumes no IDA\n> or MIDA with the new ccw data stream interface which should\n> cope with these transparently in the future.\n> \n> Signed-off-by: Halil Pasic \n\n\n> @@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> } else {\n> virtio_bus_get_vdev_config(&dev->bus, vdev->config);\n> /* XXX config space endianness */\n> - cpu_physical_memory_write(ccw.cda, vdev->config, len);\n> + ccw_dstream_write_buf(&sch->cds, vdev->config, len);\n> sch->curr_status.scsw.count = ccw.count - len;\n> ret = 0;\n> }\n> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> }\n> }\n> len = MIN(ccw.count, vdev->config_len);\n> - hw_len = len;\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else {\n> - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);\n> - if (!config) {\n> - ret = -EFAULT;\n> - } else {\n> - len = hw_len;\n> - /* XXX config space endianness */\n> - memcpy(vdev->config, config, len);\n> - cpu_physical_memory_unmap(config, hw_len, 0, hw_len);\n> + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);\n> + if (!ret) {\n> virtio_bus_set_vdev_config(&dev->bus, vdev->config);\n> sch->curr_status.scsw.count = ccw.count - len;\n> - ret = 0;\n> }\n> }\n> break;\n\nIt feels a bit odd that you remove the stale XXX comment only for one\ndirection. Care to post a patch that removes them separately? Else I\ncan do that as well.\n\n[I'd queue that patch in front of this patchset, but no need to resend\nfor that.]","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=cohuck@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xtByD3lQ6z9sPs\n\tfor ;\n\tThu, 14 Sep 2017 18:48:20 +1000 (AEST)","from localhost ([::1]:46405 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dsPoo-0003VK-Lz\n\tfor incoming@patchwork.ozlabs.org; Thu, 14 Sep 2017 04:48:18 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:52530)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dsPoK-0003Th-L7\n\tfor qemu-devel@nongnu.org; Thu, 14 Sep 2017 04:47:49 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dsPoH-0004m4-Gz\n\tfor qemu-devel@nongnu.org; Thu, 14 Sep 2017 04:47:48 -0400","from mx1.redhat.com ([209.132.183.28]:44344)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dsPoH-0004lK-AT\n\tfor qemu-devel@nongnu.org; Thu, 14 Sep 2017 04:47:45 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 3C2187EA8A;\n\tThu, 14 Sep 2017 08:47:44 +0000 (UTC)","from gondolin (ovpn-117-98.ams2.redhat.com [10.36.117.98])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 08F856C53B;\n\tThu, 14 Sep 2017 08:47:42 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 3C2187EA8A","Date":"Thu, 14 Sep 2017 10:47:39 +0200","From":"Cornelia Huck ","To":"Halil Pasic ","Message-ID":"<20170914104739.74ca3c70.cohuck@redhat.com>","In-Reply-To":"<20170913115029.47626-4-pasic@linux.vnet.ibm.com>","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-4-pasic@linux.vnet.ibm.com>","Organization":"Red Hat GmbH","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.28]);\n\tThu, 14 Sep 2017 08:47:44 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dong Jia Shi ,\n\tPierre Morel , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770633,"web_url":"http://patchwork.ozlabs.org/comment/1770633/","msgid":"<20170919033730.GD5274@bjsdjshi@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-19T03:37:30","subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","submitter":{"id":68897,"url":"http://patchwork.ozlabs.org/api/people/68897/","name":"Dong Jia Shi","email":"bjsdjshi@linux.vnet.ibm.com"},"content":"* Halil Pasic [2017-09-13 13:50:28 +0200]:\n\n> Replace direct access which implicitly assumes no IDA\n> or MIDA with the new ccw data stream interface which should\n> cope with these transparently in the future.\n> \n> Signed-off-by: Halil Pasic \n> \n> ---\n> \n> v1 --> v2:\n> Removed todo comments on possible errno change as with\n> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html\n> applied it does not matter any more.\n> \n> Error handling: At the moment we ignore errors reported\n> by stream ops to keep the change minimal. An add-on\n> patch improving on this is to be expected later.\nAdd a TODO somewhere around the code as a reminder?\n\n> ---\n> hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------\n> 1 file changed, 46 insertions(+), 110 deletions(-)\n> \n> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c\n> index b1976fdd19..a9baf6f7ab 100644\n> --- a/hw/s390x/virtio-ccw.c\n> +++ b/hw/s390x/virtio-ccw.c\n[...]\n\n> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> } else {\n> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);\n> \n> - features.index = address_space_ldub(&address_space_memory,\n> - ccw.cda\n> - + sizeof(features.features),\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> + ccw_dstream_advance(&sch->cds, sizeof(features.features));\nHow about:\nccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));\n\n> + ccw_dstream_read(&sch->cds, features.index);\n> if (features.index == 0) {\n> if (dev->revision >= 1) {\n> /* Don't offer legacy features for modern devices. */\n> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> /* Return zeroes if the guest supports more feature bits. */\n> features.features = 0;\n> }\n> - address_space_stl_le(&address_space_memory, ccw.cda,\n> - features.features, MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> + ccw_dstream_rewind(&sch->cds);\n> + cpu_to_le32s(&features.features);\n> + ccw_dstream_write(&sch->cds, features.features);\n> sch->curr_status.scsw.count = ccw.count - sizeof(features);\nHow about:\nsch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);\n\nThis also applies to other places.\n\n> ret = 0;\n> }\n[...]\n\n> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> }\n> }\n> len = MIN(ccw.count, vdev->config_len);\n> - hw_len = len;\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else {\n> - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);\n> - if (!config) {\n> - ret = -EFAULT;\n> - } else {\n> - len = hw_len;\n> - /* XXX config space endianness */\n> - memcpy(vdev->config, config, len);\n> - cpu_physical_memory_unmap(config, hw_len, 0, hw_len);\n> + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);\n> + if (!ret) {\nAdd a TODO here, and:\n\nif (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {\n ret = -EFAULT;\n} else {\n ....\n}\n\n> virtio_bus_set_vdev_config(&dev->bus, vdev->config);\n> sch->curr_status.scsw.count = ccw.count - len;\n> - ret = 0;\n> }\n> }\n> break;\n[...]\n\n> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> ret = -EFAULT;\n> break;\n> }\n> - revinfo.revision =\n> - address_space_lduw_be(&address_space_memory, ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> - revinfo.length =\n> - address_space_lduw_be(&address_space_memory,\n> - ccw.cda + sizeof(revinfo.revision),\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> + ccw_dstream_read_buf(&sch->cds, &revinfo, 4);\n ^\nA magic number? O:)\n\n> + be16_to_cpus(&revinfo.revision);\n> + be16_to_cpus(&revinfo.length);\n> if (ccw.count < len + revinfo.length ||\n> (check_len && ccw.count > len + revinfo.length)) {\n> ret = -EINVAL;\n> -- \n> 2.13.5\n> \n\nOtherwise, looks good.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xx7r15tffz9s7G\n\tfor ;\n\tTue, 19 Sep 2017 13:38:06 +1000 (AEST)","from localhost ([::1]:39892 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1du9MK-00038N-Aj\n\tfor incoming@patchwork.ozlabs.org; Mon, 18 Sep 2017 23:38:04 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47158)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1du9Lz-00037t-Au\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 23:37:44 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1du9Lu-0000mW-E4\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 23:37:43 -0400","from mx0a-001b2d01.pphosted.com ([148.163.156.1]:42925)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1du9Lu-0000lr-48\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 23:37:38 -0400","from pps.filterd (m0098393.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8J3Xb77047789\n\tfor ; Mon, 18 Sep 2017 23:37:35 -0400","from e38.co.us.ibm.com (e38.co.us.ibm.com [32.97.110.159])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d2u401d3t-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Mon, 18 Sep 2017 23:37:35 -0400","from localhost\n\tby e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tMon, 18 Sep 2017 21:37:34 -0600","from b03cxnp08025.gho.boulder.ibm.com (9.17.130.17)\n\tby e38.co.us.ibm.com (192.168.1.138) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tMon, 18 Sep 2017 21:37:33 -0600","from b03ledav001.gho.boulder.ibm.com\n\t(b03ledav001.gho.boulder.ibm.com [9.17.130.232])\n\tby b03cxnp08025.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8J3bW0Q262552; Mon, 18 Sep 2017 20:37:32 -0700","from b03ledav001.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id BE1F26E03F;\n\tMon, 18 Sep 2017 21:37:32 -0600 (MDT)","from localhost (unknown [9.115.112.23])\n\tby b03ledav001.gho.boulder.ibm.com (Postfix) with ESMTP id 166C26E03A;\n\tMon, 18 Sep 2017 21:37:31 -0600 (MDT)"],"Date":"Tue, 19 Sep 2017 11:37:30 +0800","From":"Dong Jia Shi ","To":"Halil Pasic ","Mail-Followup-To":"Halil Pasic ,\n\tCornelia Huck ,\n\tDong Jia Shi ,\n\tPierre Morel , qemu-devel@nongnu.org","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-4-pasic@linux.vnet.ibm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170913115029.47626-4-pasic@linux.vnet.ibm.com>","Organization":"(IBM CSL)","X-URL":"http://ibm.com/","User-Agent":"Mutt/1.5.21 (2010-09-15)","X-TM-AS-GCONF":"00","x-cbid":"17091903-0028-0000-0000-00000860B412","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007759; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000230; SDB=6.00919087; UDB=6.00461743;\n\tIPR=6.00699314; \n\tBA=6.00005595; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017203;\n\tXFM=3.00000015; UTC=2017-09-19 03:37:34","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091903-0029-0000-0000-0000379B59CB","Message-Id":"<20170919033730.GD5274@bjsdjshi@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_02:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709190052","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy]","X-Received-From":"148.163.156.1","Subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dong Jia Shi ,\n\tCornelia Huck ,\n\tPierre Morel , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770767,"web_url":"http://patchwork.ozlabs.org/comment/1770767/","msgid":"<20170919110631.59039743.cohuck@redhat.com>","list_archive_url":null,"date":"2017-09-19T09:06:31","subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","submitter":{"id":71914,"url":"http://patchwork.ozlabs.org/api/people/71914/","name":"Cornelia Huck","email":"cohuck@redhat.com"},"content":"On Tue, 19 Sep 2017 11:37:30 +0800\nDong Jia Shi wrote:\n\n> * Halil Pasic [2017-09-13 13:50:28 +0200]:\n> \n> > Replace direct access which implicitly assumes no IDA\n> > or MIDA with the new ccw data stream interface which should\n> > cope with these transparently in the future.\n> > \n> > Signed-off-by: Halil Pasic \n> > \n> > ---\n> > \n> > v1 --> v2:\n> > Removed todo comments on possible errno change as with\n> > https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html\n> > applied it does not matter any more.\n> > \n> > Error handling: At the moment we ignore errors reported\n> > by stream ops to keep the change minimal. An add-on\n> > patch improving on this is to be expected later. \n> Add a TODO somewhere around the code as a reminder?\n\nI expect Halil to have it on his TODO list and send a patch later ;)\n\n> \n> > ---\n> > hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------\n> > 1 file changed, 46 insertions(+), 110 deletions(-)\n> > \n> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c\n> > index b1976fdd19..a9baf6f7ab 100644\n> > --- a/hw/s390x/virtio-ccw.c\n> > +++ b/hw/s390x/virtio-ccw.c \n> [...]\n> \n> > @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> > } else {\n> > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);\n> > \n> > - features.index = address_space_ldub(&address_space_memory,\n> > - ccw.cda\n> > - + sizeof(features.features),\n> > - MEMTXATTRS_UNSPECIFIED,\n> > - NULL);\n> > + ccw_dstream_advance(&sch->cds, sizeof(features.features)); \n> How about:\n> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));\n\nI think keeping sizeof(features.features) is better: It matches the old\ncode, and we really do jump over the features flag and revisit it\nlater...\n\n> \n> > + ccw_dstream_read(&sch->cds, features.index);\n> > if (features.index == 0) {\n> > if (dev->revision >= 1) {\n> > /* Don't offer legacy features for modern devices. */\n> > @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> > /* Return zeroes if the guest supports more feature bits. */\n> > features.features = 0;\n> > }\n> > - address_space_stl_le(&address_space_memory, ccw.cda,\n> > - features.features, MEMTXATTRS_UNSPECIFIED,\n> > - NULL);\n> > + ccw_dstream_rewind(&sch->cds);\n> > + cpu_to_le32s(&features.features);\n> > + ccw_dstream_write(&sch->cds, features.features);\n> > sch->curr_status.scsw.count = ccw.count - sizeof(features); \n> How about:\n> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);\n\nHm, I thought I had commented on this, but I seem to have missed\nthese...\n\nI'd prefer to do it as a follow-up patch.\n\n> \n> This also applies to other places.\n> \n> > ret = 0;\n> > } \n> [...]\n> \n> > @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> > }\n> > }\n> > len = MIN(ccw.count, vdev->config_len);\n> > - hw_len = len;\n> > if (!ccw.cda) {\n> > ret = -EFAULT;\n> > } else {\n> > - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);\n> > - if (!config) {\n> > - ret = -EFAULT;\n> > - } else {\n> > - len = hw_len;\n> > - /* XXX config space endianness */\n> > - memcpy(vdev->config, config, len);\n> > - cpu_physical_memory_unmap(config, hw_len, 0, hw_len);\n> > + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);\n> > + if (!ret) { \n> Add a TODO here, and:\n> \n> if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {\n> ret = -EFAULT;\n> } else {\n> ....\n> }\n\nI don't think that would be correct: The function will (at least\ncurrently) return 0 or -EINVAL, and you are now mapping any error to\n-EFAULT? (Not that it has an effect in the end: We map both to a\nchannel program check as of \"s390x/css: drop data-check in\ninterpretation\".)\n\n> \n> > virtio_bus_set_vdev_config(&dev->bus, vdev->config);\n> > sch->curr_status.scsw.count = ccw.count - len;\n> > - ret = 0;\n> > }\n> > }\n> > break; \n> [...]\n> \n> > @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> > ret = -EFAULT;\n> > break;\n> > }\n> > - revinfo.revision =\n> > - address_space_lduw_be(&address_space_memory, ccw.cda,\n> > - MEMTXATTRS_UNSPECIFIED, NULL);\n> > - revinfo.length =\n> > - address_space_lduw_be(&address_space_memory,\n> > - ccw.cda + sizeof(revinfo.revision),\n> > - MEMTXATTRS_UNSPECIFIED, NULL);\n> > + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); \n> ^\n> A magic number? O:)\n\nHah :)\n\nWe can't use sizeof(revinfo), and sizeof(revinfo.revision) +\nsizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our\ncode :)\n\n> \n> > + be16_to_cpus(&revinfo.revision);\n> > + be16_to_cpus(&revinfo.length);\n> > if (ccw.count < len + revinfo.length ||\n> > (check_len && ccw.count > len + revinfo.length)) {\n> > ret = -EINVAL;\n> > -- \n> > 2.13.5\n> > \n> \n> Otherwise, looks good.\n> \n\nCan I get an ack?","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=cohuck@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxH8307jYz9sMN\n\tfor ;\n\tTue, 19 Sep 2017 19:07:31 +1000 (AEST)","from localhost ([::1]:40906 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duEV6-0007XO-V8\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 05:07:28 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:45494)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duEUK-0007VT-FY\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:06:42 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duEUH-0003x0-Aw\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:06:40 -0400","from mx1.redhat.com ([209.132.183.28]:44984)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1duEUH-0003wf-2S\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:06:37 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 2624B2C9743;\n\tTue, 19 Sep 2017 09:06:36 +0000 (UTC)","from gondolin (unknown [10.36.117.0])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id DFF7960E3B;\n\tTue, 19 Sep 2017 09:06:34 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 2624B2C9743","Date":"Tue, 19 Sep 2017 11:06:31 +0200","From":"Cornelia Huck ","To":"Dong Jia Shi ","Message-ID":"<20170919110631.59039743.cohuck@redhat.com>","In-Reply-To":"<20170919033730.GD5274@bjsdjshi@linux.vnet.ibm.com>","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-4-pasic@linux.vnet.ibm.com>\n\t<20170919033730.GD5274@bjsdjshi@linux.vnet.ibm.com>","Organization":"Red Hat GmbH","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.29]);\n\tTue, 19 Sep 2017 09:06:36 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Halil Pasic ,\n\tPierre Morel , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771009,"web_url":"http://patchwork.ozlabs.org/comment/1771009/","msgid":"<6a51f84b-9185-ec5c-b0ec-1fc21fbfb201@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-19T13:30:29","subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","submitter":{"id":68297,"url":"http://patchwork.ozlabs.org/api/people/68297/","name":"Halil Pasic","email":"pasic@linux.vnet.ibm.com"},"content":"On 09/19/2017 11:06 AM, Cornelia Huck wrote:\n> On Tue, 19 Sep 2017 11:37:30 +0800\n> Dong Jia Shi wrote:\n> \n>> * Halil Pasic [2017-09-13 13:50:28 +0200]:\n>>\n>>> Replace direct access which implicitly assumes no IDA\n>>> or MIDA with the new ccw data stream interface which should\n>>> cope with these transparently in the future.\n>>>\n>>> Signed-off-by: Halil Pasic \n>>>\n>>> ---\n>>>\n>>> v1 --> v2:\n>>> Removed todo comments on possible errno change as with\n>>> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html\n>>> applied it does not matter any more.\n>>>\n>>> Error handling: At the moment we ignore errors reported\n>>> by stream ops to keep the change minimal. An add-on\n>>> patch improving on this is to be expected later. \n>> Add a TODO somewhere around the code as a reminder?\n> \n> I expect Halil to have it on his TODO list and send a patch later ;)\n> \n>>\n>>> ---\n>>> hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------\n>>> 1 file changed, 46 insertions(+), 110 deletions(-)\n>>>\n>>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c\n>>> index b1976fdd19..a9baf6f7ab 100644\n>>> --- a/hw/s390x/virtio-ccw.c\n>>> +++ b/hw/s390x/virtio-ccw.c \n>> [...]\n>>\n>>> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n>>> } else {\n>>> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);\n>>>\n>>> - features.index = address_space_ldub(&address_space_memory,\n>>> - ccw.cda\n>>> - + sizeof(features.features),\n>>> - MEMTXATTRS_UNSPECIFIED,\n>>> - NULL);\n>>> + ccw_dstream_advance(&sch->cds, sizeof(features.features)); \n>> How about:\n>> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));\n> \n> I think keeping sizeof(features.features) is better: It matches the old\n> code, and we really do jump over the features flag and revisit it\n> later...\n> \n>>\n>>> + ccw_dstream_read(&sch->cds, features.index);\n>>> if (features.index == 0) {\n>>> if (dev->revision >= 1) {\n>>> /* Don't offer legacy features for modern devices. */\n>>> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n>>> /* Return zeroes if the guest supports more feature bits. */\n>>> features.features = 0;\n>>> }\n>>> - address_space_stl_le(&address_space_memory, ccw.cda,\n>>> - features.features, MEMTXATTRS_UNSPECIFIED,\n>>> - NULL);\n>>> + ccw_dstream_rewind(&sch->cds);\n>>> + cpu_to_le32s(&features.features);\n>>> + ccw_dstream_write(&sch->cds, features.features);\n>>> sch->curr_status.scsw.count = ccw.count - sizeof(features); \n>> How about:\n>> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);\n> \n> Hm, I thought I had commented on this, but I seem to have missed\n> these...\n> \n> I'd prefer to do it as a follow-up patch.\n> \n>>\n>> This also applies to other places.\n>>\n>>> ret = 0;\n>>> } \n>> [...]\n>>\n>>> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n>>> }\n>>> }\n>>> len = MIN(ccw.count, vdev->config_len);\n>>> - hw_len = len;\n>>> if (!ccw.cda) {\n>>> ret = -EFAULT;\n>>> } else {\n>>> - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);\n>>> - if (!config) {\n>>> - ret = -EFAULT;\n>>> - } else {\n>>> - len = hw_len;\n>>> - /* XXX config space endianness */\n>>> - memcpy(vdev->config, config, len);\n>>> - cpu_physical_memory_unmap(config, hw_len, 0, hw_len);\n>>> + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);\n>>> + if (!ret) { \n>> Add a TODO here, and:\n>>\n>> if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {\n>> ret = -EFAULT;\n>> } else {\n>> ....\n>> }\n> \n> I don't think that would be correct: The function will (at least\n> currently) return 0 or -EINVAL, and you are now mapping any error to\n> -EFAULT? (Not that it has an effect in the end: We map both to a\n> channel program check as of \"s390x/css: drop data-check in\n> interpretation\".)\n> \n>>\n>>> virtio_bus_set_vdev_config(&dev->bus, vdev->config);\n>>> sch->curr_status.scsw.count = ccw.count - len;\n>>> - ret = 0;\n>>> }\n>>> }\n>>> break; \n>> [...]\n>>\n>>> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n>>> ret = -EFAULT;\n>>> break;\n>>> }\n>>> - revinfo.revision =\n>>> - address_space_lduw_be(&address_space_memory, ccw.cda,\n>>> - MEMTXATTRS_UNSPECIFIED, NULL);\n>>> - revinfo.length =\n>>> - address_space_lduw_be(&address_space_memory,\n>>> - ccw.cda + sizeof(revinfo.revision),\n>>> - MEMTXATTRS_UNSPECIFIED, NULL);\n>>> + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); \n>> ^\n>> A magic number? O:)\n> \n> Hah :)\n> \n> We can't use sizeof(revinfo), and sizeof(revinfo.revision) +\n> sizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our\n> code :)\n> \n>>\n>>> + be16_to_cpus(&revinfo.revision);\n>>> + be16_to_cpus(&revinfo.length);\n>>> if (ccw.count < len + revinfo.length ||\n>>> (check_len && ccw.count > len + revinfo.length)) {\n>>> ret = -EINVAL;\n>>> -- \n>>> 2.13.5\n>>> \n>>\n>> Otherwise, looks good.\n>>\n> \n> Can I get an ack?\n> \n\nI agree that further cleanups are possible (e.g. using\nccw_dstream_residual_count() and tightening error handling) but\nI also prefer to see these as on-top, and prefer sticking to\nchange as little as possible in the transformation patch and stay\nas close as possible to what we had before in terms of behavior).\n\nRegards,\nHalil","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxP0d4C5lz9sBZ\n\tfor ;\n\tTue, 19 Sep 2017 23:31:29 +1000 (AEST)","from localhost ([::1]:42892 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duIcZ-0004Wu-Kp\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 09:31:27 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:45998)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duIbx-0004Up-GB\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:30:50 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duIbr-00013c-Hs\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:30:49 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:44656\n\thelo=mx0a-001b2d01.pphosted.com)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duIbr-00012w-Cz\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:30:43 -0400","from pps.filterd (m0098413.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JDUEqd047965\n\tfor ; Tue, 19 Sep 2017 09:30:41 -0400","from e06smtp11.uk.ibm.com (e06smtp11.uk.ibm.com [195.75.94.107])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2d3130kntt-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 09:30:39 -0400","from localhost\n\tby e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 19 Sep 2017 14:30:31 +0100","from b06cxnps4076.portsmouth.uk.ibm.com (9.149.109.198)\n\tby e06smtp11.uk.ibm.com (192.168.101.141) with IBM ESMTP SMTP\n\tGateway: Authorized Use Only! Violators will be prosecuted; \n\tTue, 19 Sep 2017 14:30:30 +0100","from d06av21.portsmouth.uk.ibm.com (d06av21.portsmouth.uk.ibm.com\n\t[9.149.105.232])\n\tby b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8JDUTFI12058742; Tue, 19 Sep 2017 13:30:29 GMT","from d06av21.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 1B1155203F;\n\tTue, 19 Sep 2017 13:25:29 +0100 (BST)","from oc3836556865.ibm.com (unknown [9.152.224.207])\n\tby d06av21.portsmouth.uk.ibm.com (Postfix) with ESMTP id EA7DD52041; \n\tTue, 19 Sep 2017 13:25:28 +0100 (BST)"],"To":"Cornelia Huck ,\n\tDong Jia Shi ","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-4-pasic@linux.vnet.ibm.com>\n\t<20170919033730.GD5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919110631.59039743.cohuck@redhat.com>","From":"Halil Pasic ","Date":"Tue, 19 Sep 2017 15:30:29 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170919110631.59039743.cohuck@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","X-TM-AS-GCONF":"00","x-cbid":"17091913-0040-0000-0000-000003FB5D9C","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091913-0041-0000-0000-0000209C8F33","Message-Id":"<6a51f84b-9185-ec5c-b0ec-1fc21fbfb201@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_05:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709190191","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy]","X-Received-From":"148.163.158.5","Subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Pierre Morel , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771056,"web_url":"http://patchwork.ozlabs.org/comment/1771056/","msgid":"<883c16a8-6289-18e7-bbdb-cf684b20a9eb@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-19T14:07:25","subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","submitter":{"id":66825,"url":"http://patchwork.ozlabs.org/api/people/66825/","name":"Pierre Morel","email":"pmorel@linux.vnet.ibm.com"},"content":"On 13/09/2017 13:50, Halil Pasic wrote:\n> Replace direct access which implicitly assumes no IDA\n> or MIDA with the new ccw data stream interface which should\n> cope with these transparently in the future.\n> \n> Signed-off-by: Halil Pasic \n> \n> ---\n> \n> v1 --> v2:\n> Removed todo comments on possible errno change as with\n> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html\n> applied it does not matter any more.\n> \n> Error handling: At the moment we ignore errors reported\n> by stream ops to keep the change minimal. An add-on\n> patch improving on this is to be expected later.\n> ---\n> hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------\n> 1 file changed, 46 insertions(+), 110 deletions(-)\n\n:)\n\n> \n> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c\n> index b1976fdd19..a9baf6f7ab 100644\n> --- a/hw/s390x/virtio-ccw.c\n> +++ b/hw/s390x/virtio-ccw.c\n> @@ -287,49 +287,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,\n> return -EFAULT;\n> }\n> if (is_legacy) {\n> - linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> - linfo.align = address_space_ldl_be(&address_space_memory,\n> - ccw.cda + sizeof(linfo.queue),\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> - linfo.index = address_space_lduw_be(&address_space_memory,\n> - ccw.cda + sizeof(linfo.queue)\n> - + sizeof(linfo.align),\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> - linfo.num = address_space_lduw_be(&address_space_memory,\n> - ccw.cda + sizeof(linfo.queue)\n> - + sizeof(linfo.align)\n> - + sizeof(linfo.index),\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> + ccw_dstream_read(&sch->cds, linfo);\n> + be64_to_cpus(&linfo.queue);\n> + be32_to_cpus(&linfo.align);\n> + be16_to_cpus(&linfo.index);\n> + be16_to_cpus(&linfo.num);\n> ret = virtio_ccw_set_vqs(sch, NULL, &linfo);\n> } else {\n> - info.desc = address_space_ldq_be(&address_space_memory, ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> - info.index = address_space_lduw_be(&address_space_memory,\n> - ccw.cda + sizeof(info.desc)\n> - + sizeof(info.res0),\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> - info.num = address_space_lduw_be(&address_space_memory,\n> - ccw.cda + sizeof(info.desc)\n> - + sizeof(info.res0)\n> - + sizeof(info.index),\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> - info.avail = address_space_ldq_be(&address_space_memory,\n> - ccw.cda + sizeof(info.desc)\n> - + sizeof(info.res0)\n> - + sizeof(info.index)\n> - + sizeof(info.num),\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> - info.used = address_space_ldq_be(&address_space_memory,\n> - ccw.cda + sizeof(info.desc)\n> - + sizeof(info.res0)\n> - + sizeof(info.index)\n> - + sizeof(info.num)\n> - + sizeof(info.avail),\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> + ccw_dstream_read(&sch->cds, info);\n> + be64_to_cpus(&info.desc);\n> + be16_to_cpus(&info.index);\n> + be16_to_cpus(&info.num);\n> + be64_to_cpus(&info.avail);\n> + be64_to_cpus(&info.used);\n> ret = virtio_ccw_set_vqs(sch, &info, NULL);\n> }\n> sch->curr_status.scsw.count = 0;\n> @@ -342,15 +312,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> VirtioRevInfo revinfo;\n> uint8_t status;\n> VirtioFeatDesc features;\n> - void *config;\n> hwaddr indicators;\n> VqConfigBlock vq_config;\n> VirtioCcwDevice *dev = sch->driver_data;\n> VirtIODevice *vdev = virtio_ccw_get_vdev(sch);\n> bool check_len;\n> int len;\n> - hwaddr hw_len;\n> - VirtioThinintInfo *thinint;\n> + VirtioThinintInfo thinint;\n> \n> if (!dev) {\n> return -EINVAL;\n> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> } else {\n> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);\n> \n> - features.index = address_space_ldub(&address_space_memory,\n> - ccw.cda\n> - + sizeof(features.features),\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> + ccw_dstream_advance(&sch->cds, sizeof(features.features));\n> + ccw_dstream_read(&sch->cds, features.index);\n> if (features.index == 0) {\n> if (dev->revision >= 1) {\n> /* Don't offer legacy features for modern devices. */\n> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> /* Return zeroes if the guest supports more feature bits. */\n> features.features = 0;\n> }\n> - address_space_stl_le(&address_space_memory, ccw.cda,\n> - features.features, MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> + ccw_dstream_rewind(&sch->cds);\n> + cpu_to_le32s(&features.features);\n> + ccw_dstream_write(&sch->cds, features.features);\n> sch->curr_status.scsw.count = ccw.count - sizeof(features);\n> ret = 0;\n> }\n> @@ -438,15 +403,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else {\n> - features.index = address_space_ldub(&address_space_memory,\n> - ccw.cda\n> - + sizeof(features.features),\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> - features.features = address_space_ldl_le(&address_space_memory,\n> - ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> + ccw_dstream_read(&sch->cds, features);\n> + le32_to_cpus(&features.features);\n> if (features.index == 0) {\n> virtio_set_features(vdev,\n> (vdev->guest_features & 0xffffffff00000000ULL) |\n> @@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> } else {\n> virtio_bus_get_vdev_config(&dev->bus, vdev->config);\n> /* XXX config space endianness */\n> - cpu_physical_memory_write(ccw.cda, vdev->config, len);\n> + ccw_dstream_write_buf(&sch->cds, vdev->config, len);\n> sch->curr_status.scsw.count = ccw.count - len;\n> ret = 0;\n> }\n> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> }\n> }\n> len = MIN(ccw.count, vdev->config_len);\n> - hw_len = len;\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else {\n> - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);\n> - if (!config) {\n> - ret = -EFAULT;\n> - } else {\n> - len = hw_len;\n> - /* XXX config space endianness */\n> - memcpy(vdev->config, config, len);\n> - cpu_physical_memory_unmap(config, hw_len, 0, hw_len);\n> + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);\n> + if (!ret) {\n> virtio_bus_set_vdev_config(&dev->bus, vdev->config);\n> sch->curr_status.scsw.count = ccw.count - len;\n> - ret = 0;\n> }\n> }\n> break;\n> @@ -553,8 +503,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else {\n> - status = address_space_ldub(&address_space_memory, ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> + ccw_dstream_read(&sch->cds, status);\n> if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {\n> virtio_ccw_stop_ioeventfd(dev);\n> }\n> @@ -597,8 +546,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else {\n> - indicators = address_space_ldq_be(&address_space_memory, ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> + ccw_dstream_read(&sch->cds, indicators);\n> + be64_to_cpus(&indicators);\n> dev->indicators = get_indicator(indicators, sizeof(uint64_t));\n> sch->curr_status.scsw.count = ccw.count - sizeof(indicators);\n> ret = 0;\n> @@ -618,8 +567,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else {\n> - indicators = address_space_ldq_be(&address_space_memory, ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> + ccw_dstream_read(&sch->cds, indicators);\n> + be64_to_cpus(&indicators);\n> dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));\n> sch->curr_status.scsw.count = ccw.count - sizeof(indicators);\n> ret = 0;\n> @@ -639,67 +588,58 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else {\n> - vq_config.index = address_space_lduw_be(&address_space_memory,\n> - ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> + ccw_dstream_read(&sch->cds, vq_config.index);\n> + be16_to_cpus(&vq_config.index);\n> if (vq_config.index >= VIRTIO_QUEUE_MAX) {\n> ret = -EINVAL;\n> break;\n> }\n> vq_config.num_max = virtio_queue_get_num(vdev,\n> vq_config.index);\n> - address_space_stw_be(&address_space_memory,\n> - ccw.cda + sizeof(vq_config.index),\n> - vq_config.num_max,\n> - MEMTXATTRS_UNSPECIFIED,\n> - NULL);\n> + cpu_to_be16s(&vq_config.num_max);\n> + ccw_dstream_write(&sch->cds, vq_config.num_max);\n> sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);\n> ret = 0;\n> }\n> break;\n> case CCW_CMD_SET_IND_ADAPTER:\n> if (check_len) {\n> - if (ccw.count != sizeof(*thinint)) {\n> + if (ccw.count != sizeof(thinint)) {\n> ret = -EINVAL;\n> break;\n> }\n> - } else if (ccw.count < sizeof(*thinint)) {\n> + } else if (ccw.count < sizeof(thinint)) {\n> /* Can't execute command. */\n> ret = -EINVAL;\n> break;\n> }\n> - len = sizeof(*thinint);\n> - hw_len = len;\n> if (!ccw.cda) {\n> ret = -EFAULT;\n> } else if (dev->indicators && !sch->thinint_active) {\n> /* Trigger a command reject. */\n> ret = -ENOSYS;\n> } else {\n> - thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0);\n> - if (!thinint) {\n> + if (ccw_dstream_read(&sch->cds, thinint)) {\n> ret = -EFAULT;\n> } else {\n> - uint64_t ind_bit = ldq_be_p(&thinint->ind_bit);\n> + be64_to_cpus(&thinint.ind_bit);\n> + be64_to_cpus(&thinint.summary_indicator);\n> + be64_to_cpus(&thinint.device_indicator);\n> \n> - len = hw_len;\n> dev->summary_indicator =\n> - get_indicator(ldq_be_p(&thinint->summary_indicator),\n> - sizeof(uint8_t));\n> + get_indicator(thinint.summary_indicator, sizeof(uint8_t));\n> dev->indicators =\n> - get_indicator(ldq_be_p(&thinint->device_indicator),\n> - ind_bit / 8 + 1);\n> - dev->thinint_isc = thinint->isc;\n> - dev->routes.adapter.ind_offset = ind_bit;\n> + get_indicator(thinint.device_indicator,\n> + thinint.ind_bit / 8 + 1);\n> + dev->thinint_isc = thinint.isc;\n> + dev->routes.adapter.ind_offset = thinint.ind_bit;\n> dev->routes.adapter.summary_offset = 7;\n> - cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len);\n> dev->routes.adapter.adapter_id = css_get_adapter_id(\n> CSS_IO_ADAPTER_VIRTIO,\n> dev->thinint_isc);\n> sch->thinint_active = ((dev->indicators != NULL) &&\n> (dev->summary_indicator != NULL));\n> - sch->curr_status.scsw.count = ccw.count - len;\n> + sch->curr_status.scsw.count = ccw.count - sizeof(thinint);\n> ret = 0;\n> }\n> }\n> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> ret = -EFAULT;\n> break;\n> }\n> - revinfo.revision =\n> - address_space_lduw_be(&address_space_memory, ccw.cda,\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> - revinfo.length =\n> - address_space_lduw_be(&address_space_memory,\n> - ccw.cda + sizeof(revinfo.revision),\n> - MEMTXATTRS_UNSPECIFIED, NULL);\n> + ccw_dstream_read_buf(&sch->cds, &revinfo, 4);\n> + be16_to_cpus(&revinfo.revision);\n> + be16_to_cpus(&revinfo.length);\n> if (ccw.count < len + revinfo.length ||\n> (check_len && ccw.count > len + revinfo.length)) {\n> ret = -EINVAL;\n> \n\nReviewed-by: Pierre Morel","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxPpx63VLz9s7c\n\tfor ;\n\tWed, 20 Sep 2017 00:08:09 +1000 (AEST)","from localhost ([::1]:43140 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duJC3-0003Q5-W0\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 10:08:08 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:41394)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duJBc-0003Pf-Qm\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:07:47 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duJBV-00011i-Mt\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:07:40 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:35862\n\thelo=mx0a-001b2d01.pphosted.com)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duJBV-00011c-Gd\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:07:33 -0400","from pps.filterd (m0098421.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JE6oYq057400\n\tfor ; Tue, 19 Sep 2017 10:07:32 -0400","from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d33uhkx4f-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 10:07:32 -0400","from localhost\n\tby e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 19 Sep 2017 15:07:30 +0100","from b06cxnps3075.portsmouth.uk.ibm.com (9.149.109.195)\n\tby e06smtp15.uk.ibm.com (192.168.101.145) with IBM ESMTP SMTP\n\tGateway: Authorized Use Only! Violators will be prosecuted; \n\tTue, 19 Sep 2017 15:07:27 +0100","from d06av23.portsmouth.uk.ibm.com (d06av23.portsmouth.uk.ibm.com\n\t[9.149.105.59])\n\tby b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8JE7Q0E28573860; Tue, 19 Sep 2017 14:07:26 GMT","from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id D8DC5A404D;\n\tTue, 19 Sep 2017 15:03:28 +0100 (BST)","from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 9BE82A4040;\n\tTue, 19 Sep 2017 15:03:28 +0100 (BST)","from [9.145.21.76] (unknown [9.145.21.76])\n\tby d06av23.portsmouth.uk.ibm.com (Postfix) with ESMTP;\n\tTue, 19 Sep 2017 15:03:28 +0100 (BST)"],"To":"Halil Pasic , Cornelia Huck ","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-4-pasic@linux.vnet.ibm.com>","From":"Pierre Morel ","Date":"Tue, 19 Sep 2017 16:07:25 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170913115029.47626-4-pasic@linux.vnet.ibm.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","X-TM-AS-GCONF":"00","x-cbid":"17091914-0020-0000-0000-000003B96225","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091914-0021-0000-0000-0000424B0ECD","Message-Id":"<883c16a8-6289-18e7-bbdb-cf684b20a9eb@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_06:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709190200","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by\n\tmx0a-001b2d01.pphosted.com id v8JE6oYq057400","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy]","X-Received-From":"148.163.158.5","Subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dong Jia Shi , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771488,"web_url":"http://patchwork.ozlabs.org/comment/1771488/","msgid":"<20170920011652.GC11080@bjsdjshi@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-20T01:16:52","subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","submitter":{"id":68897,"url":"http://patchwork.ozlabs.org/api/people/68897/","name":"Dong Jia Shi","email":"bjsdjshi@linux.vnet.ibm.com"},"content":"* Halil Pasic [2017-09-19 15:30:29 +0200]:\n\n> \n> \n> On 09/19/2017 11:06 AM, Cornelia Huck wrote:\n> > On Tue, 19 Sep 2017 11:37:30 +0800\n> > Dong Jia Shi wrote:\n> > \n> >> * Halil Pasic [2017-09-13 13:50:28 +0200]:\n> >>\n> >>> Replace direct access which implicitly assumes no IDA\n> >>> or MIDA with the new ccw data stream interface which should\n> >>> cope with these transparently in the future.\n> >>>\n> >>> Signed-off-by: Halil Pasic \n> >>>\n> >>> ---\n> >>>\n> >>> v1 --> v2:\n> >>> Removed todo comments on possible errno change as with\n> >>> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html\n> >>> applied it does not matter any more.\n> >>>\n> >>> Error handling: At the moment we ignore errors reported\n> >>> by stream ops to keep the change minimal. An add-on\n> >>> patch improving on this is to be expected later. \n> >> Add a TODO somewhere around the code as a reminder?\n> > \n> > I expect Halil to have it on his TODO list and send a patch later ;)\nNo problem with me.\n\n> > \n> >>\n> >>> ---\n> >>> hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------\n> >>> 1 file changed, 46 insertions(+), 110 deletions(-)\n> >>>\n> >>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c\n> >>> index b1976fdd19..a9baf6f7ab 100644\n> >>> --- a/hw/s390x/virtio-ccw.c\n> >>> +++ b/hw/s390x/virtio-ccw.c \n> >> [...]\n> >>\n> >>> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> >>> } else {\n> >>> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);\n> >>>\n> >>> - features.index = address_space_ldub(&address_space_memory,\n> >>> - ccw.cda\n> >>> - + sizeof(features.features),\n> >>> - MEMTXATTRS_UNSPECIFIED,\n> >>> - NULL);\n> >>> + ccw_dstream_advance(&sch->cds, sizeof(features.features)); \n> >> How about:\n> >> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));\n> > \n> > I think keeping sizeof(features.features) is better: It matches the old\n> > code, and we really do jump over the features flag and revisit it\n> > later...\nI was thinking 'advance' to the offset of features.index, then 'read'\nthe value for features.index seems more natural, and you do not need to\ncare too much for what exaclty the elements are that you need to\n'advance'.\n\nNot big deal for me. I could also accept the original version.\n\n> > \n> >>\n> >>> + ccw_dstream_read(&sch->cds, features.index);\n> >>> if (features.index == 0) {\n> >>> if (dev->revision >= 1) {\n> >>> /* Don't offer legacy features for modern devices. */\n> >>> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> >>> /* Return zeroes if the guest supports more feature bits. */\n> >>> features.features = 0;\n> >>> }\n> >>> - address_space_stl_le(&address_space_memory, ccw.cda,\n> >>> - features.features, MEMTXATTRS_UNSPECIFIED,\n> >>> - NULL);\n> >>> + ccw_dstream_rewind(&sch->cds);\n> >>> + cpu_to_le32s(&features.features);\n> >>> + ccw_dstream_write(&sch->cds, features.features);\n> >>> sch->curr_status.scsw.count = ccw.count - sizeof(features); \n> >> How about:\n> >> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);\n> > \n> > Hm, I thought I had commented on this, but I seem to have missed\n> > these...\n> > \n> > I'd prefer to do it as a follow-up patch.\nNo problem.\n\n> > \n> >>\n> >> This also applies to other places.\n> >>\n> >>> ret = 0;\n> >>> } \n> >> [...]\n> >>\n> >>> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> >>> }\n> >>> }\n> >>> len = MIN(ccw.count, vdev->config_len);\n> >>> - hw_len = len;\n> >>> if (!ccw.cda) {\n> >>> ret = -EFAULT;\n> >>> } else {\n> >>> - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);\n> >>> - if (!config) {\n> >>> - ret = -EFAULT;\n> >>> - } else {\n> >>> - len = hw_len;\n> >>> - /* XXX config space endianness */\n> >>> - memcpy(vdev->config, config, len);\n> >>> - cpu_physical_memory_unmap(config, hw_len, 0, hw_len);\n> >>> + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);\n> >>> + if (!ret) { \n> >> Add a TODO here, and:\n> >>\n> >> if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {\n> >> ret = -EFAULT;\n> >> } else {\n> >> ....\n> >> }\n> > \n> > I don't think that would be correct: The function will (at least\n> > currently) return 0 or -EINVAL, and you are now mapping any error to\n> > -EFAULT? (Not that it has an effect in the end: We map both to a\n> > channel program check as of \"s390x/css: drop data-check in\n> > interpretation\".)\nAh, I missed this. Now no problem from me.\n\n> > \n> >>\n> >>> virtio_bus_set_vdev_config(&dev->bus, vdev->config);\n> >>> sch->curr_status.scsw.count = ccw.count - len;\n> >>> - ret = 0;\n> >>> }\n> >>> }\n> >>> break; \n> >> [...]\n> >>\n> >>> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)\n> >>> ret = -EFAULT;\n> >>> break;\n> >>> }\n> >>> - revinfo.revision =\n> >>> - address_space_lduw_be(&address_space_memory, ccw.cda,\n> >>> - MEMTXATTRS_UNSPECIFIED, NULL);\n> >>> - revinfo.length =\n> >>> - address_space_lduw_be(&address_space_memory,\n> >>> - ccw.cda + sizeof(revinfo.revision),\n> >>> - MEMTXATTRS_UNSPECIFIED, NULL);\n> >>> + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); \n\n> >> ^\n> >> A magic number? O:)\n> > \n> > Hah :)\n> > \n> > We can't use sizeof(revinfo), and sizeof(revinfo.revision) +\n> > sizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our\n> > code :)\nNo problem. This seems the best option.\n\n> > \n> >>\n> >>> + be16_to_cpus(&revinfo.revision);\n> >>> + be16_to_cpus(&revinfo.length);\n> >>> if (ccw.count < len + revinfo.length ||\n> >>> (check_len && ccw.count > len + revinfo.length)) {\n> >>> ret = -EINVAL;\n> >>> -- \n> >>> 2.13.5\n> >>> \n> >>\n> >> Otherwise, looks good.\n> >>\n> > \n> > Can I get an ack?\nSure:\nReviewed-by: Dong Jia Shi \n\n> > \n> \n> I agree that further cleanups are possible (e.g. using\n> ccw_dstream_residual_count() and tightening error handling) but\n> I also prefer to see these as on-top, and prefer sticking to\n> change as little as possible in the transformation patch and stay\n> as close as possible to what we had before in terms of behavior).\nUnderstand and agree.\n\n> \n> Regards,\n> Halil \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxhgH1Pb1z9s81\n\tfor ;\n\tWed, 20 Sep 2017 11:17:28 +1000 (AEST)","from localhost ([::1]:46142 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duTdl-0001ZA-LC\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 21:17:25 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:59493)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duTdO-0001XN-7c\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 21:17:03 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duTdK-0004S1-Ug\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 21:17:02 -0400","from mx0a-001b2d01.pphosted.com ([148.163.156.1]:45548)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duTdK-0004Pz-Kq\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 21:16:58 -0400","from pps.filterd (m0098396.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8K1EjqX062385\n\tfor ; Tue, 19 Sep 2017 21:16:57 -0400","from e37.co.us.ibm.com (e37.co.us.ibm.com [32.97.110.158])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d3cnnnjyv-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 21:16:56 -0400","from localhost\n\tby e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 19 Sep 2017 19:16:55 -0600","from b03cxnp08027.gho.boulder.ibm.com (9.17.130.19)\n\tby e37.co.us.ibm.com (192.168.1.137) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tTue, 19 Sep 2017 19:16:54 -0600","from b03ledav003.gho.boulder.ibm.com\n\t(b03ledav003.gho.boulder.ibm.com [9.17.130.234])\n\tby b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8K1GsqL61341846; Tue, 19 Sep 2017 18:16:54 -0700","from b03ledav003.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 3254C6A03C;\n\tTue, 19 Sep 2017 19:16:54 -0600 (MDT)","from localhost (unknown [9.115.112.23])\n\tby b03ledav003.gho.boulder.ibm.com (Postfix) with ESMTP id 74C026A03B;\n\tTue, 19 Sep 2017 19:16:53 -0600 (MDT)"],"Date":"Wed, 20 Sep 2017 09:16:52 +0800","From":"Dong Jia Shi ","To":"Halil Pasic ","Mail-Followup-To":"Halil Pasic ,\n\tCornelia Huck ,\n\tDong Jia Shi ,\n\tPierre Morel , qemu-devel@nongnu.org","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-4-pasic@linux.vnet.ibm.com>\n\t<20170919033730.GD5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919110631.59039743.cohuck@redhat.com>\n\t<6a51f84b-9185-ec5c-b0ec-1fc21fbfb201@linux.vnet.ibm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<6a51f84b-9185-ec5c-b0ec-1fc21fbfb201@linux.vnet.ibm.com>","Organization":"(IBM CSL)","X-URL":"http://ibm.com/","User-Agent":"Mutt/1.5.21 (2010-09-15)","X-TM-AS-GCONF":"00","x-cbid":"17092001-0024-0000-0000-00001737FB85","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007765; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000230; SDB=6.00919520; UDB=6.00461966;\n\tIPR=6.00699746; \n\tBA=6.00005598; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017216;\n\tXFM=3.00000015; UTC=2017-09-20 01:16:55","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17092001-0025-0000-0000-00004CC98D86","Message-Id":"<20170920011652.GC11080@bjsdjshi@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_11:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709200015","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy]","X-Received-From":"148.163.156.1","Subject":"Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dong Jia Shi ,\n\tCornelia Huck ,\n\tPierre Morel , qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]