Markus Armbruster <arm...@redhat.com> writes:
> Frank Blaschka <blasc...@linux.vnet.ibm.com> writes:
>
>> On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote:
>>> Markus Armbruster <arm...@redhat.com> writes:
>>>
>>> > Cornelia Huck <cornelia.h...@de.ibm.com> writes:
>>> >
>>> >> On Tue, 20 Jan 2015 10:45:41 +0100
>>> >> Markus Armbruster <arm...@redhat.com> 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) {
