From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58538) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dupha-0002Uq-7y for qemu-devel@nongnu.org; Wed, 20 Sep 2017 20:50:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1duphX-0002Aj-3j for qemu-devel@nongnu.org; Wed, 20 Sep 2017 20:50:50 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:53774) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1duphW-00029m-IY for qemu-devel@nongnu.org; Wed, 20 Sep 2017 20:50:46 -0400 Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v8L0mXgc025115 for ; Wed, 20 Sep 2017 20:50:44 -0400 Received: from e13.ny.us.ibm.com (e13.ny.us.ibm.com [129.33.205.203]) by mx0a-001b2d01.pphosted.com with ESMTP id 2d3wmrgxt8-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 20 Sep 2017 20:50:44 -0400 Received: from localhost by e13.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 20 Sep 2017 20:50:42 -0400 Date: Thu, 21 Sep 2017 08:50:36 +0800 From: Dong Jia Shi References: <20170919182745.90280-1-pasic@linux.vnet.ibm.com> <20170919182745.90280-6-pasic@linux.vnet.ibm.com> <20170920074238.GH11080@bjsdjshi@linux.vnet.ibm.com> <20170920103312.50b8b69b.cohuck@redhat.com> <009428a2-fb31-ecc9-d477-4cdea226f2d8@linux.vnet.ibm.com> <20170920131844.066e53d7.cohuck@redhat.com> <268904f1-0b31-3255-02da-c3248abef6a2@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <268904f1-0b31-3255-02da-c3248abef6a2@linux.vnet.ibm.com> Message-Id: <20170921005036.GL11080@bjsdjshi@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Halil Pasic Cc: Cornelia Huck , Dong Jia Shi , Pierre Morel , qemu-devel@nongnu.org * Halil Pasic [2017-09-20 18:46:57 +0200]: > > > On 09/20/2017 01:18 PM, Cornelia Huck wrote: > > On Wed, 20 Sep 2017 13:13:01 +0200 > > Halil Pasic wrote: > > > >> On 09/20/2017 10:33 AM, Cornelia Huck wrote: > >>> On Wed, 20 Sep 2017 15:42:38 +0800 > >>> Dong Jia Shi wrote: > >>> > >>>> * Halil Pasic [2017-09-19 20:27:45 +0200]: > > > >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, > >>>>> + sizeof(idaw.fmt2), false); > >>>>> + cds->cda = be64_to_cpu(idaw.fmt2); > >>>>> + } else { > >>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; > >>>>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > >>>>> + return -EINVAL; /* channel program check */ > >>>>> + } > >>>>> + ret = address_space_rw(&address_space_memory, idaw_addr, > >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, > >>>>> + sizeof(idaw.fmt1), false); > >>>>> + cds->cda = be64_to_cpu(idaw.fmt1); > >>>> Still need to check bit 0x80000000 here I think. > >>> > >>> Yes, I think this is 'must be zero' for format-1 idaws, and not covered > >>> by the ccw-format specific checks above. (Although the PoP can be a bit > >>> confusing with many similar terms...) > >>> > >> > >> It's taken care of in ccw_dstream_rw_ida before the actual > >> access happens. Code looks like this: > >> + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) { > >> + ret = -EINVAL; /* channel program check */ > >> + goto err; > >> + } > >> > >> The idea was to have it similar to the non-indirect case. > > > > > > > > Ah, I was simply looking for the wrong pattern. Looks correct. > > > > > > Thinking about this some more. Since in case of IDA we are guaranteed > to never cross a block boundary with a single IDAW we won't ever cross > block boundary. So we can do the check in ida_read_next_idaw by checking > bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1 > local to ida_read_next_idaw and save one goto err. I think that would > look a bit nicer than what I have here in v3. Agree? Agree. That would also do the check in the first place. Sounds better. > > >>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len, > >>>>> + CcwDataStreamOp op) > >>>>> +{ > >>>>> + uint64_t bsz = ccw_ida_block_size(cds->flags); > >>>>> + int ret = 0; > >>>>> + uint16_t cont_left, iter_len; > >>>>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64; > >>>>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT; > >>>> Use 'const bool' either? Although I doubt the value of using const here. > >>>> ;) > >>> > >>> Both being the same is still a good idea. > >>> > >> > >> Yeah. For which one should I go (with const or without)? > > > > For the one you prefer :) (I'm not sure if the const adds value here.) > > > > I think we generally don't care about const-ness in such situations, > so I think I won't use consts. > > I intend to fix the issues we have found and do a v4 tomorrow, unless > somebody screams -- could do it today but I would like to give Dong > Jia an opportunity to react. Thanks. I'm coming. :) > On the other hand waiting more that that will IMHO do us no favor > either (I think of our storage/memory hierarchy). > > Regards, > Halil -- Dong Jia Shi