[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: |
Wed, 21 Jan 2015 14:41:25 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Peter Maydell <address@hidden> writes:
> On 21 January 2015 at 11:54, Markus Armbruster <address@hidden> wrote:
>> 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:
>>>>> > 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 :)
- [Qemu-devel] [PATCH 3/3 V3] kvm: extend kvm_irqchip_add_msi_route to work on s390, (continued)
- [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, 2015/01/20
- 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 <=
- 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