Patchwork [U-Boot] arm: lib: memcpy: Do not copy to same address

login
register
mail settings
Submitter Matthias Weisser
Date May 23, 2011, 9:06 a.m.
Message ID <1306141610-24117-1-git-send-email-weisserm@arcor.de>
Download mbox | patch
Permalink /patch/96842/
State Accepted
Commit 34fe8281d7323784544e94d2f7218f52b8a2899d
Delegated to: Albert ARIBAUD
Headers show

Comments

Matthias Weisser - May 23, 2011, 9:06 a.m.
In some cases (e.g. bootm with a elf payload which is already at the right
position) there is a in place copy of data to the same address. Catching this
saves some ms while booting.

Signed-off-by: Matthias Weisser <weisserm@arcor.de>
---
 arch/arm/lib/memcpy.S |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Alexander Holler - May 23, 2011, 9:30 a.m.
Am 23.05.2011 11:06, schrieb Matthias Weisser:
> In some cases (e.g. bootm with a elf payload which is already at the right
> position) there is a in place copy of data to the same address. Catching this
> saves some ms while booting.
>
> Signed-off-by: Matthias Weisser<weisserm@arcor.de>
> ---
>   arch/arm/lib/memcpy.S |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/lib/memcpy.S b/arch/arm/lib/memcpy.S
> index 3b5aeec..f655256 100644
> --- a/arch/arm/lib/memcpy.S
> +++ b/arch/arm/lib/memcpy.S
> @@ -60,6 +60,9 @@
>   .globl memcpy
>   memcpy:
>
> +		cmp	r0, r1
> +		moveq	pc, lr
> +
>   		enter	r4, lr
>
>   		subs	r2, r2, #4

The standard clearly say to both memory regions should not overlap when 
memcpy() is used, so I would say this is the wrong place to fix that.

Regards,

Alexander
Matthias Weisser - May 23, 2011, 9:46 a.m.
Am 23.05.2011 11:30, schrieb Alexander Holler:
> Am 23.05.2011 11:06, schrieb Matthias Weisser:
>> In some cases (e.g. bootm with a elf payload which is already at the right
>> position) there is a in place copy of data to the same address. Catching this
>> saves some ms while booting.
>>
>> Signed-off-by: Matthias Weisser<weisserm@arcor.de>
>> ---
>>    arch/arm/lib/memcpy.S |    3 +++
>>    1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/lib/memcpy.S b/arch/arm/lib/memcpy.S
>> index 3b5aeec..f655256 100644
>> --- a/arch/arm/lib/memcpy.S
>> +++ b/arch/arm/lib/memcpy.S
>> @@ -60,6 +60,9 @@
>>    .globl memcpy
>>    memcpy:
>>
>> +		cmp	r0, r1
>> +		moveq	pc, lr
>> +
>>    		enter	r4, lr
>>
>>    		subs	r2, r2, #4
>
> The standard clearly say to both memory regions should not overlap when
> memcpy() is used, so I would say this is the wrong place to fix that.

Well, real world applications do this. And these two instructions 
shouldn't hurt a lot.

I first send a patch fixing only "my" problem in cmd_elf.c but Wolfgang 
suggested to do this globally. Please see 
http://www.mail-archive.com/u-boot@lists.denx.de/msg50612.html as reference.

Matthias
Albert ARIBAUD - May 23, 2011, 9:49 a.m.
Le 23/05/2011 11:30, Alexander Holler a écrit :
> Am 23.05.2011 11:06, schrieb Matthias Weisser:
>> In some cases (e.g. bootm with a elf payload which is already at the right
>> position) there is a in place copy of data to the same address. Catching this
>> saves some ms while booting.
>>
>> Signed-off-by: Matthias Weisser<weisserm@arcor.de>
>> ---
>>    arch/arm/lib/memcpy.S |    3 +++
>>    1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/lib/memcpy.S b/arch/arm/lib/memcpy.S
>> index 3b5aeec..f655256 100644
>> --- a/arch/arm/lib/memcpy.S
>> +++ b/arch/arm/lib/memcpy.S
>> @@ -60,6 +60,9 @@
>>    .globl memcpy
>>    memcpy:
>>
>> +		cmp	r0, r1
>> +		moveq	pc, lr
>> +
>>    		enter	r4, lr
>>
>>    		subs	r2, r2, #4
>
> The standard clearly say to both memory regions should not overlap when
> memcpy() is used, so I would say this is the wrong place to fix that.

I think the intent here is not to enforce the standard but to handle an 
actual, and degenerate, copy request in the most efficient manner, and 
in that respect, the patch does its job.

