[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions |
Date: |
Tue, 20 Jan 2015 21:24:37 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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.
>> > 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
>>
- [Qemu-devel] [PATCH 0/3 V3] add PCI support for the s390 platform, Frank Blaschka, 2015/01/09
- [Qemu-devel] [PATCH 3/3 V3] kvm: extend kvm_irqchip_add_msi_route to work on s390, Frank Blaschka, 2015/01/09
- [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Frank Blaschka, 2015/01/09
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Markus Armbruster, 2015/01/20
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Cornelia Huck, 2015/01/20
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Markus Armbruster, 2015/01/20
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Markus Armbruster, 2015/01/20
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Frank Blaschka, 2015/01/20
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Markus Armbruster, 2015/01/21
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Peter Maydell, 2015/01/21
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Markus Armbruster, 2015/01/21
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Peter Maydell, 2015/01/21
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Paolo Bonzini, 2015/01/21
- Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions, Cornelia Huck, 2015/01/21
[Qemu-devel] [PATCH 1/3 V3] s390: Add PCI bus support, Frank Blaschka, 2015/01/09
Re: [Qemu-devel] [PATCH 0/3 V3] add PCI support for the s390 platform, Cornelia Huck, 2015/01/09