From: Frank Blaschka <blaschka@linux.vnet.ibm.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>,
borntraeger@de.ibm.com,
Frank Blaschka <frank.blaschka@de.ibm.com>,
qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions
Date: Tue, 20 Jan 2015 15:20:52 +0100 [thread overview]
Message-ID: <20150120142052.GA44419@tuxmaker.boeblingen.de.ibm.com> (raw)
In-Reply-To: <87bnlthaqe.fsf@blackfin.pond.sub.org>
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
>
next prev parent reply other threads:[~2015-01-20 14:21 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-09 8:04 [Qemu-devel] [PATCH 0/3 V3] add PCI support for the s390 platform Frank Blaschka
2015-01-09 8:04 ` [Qemu-devel] [PATCH 1/3 V3] s390: Add PCI bus support Frank Blaschka
2015-01-09 11:54 ` Cornelia Huck
2015-01-09 8:04 ` [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions Frank Blaschka
2015-01-20 9:45 ` Markus Armbruster
2015-01-20 10:03 ` Cornelia Huck
2015-01-20 12:33 ` Markus Armbruster
2015-01-20 12:56 ` Markus Armbruster
2015-01-20 14:20 ` Frank Blaschka [this message]
2015-01-20 20:24 ` Markus Armbruster
2015-01-21 11:54 ` Markus Armbruster
2015-01-21 13:12 ` Peter Maydell
2015-01-21 13:41 ` Markus Armbruster
2015-01-21 14:41 ` Peter Maydell
2015-01-21 15:32 ` Paolo Bonzini
2015-01-21 9:49 ` Cornelia Huck
2015-01-09 8:04 ` [Qemu-devel] [PATCH 3/3 V3] kvm: extend kvm_irqchip_add_msi_route to work on s390 Frank Blaschka
2015-01-09 11:59 ` [Qemu-devel] [PATCH 0/3 V3] add PCI support for the s390 platform Cornelia Huck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150120142052.GA44419@tuxmaker.boeblingen.de.ibm.com \
--to=blaschka@linux.vnet.ibm.com \
--cc=armbru@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=cornelia.huck@de.ibm.com \
--cc=frank.blaschka@de.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.