From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48881) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDZge-0007tC-Ge for qemu-devel@nongnu.org; Tue, 20 Jan 2015 09:21:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YDZgZ-0001yr-La for qemu-devel@nongnu.org; Tue, 20 Jan 2015 09:21:44 -0500 Received: from e06smtp11.uk.ibm.com ([195.75.94.107]:48883) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDZgZ-0001or-CF for qemu-devel@nongnu.org; Tue, 20 Jan 2015 09:21:39 -0500 Received: from /spool/local by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 20 Jan 2015 14:20:57 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 2343017D8042 for ; Tue, 20 Jan 2015 14:20:56 +0000 (GMT) Received: from d06av04.portsmouth.uk.ibm.com (d06av04.portsmouth.uk.ibm.com [9.149.37.216]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t0KEKsP559965516 for ; Tue, 20 Jan 2015 14:20:54 GMT Received: from d06av04.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av04.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t0KEKrNN018579 for ; Tue, 20 Jan 2015 07:20:53 -0700 Date: Tue, 20 Jan 2015 15:20:52 +0100 From: Frank Blaschka Message-ID: <20150120142052.GA44419@tuxmaker.boeblingen.de.ibm.com> References: <1420790680-3266-1-git-send-email-blaschka@linux.vnet.ibm.com> <1420790680-3266-3-git-send-email-blaschka@linux.vnet.ibm.com> <87h9vln5tm.fsf@blackfin.pond.sub.org> <20150120110348.66afc423.cornelia.huck@de.ibm.com> <87ppa9iqco.fsf@blackfin.pond.sub.org> <87bnlthaqe.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87bnlthaqe.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Cornelia Huck , borntraeger@de.ibm.com, Frank Blaschka , qemu-devel@nongnu.org On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote: > Markus Armbruster writes: > > > Cornelia Huck writes: > > > >> On Tue, 20 Jan 2015 10:45:41 +0100 > >> Markus Armbruster wrote: > >> > >>> This patch makes Coverity unhappy: > >>> > >>> *** CID 1264326: Unintended sign extension (SIGN_EXTENSION) > >>> /hw/s390x/s390-pci-inst.c: 787 in stpcifc_service_call() > >>> 781 stq_p(&fib.pal, pbdev->pal); > >>> 782 stq_p(&fib.iota, pbdev->g_iota); > >>> 783 stq_p(&fib.aibv, pbdev->routes.adapter.ind_addr); > >>> 784 stq_p(&fib.aisb, pbdev->routes.adapter.summary_addr); > >>> 785 stq_p(&fib.fmb_addr, pbdev->fmb_addr); > >>> 786 > >>> >>> CID 1264326: Unintended sign extension (SIGN_EXTENSION) > >>> >>> Suspicious implicit sign extension: "pbdev->isc" with type > >>> >>> "unsigned char" (8 bits, unsigned) is promoted in "(pbdev->isc << > >>> >>> 28) | (pbdev->noi << 16)" to type "int" (32 bits, signed), then > >>> >>> sign-extended to type "unsigned long" (64 bits, unsigned). If > >>> >>> "(pbdev->isc << 28) | (pbdev->noi << 16)" is greater than > >>> >>> 0x7FFFFFFF, the upper bits of the result will all be 1. > >>> 787 data = (pbdev->isc << 28) | (pbdev->noi << 16) | > >>> 788 (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) | > >>> 789 pbdev->routes.adapter.summary_offset; > >>> 790 stw_p(&fib.data, data); > >>> 791 > >>> 792 if (pbdev->fh >> ENABLE_BIT_OFFSET) { > >> > >> There's a fix for this (and the memory leak): > >> > >> http://marc.info/?l=qemu-devel&m=142124886620078&w=2 > >> > >> The patch is sitting in my queue, will send with the next pile of s390x > >> updates. > > > > I can't see how > > > > @@ -787,7 +787,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba) > > data = (pbdev->isc << 28) | (pbdev->noi << 16) | > > (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) | > > pbdev->routes.adapter.summary_offset; > > - stw_p(&fib.data, data); > > + stl_p(&fib.data, data); > > > > if (pbdev->fh >> ENABLE_BIT_OFFSET) { > > fib.fc |= 0x80; > > > > fixes the implicit sign extension within the assignment preceding it. > > Let me explain it again real slow: > > > > 1. pbdev->isc gets promoted from uint8_t to int as operand of binary << > > (usual arithmetic conversions ISO/IEC 9899:1999 6.3.1.8) > > > > 2. The int result is shifted left 28 bits. This can set the MSB. > > > > 3. Likewise: pbdev->noi gets promoted from uint64_t to int, and shifted > > left 16 bits. uint16_t to int > > > > 4. The two shift results stay int and get ored. > > > > 5. pbdev->routes.adapter.ind_offset stays uint64_t, and is shifted left > > 8 bits. > > > > 6. The next or's left operand is the int result of 4 and the right > > operant is the uint64_t result of 5. Therefore, the left operand is > > *sign-extended* from int to uint64_t. This copies bit#7 of > > pbdev->isc to bits#31..63. Whoops. > > I neglected to say: we don't currently use the upper 32 bits, and as > long as we do that, the sign extension is harmless. I'd recommend to > avoid it all the same, for robustness, and to hush up Coverity. > Hi Markus, thx for your explanation. I did not see a problem since ISC is not bigger than 0x7 so MSB is never set. But the time I wrote the code I was not aware of ind_offset is uint64_t since zpci defines only a 6 bit field for this value. How can I avoid the sign extension and make Coverity happy? > > Regarding the leak, I prefer my patch, because it avoids the free on > > error. But you're the maintainer. This is fine for me as well ... Thx, Frank >