Hi Quentin, On Tue, 8 Jan 2019 at 04:03, Quentin Schulz <[email protected]> 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 > > <[email protected]> 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 <[email protected]> > 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 <[email protected]> > Signed-off-by: Christoph Muellner > <[email protected]> > > 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
