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

[Frank Blaschka <blaschka@linux.vnet.ibm.com>]

On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> >
> >> On Tue, 20 Jan 2015 10:45:41 +0100
> >> Markus Armbruster <armbru@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

> >
> > 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.

How can I avoid the sign extension and make Coverity happy?

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