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

[Simon Glass]

Hi Quentin,

On Tue, 8 Jan 2019 at 04:03, Quentin Schulz <quentin.schulz at bootlin.com> wrote:
>
> 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).

OK good that it is fixed.

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

The particular case introduced by my patch shows up this problem more
often, I suppose.


- Simon