[U-Boot] [PATCH] Do not copy elf section to same adress

[U-Boot] [PATCH] Do not copy elf section to same adress

[Matthias Weisser]

When an elf section is already at the right position (e.g. after image
decompression by bootm) there is no need to copy it.

Signed-off-by: Matthias Weisser <weisserm@arcor.de>
---
 common/cmd_elf.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/common/cmd_elf.c b/common/cmd_elf.c
index bf32612..aec4579 100644
--- a/common/cmd_elf.c
+++ b/common/cmd_elf.c
@@ -342,9 +342,10 @@ static unsigned long load_elf_image_shdr(unsigned long addr)
 			memset ((void *)shdr->sh_addr, 0, shdr->sh_size);
 		} else {
 			image = (unsigned char *) addr + shdr->sh_offset;
-			memcpy ((void *) shdr->sh_addr,
-				(const void *) image,
-				shdr->sh_size);
+			if ((void *) shdr->sh_addr != (void *) image)
+				memcpy((void *) shdr->sh_addr,
+					(const void *) image,
+					shdr->sh_size);
 		}
 		flush_cache (shdr->sh_addr, shdr->sh_size);
 	}
-- 
1.7.0.4

[U-Boot] [PATCH] Do not copy elf section to same adress

[Wolfgang Denk]

Dear Matthias Weisser,

In message <1295367283-4696-1-git-send-email-weisserm@arcor.de> you wrote:
> When an elf section is already at the right position (e.g. after image
> decompression by bootm) there is no need to copy it.
> 
> Signed-off-by: Matthias Weisser <weisserm@arcor.de>
> ---
>  common/cmd_elf.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/common/cmd_elf.c b/common/cmd_elf.c
> index bf32612..aec4579 100644
> --- a/common/cmd_elf.c
> +++ b/common/cmd_elf.c
> @@ -342,9 +342,10 @@ static unsigned long load_elf_image_shdr(unsigned long addr)
>  			memset ((void *)shdr->sh_addr, 0, shdr->sh_size);
>  		} else {
>  			image = (unsigned char *) addr + shdr->sh_offset;
> -			memcpy ((void *) shdr->sh_addr,
> -				(const void *) image,
> -				shdr->sh_size);
> +			if ((void *) shdr->sh_addr != (void *) image)
> +				memcpy((void *) shdr->sh_addr,
> +					(const void *) image,
> +					shdr->sh_size);

Braces required for multi-line statement.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Egotist: A person of low taste, more interested in  himself  than  in
me.                                                  - Ambrose Bierce

[U-Boot] [PATCH V2] Do not copy elf section to same adress

[Matthias Weisser]

When an elf section is already at the right position (e.g. after image
decompression by bootm) there is no need to copy it. This saves some ms
when bootig an elf image.

Changes since V1
  - Fixed style issues

Signed-off-by: Matthias Weisser <weisserm@arcor.de>
---
 common/cmd_elf.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/common/cmd_elf.c b/common/cmd_elf.c
index bf32612..3537769 100644
--- a/common/cmd_elf.c
+++ b/common/cmd_elf.c
@@ -342,9 +342,11 @@ static unsigned long load_elf_image_shdr(unsigned long addr)
 			memset ((void *)shdr->sh_addr, 0, shdr->sh_size);
 		} else {
 			image = (unsigned char *) addr + shdr->sh_offset;
-			memcpy ((void *) shdr->sh_addr,
-				(const void *) image,
-				shdr->sh_size);
+			if ((void *) shdr->sh_addr != (void *) image) {
+				memcpy((void *) shdr->sh_addr,
+					(const void *) image,
+					shdr->sh_size);
+			}
 		}
 		flush_cache (shdr->sh_addr, shdr->sh_size);
 	}
-- 
1.7.0.4

[U-Boot] [PATCH V2] Do not copy elf section to same adress

[Matthias Weisser]

Hi

Am 19.01.2011 12:03, schrieb Matthias Weisser:
> When an elf section is already at the right position (e.g. after image
> decompression by bootm) there is no need to copy it. This saves some ms
> when bootig an elf image.
> 
> Changes since V1
>   - Fixed style issues
> 
> Signed-off-by: Matthias Weisser <weisserm@arcor.de>
> ---
>  common/cmd_elf.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/common/cmd_elf.c b/common/cmd_elf.c
> index bf32612..3537769 100644
> --- a/common/cmd_elf.c
> +++ b/common/cmd_elf.c
> @@ -342,9 +342,11 @@ static unsigned long load_elf_image_shdr(unsigned long addr)
>  			memset ((void *)shdr->sh_addr, 0, shdr->sh_size);
>  		} else {
>  			image = (unsigned char *) addr + shdr->sh_offset;
> -			memcpy ((void *) shdr->sh_addr,
> -				(const void *) image,
> -				shdr->sh_size);
> +			if ((void *) shdr->sh_addr != (void *) image) {
> +				memcpy((void *) shdr->sh_addr,
> +					(const void *) image,
> +					shdr->sh_size);
> +			}
>  		}
>  		flush_cache (shdr->sh_addr, shdr->sh_size);
>  	}

