Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Frank Blaschka <address@hidden> writes:
>
>> On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote:
>>> Markus Armbruster <address@hidden> writes:
>>> 
>>> > Cornelia Huck <address@hidden> writes:
>>> >
>>> >> On Tue, 20 Jan 2015 10:45:41 +0100
>>> >> Markus Armbruster <address@hidden> 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
>
> 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.
>>>
>>
>> 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.
>
> Okay.
>
>> How can I avoid the sign extension and make Coverity happy?
>
> Casting pbdev->routes.adapter.ind_offset to uint32_t should do.  Then,
> all operands of | are either int (promoted from narrower unsigned type)
> or uint32_t (type cast).  Conversion from int to uint32_t won't
> sign-extend as long as int is at least 32 bits.  Surely the case for
> anything that can run QEMU.

Yup, it hushes up Coverity (I checked).

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) {