[PATCH] x86/boot: drop memset from copy.S

[PATCH] x86/boot: drop memset from copy.S

[Cao jin]

According to objdump output of setup, function memset is not used in
setup code. Currently, all usage of memset in setup come from macro
definition of string.h.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
Compiled and booted under x86_64; compiled under i386.

Questions: now there is 2 definition of memcpy, one lies in copy.S,
another lies in string.h which is mapped to gcc builtin function. Do we
still need 2 definition? Could we move the content of copy.S to
boot/string.c?

At first glance, the usage of string.{c.h} of setup is kind of confusing,
they are also used in compressed/ and purgatory/

 arch/x86/boot/copy.S | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/arch/x86/boot/copy.S b/arch/x86/boot/copy.S
index 15d9f74b0008..5157d08b0ff2 100644
--- a/arch/x86/boot/copy.S
+++ b/arch/x86/boot/copy.S
@@ -33,21 +33,6 @@ GLOBAL(memcpy)
 	retl
 ENDPROC(memcpy)
 
-GLOBAL(memset)
-	pushw	%di
-	movw	%ax, %di
-	movzbl	%dl, %eax
-	imull	$0x01010101,%eax
-	pushw	%cx
-	shrw	$2, %cx
-	rep; stosl
-	popw	%cx
-	andw	$3, %cx
-	rep; stosb
-	popw	%di
-	retl
-ENDPROC(memset)
-
 GLOBAL(copy_from_fs)
 	pushw	%ds
 	pushw	%fs
-- 
2.17.0

Re: [PATCH] x86/boot: drop memset from copy.S

[hpa]

On January 6, 2019 11:40:56 PM PST, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>According to objdump output of setup, function memset is not used in
>setup code. Currently, all usage of memset in setup come from macro
>definition of string.h.
>
>Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>---
>Compiled and booted under x86_64; compiled under i386.
>
>Questions: now there is 2 definition of memcpy, one lies in copy.S,
>another lies in string.h which is mapped to gcc builtin function. Do we
>still need 2 definition? Could we move the content of copy.S to
>boot/string.c?
>
>At first glance, the usage of string.{c.h} of setup is kind of
>confusing,
>they are also used in compressed/ and purgatory/
>
> arch/x86/boot/copy.S | 15 ---------------
> 1 file changed, 15 deletions(-)
>
>diff --git a/arch/x86/boot/copy.S b/arch/x86/boot/copy.S
>index 15d9f74b0008..5157d08b0ff2 100644
>--- a/arch/x86/boot/copy.S
>+++ b/arch/x86/boot/copy.S
>@@ -33,21 +33,6 @@ GLOBAL(memcpy)
> 	retl
> ENDPROC(memcpy)
> 
>-GLOBAL(memset)
>-	pushw	%di
>-	movw	%ax, %di
>-	movzbl	%dl, %eax
>-	imull	$0x01010101,%eax
>-	pushw	%cx
>-	shrw	$2, %cx
>-	rep; stosl
>-	popw	%cx
>-	andw	$3, %cx
>-	rep; stosb
>-	popw	%di
>-	retl
>-ENDPROC(memset)
>-
> GLOBAL(copy_from_fs)
> 	pushw	%ds
> 	pushw	%fs

This is dependent on both gcc version and flags.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] x86/boot: drop memset from copy.S

[Cao jin]

Hi,

On 1/7/19 3:59 PM, hpa@zytor.com wrote:
> On January 6, 2019 11:40:56 PM PST, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>> According to objdump output of setup, function memset is not used in
>> setup code. Currently, all usage of memset in setup come from macro
>> definition of string.h.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> Compiled and booted under x86_64; compiled under i386.
>>
>> Questions: now there is 2 definition of memcpy, one lies in copy.S,
>> another lies in string.h which is mapped to gcc builtin function. Do we
>> still need 2 definition? Could we move the content of copy.S to
>> boot/string.c?
>>
>> At first glance, the usage of string.{c.h} of setup is kind of
>> confusing,
>> they are also used in compressed/ and purgatory/
>>
>> arch/x86/boot/copy.S | 15 ---------------
>> 1 file changed, 15 deletions(-)
>>
>> diff --git a/arch/x86/boot/copy.S b/arch/x86/boot/copy.S
>> index 15d9f74b0008..5157d08b0ff2 100644
>> --- a/arch/x86/boot/copy.S
>> +++ b/arch/x86/boot/copy.S
>> @@ -33,21 +33,6 @@ GLOBAL(memcpy)
>> 	retl
>> ENDPROC(memcpy)
>>
>> -GLOBAL(memset)
>> -	pushw	%di
>> -	movw	%ax, %di
>> -	movzbl	%dl, %eax
>> -	imull	$0x01010101,%eax
>> -	pushw	%cx
>> -	shrw	$2, %cx
>> -	rep; stosl
>> -	popw	%cx
>> -	andw	$3, %cx
>> -	rep; stosb
>> -	popw	%di
>> -	retl
>> -ENDPROC(memset)
>> -
>> GLOBAL(copy_from_fs)
>> 	pushw	%ds
>> 	pushw	%fs
> 
> This is dependent on both gcc version and flags.
> 

