From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53390) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDrvD-0004xJ-JD for qemu-devel@nongnu.org; Wed, 21 Jan 2015 04:50:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YDrvA-00005C-7t for qemu-devel@nongnu.org; Wed, 21 Jan 2015 04:49:59 -0500 Received: from e06smtp11.uk.ibm.com ([195.75.94.107]:50391) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDrv9-0008WR-V5 for qemu-devel@nongnu.org; Wed, 21 Jan 2015 04:49:56 -0500 Received: from /spool/local by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 21 Jan 2015 09:49:53 -0000 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id BFC531B0806B for ; Wed, 21 Jan 2015 09:49:52 +0000 (GMT) Received: from d06av11.portsmouth.uk.ibm.com (d06av11.portsmouth.uk.ibm.com [9.149.37.252]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t0L9nptk54395034 for ; Wed, 21 Jan 2015 09:49:51 GMT Received: from d06av11.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av11.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t0L9nobZ000658 for ; Wed, 21 Jan 2015 02:49:51 -0700 Date: Wed, 21 Jan 2015 10:49:49 +0100 From: Cornelia Huck Message-ID: <20150121104949.6bb9af74.cornelia.huck@de.ibm.com> In-Reply-To: <87ppa9iqco.fsf@blackfin.pond.sub.org> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: borntraeger@de.ibm.com, Frank Blaschka , Frank Blaschka , qemu-devel@nongnu.org On Tue, 20 Jan 2015 13:33:27 +0100 Markus Armbruster wrote: > 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. What, I am expected to actually read the explanations? :) > Regarding the leak, I prefer my patch, because it avoids the free on > error. But you're the maintainer. Indeed, that's a good point. I'll drop Frank's original patch and instead take your memory leak fix. Will take a patch from Frank for the sign extension stuff (and the stw/stl fix) as well once it has been posted.