Besides, if the patch was about enforcing the standard, then I would 
consider it highly more efficient to check the areas once in the memcpy 
function than duplicating this check before each call to the function, 
considering that the place where the copy happens is not necessarily the 
one where the source and destination were computed.

> Regards,
>
> Alexander

Amicalement,
Alexander Holler - May 23, 2011, 9:51 a.m.
Am 23.05.2011 11:46, schrieb Matthias Weißer:

>> The standard clearly say to both memory regions should not overlap when
>> memcpy() is used, so I would say this is the wrong place to fix that.
>
> Well, real world applications do this. And these two instructions
> shouldn't hurt a lot.

Real bugs to this. Just see e.g the long discussion about some changes 
fo memcpy done in the glibc lately and what that did for flash-users 
because flash seemed to the same stupid stuff.

Regards,

Alexander
Alexander Holler - May 23, 2011, 10:01 a.m.
Hello,

Am 23.05.2011 11:49, schrieb Albert ARIBAUD:

>> The standard clearly say to both memory regions should not overlap when
>> memcpy() is used, so I would say this is the wrong place to fix that.
>
> I think the intent here is not to enforce the standard but to handle an
> actual, and degenerate, copy request in the most efficient manner, and
> in that respect, the patch does its job.
>
> Besides, if the patch was about enforcing the standard, then I would
> consider it highly more efficient to check the areas once in the memcpy
> function than duplicating this check before each call to the function,
> considering that the place where the copy happens is not necessarily the
> one where the source and destination were computed.

A fool proof solution would be to always use memmove() and get rid of 
memcpy(). But checking for overlapped regions in memcpy() is imho the 
wrong way. This just leads to more possible wrong code which uses 
memcpy() when it should use memmove().

Regards,

Alexander
Matthias Weisser - July 26, 2011, 6:02 a.m.
Dear Albert

Am 23.05.2011 11:49, schrieb Albert ARIBAUD:
> Le 23/05/2011 11:30, Alexander Holler a écrit :
>> Am 23.05.2011 11:06, schrieb Matthias Weisser:
>>> In some cases (e.g. bootm with a elf payload which is already at the right
>>> position) there is a in place copy of data to the same address. Catching this
>>> saves some ms while booting.
>>>
>>> Signed-off-by: Matthias Weisser<weisserm@arcor.de>
>>> ---
>>>     arch/arm/lib/memcpy.S |    3 +++
>>>     1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/lib/memcpy.S b/arch/arm/lib/memcpy.S
>>> index 3b5aeec..f655256 100644
>>> --- a/arch/arm/lib/memcpy.S
>>> +++ b/arch/arm/lib/memcpy.S
>>> @@ -60,6 +60,9 @@
>>>     .globl memcpy
>>>     memcpy:
>>>
>>> +		cmp	r0, r1
>>> +		moveq	pc, lr
>>> +
>>>     		enter	r4, lr
>>>
>>>     		subs	r2, r2, #4
>>
>> The standard clearly say to both memory regions should not overlap when
>> memcpy() is used, so I would say this is the wrong place to fix that.
>
> I think the intent here is not to enforce the standard but to handle an
> actual, and degenerate, copy request in the most efficient manner, and
> in that respect, the patch does its job.

Can this patch go in or do I need to change anything? I really would 
like to see it in mainline.

Regards,
Matthias
Albert ARIBAUD - Aug. 12, 2011, 8:49 a.m.
On 23/05/2011 11:06, Matthias Weisser wrote:
> In some cases (e.g. bootm with a elf payload which is already at the right
> position) there is a in place copy of data to the same address. Catching this
> saves some ms while booting.
>
> Signed-off-by: Matthias Weisser<weisserm@arcor.de>
> ---
>   arch/arm/lib/memcpy.S |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/lib/memcpy.S b/arch/arm/lib/memcpy.S
> index 3b5aeec..f655256 100644
> --- a/arch/arm/lib/memcpy.S
> +++ b/arch/arm/lib/memcpy.S
> @@ -60,6 +60,9 @@
>   .globl memcpy
>   memcpy:
>
> +		cmp	r0, r1
> +		moveq	pc, lr
> +
>   		enter	r4, lr
>
>   		subs	r2, r2, #4

Applied to u-boot-arm/master, thanks.

Amicalement,

Patch

diff --git a/arch/arm/lib/memcpy.S b/arch/arm/lib/memcpy.S
index 3b5aeec..f655256 100644
--- a/arch/arm/lib/memcpy.S
+++ b/arch/arm/lib/memcpy.S
@@ -60,6 +60,9 @@ 
 .globl memcpy
 memcpy:
 
+		cmp	r0, r1
+		moveq	pc, lr
+
 		enter	r4, lr
 
 		subs	r2, r2, #4