Thanks for your info, but I still don't quite get your point. All files
who has memset reference in setup will also #include "string.h", so how
gcc version and flags will associate memset reference to the assembly
function by force?  Hope to get a little more details when you are
convenient:)

Re: [PATCH] x86/boot: drop memset from copy.S

[hpa]

On January 7, 2019 12:52:57 AM PST, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>Hi,
>
>On 1/7/19 3:59 PM, hpa@zytor.com wrote:
>> On January 6, 2019 11:40:56 PM PST, Cao jin
><caoj.fnst@cn.fujitsu.com> wrote:
>>> According to objdump output of setup, function memset is not used in
>>> setup code. Currently, all usage of memset in setup come from macro
>>> definition of string.h.
>>>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>> ---
>>> Compiled and booted under x86_64; compiled under i386.
>>>
>>> Questions: now there is 2 definition of memcpy, one lies in copy.S,
>>> another lies in string.h which is mapped to gcc builtin function. Do
>we
>>> still need 2 definition? Could we move the content of copy.S to
>>> boot/string.c?
>>>
>>> At first glance, the usage of string.{c.h} of setup is kind of
>>> confusing,
>>> they are also used in compressed/ and purgatory/
>>>
>>> arch/x86/boot/copy.S | 15 ---------------
>>> 1 file changed, 15 deletions(-)
>>>
>>> diff --git a/arch/x86/boot/copy.S b/arch/x86/boot/copy.S
>>> index 15d9f74b0008..5157d08b0ff2 100644
>>> --- a/arch/x86/boot/copy.S
>>> +++ b/arch/x86/boot/copy.S
>>> @@ -33,21 +33,6 @@ GLOBAL(memcpy)
>>> 	retl
>>> ENDPROC(memcpy)
>>>
>>> -GLOBAL(memset)
>>> -	pushw	%di
>>> -	movw	%ax, %di
>>> -	movzbl	%dl, %eax
>>> -	imull	$0x01010101,%eax
>>> -	pushw	%cx
>>> -	shrw	$2, %cx
>>> -	rep; stosl
>>> -	popw	%cx
>>> -	andw	$3, %cx
>>> -	rep; stosb
>>> -	popw	%di
>>> -	retl
>>> -ENDPROC(memset)
>>> -
>>> GLOBAL(copy_from_fs)
>>> 	pushw	%ds
>>> 	pushw	%fs
>> 
>> This is dependent on both gcc version and flags.
>> 
>
>Thanks for your info, but I still don't quite get your point. All files
>who has memset reference in setup will also #include "string.h", so how
>gcc version and flags will associate memset reference to the assembly
>function by force?  Hope to get a little more details when you are
>convenient:)

GCC will sometimes emit calls to certain functions including memcpy().
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] x86/boot: drop memset from copy.S

[Cao jin]

One more question: in compressed/, for mem*(), it seems we both use the
macros of boot/string.h, and the functions of compressed/string.c. Is
that what we want?

compressed/ is compiled with -O2, so it cannot be told by objdump -d,
but still can be confirmed by nm <*.o>, for example:

$nm arch/x86/boot/compressed/eboot.o
                 U memcpy
                 U memset

$nm arch/x86/boot/compressed/pgtable_64.o
                 # No entry of mem*()

both of eboot.c and pgtable_64.c #include "../string.h", and use some of
 mem*(), it is counter-intuitive to me. Very appreciate it someone can
leave a hint.

-- 
Sincerely,
Cao jin

On 1/7/19 3:40 PM, Cao jin wrote:
> According to objdump output of setup, function memset is not used in
> setup code. Currently, all usage of memset in setup come from macro
> definition of string.h.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> Compiled and booted under x86_64; compiled under i386.
> 
> Questions: now there is 2 definition of memcpy, one lies in copy.S,
> another lies in string.h which is mapped to gcc builtin function. Do we
> still need 2 definition? Could we move the content of copy.S to
> boot/string.c?
> 
> At first glance, the usage of string.{c.h} of setup is kind of confusing,
> they are also used in compressed/ and purgatory/
> 
>  arch/x86/boot/copy.S | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/arch/x86/boot/copy.S b/arch/x86/boot/copy.S
> index 15d9f74b0008..5157d08b0ff2 100644
> --- a/arch/x86/boot/copy.S
> +++ b/arch/x86/boot/copy.S
> @@ -33,21 +33,6 @@ GLOBAL(memcpy)
>  	retl
>  ENDPROC(memcpy)
>  
> -GLOBAL(memset)
> -	pushw	%di
> -	movw	%ax, %di
> -	movzbl	%dl, %eax
> -	imull	$0x01010101,%eax
> -	pushw	%cx
> -	shrw	$2, %cx
> -	rep; stosl
> -	popw	%cx
> -	andw	$3, %cx
> -	rep; stosb
> -	popw	%di
> -	retl
> -ENDPROC(memset)
> -
>  GLOBAL(copy_from_fs)
>  	pushw	%ds
>  	pushw	%fs
> 