Any comments on this patch? Any problems with it? I would like to see it
in mainline and I am open for any comments.

Regards,
Matthias Wei?er

[U-Boot] [PATCH V2] Do not copy elf section to same adress

[Wolfgang Denk]

Dear Matthias Weisser,

In message <1295435020-14190-1-git-send-email-weisserm@arcor.de> you wrote:
> When an elf section is already at the right position (e.g. after image
> decompression by bootm) there is no need to copy it. This saves some ms
> when bootig an elf image.
> 
> Changes since V1
>   - Fixed style issues
> 
> Signed-off-by: Matthias Weisser <weisserm@arcor.de>
> ---
>  common/cmd_elf.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/common/cmd_elf.c b/common/cmd_elf.c
> index bf32612..3537769 100644
> --- a/common/cmd_elf.c
> +++ b/common/cmd_elf.c
> @@ -342,9 +342,11 @@ static unsigned long load_elf_image_shdr(unsigned long addr)
>  			memset ((void *)shdr->sh_addr, 0, shdr->sh_size);
>  		} else {
>  			image = (unsigned char *) addr + shdr->sh_offset;
> -			memcpy ((void *) shdr->sh_addr,
> -				(const void *) image,
> -				shdr->sh_size);
> +			if ((void *) shdr->sh_addr != (void *) image) {
> +				memcpy((void *) shdr->sh_addr,
> +					(const void *) image,
> +					shdr->sh_size);
> +			}

The idea is correct, but I think the implementation is suboptimal.
Instead of fixing this use case only, the test should be moved into
the implementation of memcpy() itself so any other callers with such a
situation benefit from it, too.

While we are at it, we should do the same with bcopy() and memmove(),
too.

Thanks.

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Marriage is the triumph  of  imagination  over  intelligence.  Second
marriage is the triumph of hope over experience.

[U-Boot] [PATCH V2] Do not copy elf section to same adress

[Matthias Weisser]

Am 11.04.2011 21:59, schrieb Wolfgang Denk:
> Dear Matthias Weisser,
> 
> In message <1295435020-14190-1-git-send-email-weisserm@arcor.de> you wrote:
>> > When an elf section is already at the right position (e.g. after image
>> > decompression by bootm) there is no need to copy it. This saves some ms
>> > when bootig an elf image.
>> > 
>> > Changes since V1
>> >   - Fixed style issues
>> > 
>> > Signed-off-by: Matthias Weisser <weisserm@arcor.de>
>> > ---
>> >  common/cmd_elf.c |    8 +++++---
>> >  1 files changed, 5 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/common/cmd_elf.c b/common/cmd_elf.c
>> > index bf32612..3537769 100644
>> > --- a/common/cmd_elf.c
>> > +++ b/common/cmd_elf.c
>> > @@ -342,9 +342,11 @@ static unsigned long load_elf_image_shdr(unsigned long addr)
>> >  			memset ((void *)shdr->sh_addr, 0, shdr->sh_size);
>> >  		} else {
>> >  			image = (unsigned char *) addr + shdr->sh_offset;
>> > -			memcpy ((void *) shdr->sh_addr,
>> > -				(const void *) image,
>> > -				shdr->sh_size);
>> > +			if ((void *) shdr->sh_addr != (void *) image) {
>> > +				memcpy((void *) shdr->sh_addr,
>> > +					(const void *) image,
>> > +					shdr->sh_size);
>> > +			}
> The idea is correct, but I think the implementation is suboptimal.
> Instead of fixing this use case only, the test should be moved into
> the implementation of memcpy() itself so any other callers with such a
> situation benefit from it, too.

Well, I thought about that too. But decided against it as I thought it
will be a bit too intervening for a patch from me as this will hit all
boards.

I can't come up with an example where this may produce a problem but who
knows which exotic hardware is out there which expects that a memcpy
with identical src and dst addresses is executed exactly in that way.
But maybe we can ignore this and let these exotic boards come up with a
solution handling that special situation.

> While we are at it, we should do the same with bcopy() and memmove(),
> too.

Maybe I can post a patch tomorrow. The only thing which I can't handle
are architecture specific memcpy/memmove/... functions besides ARM.

Regards,
Matthias Wei?er

[U-Boot] [PATCH V2] Do not copy elf section to same adress

[Wolfgang Denk]

Dear Matthias Weisser,

In message <4DA360A9.10401@arcor.de> you wrote:
>
> I can't come up with an example where this may produce a problem but who
> knows which exotic hardware is out there which expects that a memcpy
> with identical src and dst addresses is executed exactly in that way.
> But maybe we can ignore this and let these exotic boards come up with a
> solution handling that special situation.

Right.

> > While we are at it, we should do the same with bcopy() and memmove(),
> > too.
> 
> Maybe I can post a patch tomorrow. The only thing which I can't handle
> are architecture specific memcpy/memmove/... functions besides ARM.

That's fine. Optimized arch specific code probably already implements
such a shortcut anyway.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Winners never talk about glorious victories. That's  because  they're
the  ones  who  see  what the battlefield looks like afterwards. It's
only the losers who have glorious victories.
                                      - Terry Pratchett, _Small Gods_