2019-01-07 07:44:02

by Cao jin

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

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 <[email protected]>
---
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





2019-01-07 08:02:21

by H. Peter Anvin

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

On January 6, 2019 11:40:56 PM PST, Cao jin <[email protected]> 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 <[email protected]>
>---
>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.

2019-01-07 08:54:44

by Cao jin

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

Hi,

On 1/7/19 3:59 PM, [email protected] wrote:
> On January 6, 2019 11:40:56 PM PST, Cao jin <[email protected]> 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 <[email protected]>
>> ---
>> 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:)



2019-01-08 08:42:07

by H. Peter Anvin

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

On January 7, 2019 12:52:57 AM PST, Cao jin <[email protected]> wrote:
>Hi,
>
>On 1/7/19 3:59 PM, [email protected] wrote:
>> On January 6, 2019 11:40:56 PM PST, Cao jin
><[email protected]> 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 <[email protected]>
>>> ---
>>> 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.

2019-01-08 08:59:47

by Cao jin

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

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 <[email protected]>
> ---
> 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
>



2019-01-10 08:30:51

by Cao jin

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

Hello HPA,

On 1/8/19 4:38 PM, [email protected] wrote:
> On January 7, 2019 12:52:57 AM PST, Cao jin <[email protected]> wrote:
>> Hi,
>>
>> On 1/7/19 3:59 PM, [email protected] wrote:
>>> On January 6, 2019 11:40:56 PM PST, Cao jin
>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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



2019-01-11 06:25:26

by Cao jin

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

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