Re: [PATCH] x86/boot: drop memset from copy.S

[Cao jin]

Hello HPA,

On 1/8/19 4:38 PM, hpa@zytor.com wrote:
> On January 7, 2019 12:52:57 AM PST, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>> Hi,
>>
>> On 1/7/19 3:59 PM, hpa@zytor.com wrote:
>>> On January 6, 2019 11:40:56 PM PST, Cao jin
>> <caoj.fnst@cn.fujitsu.com> wrote:
>>>> According to objdump output of setup, function memset is not used in
>>>> setup code. Currently, all usage of memset in setup come from macro
>>>> definition of string.h.
>>>>
>>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>>> ---
>>>> Compiled and booted under x86_64; compiled under i386.
>>>>
>>>> Questions: now there is 2 definition of memcpy, one lies in copy.S,
>>>> another lies in string.h which is mapped to gcc builtin function. Do
>> we
>>>> still need 2 definition? Could we move the content of copy.S to
>>>> boot/string.c?
>>>>
>>>> At first glance, the usage of string.{c.h} of setup is kind of
>>>> confusing,
>>>> they are also used in compressed/ and purgatory/
>>>>
>>>> arch/x86/boot/copy.S | 15 ---------------
>>>> 1 file changed, 15 deletions(-)
>>>>
>>>> diff --git a/arch/x86/boot/copy.S b/arch/x86/boot/copy.S
>>>> index 15d9f74b0008..5157d08b0ff2 100644
>>>> --- a/arch/x86/boot/copy.S
>>>> +++ b/arch/x86/boot/copy.S
>>>> @@ -33,21 +33,6 @@ GLOBAL(memcpy)
>>>> 	retl
>>>> ENDPROC(memcpy)
>>>>
>>>> -GLOBAL(memset)
>>>> -	pushw	%di
>>>> -	movw	%ax, %di
>>>> -	movzbl	%dl, %eax
>>>> -	imull	$0x01010101,%eax
>>>> -	pushw	%cx
>>>> -	shrw	$2, %cx
>>>> -	rep; stosl
>>>> -	popw	%cx
>>>> -	andw	$3, %cx
>>>> -	rep; stosb
>>>> -	popw	%di
>>>> -	retl
>>>> -ENDPROC(memset)
>>>> -
>>>> GLOBAL(copy_from_fs)
>>>> 	pushw	%ds
>>>> 	pushw	%fs
>>>
>>> This is dependent on both gcc version and flags.
>>>
>>
>> Thanks for your info, but I still don't quite get your point. All files
>> who has memset reference in setup will also #include "string.h", so how
>> gcc version and flags will associate memset reference to the assembly
>> function by force?  Hope to get a little more details when you are
>> convenient:)
> 
> GCC will sometimes emit calls to certain functions including memcpy().
> 

Thanks very much. After spending some time on GCC document, I think you
are talking about a condition that, for example, __builtin_memcpy() is
not optimized as inline code, but a function call to memcpy() in copy.S.

But I failed to find out the details how would gcc version & flags make
it this way, even I checked out the .cmd file of these .c. Or is this
born to be obscure for programmers?

-- 
Sincerely,
Cao jin

Re: [PATCH] x86/boot: drop memset from copy.S

[Cao jin]

On 1/8/19 4:46 PM, Cao jin wrote:
> One more question: in compressed/, for mem*(), it seems we both use the
> macros of boot/string.h, and the functions of compressed/string.c. Is
> that what we want?
> 
> compressed/ is compiled with -O2, so it cannot be told by objdump -d,
> but still can be confirmed by nm <*.o>, for example:
> 
> $nm arch/x86/boot/compressed/eboot.o
>                  U memcpy
>                  U memset
> 
> $nm arch/x86/boot/compressed/pgtable_64.o
>                  # No entry of mem*()
> 
> both of eboot.c and pgtable_64.c #include "../string.h", and use some of
>  mem*(), it is counter-intuitive to me. Very appreciate it someone can
> leave a hint.
> 

Well, I think HPA's previous answer is also suitable for this question,
with -O2, sometimes __builtin_mem*() is optimized as inline code, while
sometimes just emit a call to corresponding self-defined mem*() functions.

-- 
Sincerely,
Cao jin