From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49026) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDvXQ-0002Gn-Ca for qemu-devel@nongnu.org; Wed, 21 Jan 2015 08:41:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YDvXN-0005rg-4A for qemu-devel@nongnu.org; Wed, 21 Jan 2015 08:41:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57293) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDvXM-0005ra-TR for qemu-devel@nongnu.org; Wed, 21 Jan 2015 08:41:37 -0500 From: Markus Armbruster 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> <20150120142052.GA44419@tuxmaker.boeblingen.de.ibm.com> <87bnltryii.fsf@blackfin.pond.sub.org> <87twzkmjqx.fsf@blackfin.pond.sub.org> Date: Wed, 21 Jan 2015 14:41:25 +0100 In-Reply-To: (Peter Maydell's message of "Wed, 21 Jan 2015 13:12:36 +0000") Message-ID: <87mw5cfdyy.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Peter Maydell Cc: Cornelia Huck , Christian Borntraeger , Frank Blaschka , Frank Blaschka , QEMU Developers Peter Maydell writes: > On 21 January 2015 at 11:54, Markus Armbruster wrote: >> Markus Armbruster writes: >> >>> Frank Blaschka writes: >>> >>>> On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote: >>>>> Markus Armbruster writes: >>>>> > 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 >>> >>> Yes, that's what I meant :) >>> >>>>> > >>>>> > 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. >>>>> > >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >> index 5ea13e5..2bed3f5 100644 >> --- a/hw/s390x/s390-pci-inst.c >> +++ b/hw/s390x/s390-pci-inst.c >> @@ -785,8 +785,8 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t >> r1, uint64_t fiba) >> stq_p(&fib.fmb_addr, pbdev->fmb_addr); >> >> data = (pbdev->isc << 28) | (pbdev->noi << 16) | >> - (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) | >> - pbdev->routes.adapter.summary_offset; >> + ((uint32_t)pbdev->routes.adapter.ind_offset << 8) | >> + (pbdev->sum << 7) | pbdev->routes.adapter.summary_offset; >> stw_p(&fib.data, data); >> >> if (pbdev->fh >> ENABLE_BIT_OFFSET) { >> > > This doesn't make sense to me as a fix for the problem you describe > above. Either > (1) pbdev->isc may have bit 3 set: in this case shifting it left > by 28 is undefined behaviour in C, Correct. > and we must not do it I suspect we shift signed values all over the place, without regard for signed overflow. Machines are fine with that, but some day some compiler wiseguy may find a way to save a femtosecond or two for some program that never does that, breaking programs that do it, and then we'll be in trouble. We should follow the kernel's lead and compile with -fno-strict-overflow. > (and adding a cast to ind_offset doesn't help us at all) Correct, it doesn't help with the signed left shift of pbdev->isc. > (2) pbdev->isc is guaranteed never to have bit 3 set: in this > case the sign extension to uint64_t in step 6 above will > have no effect, because the sign bit in the int result will > be clear > > So you can either: > (1) cast pbdev->isc to uint32_t before shifting, thus ensuring that > we do all our | operations on unsigned types and that we won't > shift into the sign bit regardless of pbdev->isc's value > (2) state that we know pbdev->isc is always less than 8 and so this > is a coverity false positive to be suppressed via the web UI > > But the patch you have doesn't seem like the right thing to me. Frank's code, Frank's choice :)