[{"id":1768439,"web_url":"http://patchwork.ozlabs.org/comment/1768439/","msgid":"<20170914111423.008791d0.cohuck@redhat.com>","list_archive_url":null,"date":"2017-09-14T09:14:23","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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:29 +0200\nHalil Pasic wrote:\n\n> Let's add indirect data addressing support for our virtual channel\n> subsystem. This implementation does no bother with any kind of\n\ns/no/not/\n\n> prefetching. We simply step trough the IDAL on demand.\n\ns/trough/through/\n\n> \n> Signed-off-by: Halil Pasic \n> ---\n> hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 108 insertions(+), 1 deletion(-)\n> \n> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n> index 6b0cd8861b..e34b2af4eb 100644\n> --- a/hw/s390x/css.c\n> +++ b/hw/s390x/css.c\n> @@ -819,6 +819,113 @@ incr:\n> return 0;\n> }\n> \n> +/* returns values between 1 and bsz, where bs is a power of 2 */\n\ns/bz/bsz/ (missed the second one)\n\nI can fix the typos while applying.","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-mx08.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx08.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 3xtCXy15dRz9sBZ\n\tfor ;\n\tThu, 14 Sep 2017 19:14:53 +1000 (AEST)","from localhost ([::1]:46516 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dsQEV-0007tW-BP\n\tfor incoming@patchwork.ozlabs.org; Thu, 14 Sep 2017 05:14:51 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36017)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dsQEC-0007tI-GW\n\tfor qemu-devel@nongnu.org; Thu, 14 Sep 2017 05:14:33 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dsQE8-0005v8-FN\n\tfor qemu-devel@nongnu.org; Thu, 14 Sep 2017 05:14:32 -0400","from mx1.redhat.com ([209.132.183.28]:33816)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dsQE8-0005uT-8u\n\tfor qemu-devel@nongnu.org; Thu, 14 Sep 2017 05:14:28 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\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 1FA4DC0587DD;\n\tThu, 14 Sep 2017 09:14:27 +0000 (UTC)","from gondolin (ovpn-117-98.ams2.redhat.com [10.36.117.98])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id F2F6B5C66F;\n\tThu, 14 Sep 2017 09:14:25 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 1FA4DC0587DD","Date":"Thu, 14 Sep 2017 11:14:23 +0200","From":"Cornelia Huck ","To":"Halil Pasic ","Message-ID":"<20170914111423.008791d0.cohuck@redhat.com>","In-Reply-To":"<20170913115029.47626-5-pasic@linux.vnet.ibm.com>","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-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.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tThu, 14 Sep 2017 09:14:27 +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 4/4] s390x/css: support ccw IDA","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":1770668,"web_url":"http://patchwork.ozlabs.org/comment/1770668/","msgid":"<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-19T05:50:05","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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:29 +0200]:\n\n> Let's add indirect data addressing support for our virtual channel\n> subsystem. This implementation does no bother with any kind of\n> prefetching. We simply step trough the IDAL on demand.\n> \n> Signed-off-by: Halil Pasic \n> ---\n> hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 108 insertions(+), 1 deletion(-)\n> \n> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n> index 6b0cd8861b..e34b2af4eb 100644\n> --- a/hw/s390x/css.c\n> +++ b/hw/s390x/css.c\n> @@ -819,6 +819,113 @@ incr:\n> return 0;\n> }\n> \n> +/* returns values between 1 and bsz, where bs is a power of 2 */\n> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)\n> +{\n> + return bsz - (cda & (bsz - 1));\n> +}\n> +\n> +static inline uint64_t ccw_ida_block_size(uint8_t flags)\n> +{\n> + return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);\nIf CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will\nbe the result regardless the I2K flag? The logic seems wrong.\n\nI2K is meaningful only when C64 is 1, otherwise it is ignored. The logic\nhere should be:\nif ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {\n return 1ULL << 12;\n}\n return 1ULL << 11;\n\n> +}\n> +\n> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n> +{\n> + union {uint64_t fmt2; uint32_t fmt1; } idaw;\n ^\nNit.\n\n> + bool is_fmt2 = cds->flags & CDS_F_C64;\n> + int ret;\n> + hwaddr idaw_addr;\n> +\n> + if (is_fmt2) {\n> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n> + if (idaw_addr & 0x07) {\n> + return -EINVAL; /* channel program check */\n> + }\n> + ret = address_space_rw(&address_space_memory, idaw_addr,\n> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n> + sizeof(idaw.fmt2), false);\n> + cds->cda = be64_to_cpu(idaw.fmt2);\n> + } else {\n> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n> + if (idaw_addr & 0x03) {\n?:\n(idaw_addr & 0x80000003)\n\n> + return -EINVAL; /* channel program check */\n> +\n> + }\n> + ret = address_space_rw(&address_space_memory, idaw_addr,\n> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,\n> + sizeof(idaw.fmt1), false);\n> + cds->cda = be64_to_cpu(idaw.fmt1);\n> + }\n> + ++(cds->at_idaw);\n> + if (ret != MEMTX_OK) {\n> + /* assume inaccessible address */\n> + return -EINVAL; /* channel program check */\n> +\n> + }\n> + return 0;\n> +}\n> +\n> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,\n> + CcwDataStreamOp op)\n> +{\n> + uint64_t bsz = ccw_ida_block_size(cds->flags);\n> + int ret = 0;\n> + uint16_t cont_left, iter_len;\n> +\n> + ret = cds_check_len(cds, len);\n> + if (ret <= 0) {\n> + return ret;\n> + }\n> + if (!cds->at_idaw) {\n> + /* read first idaw */\n> + ret = ida_read_next_idaw(cds);\n> + if (ret) {\n> + goto err;\n> + }\n> + cont_left = ida_continuous_left(cds->cda, bsz);\n> + } else {\n> + cont_left = ida_continuous_left(cds->cda, bsz);\n> + if (cont_left == bsz) {\n> + ret = ida_read_next_idaw(cds);\n> + if (ret) {\n> + goto err;\n> + }\n> + if (cds->cda & (bsz - 1)) {\nCould move this check into ida_read_next_idaw?\n\n> + ret = -EINVAL; /* channel program check */\n> + goto err;\n> + }\n> + }\n> + }\n> + do {\n> + iter_len = MIN(len, cont_left);\n> + if (op != CDS_OP_A) {\n> + ret = address_space_rw(&address_space_memory, cds->cda,\n> + MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);\nAhh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to\n1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to\nmake it more obvious by adding some comment there?\n\n> + if (ret != MEMTX_OK) {\n> + /* assume inaccessible address */\n> + ret = -EINVAL; /* channel program check */\n> + goto err;\n> + }\n> + }\n> + cds->at_byte += iter_len;\n> + cds->cda += iter_len;\n> + len -= iter_len;\n> + if (!len) {\n> + break;\n> + }\n> + ret = ida_read_next_idaw(cds);\n> + if (ret) {\n> + goto err;\n> + }\n> + cont_left = bsz;\n> + } while (true);\n> + return ret;\n> +err:\n> + cds->flags |= CDS_F_STREAM_BROKEN;\n> + return ret;\n> +}\n> +\n> void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n> {\n> /*\n> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n> if (!(cds->flags & CDS_F_IDA)) {\n> cds->op_handler = ccw_dstream_rw_noflags;\n> } else {\n> - assert(false);\n> + cds->op_handler = ccw_dstream_rw_ida;\n> }\n> }\n> \n> -- \n> 2.13.5\n> \n\nGenerally, the logic looks fine to me.","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 3xxBmw74W1z9s7B\n\tfor ;\n\tTue, 19 Sep 2017 15:50:40 +1000 (AEST)","from localhost ([::1]:40158 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duBQd-0007KS-5m\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 01:50:39 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:44824)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duBQJ-0007KH-09\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 01:50:20 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duBQE-0006H9-0d\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 01:50:19 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:40814\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 1duBQD-0006Gr-Rh\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 01:50:13 -0400","from pps.filterd (m0098420.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8J5jIXl006455\n\tfor ; Tue, 19 Sep 2017 01:50:12 -0400","from e34.co.us.ibm.com (e34.co.us.ibm.com [32.97.110.152])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2d2vemkvmj-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 01:50:12 -0400","from localhost\n\tby e34.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 23:50:11 -0600","from b03cxnp08028.gho.boulder.ibm.com (9.17.130.20)\n\tby e34.co.us.ibm.com (192.168.1.134) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tMon, 18 Sep 2017 23:50:08 -0600","from b03ledav003.gho.boulder.ibm.com\n\t(b03ledav003.gho.boulder.ibm.com [9.17.130.234])\n\tby b03cxnp08028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8J5o7kR30736584; Mon, 18 Sep 2017 22:50:07 -0700","from b03ledav003.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 66D6F6A03B;\n\tMon, 18 Sep 2017 23:50:07 -0600 (MDT)","from localhost (unknown [9.115.112.23])\n\tby b03ledav003.gho.boulder.ibm.com (Postfix) with ESMTP id B2A9E6A03D;\n\tMon, 18 Sep 2017 23:50:06 -0600 (MDT)"],"Date":"Tue, 19 Sep 2017 13:50:05 +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-5-pasic@linux.vnet.ibm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170913115029.47626-5-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":"17091905-0016-0000-0000-000007895565","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007760; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000230; SDB=6.00919131; UDB=6.00461769;\n\tIPR=6.00699358; \n\tBA=6.00005597; 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.00017204;\n\tXFM=3.00000015; UTC=2017-09-19 05:50:09","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091905-0017-0000-0000-00003B869E44","Message-Id":"<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_03:, , 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-1709190084","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 4/4] s390x/css: support ccw IDA","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":1770801,"web_url":"http://patchwork.ozlabs.org/comment/1770801/","msgid":"<20170919114859.3e2d895b.cohuck@redhat.com>","list_archive_url":null,"date":"2017-09-19T09:48:59","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","submitter":{"id":71914,"url":"http://patchwork.ozlabs.org/api/people/71914/","name":"Cornelia Huck","email":"cohuck@redhat.com"},"content":"On Tue, 19 Sep 2017 13:50:05 +0800\nDong Jia Shi wrote:\n\n> * Halil Pasic [2017-09-13 13:50:29 +0200]:\n> \n> > Let's add indirect data addressing support for our virtual channel\n> > subsystem. This implementation does no bother with any kind of\n> > prefetching. We simply step trough the IDAL on demand.\n> > \n> > Signed-off-by: Halil Pasic \n> > ---\n> > hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> > 1 file changed, 108 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n> > index 6b0cd8861b..e34b2af4eb 100644\n> > --- a/hw/s390x/css.c\n> > +++ b/hw/s390x/css.c\n> > @@ -819,6 +819,113 @@ incr:\n> > return 0;\n> > }\n> > \n> > +/* returns values between 1 and bsz, where bs is a power of 2 */\n> > +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)\n> > +{\n> > + return bsz - (cda & (bsz - 1));\n> > +}\n> > +\n> > +static inline uint64_t ccw_ida_block_size(uint8_t flags)\n> > +{\n> > + return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12); \n> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will\n> be the result regardless the I2K flag? The logic seems wrong.\n\nI've stared at that condition now for a bit, but all it managed was to\nget me more confused... probably just need a break.\n\n> \n> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic\n> here should be:\n> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {\n> return 1ULL << 12;\n> }\n> return 1ULL << 11;\n\nBut I do think your version is more readable...\n\n> \n> > +}\n> > +\n> > +static inline int ida_read_next_idaw(CcwDataStream *cds)\n> > +{\n> > + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n> ^\n> Nit.\n> \n> > + bool is_fmt2 = cds->flags & CDS_F_C64;\n> > + int ret;\n> > + hwaddr idaw_addr;\n> > +\n> > + if (is_fmt2) {\n> > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n> > + if (idaw_addr & 0x07) {\n> > + return -EINVAL; /* channel program check */\n> > + }\n> > + ret = address_space_rw(&address_space_memory, idaw_addr,\n> > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n> > + sizeof(idaw.fmt2), false);\n> > + cds->cda = be64_to_cpu(idaw.fmt2);\n> > + } else {\n> > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n> > + if (idaw_addr & 0x03) { \n> ?:\n> (idaw_addr & 0x80000003)\n\nYes.\n\n> \n> > + return -EINVAL; /* channel program check */\n> > +\n> > + }\n> > + ret = address_space_rw(&address_space_memory, idaw_addr,\n> > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,\n> > + sizeof(idaw.fmt1), false);\n> > + cds->cda = be64_to_cpu(idaw.fmt1);\n> > + }\n> > + ++(cds->at_idaw);\n> > + if (ret != MEMTX_OK) {\n> > + /* assume inaccessible address */\n> > + return -EINVAL; /* channel program check */\n> > +\n> > + }\n> > + return 0;\n> > +}\n> > +\n> > +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,\n> > + CcwDataStreamOp op)\n> > +{\n> > + uint64_t bsz = ccw_ida_block_size(cds->flags);\n> > + int ret = 0;\n> > + uint16_t cont_left, iter_len;\n> > +\n> > + ret = cds_check_len(cds, len);\n> > + if (ret <= 0) {\n> > + return ret;\n> > + }\n> > + if (!cds->at_idaw) {\n> > + /* read first idaw */\n> > + ret = ida_read_next_idaw(cds);\n> > + if (ret) {\n> > + goto err;\n> > + }\n> > + cont_left = ida_continuous_left(cds->cda, bsz);\n> > + } else {\n> > + cont_left = ida_continuous_left(cds->cda, bsz);\n> > + if (cont_left == bsz) {\n> > + ret = ida_read_next_idaw(cds);\n> > + if (ret) {\n> > + goto err;\n> > + }\n> > + if (cds->cda & (bsz - 1)) { \n> Could move this check into ida_read_next_idaw?\n\nI'd like to avoid further code movement...\n\n> \n> > + ret = -EINVAL; /* channel program check */\n> > + goto err;\n> > + }\n> > + }\n> > + }\n> > + do {\n> > + iter_len = MIN(len, cont_left);\n> > + if (op != CDS_OP_A) {\n> > + ret = address_space_rw(&address_space_memory, cds->cda,\n> > + MEMTXATTRS_UNSPECIFIED, buff, iter_len, op); \n> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to\n> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to\n> make it more obvious by adding some comment there?\n\nWould you have a good text for that?\n\n> \n> > + if (ret != MEMTX_OK) {\n> > + /* assume inaccessible address */\n> > + ret = -EINVAL; /* channel program check */\n> > + goto err;\n> > + }\n> > + }\n> > + cds->at_byte += iter_len;\n> > + cds->cda += iter_len;\n> > + len -= iter_len;\n> > + if (!len) {\n> > + break;\n> > + }\n> > + ret = ida_read_next_idaw(cds);\n> > + if (ret) {\n> > + goto err;\n> > + }\n> > + cont_left = bsz;\n> > + } while (true);\n> > + return ret;\n> > +err:\n> > + cds->flags |= CDS_F_STREAM_BROKEN;\n> > + return ret;\n> > +}\n> > +\n> > void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n> > {\n> > /*\n> > @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n> > if (!(cds->flags & CDS_F_IDA)) {\n> > cds->op_handler = ccw_dstream_rw_noflags;\n> > } else {\n> > - assert(false);\n> > + cds->op_handler = ccw_dstream_rw_ida;\n> > }\n> > }\n> > \n> > -- \n> > 2.13.5\n> > \n> \n> Generally, the logic looks fine to me.\n> \n\nIt did pass Halil's test; but that can only test fmt-2 + 4k blocks, as\nthis is what the kernel infrastructure provides.\n\nHalil, do you have some more comments?","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-mx02.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx02.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 3xxJ780nFZz9s7B\n\tfor ;\n\tTue, 19 Sep 2017 19:51:48 +1000 (AEST)","from localhost ([::1]:41063 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duFBw-00085A-4L\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 05:51:44 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:59318)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duF9P-0006bE-OT\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:49:11 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duF9M-0003pj-Vw\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:49:07 -0400","from mx1.redhat.com ([209.132.183.28]:38572)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1duF9M-0003ox-MS\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 05:49:04 -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 AD4E3806A7;\n\tTue, 19 Sep 2017 09:49:03 +0000 (UTC)","from gondolin (unknown [10.36.117.0])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 9473E5D97A;\n\tTue, 19 Sep 2017 09:49:02 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com AD4E3806A7","Date":"Tue, 19 Sep 2017 11:48:59 +0200","From":"Cornelia Huck ","To":"Dong Jia Shi ","Message-ID":"<20170919114859.3e2d895b.cohuck@redhat.com>","In-Reply-To":"<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@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.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tTue, 19 Sep 2017 09:49:03 +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 4/4] s390x/css: support ccw IDA","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":1770854,"web_url":"http://patchwork.ozlabs.org/comment/1770854/","msgid":"","list_archive_url":null,"date":"2017-09-19T10:36:33","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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:48 AM, Cornelia Huck wrote:\n> On Tue, 19 Sep 2017 13:50:05 +0800\n> Dong Jia Shi wrote:\n> \n>> * Halil Pasic [2017-09-13 13:50:29 +0200]:\n>>\n>>> Let's add indirect data addressing support for our virtual channel\n>>> subsystem. This implementation does no bother with any kind of\n>>> prefetching. We simply step trough the IDAL on demand.\n>>>\n>>> Signed-off-by: Halil Pasic \n>>> ---\n>>> hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n>>> 1 file changed, 108 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n>>> index 6b0cd8861b..e34b2af4eb 100644\n>>> --- a/hw/s390x/css.c\n>>> +++ b/hw/s390x/css.c\n>>> @@ -819,6 +819,113 @@ incr:\n>>> return 0;\n>>> }\n>>>\n>>> +/* returns values between 1 and bsz, where bs is a power of 2 */\n>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)\n>>> +{\n>>> + return bsz - (cda & (bsz - 1));\n>>> +}\n>>> +\n>>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)\n>>> +{\n>>> + return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12); \n>> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will\n>> be the result regardless the I2K flag? The logic seems wrong.\n\nNo. If CDS_F_C64 is set then the outcome depends on the fact if\nCDS_F_I2K is set or not.\n(flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)\n\"(flags ^ CDS_F_C64) will be 0\" is wrong. flags ^ CDS_F_C64\njust flips the CDS_F_C64.\n\nOTOH if the CDS_F_C64 was not set we have the corresponding\nbit set in flags ^ CDS_F_C64 so then the CDS_F_I2K bit does\nnot matter: we have 1ULL << 11.\n\nIn my reading the logic is good.\n\n> \n> I've stared at that condition now for a bit, but all it managed was to\n> get me more confused... probably just need a break.\n> \n>>\n>> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic\n>> here should be:\n>> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {\n>> return 1ULL << 12;\n>> }\n>> return 1ULL << 11;\n> \n> But I do think your version is more readable...\n> \n\nI won't argue with this.\n\n>>\n>>> +}\n>>> +\n>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n>>> +{\n>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n>> ^\n>> Nit.\n>>\n\nMaybe checkpatch wanted it this way. My memories are blurry.\n\n>>> + bool is_fmt2 = cds->flags & CDS_F_C64;\n>>> + int ret;\n>>> + hwaddr idaw_addr;\n>>> +\n>>> + if (is_fmt2) {\n>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n>>> + if (idaw_addr & 0x07) {\n>>> + return -EINVAL; /* channel program check */\n>>> + }\n>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n>>> + sizeof(idaw.fmt2), false);\n>>> + cds->cda = be64_to_cpu(idaw.fmt2);\n>>> + } else {\n>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n>>> + if (idaw_addr & 0x03) { \n>> ?:\n>> (idaw_addr & 0x80000003)\n> \n> Yes.\n> \n\nI will double check this. Does not seem unreasonable but\ndouble-checking is better.\n\n>>\n>>> + return -EINVAL; /* channel program check */\n>>> +\n>>> + }\n>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,\n>>> + sizeof(idaw.fmt1), false);\n>>> + cds->cda = be64_to_cpu(idaw.fmt1);\n>>> + }\n>>> + ++(cds->at_idaw);\n>>> + if (ret != MEMTX_OK) {\n>>> + /* assume inaccessible address */\n>>> + return -EINVAL; /* channel program check */\n>>> +\n>>> + }\n>>> + return 0;\n>>> +}\n>>> +\n>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,\n>>> + CcwDataStreamOp op)\n>>> +{\n>>> + uint64_t bsz = ccw_ida_block_size(cds->flags);\n>>> + int ret = 0;\n>>> + uint16_t cont_left, iter_len;\n>>> +\n>>> + ret = cds_check_len(cds, len);\n>>> + if (ret <= 0) {\n>>> + return ret;\n>>> + }\n>>> + if (!cds->at_idaw) {\n>>> + /* read first idaw */\n>>> + ret = ida_read_next_idaw(cds);\n>>> + if (ret) {\n>>> + goto err;\n>>> + }\n>>> + cont_left = ida_continuous_left(cds->cda, bsz);\n>>> + } else {\n>>> + cont_left = ida_continuous_left(cds->cda, bsz);\n>>> + if (cont_left == bsz) {\n>>> + ret = ida_read_next_idaw(cds);\n>>> + if (ret) {\n>>> + goto err;\n>>> + }\n>>> + if (cds->cda & (bsz - 1)) { \n>> Could move this check into ida_read_next_idaw?\n> \n> I'd like to avoid further code movement...\n> \n\nThe first idaw is special. I don't think moving is possible.\n\n>>\n>>> + ret = -EINVAL; /* channel program check */\n>>> + goto err;\n>>> + }\n>>> + }\n>>> + }\n>>> + do {\n>>> + iter_len = MIN(len, cont_left);\n>>> + if (op != CDS_OP_A) {\n>>> + ret = address_space_rw(&address_space_memory, cds->cda,\n>>> + MEMTXATTRS_UNSPECIFIED, buff, iter_len, op); \n>> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to\n>> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to\n>> make it more obvious by adding some comment there?\n> \n> Would you have a good text for that?\n> \n\nI'm fine with clarifications.\n\n>>\n>>> + if (ret != MEMTX_OK) {\n>>> + /* assume inaccessible address */\n>>> + ret = -EINVAL; /* channel program check */\n>>> + goto err;\n>>> + }\n>>> + }\n>>> + cds->at_byte += iter_len;\n>>> + cds->cda += iter_len;\n>>> + len -= iter_len;\n>>> + if (!len) {\n>>> + break;\n>>> + }\n>>> + ret = ida_read_next_idaw(cds);\n>>> + if (ret) {\n>>> + goto err;\n>>> + }\n>>> + cont_left = bsz;\n>>> + } while (true);\n>>> + return ret;\n>>> +err:\n>>> + cds->flags |= CDS_F_STREAM_BROKEN;\n>>> + return ret;\n>>> +}\n>>> +\n>>> void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n>>> {\n>>> /*\n>>> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n>>> if (!(cds->flags & CDS_F_IDA)) {\n>>> cds->op_handler = ccw_dstream_rw_noflags;\n>>> } else {\n>>> - assert(false);\n>>> + cds->op_handler = ccw_dstream_rw_ida;\n>>> }\n>>> }\n>>>\n>>> -- \n>>> 2.13.5\n>>> \n>>\n>> Generally, the logic looks fine to me.\n>>\n> \n> It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as\n> this is what the kernel infrastructure provides.\n\nNod.\n\n> \n> Halil, do you have some more comments?\n> \n\nJust a question. Do I have to respin?\n\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 3xxKRw4VHDz9sBZ\n\tfor ;\n\tTue, 19 Sep 2017 20:51:24 +1000 (AEST)","from localhost ([::1]:41426 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duG7e-0004ar-N4\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 06:51:22 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:51115)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duFtU-0001E2-LJ\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:36:49 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duFtR-0002o4-1E\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:36:44 -0400","from mx0a-001b2d01.pphosted.com ([148.163.156.1]:35930)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1duFtQ-0002mB-Lw\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:36:40 -0400","from pps.filterd (m0098404.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JAYrKe055318\n\tfor ; Tue, 19 Sep 2017 06:36:39 -0400","from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d3010wded-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 06:36:39 -0400","from localhost\n\tby e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 19 Sep 2017 11:36:36 +0100","from b06cxnps4075.portsmouth.uk.ibm.com (9.149.109.197)\n\tby e06smtp13.uk.ibm.com (192.168.101.143) with IBM ESMTP SMTP\n\tGateway: Authorized Use Only! Violators will be prosecuted; \n\tTue, 19 Sep 2017 11:36:34 +0100","from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com\n\t[9.149.105.61])\n\tby b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8JAaYHo17629406; Tue, 19 Sep 2017 10:36:34 GMT","from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 17D7011C04A;\n\tTue, 19 Sep 2017 11:32:38 +0100 (BST)","from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id B310E11C04C;\n\tTue, 19 Sep 2017 11:32:37 +0100 (BST)","from oc3836556865.ibm.com (unknown [9.152.224.207])\n\tby d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP;\n\tTue, 19 Sep 2017 11:32:37 +0100 (BST)"],"To":"Cornelia Huck ,\n\tDong Jia Shi ","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>","From":"Halil Pasic ","Date":"Tue, 19 Sep 2017 12:36:33 +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":"<20170919114859.3e2d895b.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":"17091910-0012-0000-0000-0000057A5764","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091910-0013-0000-0000-000018F38A64","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_04:, , 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-1709190151","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 4/4] s390x/css: support ccw IDA","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":1770864,"web_url":"http://patchwork.ozlabs.org/comment/1770864/","msgid":"<20170919125717.1ae0cd38.cohuck@redhat.com>","list_archive_url":null,"date":"2017-09-19T10:57:17","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","submitter":{"id":71914,"url":"http://patchwork.ozlabs.org/api/people/71914/","name":"Cornelia Huck","email":"cohuck@redhat.com"},"content":"On Tue, 19 Sep 2017 12:36:33 +0200\nHalil Pasic wrote:\n\n> On 09/19/2017 11:48 AM, Cornelia Huck wrote:\n> > On Tue, 19 Sep 2017 13:50:05 +0800\n> > Dong Jia Shi wrote:\n> > \n> >> * Halil Pasic [2017-09-13 13:50:29 +0200]:\n> >> \n> >>> Let's add indirect data addressing support for our virtual channel\n> >>> subsystem. This implementation does no bother with any kind of\n> >>> prefetching. We simply step trough the IDAL on demand.\n> >>>\n> >>> Signed-off-by: Halil Pasic \n> >>> ---\n> >>> hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> >>> 1 file changed, 108 insertions(+), 1 deletion(-)\n> >>>\n> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n> >>> index 6b0cd8861b..e34b2af4eb 100644\n> >>> --- a/hw/s390x/css.c\n> >>> +++ b/hw/s390x/css.c\n> >>> @@ -819,6 +819,113 @@ incr:\n> >>> return 0;\n> >>> }\n> >>>\n> >>> +/* returns values between 1 and bsz, where bs is a power of 2 */\n> >>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)\n> >>> +{\n> >>> + return bsz - (cda & (bsz - 1));\n> >>> +}\n> >>> +\n> >>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)\n> >>> +{\n> >>> + return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12); \n> >> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will\n> >> be the result regardless the I2K flag? The logic seems wrong. \n> \n> No. If CDS_F_C64 is set then the outcome depends on the fact if\n> CDS_F_I2K is set or not.\n> (flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)\n> \"(flags ^ CDS_F_C64) will be 0\" is wrong. flags ^ CDS_F_C64\n> just flips the CDS_F_C64.\n> \n> OTOH if the CDS_F_C64 was not set we have the corresponding\n> bit set in flags ^ CDS_F_C64 so then the CDS_F_I2K bit does\n> not matter: we have 1ULL << 11.\n> \n> In my reading the logic is good.\n\nSo I'll just leave it...\n\n> \n> > \n> > I've stared at that condition now for a bit, but all it managed was to\n> > get me more confused... probably just need a break.\n> > \n> >>\n> >> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic\n> >> here should be:\n> >> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {\n> >> return 1ULL << 12;\n> >> }\n> >> return 1ULL << 11; \n> > \n> > But I do think your version is more readable...\n> > \n> \n> I won't argue with this.\n\n...and we could change that in a patch on top to avoid future confusion.\n\n> \n> >> \n> >>> +}\n> >>> +\n> >>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n> >>> +{\n> >>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n> >> ^\n> >> Nit.\n> >> \n> \n> Maybe checkpatch wanted it this way. My memories are blurry.\n\n\nI'd just leave it like that, tbh.\n\n> \n> >>> + bool is_fmt2 = cds->flags & CDS_F_C64;\n> >>> + int ret;\n> >>> + hwaddr idaw_addr;\n> >>> +\n> >>> + if (is_fmt2) {\n> >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n> >>> + if (idaw_addr & 0x07) {\n> >>> + return -EINVAL; /* channel program check */\n> >>> + }\n> >>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n> >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n> >>> + sizeof(idaw.fmt2), false);\n> >>> + cds->cda = be64_to_cpu(idaw.fmt2);\n> >>> + } else {\n> >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n> >>> + if (idaw_addr & 0x03) { \n> >> ?:\n> >> (idaw_addr & 0x80000003) \n> > \n> > Yes.\n> > \n> \n> I will double check this. Does not seem unreasonable but\n> double-checking is better.\n\nPlease let me know. I think the architecture says that the bit must be\nzero, and that we may (...) generate a channel program check.\n\n> \n> >> \n> >>> + return -EINVAL; /* channel program check */\n> >>> +\n> >>> + }\n> >>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n> >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,\n> >>> + sizeof(idaw.fmt1), false);\n> >>> + cds->cda = be64_to_cpu(idaw.fmt1);\n> >>> + }\n> >>> + ++(cds->at_idaw);\n> >>> + if (ret != MEMTX_OK) {\n> >>> + /* assume inaccessible address */\n> >>> + return -EINVAL; /* channel program check */\n> >>> +\n> >>> + }\n> >>> + return 0;\n> >>> +}\n> >>> +\n> >>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,\n> >>> + CcwDataStreamOp op)\n> >>> +{\n> >>> + uint64_t bsz = ccw_ida_block_size(cds->flags);\n> >>> + int ret = 0;\n> >>> + uint16_t cont_left, iter_len;\n> >>> +\n> >>> + ret = cds_check_len(cds, len);\n> >>> + if (ret <= 0) {\n> >>> + return ret;\n> >>> + }\n> >>> + if (!cds->at_idaw) {\n> >>> + /* read first idaw */\n> >>> + ret = ida_read_next_idaw(cds);\n> >>> + if (ret) {\n> >>> + goto err;\n> >>> + }\n> >>> + cont_left = ida_continuous_left(cds->cda, bsz);\n> >>> + } else {\n> >>> + cont_left = ida_continuous_left(cds->cda, bsz);\n> >>> + if (cont_left == bsz) {\n> >>> + ret = ida_read_next_idaw(cds);\n> >>> + if (ret) {\n> >>> + goto err;\n> >>> + }\n> >>> + if (cds->cda & (bsz - 1)) { \n> >> Could move this check into ida_read_next_idaw? \n> > \n> > I'd like to avoid further code movement...\n> > \n> \n> The first idaw is special. I don't think moving is possible.\n\nSo, the code is correct and I'll just leave it like that.\n\n> \n> >> \n> >>> + ret = -EINVAL; /* channel program check */\n> >>> + goto err;\n> >>> + }\n> >>> + }\n> >>> + }\n> >>> + do {\n> >>> + iter_len = MIN(len, cont_left);\n> >>> + if (op != CDS_OP_A) {\n> >>> + ret = address_space_rw(&address_space_memory, cds->cda,\n> >>> + MEMTXATTRS_UNSPECIFIED, buff, iter_len, op); \n> >> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to\n> >> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to\n> >> make it more obvious by adding some comment there? \n> > \n> > Would you have a good text for that?\n> > \n> \n> I'm fine with clarifications.\n\nLet's do it as a patch on top.\n\n> \n> >> \n> >>> + if (ret != MEMTX_OK) {\n> >>> + /* assume inaccessible address */\n> >>> + ret = -EINVAL; /* channel program check */\n> >>> + goto err;\n> >>> + }\n> >>> + }\n> >>> + cds->at_byte += iter_len;\n> >>> + cds->cda += iter_len;\n> >>> + len -= iter_len;\n> >>> + if (!len) {\n> >>> + break;\n> >>> + }\n> >>> + ret = ida_read_next_idaw(cds);\n> >>> + if (ret) {\n> >>> + goto err;\n> >>> + }\n> >>> + cont_left = bsz;\n> >>> + } while (true);\n> >>> + return ret;\n> >>> +err:\n> >>> + cds->flags |= CDS_F_STREAM_BROKEN;\n> >>> + return ret;\n> >>> +}\n> >>> +\n> >>> void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n> >>> {\n> >>> /*\n> >>> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n> >>> if (!(cds->flags & CDS_F_IDA)) {\n> >>> cds->op_handler = ccw_dstream_rw_noflags;\n> >>> } else {\n> >>> - assert(false);\n> >>> + cds->op_handler = ccw_dstream_rw_ida;\n> >>> }\n> >>> }\n> >>>\n> >>> -- \n> >>> 2.13.5\n> >>> \n> >>\n> >> Generally, the logic looks fine to me.\n> >> \n> > \n> > It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as\n> > this is what the kernel infrastructure provides. \n> \n> Nod.\n> \n> > \n> > Halil, do you have some more comments?\n> > \n> \n> Just a question. Do I have to respin?\n\nI don't think so. If you could confirm the check for format-1, I'll\njust fixup that one and get the queued patches out of the door.\n\nWe can do more changes on top; it's not like I don't have more patches\nwaiting...","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-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.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 3xxKdF5W87z9s7m\n\tfor ;\n\tTue, 19 Sep 2017 20:59:29 +1000 (AEST)","from localhost ([::1]:41476 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duGFT-0002jb-Pr\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 06:59:27 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:33912)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duGDX-0001jD-G8\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:57:29 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duGDS-0005lH-Kx\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:57:27 -0400","from mx1.redhat.com ([209.132.183.28]:49304)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1duGDS-0005kn-CZ\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 06:57:22 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\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 5F7AF83F3D;\n\tTue, 19 Sep 2017 10:57:21 +0000 (UTC)","from gondolin (unknown [10.36.117.0])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 35CBD5C542;\n\tTue, 19 Sep 2017 10:57:20 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 5F7AF83F3D","Date":"Tue, 19 Sep 2017 12:57:17 +0200","From":"Cornelia Huck ","To":"Halil Pasic ","Message-ID":"<20170919125717.1ae0cd38.cohuck@redhat.com>","In-Reply-To":"","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t","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.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tTue, 19 Sep 2017 10:57:21 +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 4/4] s390x/css: support ccw IDA","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":1770912,"web_url":"http://patchwork.ozlabs.org/comment/1770912/","msgid":"<9872cee8-3503-8d52-c68f-d9c61143201c@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-19T12:04:03","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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 12:57 PM, Cornelia Huck wrote:\n>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n>>>>> +{\n>>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n>>>> ^\n>>>> Nit.\n>>>> \n>> Maybe checkpatch wanted it this way. My memories are blurry.\n> \n> I'd just leave it like that, tbh.\n> \n>>>>> + bool is_fmt2 = cds->flags & CDS_F_C64;\n>>>>> + int ret;\n>>>>> + hwaddr idaw_addr;\n>>>>> +\n>>>>> + if (is_fmt2) {\n>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n>>>>> + if (idaw_addr & 0x07) {\n>>>>> + return -EINVAL; /* channel program check */\n>>>>> + }\n>>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n>>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n>>>>> + sizeof(idaw.fmt2), false);\n>>>>> + cds->cda = be64_to_cpu(idaw.fmt2);\n\n\n>>>>> + } else {\n>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n>>>>> + if (idaw_addr & 0x03) { \n>>>> ?:\n>>>> (idaw_addr & 0x80000003) \n>>> Yes.\n>>> \n>> I will double check this. Does not seem unreasonable but\n>> double-checking is better.\n> Please let me know. I think the architecture says that the bit must be\n> zero, and that we may (...) generate a channel program check.\n> \n\nNot exactly. The more significant bits part of the check\ndepend on the ccw format. This needs to be done for both\nidaw formats. I would need to introduce a new flag, or\naccess the SubchDev to do this properly.\n\nArchitecturally we also need to check the data addresses\nfrom which we read so we have nothing bigger than \n(1 << 31) - 1 if we are working with format-1 idaws.\n\nI also think we did not take proper care of proper\nmaximum data address checks prior CwwDataStream which also\ndepend on the ccw format (in absence of IDAW or MIDAW).\n\nThe ccw format dependent maximum address checks are (1 << 24) - 1\nand (1 << 31) - 1 respectively for format-0 and format-1 (on\nthe first indirection level that is for non-IDA for the data,\nand for (M)IDA for the (M)IDAWs).\n\nReference:\nPoP pages 16-25 and 16-26 \"Invalid IDAW or MIDAW Addre\" and\n\"Invalid Data Address\".\n\nHow shall we proceed?\n\nHalil\n\n>>>> \n>>>>> + return -EINVAL; /* channel program check */\n>>>>> +\n>>>>> + }\n>>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n>>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,\n>>>>> + sizeof(idaw.fmt1), false);\n>>>>> + cds->cda = be64_to_cpu(idaw.fmt1);>>>>> + }\n>>>>> + ++(cds->at_idaw);\n>>>>> + if (ret != MEMTX_OK) {\n>>>>> + /* assume inaccessible address */\n>>>>> + return -EINVAL; /* channel program check */\n>>>>> +\n>>>>> + }\n>>>>> + return 0;\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 3xxM4X2WxXz9s4q\n\tfor ;\n\tTue, 19 Sep 2017 22:04:44 +1000 (AEST)","from localhost ([::1]:42476 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duHGc-0002rv-Hd\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 08:04:42 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:45776)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duHGE-0002rX-AI\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:04:22 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duHG8-0006DR-73\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:04:18 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:44756\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 1duHG8-0006D1-1z\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:04:12 -0400","from pps.filterd (m0098414.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JBwZ56032597\n\tfor ; Tue, 19 Sep 2017 08:04:11 -0400","from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2d319pe67f-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 08:04:10 -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 13:04:07 +0100","from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196)\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 13:04:04 +0100","from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com\n\t[9.149.105.61])\n\tby b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8JC43MX21954694; Tue, 19 Sep 2017 12:04:03 GMT","from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 8920B11C0EB;\n\tTue, 19 Sep 2017 13:00:07 +0100 (BST)","from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 4CD1411C0F9;\n\tTue, 19 Sep 2017 13:00:07 +0100 (BST)","from oc3836556865.ibm.com (unknown [9.152.224.207])\n\tby d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP;\n\tTue, 19 Sep 2017 13:00:07 +0100 (BST)"],"To":"Cornelia Huck ","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>","From":"Halil Pasic ","Date":"Tue, 19 Sep 2017 14:04:03 +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":"<20170919125717.1ae0cd38.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":"17091912-0020-0000-0000-000003B956EC","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091912-0021-0000-0000-0000424B01AB","Message-Id":"<9872cee8-3503-8d52-c68f-d9c61143201c@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-1709190171","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 4/4] s390x/css: support ccw IDA","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":1770948,"web_url":"http://patchwork.ozlabs.org/comment/1770948/","msgid":"<20170919142351.1e3b66d8.cohuck@redhat.com>","list_archive_url":null,"date":"2017-09-19T12:23:51","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","submitter":{"id":71914,"url":"http://patchwork.ozlabs.org/api/people/71914/","name":"Cornelia Huck","email":"cohuck@redhat.com"},"content":"On Tue, 19 Sep 2017 14:04:03 +0200\nHalil Pasic wrote:\n\n> On 09/19/2017 12:57 PM, Cornelia Huck wrote:\n> >>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n> >>>>> +{\n> >>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n> >>>> ^\n> >>>> Nit.\n> >>>> \n> >> Maybe checkpatch wanted it this way. My memories are blurry. \n> > \n> > I'd just leave it like that, tbh.\n> > \n> >>>>> + bool is_fmt2 = cds->flags & CDS_F_C64;\n> >>>>> + int ret;\n> >>>>> + hwaddr idaw_addr;\n> >>>>> +\n> >>>>> + if (is_fmt2) {\n> >>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n> >>>>> + if (idaw_addr & 0x07) {\n> >>>>> + return -EINVAL; /* channel program check */\n> >>>>> + }\n> >>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n> >>>>> + sizeof(idaw.fmt2), false);\n> >>>>> + cds->cda = be64_to_cpu(idaw.fmt2); \n> \n> \n> >>>>> + } else {\n> >>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n> >>>>> + if (idaw_addr & 0x03) { \n> >>>> ?:\n> >>>> (idaw_addr & 0x80000003) \n> >>> Yes.\n> >>> \n> >> I will double check this. Does not seem unreasonable but\n> >> double-checking is better. \n> > Please let me know. I think the architecture says that the bit must be\n> > zero, and that we may (...) generate a channel program check.\n> > \n> \n> Not exactly. The more significant bits part of the check\n> depend on the ccw format. This needs to be done for both\n> idaw formats. I would need to introduce a new flag, or\n> access the SubchDev to do this properly.\n> \n> Architecturally we also need to check the data addresses\n> from which we read so we have nothing bigger than \n> (1 << 31) - 1 if we are working with format-1 idaws.\n> \n> I also think we did not take proper care of proper\n> maximum data address checks prior CwwDataStream which also\n> depend on the ccw format (in absence of IDAW or MIDAW).\n> \n> The ccw format dependent maximum address checks are (1 << 24) - 1\n> and (1 << 31) - 1 respectively for format-0 and format-1 (on\n> the first indirection level that is for non-IDA for the data,\n> and for (M)IDA for the (M)IDAWs).\n> \n> Reference:\n> PoP pages 16-25 and 16-26 \"Invalid IDAW or MIDAW Addre\" and\n> \"Invalid Data Address\".\n\nSigh, when are things ever easy :(\n\nI'll need some time to review this. But more urgently, I need a break.\n\n> \n> How shall we proceed?\n\nGiven the discussion about this patch in particular, I'll probably\ndequeue it and send it with the next batch of patches. We can also\ninclude the other improvements, then. Not sure whether I should just\ndequeue this one or the whole series (I really want to get the rest of\nthe patches out...)\n\nMore after the break :)","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-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.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 3xxMWS6Ww7z9rxl\n\tfor ;\n\tTue, 19 Sep 2017 22:24:35 +1000 (AEST)","from localhost ([::1]:42538 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duHZp-0000yW-8p\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 08:24:33 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56033)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duHZK-0000xr-Da\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:24:08 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duHZG-0001JN-ED\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:24:02 -0400","from mx1.redhat.com ([209.132.183.28]:57652)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1duHZG-0001Is-4z\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:23:58 -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 7787E4A708;\n\tTue, 19 Sep 2017 12:23:56 +0000 (UTC)","from gondolin (unknown [10.36.117.0])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 20CE25D980;\n\tTue, 19 Sep 2017 12:23:54 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 7787E4A708","Date":"Tue, 19 Sep 2017 14:23:51 +0200","From":"Cornelia Huck ","To":"Halil Pasic ","Message-ID":"<20170919142351.1e3b66d8.cohuck@redhat.com>","In-Reply-To":"<9872cee8-3503-8d52-c68f-d9c61143201c@linux.vnet.ibm.com>","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>\n\t<9872cee8-3503-8d52-c68f-d9c61143201c@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.38]);\n\tTue, 19 Sep 2017 12:23:56 +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 4/4] s390x/css: support ccw IDA","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":1770993,"web_url":"http://patchwork.ozlabs.org/comment/1770993/","msgid":"<8e6eaeef-0ed3-4660-b445-fd0440afbe1d@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-19T12:32:33","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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 02:23 PM, Cornelia Huck wrote:\n> On Tue, 19 Sep 2017 14:04:03 +0200\n> Halil Pasic wrote:\n> \n>> On 09/19/2017 12:57 PM, Cornelia Huck wrote:\n>>>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n>>>>>>> +{\n>>>>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n>>>>>> ^\n>>>>>> Nit.\n>>>>>> \n>>>> Maybe checkpatch wanted it this way. My memories are blurry. \n>>>\n>>> I'd just leave it like that, tbh.\n>>> \n>>>>>>> + bool is_fmt2 = cds->flags & CDS_F_C64;\n>>>>>>> + int ret;\n>>>>>>> + hwaddr idaw_addr;\n>>>>>>> +\n>>>>>>> + if (is_fmt2) {\n>>>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n>>>>>>> + if (idaw_addr & 0x07) {\n>>>>>>> + return -EINVAL; /* channel program check */\n>>>>>>> + }\n>>>>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n>>>>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n>>>>>>> + sizeof(idaw.fmt2), false);\n>>>>>>> + cds->cda = be64_to_cpu(idaw.fmt2); \n>>\n>>\n>>>>>>> + } else {\n>>>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n>>>>>>> + if (idaw_addr & 0x03) { \n>>>>>> ?:\n>>>>>> (idaw_addr & 0x80000003) \n>>>>> Yes.\n>>>>> \n>>>> I will double check this. Does not seem unreasonable but\n>>>> double-checking is better. \n>>> Please let me know. I think the architecture says that the bit must be\n>>> zero, and that we may (...) generate a channel program check.\n>>> \n>>\n>> Not exactly. The more significant bits part of the check\n>> depend on the ccw format. This needs to be done for both\n>> idaw formats. I would need to introduce a new flag, or\n>> access the SubchDev to do this properly.\n>>\n>> Architecturally we also need to check the data addresses\n>> from which we read so we have nothing bigger than \n>> (1 << 31) - 1 if we are working with format-1 idaws.\n>>\n>> I also think we did not take proper care of proper\n>> maximum data address checks prior CwwDataStream which also\n>> depend on the ccw format (in absence of IDAW or MIDAW).\n>>\n>> The ccw format dependent maximum address checks are (1 << 24) - 1\n>> and (1 << 31) - 1 respectively for format-0 and format-1 (on\n>> the first indirection level that is for non-IDA for the data,\n>> and for (M)IDA for the (M)IDAWs).\n>>\n>> Reference:\n>> PoP pages 16-25 and 16-26 \"Invalid IDAW or MIDAW Addre\" and\n>> \"Invalid Data Address\".\n> \n> Sigh, when are things ever easy :(\n> \n> I'll need some time to review this. But more urgently, I need a break.\n> \n>>\n>> How shall we proceed?\n> \n> Given the discussion about this patch in particular, I'll probably\n> dequeue it and send it with the next batch of patches. We can also\n> include the other improvements, then. Not sure whether I should just\n> dequeue this one or the whole series (I really want to get the rest of\n> the patches out...)\n> \n> More after the break :)\n> \n\nI think I can fix this pretty fast, and fixing this later is\nan option too in my opinion as the patch is consistent with our\nprior art (we ignored this maximum address checking requirement,\nand we keep ignoring it). \n\nI would prefer not dequeue-ing the whole series because a\n3270 fix of mine (which just made the internal review) depends\non this. IMHO, it's not like the series is fundamentally broken,\nnot ain't an improvement at all. It is not perfect, but it's\nIMHO certainly an improvement over what we have.\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 3xxNfv241vz9sBZ\n\tfor ;\n\tTue, 19 Sep 2017 23:16:07 +1000 (AEST)","from localhost ([::1]:42800 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duINg-0000ae-Vk\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 09:16:05 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:33254)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duHhj-0006La-FQ\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:32:44 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duHhf-00071K-Fi\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:32:43 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:42754\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 1duHhf-00071B-AM\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 08:32:39 -0400","from pps.filterd (m0098417.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JCTJ7j085363\n\tfor ; Tue, 19 Sep 2017 08:32:38 -0400","from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d30nvhrw8-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 08:32:38 -0400","from localhost\n\tby e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 19 Sep 2017 13:32:36 +0100","from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196)\n\tby e06smtp13.uk.ibm.com (192.168.101.143) with IBM ESMTP SMTP\n\tGateway: Authorized Use Only! Violators will be prosecuted; \n\tTue, 19 Sep 2017 13:32:34 +0100","from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com\n\t[9.149.105.61])\n\tby b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8JCWXoI26476690; Tue, 19 Sep 2017 12:32:33 GMT","from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 6D30511C04C;\n\tTue, 19 Sep 2017 13:28:37 +0100 (BST)","from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 2B38311C04A;\n\tTue, 19 Sep 2017 13:28:37 +0100 (BST)","from oc3836556865.ibm.com (unknown [9.152.224.207])\n\tby d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP;\n\tTue, 19 Sep 2017 13:28:37 +0100 (BST)"],"To":"Cornelia Huck ","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>\n\t<9872cee8-3503-8d52-c68f-d9c61143201c@linux.vnet.ibm.com>\n\t<20170919142351.1e3b66d8.cohuck@redhat.com>","From":"Halil Pasic ","Date":"Tue, 19 Sep 2017 14:32:33 +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":"<20170919142351.1e3b66d8.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":"17091912-0012-0000-0000-0000057A6155","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091912-0013-0000-0000-000018F394EF","Message-Id":"<8e6eaeef-0ed3-4660-b445-fd0440afbe1d@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-1709190179","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 4/4] s390x/css: support ccw IDA","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":1771038,"web_url":"http://patchwork.ozlabs.org/comment/1771038/","msgid":"<0c6660b7-9d81-7ada-7388-cfdaf89e25f3@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-19T13:46:20","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","submitter":{"id":66825,"url":"http://patchwork.ozlabs.org/api/people/66825/","name":"Pierre Morel","email":"pmorel@linux.vnet.ibm.com"},"content":"On 19/09/2017 12:57, Cornelia Huck wrote:\n> On Tue, 19 Sep 2017 12:36:33 +0200\n> Halil Pasic wrote:\n> \n>> On 09/19/2017 11:48 AM, Cornelia Huck wrote:\n>>> On Tue, 19 Sep 2017 13:50:05 +0800\n>>> Dong Jia Shi wrote:\n>>> \n>>>> * Halil Pasic [2017-09-13 13:50:29 +0200]:\n>>>> \n>>>>> Let's add indirect data addressing support for our virtual channel\n>>>>> subsystem. This implementation does no bother with any kind of\n>>>>> prefetching. We simply step trough the IDAL on demand.\n>>>>>\n>>>>> Signed-off-by: Halil Pasic \n>>>>> ---\n>>>>> hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n>>>>> 1 file changed, 108 insertions(+), 1 deletion(-)\n>>>>>\n>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\n>>>>> index 6b0cd8861b..e34b2af4eb 100644\n>>>>> --- a/hw/s390x/css.c\n>>>>> +++ b/hw/s390x/css.c\n>>>>> @@ -819,6 +819,113 @@ incr:\n>>>>> return 0;\n>>>>> }\n>>>>>\n>>>>> +/* returns values between 1 and bsz, where bs is a power of 2 */\n>>>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)\n>>>>> +{\n>>>>> + return bsz - (cda & (bsz - 1));\n>>>>> +}\n>>>>> +\n>>>>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)\n>>>>> +{\n>>>>> + return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);\n>>>> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will\n>>>> be the result regardless the I2K flag? The logic seems wrong.\n>>\n>> No. If CDS_F_C64 is set then the outcome depends on the fact if\n>> CDS_F_I2K is set or not.\n>> (flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)\n>> \"(flags ^ CDS_F_C64) will be 0\" is wrong. flags ^ CDS_F_C64\n>> just flips the CDS_F_C64.\n>>\n>> OTOH if the CDS_F_C64 was not set we have the corresponding\n>> bit set in flags ^ CDS_F_C64 so then the CDS_F_I2K bit does\n>> not matter: we have 1ULL << 11.\n>>\n>> In my reading the logic is good.\n> \n> So I'll just leave it...\n> \n>>\n>>>\n>>> I've stared at that condition now for a bit, but all it managed was to\n>>> get me more confused... probably just need a break.\n>>> \n>>>>\n>>>> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic\n>>>> here should be:\n>>>> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {\n>>>> return 1ULL << 12;\n>>>> }\n>>>> return 1ULL << 11;\n>>>\n>>> But I do think your version is more readable... >>>\n>>\n>> I won't argue with this.\n\n:)\n\n> \n> ...and we could change that in a patch on top to avoid future confusion.\n> \n>>\n>>>> \n>>>>> +}\n>>>>> +\n>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n>>>>> +{\n>>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw;\n>>>> ^\n>>>> Nit.\n>>>> \n>>\n>> Maybe checkpatch wanted it this way. My memories are blurry.\n> \n> \n> I'd just leave it like that, tbh.\n> \n>>\n>>>>> + bool is_fmt2 = cds->flags & CDS_F_C64;\n>>>>> + int ret;\n>>>>> + hwaddr idaw_addr;\n>>>>> +\n>>>>> + if (is_fmt2) {\n>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n>>>>> + if (idaw_addr & 0x07) {\n>>>>> + return -EINVAL; /* channel program check */\n>>>>> + }\n>>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n>>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n>>>>> + sizeof(idaw.fmt2), false);\n>>>>> + cds->cda = be64_to_cpu(idaw.fmt2);\n>>>>> + } else {\n>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n>>>>> + if (idaw_addr & 0x03) {\n>>>> ?:\n>>>> (idaw_addr & 0x80000003)\n>>>\n>>> Yes.\n>>> \n>>\n>> I will double check this. Does not seem unreasonable but\n>> double-checking is better.\n> \n> Please let me know. I think the architecture says that the bit must be\n> zero, and that we may (...) generate a channel program check.\n> \n\nRight (0x80000003) !=0 implies program check .\nIt is what is done , except for the bit 0 that was forgotten.\n\n>>\n>>>> \n>>>>> + return -EINVAL; /* channel program check */\n>>>>> +\n>>>>> + }\n>>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n>>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,\n>>>>> + sizeof(idaw.fmt1), false);\n>>>>> + cds->cda = be64_to_cpu(idaw.fmt1);\n>>>>> + }\n>>>>> + ++(cds->at_idaw);\n>>>>> + if (ret != MEMTX_OK) {\n>>>>> + /* assume inaccessible address */\n>>>>> + return -EINVAL; /* channel program check */\n>>>>> +\n>>>>> + }\n>>>>> + return 0;\n>>>>> +}\n>>>>> +\n>>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,\n>>>>> + CcwDataStreamOp op)\n>>>>> +{\n>>>>> + uint64_t bsz = ccw_ida_block_size(cds->flags);\n>>>>> + int ret = 0;\n>>>>> + uint16_t cont_left, iter_len;\n>>>>> +\n>>>>> + ret = cds_check_len(cds, len);\n>>>>> + if (ret <= 0) {\n>>>>> + return ret;\n>>>>> + }\n>>>>> + if (!cds->at_idaw) {\n>>>>> + /* read first idaw */\n>>>>> + ret = ida_read_next_idaw(cds);\n>>>>> + if (ret) {\n>>>>> + goto err;\n>>>>> + }\n>>>>> + cont_left = ida_continuous_left(cds->cda, bsz);\n>>>>> + } else {\n>>>>> + cont_left = ida_continuous_left(cds->cda, bsz);\n>>>>> + if (cont_left == bsz) {\n>>>>> + ret = ida_read_next_idaw(cds);\n>>>>> + if (ret) {\n>>>>> + goto err;\n>>>>> + }\n>>>>> + if (cds->cda & (bsz - 1)) {\n>>>> Could move this check into ida_read_next_idaw?\n>>>\n>>> I'd like to avoid further code movement...\n>>> \n>>\n>> The first idaw is special. I don't think moving is possible.\n> \n> So, the code is correct and I'll just leave it like that.\n\nhum.\nIt seems to me that the handling of the first IDAW is indeed a little \nbit different.\nIt is accessed directly from the CCW->CDA and generated dedicated status \nbut I do not see the handling.\n\nThe channel status must be made pending with primary, secondary and \nalert status.\n\nI do not find this code, may be it is somewhere before, I searched but \ndid not find it.\nAlso, I do not find in the documentation that we have a program check \nfor this case.\n\n> \n>>\n>>>> \n>>>>> + ret = -EINVAL; /* channel program check */\n>>>>> + goto err;\n>>>>> + }\n>>>>> + }\n>>>>> + }\n>>>>> + do {\n>>>>> + iter_len = MIN(len, cont_left);\n>>>>> + if (op != CDS_OP_A) {\n>>>>> + ret = address_space_rw(&address_space_memory, cds->cda,\n>>>>> + MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);\n>>>> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to\n>>>> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to\n>>>> make it more obvious by adding some comment there?\n>>>\n>>> Would you have a good text for that?\n>>> \n>>\n>> I'm fine with clarifications.\n> \n> Let's do it as a patch on top.\n> \n>>\n>>>> \n>>>>> + if (ret != MEMTX_OK) {\n>>>>> + /* assume inaccessible address */\n>>>>> + ret = -EINVAL; /* channel program check */\n>>>>> + goto err;\n>>>>> + }\n>>>>> + }\n>>>>> + cds->at_byte += iter_len;\n>>>>> + cds->cda += iter_len;\n>>>>> + len -= iter_len;\n>>>>> + if (!len) {\n>>>>> + break;\n>>>>> + }\n>>>>> + ret = ida_read_next_idaw(cds);\n>>>>> + if (ret) {\n>>>>> + goto err;\n>>>>> + }\n>>>>> + cont_left = bsz;\n>>>>> + } while (true);\n>>>>> + return ret;\n>>>>> +err:\n>>>>> + cds->flags |= CDS_F_STREAM_BROKEN;\n>>>>> + return ret;\n>>>>> +}\n>>>>> +\n>>>>> void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n>>>>> {\n>>>>> /*\n>>>>> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\n>>>>> if (!(cds->flags & CDS_F_IDA)) {\n>>>>> cds->op_handler = ccw_dstream_rw_noflags;\n>>>>> } else {\n>>>>> - assert(false);\n>>>>> + cds->op_handler = ccw_dstream_rw_ida;\n>>>>> }\n>>>>> }\n>>>>>\n>>>>> -- \n>>>>> 2.13.5\n>>>>> \n>>>>\n>>>> Generally, the logic looks fine to me.\n>>>> \n>>>\n>>> It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as\n>>> this is what the kernel infrastructure provides.\n>>\n>> Nod.\n>>\n>>>\n>>> Halil, do you have some more comments?\n>>> \n>>\n>> Just a question. Do I have to respin?\n> \n> I don't think so. If you could confirm the check for format-1, I'll\n> just fixup that one and get the queued patches out of the door.\n\ngenerally LGTM but in my opinion the check for format-1 and the handling \nof the error status for the first IDA for format 1 and 2 must be cleared.\n\n\n> \n> We can do more changes on top; it's not like I don't have more patches\n> waiting...\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 3xxPQ20kGZz9sMN\n\tfor ;\n\tTue, 19 Sep 2017 23:50:02 +1000 (AEST)","from localhost ([::1]:43016 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duIuW-00029o-59\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 09:50:00 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56654)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duIr9-0008Dy-05\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:46:32 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duIr6-00042T-BY\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:46:31 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:50034\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 1duIr6-00041r-3w\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 09:46:28 -0400","from pps.filterd (m0098419.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JDiQ4D130298\n\tfor ; Tue, 19 Sep 2017 09:46:27 -0400","from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2d313wd07g-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 09:46:26 -0400","from localhost\n\tby e06smtp13.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:46:23 +0100","from b06cxnps3074.portsmouth.uk.ibm.com (9.149.109.194)\n\tby e06smtp13.uk.ibm.com (192.168.101.143) with IBM ESMTP SMTP\n\tGateway: Authorized Use Only! Violators will be prosecuted; \n\tTue, 19 Sep 2017 14:46:21 +0100","from d06av23.portsmouth.uk.ibm.com (d06av23.portsmouth.uk.ibm.com\n\t[9.149.105.59])\n\tby b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8JDkK5824903796; Tue, 19 Sep 2017 13:46:20 GMT","from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id B94BAA4053;\n\tTue, 19 Sep 2017 14:42:22 +0100 (BST)","from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 7DA4BA404D;\n\tTue, 19 Sep 2017 14:42:22 +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 14:42:22 +0100 (BST)"],"To":"Cornelia Huck , Halil Pasic ","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>","From":"Pierre Morel ","Date":"Tue, 19 Sep 2017 15:46:20 +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":"<20170919125717.1ae0cd38.cohuck@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","X-TM-AS-GCONF":"00","x-cbid":"17091913-0012-0000-0000-0000057A6824","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091913-0013-0000-0000-000018F39C32","Message-Id":"<0c6660b7-9d81-7ada-7388-cfdaf89e25f3@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-1709190195","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by\n\tmx0b-001b2d01.pphosted.com id v8JDiQ4D130298","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 4/4] s390x/css: support ccw IDA","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":1771088,"web_url":"http://patchwork.ozlabs.org/comment/1771088/","msgid":"<20170919163411.649d3b66.cohuck@redhat.com>","list_archive_url":null,"date":"2017-09-19T14:34:11","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","submitter":{"id":71914,"url":"http://patchwork.ozlabs.org/api/people/71914/","name":"Cornelia Huck","email":"cohuck@redhat.com"},"content":"On Tue, 19 Sep 2017 14:32:33 +0200\nHalil Pasic wrote:\n\n> On 09/19/2017 02:23 PM, Cornelia Huck wrote:\n> > On Tue, 19 Sep 2017 14:04:03 +0200\n> > Halil Pasic wrote:\n> > \n> >> On 09/19/2017 12:57 PM, Cornelia Huck wrote: \n> >>>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n> >>>>>>> +{\n> >>>>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n> >>>>>> ^\n> >>>>>> Nit.\n> >>>>>> \n> >>>> Maybe checkpatch wanted it this way. My memories are blurry. \n> >>>\n> >>> I'd just leave it like that, tbh.\n> >>> \n> >>>>>>> + bool is_fmt2 = cds->flags & CDS_F_C64;\n> >>>>>>> + int ret;\n> >>>>>>> + hwaddr idaw_addr;\n> >>>>>>> +\n> >>>>>>> + if (is_fmt2) {\n> >>>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n> >>>>>>> + if (idaw_addr & 0x07) {\n> >>>>>>> + return -EINVAL; /* channel program check */\n> >>>>>>> + }\n> >>>>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n> >>>>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n> >>>>>>> + sizeof(idaw.fmt2), false);\n> >>>>>>> + cds->cda = be64_to_cpu(idaw.fmt2); \n> >>\n> >> \n> >>>>>>> + } else {\n> >>>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n> >>>>>>> + if (idaw_addr & 0x03) { \n> >>>>>> ?:\n> >>>>>> (idaw_addr & 0x80000003) \n> >>>>> Yes.\n> >>>>> \n> >>>> I will double check this. Does not seem unreasonable but\n> >>>> double-checking is better. \n> >>> Please let me know. I think the architecture says that the bit must be\n> >>> zero, and that we may (...) generate a channel program check.\n> >>> \n> >>\n> >> Not exactly. The more significant bits part of the check\n> >> depend on the ccw format. This needs to be done for both\n> >> idaw formats. I would need to introduce a new flag, or\n> >> access the SubchDev to do this properly.\n> >>\n> >> Architecturally we also need to check the data addresses\n> >> from which we read so we have nothing bigger than \n> >> (1 << 31) - 1 if we are working with format-1 idaws.\n> >>\n> >> I also think we did not take proper care of proper\n> >> maximum data address checks prior CwwDataStream which also\n> >> depend on the ccw format (in absence of IDAW or MIDAW).\n> >>\n> >> The ccw format dependent maximum address checks are (1 << 24) - 1\n> >> and (1 << 31) - 1 respectively for format-0 and format-1 (on\n> >> the first indirection level that is for non-IDA for the data,\n> >> and for (M)IDA for the (M)IDAWs).\n> >>\n> >> Reference:\n> >> PoP pages 16-25 and 16-26 \"Invalid IDAW or MIDAW Addre\" and\n> >> \"Invalid Data Address\". \n> > \n> > Sigh, when are things ever easy :(\n> > \n> > I'll need some time to review this. But more urgently, I need a break.\n> > \n> >>\n> >> How shall we proceed? \n> > \n> > Given the discussion about this patch in particular, I'll probably\n> > dequeue it and send it with the next batch of patches. We can also\n> > include the other improvements, then. Not sure whether I should just\n> > dequeue this one or the whole series (I really want to get the rest of\n> > the patches out...)\n> > \n> > More after the break :)\n> > \n> \n> I think I can fix this pretty fast, and fixing this later is\n> an option too in my opinion as the patch is consistent with our\n> prior art (we ignored this maximum address checking requirement,\n> and we keep ignoring it). \n> \n> I would prefer not dequeue-ing the whole series because a\n> 3270 fix of mine (which just made the internal review) depends\n> on this. IMHO, it's not like the series is fundamentally broken,\n> not ain't an improvement at all. It is not perfect, but it's\n> IMHO certainly an improvement over what we have.\n\nThe issue is not that the series is problematic, but that I need to put\na stop on changes at some point in time and just flush my queue -\notherwise this is not workable.\n\nTherefore, I dequeued the series for now. I'll probably do another pull\nrequest in the next weeks anyway - so there's unlikely to be a long\ndelay, and we can hash out everything without pressure.\n\nIt would be best to just think this through; then you can send a v3\nwith the feedback addressed and your 3270 patch on top.","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 3xxQRt2fG1z9sNw\n\tfor ;\n\tWed, 20 Sep 2017 00:36:42 +1000 (AEST)","from localhost ([::1]:43275 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duJdg-0004lN-GG\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 10:36:40 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:57690)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duJbT-0003g0-Kd\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:34:25 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duJbN-0000Xq-Dp\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:34:23 -0400","from mx1.redhat.com ([209.132.183.28]:35382)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1duJbN-0000XR-5A\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 10:34:17 -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 3445813AA4;\n\tTue, 19 Sep 2017 14:34:16 +0000 (UTC)","from gondolin (unknown [10.36.117.0])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id EA0E75D97D;\n\tTue, 19 Sep 2017 14:34:14 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 3445813AA4","Date":"Tue, 19 Sep 2017 16:34:11 +0200","From":"Cornelia Huck ","To":"Halil Pasic ","Message-ID":"<20170919163411.649d3b66.cohuck@redhat.com>","In-Reply-To":"<8e6eaeef-0ed3-4660-b445-fd0440afbe1d@linux.vnet.ibm.com>","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>\n\t<9872cee8-3503-8d52-c68f-d9c61143201c@linux.vnet.ibm.com>\n\t<20170919142351.1e3b66d8.cohuck@redhat.com>\n\t<8e6eaeef-0ed3-4660-b445-fd0440afbe1d@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.29]);\n\tTue, 19 Sep 2017 14:34:16 +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 4/4] s390x/css: support ccw IDA","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":1771222,"web_url":"http://patchwork.ozlabs.org/comment/1771222/","msgid":"","list_archive_url":null,"date":"2017-09-19T16:49:08","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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 03:46 PM, Pierre Morel wrote:\r\n> On 19/09/2017 12:57, Cornelia Huck wrote:\r\n>> On Tue, 19 Sep 2017 12:36:33 +0200\r\n>> Halil Pasic wrote:\r\n>>\r\n>>> On 09/19/2017 11:48 AM, Cornelia Huck wrote:\r\n>>>> On Tue, 19 Sep 2017 13:50:05 +0800\r\n>>>> Dong Jia Shi wrote:\r\n>>>>   \r\n>>>>> * Halil Pasic [2017-09-13 13:50:29 +0200]:\r\n>>>>>  \r\n>>>>>> Let's add indirect data addressing support for our virtual channel\r\n>>>>>> subsystem. This implementation does no bother with any kind of\r\n>>>>>> prefetching. We simply step trough the IDAL on demand.\r\n>>>>>>\r\n>>>>>> Signed-off-by: Halil Pasic \r\n>>>>>> ---\r\n>>>>>>   hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\r\n>>>>>>   1 file changed, 108 insertions(+), 1 deletion(-)\r\n>>>>>>\r\n>>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c\r\n>>>>>> index 6b0cd8861b..e34b2af4eb 100644\r\n>>>>>> --- a/hw/s390x/css.c\r\n>>>>>> +++ b/hw/s390x/css.c\r\n>>>>>> @@ -819,6 +819,113 @@ incr:\r\n>>>>>>       return 0;\r\n>>>>>>   }\r\n>>>>>>\r\n>>>>>> +/* returns values between 1 and bsz, where bs is a power of 2 */\r\n>>>>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)\r\n>>>>>> +{\r\n>>>>>> +    return bsz - (cda & (bsz - 1));\r\n>>>>>> +}\r\n>>>>>> +\r\n>>>>>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)\r\n>>>>>> +{\r\n>>>>>> +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);\r\n>>>>> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will\r\n>>>>> be the result regardless the I2K flag? The logic seems wrong.\r\n>>>\r\n>>> No. If CDS_F_C64 is set then the outcome depends on the fact if\r\n>>> CDS_F_I2K is set or not.\r\n>>> (flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)\r\n>>> \"(flags ^ CDS_F_C64) will be 0\" is wrong. flags ^ CDS_F_C64\r\n>>> just flips the CDS_F_C64.\r\n>>>\r\n>>> OTOH if the CDS_F_C64 was not set we have the corresponding\r\n>>> bit set in flags ^ CDS_F_C64 so then the  CDS_F_I2K bit does\r\n>>> not matter: we have 1ULL << 11.\r\n>>>\r\n>>> In my reading the logic is good.\r\n>>\r\n>> So I'll just leave it...\r\n>>\r\n>>>\r\n>>>>\r\n>>>> I've stared at that condition now for a bit, but all it managed was to\r\n>>>> get me more confused... probably just need a break.\r\n>>>>   \r\n>>>>>\r\n>>>>> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic\r\n>>>>> here should be:\r\n>>>>> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {\r\n>>>>>      return 1ULL << 12;\r\n>>>>> }\r\n>>>>>      return 1ULL << 11;\r\n>>>>\r\n>>>> But I do think your version is more readable... >>>\r\n>>>\r\n>>> I won't argue with this.\r\n> \r\n> :)\r\n> \r\n>>\r\n>> ...and we could change that in a patch on top to avoid future confusion.\r\n>>\r\n>>>\r\n>>>>>  \r\n>>>>>> +}\r\n>>>>>> +\r\n>>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\r\n>>>>>> +{\r\n>>>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;\r\n>>>>>                                             ^\r\n>>>>> Nit.\r\n>>>>>   \r\n>>>\r\n>>> Maybe checkpatch wanted it this way. My memories are blurry.\r\n>>\r\n>>\r\n>> I'd just leave it like that, tbh.\r\n>>\r\n>>>\r\n>>>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;\r\n>>>>>> +    int ret;\r\n>>>>>> +    hwaddr idaw_addr;\r\n>>>>>> +\r\n>>>>>> +    if (is_fmt2) {\r\n>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\r\n>>>>>> +        if (idaw_addr & 0x07) {\r\n>>>>>> +            return -EINVAL; /* channel program check */\r\n>>>>>> +        }\r\n>>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,\r\n>>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\r\n>>>>>> +                               sizeof(idaw.fmt2), false);\r\n>>>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);\r\n>>>>>> +    } else {\r\n>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\r\n>>>>>> +        if (idaw_addr & 0x03) {\r\n>>>>> ?:\r\n>>>>> (idaw_addr & 0x80000003)\r\n>>>>\r\n>>>> Yes.\r\n>>>>    \r\n>>>\r\n>>> I will double check this. Does not seem unreasonable but\r\n>>> double-checking is better.\r\n>>\r\n>> Please let me know. I think the architecture says that the bit must be\r\n>> zero, and that we may (...) generate a channel program check.\r\n>>\r\n> \r\n> Right (0x80000003) !=0 implies program check .\r\n> It is what is done , except for the bit 0 that was forgotten.\r\n\r\nReference?\r\n\r\nI've just explained something different above (depends\r\non the ccw format and yes in case of format 1 I guess\r\nthe check can be done like that). If you are arguing with\r\nthat, please be more verbose.\r\n\r\n> \r\n>>>\r\n>>>>>  \r\n>>>>>> +            return -EINVAL; /* channel program check */\r\n>>>>>> +\r\n>>>>>> +        }\r\n>>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,\r\n>>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,\r\n>>>>>> +                               sizeof(idaw.fmt1), false);\r\n>>>>>> +        cds->cda = be64_to_cpu(idaw.fmt1);\r\n>>>>>> +    }\r\n>>>>>> +    ++(cds->at_idaw);\r\n>>>>>> +    if (ret != MEMTX_OK) {\r\n>>>>>> +        /* assume inaccessible address */\r\n>>>>>> +        return -EINVAL; /* channel program check */\r\n>>>>>> +\r\n>>>>>> +    }\r\n>>>>>> +    return 0;\r\n>>>>>> +}\r\n>>>>>> +\r\n>>>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,\r\n>>>>>> +                              CcwDataStreamOp op)\r\n>>>>>> +{\r\n>>>>>> +    uint64_t bsz = ccw_ida_block_size(cds->flags);\r\n>>>>>> +    int ret = 0;\r\n>>>>>> +    uint16_t cont_left, iter_len;\r\n>>>>>> +\r\n>>>>>> +    ret = cds_check_len(cds, len);\r\n>>>>>> +    if (ret <= 0) {\r\n>>>>>> +        return ret;\r\n>>>>>> +    }\r\n>>>>>> +    if (!cds->at_idaw) {\r\n>>>>>> +        /* read first idaw */\r\n>>>>>> +        ret = ida_read_next_idaw(cds);\r\n>>>>>> +        if (ret) {\r\n>>>>>> +            goto err;\r\n>>>>>> +        }\r\n>>>>>> +        cont_left = ida_continuous_left(cds->cda, bsz);\r\n>>>>>> +    } else {\r\n>>>>>> +        cont_left = ida_continuous_left(cds->cda, bsz);\r\n>>>>>> +        if (cont_left == bsz) {\r\n>>>>>> +            ret = ida_read_next_idaw(cds);\r\n>>>>>> +            if (ret) {\r\n>>>>>> +                goto err;\r\n>>>>>> +            }\r\n>>>>>> +            if (cds->cda & (bsz - 1)) {\r\n>>>>> Could move this check into ida_read_next_idaw?\r\n>>>>\r\n>>>> I'd like to avoid further code movement...\r\n>>>>    \r\n>>>\r\n>>> The first idaw is special. I don't think moving is possible.\r\n>>\r\n>> So, the code is correct and I'll just leave it like that.\r\n> \r\n> hum.\r\n> It seems to me that the handling of the first IDAW is indeed a little bit different.\r\n> It is accessed directly from the CCW->CDA and generated dedicated status but I do not see the handling.\r\n> \r\n> The channel status must be made pending with primary, secondary and alert status.\r\n>\r\n\r\nReference?\r\n \r\n> I do not find this code, may be it is somewhere before, I searched but did not find it.\r\n> Also, I do not find in the documentation that we have a program check for this case.\r\n> \r\n\r\nI don't understand a thing. Aren't you confusing this with some internal\r\ndiscussion?\r\n\r\nSorry, but I think, I will ignore the comment for now.\r\n\r\n>>\r\n>>>\r\n>>>>>  \r\n>>>>>> +                ret = -EINVAL; /* channel program check */\r\n>>>>>> +                goto err;\r\n>>>>>> +            }\r\n>>>>>> +        }\r\n>>>>>> +    }\r\n>>>>>> +    do {\r\n>>>>>> +        iter_len = MIN(len, cont_left);\r\n>>>>>> +        if (op != CDS_OP_A) {\r\n>>>>>> +            ret = address_space_rw(&address_space_memory, cds->cda,\r\n>>>>>> +                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);\r\n>>>>> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to\r\n>>>>> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to\r\n>>>>> make it more obvious by adding some comment there?\r\n>>>>\r\n>>>> Would you have a good text for that?\r\n>>>>    \r\n>>>\r\n>>> I'm fine with clarifications.\r\n>>\r\n>> Let's do it as a patch on top.\r\n>>\r\n>>>\r\n>>>>>  \r\n>>>>>> +            if (ret != MEMTX_OK) {\r\n>>>>>> +                /* assume inaccessible address */\r\n>>>>>> +                ret = -EINVAL; /* channel program check */\r\n>>>>>> +                goto err;\r\n>>>>>> +            }\r\n>>>>>> +        }\r\n>>>>>> +        cds->at_byte += iter_len;\r\n>>>>>> +        cds->cda += iter_len;\r\n>>>>>> +        len -= iter_len;\r\n>>>>>> +        if (!len) {\r\n>>>>>> +            break;\r\n>>>>>> +        }\r\n>>>>>> +        ret = ida_read_next_idaw(cds);\r\n>>>>>> +        if (ret) {\r\n>>>>>> +            goto err;\r\n>>>>>> +        }\r\n>>>>>> +        cont_left = bsz;\r\n>>>>>> +    } while (true);\r\n>>>>>> +    return ret;\r\n>>>>>> +err:\r\n>>>>>> +    cds->flags |= CDS_F_STREAM_BROKEN;\r\n>>>>>> +    return ret;\r\n>>>>>> +}\r\n>>>>>> +\r\n>>>>>>   void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\r\n>>>>>>   {\r\n>>>>>>       /*\r\n>>>>>> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)\r\n>>>>>>       if (!(cds->flags & CDS_F_IDA)) {\r\n>>>>>>           cds->op_handler = ccw_dstream_rw_noflags;\r\n>>>>>>       } else {\r\n>>>>>> -        assert(false);\r\n>>>>>> +        cds->op_handler = ccw_dstream_rw_ida;\r\n>>>>>>       }\r\n>>>>>>   }\r\n>>>>>>\r\n>>>>>> -- \r\n>>>>>> 2.13.5\r\n>>>>>>      \r\n>>>>>\r\n>>>>> Generally, the logic looks fine to me.\r\n>>>>>   \r\n>>>>\r\n>>>> It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as\r\n>>>> this is what the kernel infrastructure provides.\r\n>>>\r\n>>> Nod.\r\n>>>\r\n>>>>\r\n>>>> Halil, do you have some more comments?\r\n>>>>    \r\n>>>\r\n>>> Just a question. Do I have to respin?\r\n>>\r\n>> I don't think so. If you could confirm the check for format-1, I'll\r\n>> just fixup that one and get the queued patches out of the door.\r\n> \r\n> generally LGTM but in my opinion the check for format-1 and the handling of the error status for the first IDA for format 1 and 2 must be cleared.\r\n\r\nOK, I'm not adding any r-b or ack for v3 unless told otherwise.\r\n\r\nThanks for your review!\r\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 3xxTzv6pJYz9ryr\n\tfor ;\n\tWed, 20 Sep 2017 03:16:11 +1000 (AEST)","from localhost ([::1]:44223 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duM82-0000Ea-0y\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 13:16:10 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:43011)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duLi1-0001wn-DW\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 12:49:19 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duLhy-000706-GF\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 12:49:17 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:34720\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 1duLhy-0006zW-9U\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 12:49:14 -0400","from pps.filterd (m0098417.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JGmixb141717\n\tfor ; Tue, 19 Sep 2017 12:49:13 -0400","from e06smtp14.uk.ibm.com (e06smtp14.uk.ibm.com [195.75.94.110])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d35e86bff-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 12:49:13 -0400","from localhost\n\tby e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 19 Sep 2017 17:49:11 +0100","from b06cxnps4076.portsmouth.uk.ibm.com (9.149.109.198)\n\tby e06smtp14.uk.ibm.com (192.168.101.144) with IBM ESMTP SMTP\n\tGateway: Authorized Use Only! Violators will be prosecuted; \n\tTue, 19 Sep 2017 17:49:09 +0100","from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com\n\t[9.149.105.58])\n\tby b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8JGn8De16646274; Tue, 19 Sep 2017 16:49:08 GMT","from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 1F40F4C046;\n\tTue, 19 Sep 2017 17:45:34 +0100 (BST)","from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id C2A404C040;\n\tTue, 19 Sep 2017 17:45:33 +0100 (BST)","from oc3836556865.ibm.com (unknown [9.152.224.207])\n\tby d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP;\n\tTue, 19 Sep 2017 17:45:33 +0100 (BST)"],"To":"Pierre Morel ,\n\tCornelia Huck ","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>\n\t<0c6660b7-9d81-7ada-7388-cfdaf89e25f3@linux.vnet.ibm.com>","From":"Halil Pasic ","Date":"Tue, 19 Sep 2017 18:49:08 +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":"<0c6660b7-9d81-7ada-7388-cfdaf89e25f3@linux.vnet.ibm.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-TM-AS-GCONF":"00","x-cbid":"17091916-0016-0000-0000-000004EE6F68","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091916-0017-0000-0000-00002828A3B4","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_07:, , 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-1709190238","Content-Transfer-Encoding":"base64","X-MIME-Autoconverted":"from 8bit to base64 by mx0a-001b2d01.pphosted.com id\n\tv8JGmixb141717","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 4/4] s390x/css: support ccw IDA","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":1771265,"web_url":"http://patchwork.ozlabs.org/comment/1771265/","msgid":"<07b62387-ded5-ab32-e493-d3ffabc5c21b@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-19T18:05:48","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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 02:23 PM, Cornelia Huck wrote:\n>>>>> +{\n>>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n>>>> ^\n>>>> Nit.\n>>>> \n>> Maybe checkpatch wanted it this way. My memories are blurry. \n> I'd just leave it like that, tbh.\n\nYes, if I remove the space before the } checkpatch.pl complains.\n\nGoing to keep it as is for v3.\n\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 3xxW8y3g6Qz9s7c\n\tfor ;\n\tWed, 20 Sep 2017 04:09:06 +1000 (AEST)","from localhost ([::1]:44682 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duMxE-0003Z5-BX\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 14:09:04 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:39775)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duMuM-0001kL-UN\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 14:06:07 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duMuH-0005xb-JI\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 14:06:04 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:33992\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 1duMuH-0005pj-Dj\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 14:06:01 -0400","from pps.filterd (m0098419.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JI5XDo118394\n\tfor ; Tue, 19 Sep 2017 14:05:57 -0400","from e06smtp10.uk.ibm.com (e06smtp10.uk.ibm.com [195.75.94.106])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2d35np11ef-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 14:05:57 -0400","from localhost\n\tby e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 19 Sep 2017 19:05:50 +0100","from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196)\n\tby e06smtp10.uk.ibm.com (192.168.101.140) with IBM ESMTP SMTP\n\tGateway: Authorized Use Only! Violators will be prosecuted; \n\tTue, 19 Sep 2017 19:05:49 +0100","from d06av26.portsmouth.uk.ibm.com (d06av26.portsmouth.uk.ibm.com\n\t[9.149.105.62])\n\tby b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8JI5nXJ19202204; Tue, 19 Sep 2017 18:05:49 GMT","from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 3AEABAE04D;\n\tTue, 19 Sep 2017 19:00:43 +0100 (BST)","from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id BC557AE045;\n\tTue, 19 Sep 2017 19:00:42 +0100 (BST)","from oc3836556865.ibm.com (unknown [9.152.224.207])\n\tby d06av26.portsmouth.uk.ibm.com (Postfix) with ESMTP;\n\tTue, 19 Sep 2017 19:00:42 +0100 (BST)"],"To":"Cornelia Huck ","References":"<20170913115029.47626-1-pasic@linux.vnet.ibm.com>\n\t<20170913115029.47626-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>\n\t<9872cee8-3503-8d52-c68f-d9c61143201c@linux.vnet.ibm.com>\n\t<20170919142351.1e3b66d8.cohuck@redhat.com>","From":"Halil Pasic ","Date":"Tue, 19 Sep 2017 20:05:48 +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":"<20170919142351.1e3b66d8.cohuck@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-TM-AS-GCONF":"00","x-cbid":"17091918-0040-0000-0000-000003DB7222","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17091918-0041-0000-0000-000025DCA415","Message-Id":"<07b62387-ded5-ab32-e493-d3ffabc5c21b@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_08:, , 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-1709190254","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 4/4] s390x/css: support ccw IDA","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":1771491,"web_url":"http://patchwork.ozlabs.org/comment/1771491/","msgid":"<20170920013720.GD11080@bjsdjshi@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-20T01:37:20","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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 20:05:48 +0200]:\n\n> \n> \n> On 09/19/2017 02:23 PM, Cornelia Huck wrote:\n> >>>>> +{\n> >>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n> >>>> ^\n> >>>> Nit.\n> >>>> \n> >> Maybe checkpatch wanted it this way. My memories are blurry. \n> > I'd just leave it like that, tbh.\n> \n> Yes, if I remove the space before the } checkpatch.pl complains.\n> \n> Going to keep it as is for v3.\nMy bad. :-/\n\n> \n> Halil","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 3xxj6s59Zfz9s8J\n\tfor ;\n\tWed, 20 Sep 2017 11:37:56 +1000 (AEST)","from localhost ([::1]:46197 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duTxY-00055g-BD\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 21:37:52 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:41320)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duTxD-00055F-IH\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 21:37:32 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duTx9-0000dZ-Hj\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 21:37:31 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:33660\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 1duTx9-0000dN-CB\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 21:37:27 -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\tv8K1YNlm101227\n\tfor ; Tue, 19 Sep 2017 21:37:25 -0400","from e38.co.us.ibm.com (e38.co.us.ibm.com [32.97.110.159])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2d3cpfe7yj-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 19 Sep 2017 21:37:25 -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\tTue, 19 Sep 2017 19:37:25 -0600","from b03cxnp07029.gho.boulder.ibm.com (9.17.130.16)\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\tTue, 19 Sep 2017 19:37:22 -0600","from b03ledav004.gho.boulder.ibm.com\n\t(b03ledav004.gho.boulder.ibm.com [9.17.130.235])\n\tby b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8K1bMpK8389078; Tue, 19 Sep 2017 18:37:22 -0700","from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 2F5E578047;\n\tTue, 19 Sep 2017 19:37:22 -0600 (MDT)","from localhost (unknown [9.115.112.23])\n\tby b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP id 7B18D78038;\n\tTue, 19 Sep 2017 19:37:21 -0600 (MDT)"],"Date":"Wed, 20 Sep 2017 09:37:20 +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-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>\n\t<9872cee8-3503-8d52-c68f-d9c61143201c@linux.vnet.ibm.com>\n\t<20170919142351.1e3b66d8.cohuck@redhat.com>\n\t<07b62387-ded5-ab32-e493-d3ffabc5c21b@linux.vnet.ibm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<07b62387-ded5-ab32-e493-d3ffabc5c21b@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-0028-0000-0000-000008620907","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.00919527; UDB=6.00461970;\n\tIPR=6.00699753; \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:37:24","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17092001-0029-0000-0000-0000379EDD34","Message-Id":"<20170920013720.GD11080@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-1709200019","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 4/4] s390x/css: support ccw IDA","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":1771618,"web_url":"http://patchwork.ozlabs.org/comment/1771618/","msgid":"<20170920064058.GE11080@bjsdjshi@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-20T06:40:58","subject":"Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA","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 14:04:03 +0200]:\n\nI have no problem with the rest parts of the discussion in this thread.\n\n> \n> \n> On 09/19/2017 12:57 PM, Cornelia Huck wrote:\n> >>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)\n> >>>>> +{\n> >>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; \n> >>>> ^\n> >>>> Nit.\n> >>>> \n> >> Maybe checkpatch wanted it this way. My memories are blurry.\n> > \n> > I'd just leave it like that, tbh.\n> > \n> >>>>> + bool is_fmt2 = cds->flags & CDS_F_C64;\n> >>>>> + int ret;\n> >>>>> + hwaddr idaw_addr;\n> >>>>> +\n> >>>>> + if (is_fmt2) {\n> >>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;\n> >>>>> + if (idaw_addr & 0x07) {\n> >>>>> + return -EINVAL; /* channel program check */\n> >>>>> + }\n> >>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,\n> >>>>> + sizeof(idaw.fmt2), false);\n> >>>>> + cds->cda = be64_to_cpu(idaw.fmt2);\n> \n> \n> >>>>> + } else {\n> >>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;\n> >>>>> + if (idaw_addr & 0x03) { \n> >>>> ?:\n> >>>> (idaw_addr & 0x80000003) \n> >>> Yes.\n> >>> \n> >> I will double check this. Does not seem unreasonable but\n> >> double-checking is better.\n> > Please let me know. I think the architecture says that the bit must be\n> > zero, and that we may (...) generate a channel program check.\n> > \nMy fault... This is the address of an IDAW, not the content (data\naddress) in an IDAW. So what Halil pointed out is the right direction to\ngo I think.\n\nI will review in the thread of the new version (v3).\n\n> \n> Not exactly. The more significant bits part of the check\n> depend on the ccw format. This needs to be done for both\n> idaw formats. I would need to introduce a new flag, or\n> access the SubchDev to do this properly.\n> \n> Architecturally we also need to check the data addresses\n> from which we read so we have nothing bigger than \n> (1 << 31) - 1 if we are working with format-1 idaws.\nRight. This is what I actually wanted to say.\n\n> \n> I also think we did not take proper care of proper\n> maximum data address checks prior CwwDataStream which also\n> depend on the ccw format (in absence of IDAW or MIDAW).\n> \n> The ccw format dependent maximum address checks are (1 << 24) - 1\n> and (1 << 31) - 1 respectively for format-0 and format-1 (on\n> the first indirection level that is for non-IDA for the data,\n> and for (M)IDA for the (M)IDAWs).\n> \n> Reference:\n> PoP pages 16-25 and 16-26 \"Invalid IDAW or MIDAW Addre\" and\n> \"Invalid Data Address\".\n> \n> How shall we proceed?\n> \n> Halil\n> \n> >>>> \n> >>>>> + return -EINVAL; /* channel program check */\n> >>>>> +\n> >>>>> + }\n> >>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,\n> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,\n> >>>>> + sizeof(idaw.fmt1), false);\n> >>>>> + cds->cda = be64_to_cpu(idaw.fmt1);>>>>> + }\n> >>>>> + ++(cds->at_idaw);\n> >>>>> + if (ret != MEMTX_OK) {\n> >>>>> + /* assume inaccessible address */\n> >>>>> + return -EINVAL; /* channel program check */\n> >>>>> +\n> >>>>> + }\n> >>>>> + return 0;\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 3xxqsQ00hFz9s7f\n\tfor ;\n\tWed, 20 Sep 2017 16:41:45 +1000 (AEST)","from localhost ([::1]:47020 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duYhc-00036b-4H\n\tfor incoming@patchwork.ozlabs.org; Wed, 20 Sep 2017 02:41:44 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:54504)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duYh4-000360-6P\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 02:41:11 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duYgz-0003zA-58\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 02:41:10 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:38920\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 1duYgy-0003y3-Vm\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 02:41:05 -0400","from pps.filterd (m0098417.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8K6dYux019938\n\tfor ; Wed, 20 Sep 2017 02:41:04 -0400","from e17.ny.us.ibm.com (e17.ny.us.ibm.com [129.33.205.207])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d3brrqpvs-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Wed, 20 Sep 2017 02:41:03 -0400","from localhost\n\tby e17.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tWed, 20 Sep 2017 02:41:03 -0400","from b01cxnp23034.gho.pok.ibm.com (9.57.198.29)\n\tby e17.ny.us.ibm.com (146.89.104.204) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tWed, 20 Sep 2017 02:41:01 -0400","from b01ledav005.gho.pok.ibm.com (b01ledav005.gho.pok.ibm.com\n\t[9.57.199.110])\n\tby b01cxnp23034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP\n\tid v8K6f0Ho40829150; Wed, 20 Sep 2017 06:41:00 GMT","from b01ledav005.gho.pok.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 038D3AE043;\n\tWed, 20 Sep 2017 02:41:32 -0400 (EDT)","from localhost (unknown [9.115.112.23])\n\tby b01ledav005.gho.pok.ibm.com (Postfix) with ESMTP id 2D0C5AE03B;\n\tWed, 20 Sep 2017 02:41:30 -0400 (EDT)"],"Date":"Wed, 20 Sep 2017 14:40:58 +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-5-pasic@linux.vnet.ibm.com>\n\t<20170919055005.GE5274@bjsdjshi@linux.vnet.ibm.com>\n\t<20170919114859.3e2d895b.cohuck@redhat.com>\n\t\n\t<20170919125717.1ae0cd38.cohuck@redhat.com>\n\t<9872cee8-3503-8d52-c68f-d9c61143201c@linux.vnet.ibm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<9872cee8-3503-8d52-c68f-d9c61143201c@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":"17092006-0040-0000-0000-000003A4FDF3","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007766; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000230; SDB=6.00919628; UDB=6.00462027;\n\tIPR=6.00699854; \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.00017219;\n\tXFM=3.00000015; UTC=2017-09-20 06:41:02","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17092006-0041-0000-0000-0000079A01F1","Message-Id":"<20170920064058.GE11080@bjsdjshi@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-20_01:, , 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-1709200091","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 4/4] s390x/css: support ccw IDA","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"}}]