[U-Boot] Issue with memcpy when booting a fitImage on SPEAr600

[Quentin Schulz]

Hi Simon,

On Mon, Jan 07, 2019 at 05:38:19PM -0700, Simon Glass wrote:
> Hi,
> 
> On Thu, 6 Dec 2018 at 03:37, Thomas Petazzoni
> <thomas.petazzoni at bootlin.com> wrote:
> >
> > Hello Stefan,
> >
> > On Wed, 5 Dec 2018 12:30:57 +0100, Stefan Roese wrote:
> >
> > > > It's weird to me that it's happening with this SoC because it's based on
> > > > ARM926ejs which is widely used I assume. Shouldn't have anyone already
> > > > encountered the bug? Or is nobody actually booting a fitImage and had the
> > > > luck to never call memcpy with src == dest anywhere else in the code?
> > >
> > > I did some work on the SPEAr600 some years ago and I'm pretty sure that
> > > I never used FIT image at that time. Sorry, but I can't remember any
> > > similar issues like these.
> >
> > Well, the issue is in generic ARM code, so whether it's SPEAr600 or any
> > other ARM SoC should not really matter here.
> >
> > > FWIW, I would not oppose to having at least this "src == dest" check
> > > in the code again, since it doesn't really make sense to overwrite
> > > an area with the same value - other than for testing purposes.
> >
> > The thing is that the memcpy() that gets called does have this src ==
> > dest check, as its code starts with:
> >
> > ENTRY(memcpy)
> >                 cmp     r0, r1
> >                 bxeq    lr
> >
> > which, if my assembly-fu is not bad, means: if first argument == second
> > argument, then return. But even with this src == dest check within
> > memcpy() itself, Quentin is seeing memmove() hang when src == dest.
> >
> > Another thing is that the memmove() code looks like this:
> >
> > {
> >         char *tmp, *s;
> >
> >         if (dest <= src) {
> >                 memcpy(dest, src, count);
> >
> > However, having dest <= src does not guarantee that the destination and
> > source areas are non-overlapping. And the normal semantic for memcpy()
> > is that it doesn't work for overlapping areas. From memcpy(3):
> >
> >   The memcpy() function copies n bytes from memory area src to memory
> >   area dest.  The memory areas must not overlap.  Use memmove(3) if the
> >   memory areas do overlap.
> >
> > Of course, this man page documents the memcpy() implementation from the
> > C library, and perhaps the semantic of U-Boot memcpy is different.
> 
> Yes I suppose it is different.
> 
> >
> > So is it correct to use memcpy() to implement memmove() when the areas
> > are overlapping ?
> 
> Strictly speaking, no. I think perhaps my patch should have been more careful.
> 
> >
> > Perhaps Simon Glass, who did the change to use memcpy() in
> > cb0eae8cf8aaca76910dee4c7eb536d0814d1bd2 could comment on that ?
> >
> 
> That said, I cannot see why the memcpy() fails. How do you explain
> that? If you revert my change, does it work?
> 

Yes.
2018.11 does not work.
2018.11 with your patch reverted works.

That being said, I was working on updating to 2019.01-rc3 and noticed it
was working fine now.

We found out that:
commit ba08afe8377ac72f834f8baad38e9631957b2ea8
Author: Klaus Goger <klaus.goger at theobroma-systems.com>
Date:   Thu Apr 26 20:18:10 2018 +0200

    arm: Make arch specific memcpy thumb-safe.

    The current arch implementation of memcpy cannot be called
    from thumb code, because it does not use bx instructions on return.
    This patch addresses that. Note, that this patch does not touch
    the hot loop of memcpy, so performance is not affected.

    Tested on MXS (arm926ejs) with and without thumb-mode enabled.

    Signed-off-by: Klaus Goger <klaus.goger at theobroma-systems.com>
    Signed-off-by: Christoph Muellner <christoph.muellner at theobroma-systems.com>

makes it work on 2018.11 (it was merged for 2019.01-rc1).

Can't understand why all memcpy would work but not some, though.

Best regards,
Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190108/5b9c50cd/attachment.sig>