Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1583324 > unrolled thread

[PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data

Back to article view | Back to linux.kernel

Contents

  [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Jiri Slaby <jslaby@suse.cz> - 2017-02-17 12:00 +0100
    [RFC 09/10] x86: boot, extract efi_pe_entry from startup_64 Jiri Slaby <jslaby@suse.cz> - 2017-02-17 12:00 +0100
    [PATCH 03/10] x86: boot, annotate functions properly Jiri Slaby <jslaby@suse.cz> - 2017-02-17 12:00 +0100
    Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Juergen Gross <jgross@suse.com> - 2017-02-17 12:20 +0100
    Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Jiri Slaby <jslaby@suse.cz> - 2017-03-01 11:00 +0100
    Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 11:10 +0100
      Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Ingo Molnar <mingo@kernel.org> - 2017-03-01 12:10 +0100
        Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Jiri Slaby <jslaby@suse.cz> - 2017-03-03 15:20 +0100
        Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data hpa@zytor.com - 2017-03-03 19:30 +0100
          Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Jiri Slaby <jslaby@suse.cz> - 2017-03-06 15:20 +0100
          Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Ingo Molnar <mingo@kernel.org> - 2017-03-07 09:10 +0100
        Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data hpa@zytor.com - 2017-03-03 20:50 +0100
          Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Ingo Molnar <mingo@kernel.org> - 2017-03-07 09:40 +0100
            [RFC] linkage: new macros for functions and data Jiri Slaby <jslaby@suse.cz> - 2017-03-07 18:50 +0100
    Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Ingo Molnar <mingo@kernel.org> - 2017-03-02 00:00 +0100

#1583324 — [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data

This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END,
and other macros across x86. When we have all this sorted out, this will
help to inject DWARF unwinding info by objtool later.

So, let us use the macros this way:
* ENTRY -- start of a global function
* ENDPROC -- end of a local/global function
* GLOBAL -- start of a globally visible data symbol
* END -- end of local/global data symbol

The goal is forcing ENTRY to emit .cfi_startproc and ENDPROC to emit
.cfi_endproc.

This particular patch makes proper use of GLOBAL on data and ENTRY on a
function which was not the case on 4 locations.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: <x86@kernel.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: <xen-devel@lists.xenproject.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: <linux-pm@vger.kernel.org>
---
 arch/x86/kernel/acpi/wakeup_64.S | 14 +++++++-------
 arch/x86/kernel/head_64.S        |  2 +-
 arch/x86/kernel/mcount_64.S      |  2 +-
 arch/x86/xen/xen-head.S          |  2 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
index 50b8ed0317a3..bfd0ddefa5e8 100644
--- a/arch/x86/kernel/acpi/wakeup_64.S
+++ b/arch/x86/kernel/acpi/wakeup_64.S
@@ -125,12 +125,12 @@ ENTRY(do_suspend_lowlevel)
 ENDPROC(do_suspend_lowlevel)
 
 .data
-ENTRY(saved_rbp)	.quad	0
-ENTRY(saved_rsi)	.quad	0
-ENTRY(saved_rdi)	.quad	0
-ENTRY(saved_rbx)	.quad	0
+GLOBAL(saved_rbp)	.quad	0
+GLOBAL(saved_rsi)	.quad	0
+GLOBAL(saved_rdi)	.quad	0
+GLOBAL(saved_rbx)	.quad	0
 
-ENTRY(saved_rip)	.quad	0
-ENTRY(saved_rsp)	.quad	0
+GLOBAL(saved_rip)	.quad	0
+GLOBAL(saved_rsp)	.quad	0
 
-ENTRY(saved_magic)	.quad	0
+GLOBAL(saved_magic)	.quad	0
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index b467b14b03eb..7c14ab3a0f3b 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -489,7 +489,7 @@ early_gdt_descr:
 early_gdt_descr_base:
 	.quad	INIT_PER_CPU_VAR(gdt_page)
 
-ENTRY(phys_base)
+GLOBAL(phys_base)
 	/* This must match the first entry in level2_kernel_pgt */
 	.quad   0x0000000000000000
 EXPORT_SYMBOL(phys_base)
diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index b50b283f90e4..3e35675e201e 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -314,7 +314,7 @@ ENTRY(ftrace_graph_caller)
 	retq
 END(ftrace_graph_caller)
 
-GLOBAL(return_to_handler)
+ENTRY(return_to_handler)
 	subq  $24, %rsp
 
 	/* Save the return values */
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 37794e42b67d..39ea5484763a 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -37,7 +37,7 @@ ENTRY(startup_xen)
 
 .pushsection .text
 	.balign PAGE_SIZE
-ENTRY(hypercall_page)
+GLOBAL(hypercall_page)
 	.skip PAGE_SIZE
 
 #define HYPERCALL(n) \
-- 
2.11.1

[toc] | [next] | [standalone]

#1583325 — [RFC 09/10] x86: boot, extract efi_pe_entry from startup_64

efi_pe_entry body is somehow squashed into startup_64. It makes the code
less readable and illogical. Extract the inlined efi_pe_entry body from
startup_64 into a separate function and mark it appropriatelly by
ENTRY+ENDPROC.

ABI offsets are preserved:
0000000000000200 T startup_64
0000000000000280 T efi_pe_entry
00000000000002d0 t handover_entry
00000000000002fd t fail
0000000000000390 T efi64_stub_entry

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: <x86@kernel.org>
---
 arch/x86/boot/compressed/head_64.S | 112 ++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 59 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 59eccbc46ad7..7983e0de4069 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -243,65 +243,6 @@ ENTRY(startup_64)
 	 * that maps our entire kernel(text+data+bss+brk), zero page
 	 * and command line.
 	 */
-#ifdef CONFIG_EFI_STUB
-	/*
-	 * The entry point for the PE/COFF executable is efi_pe_entry, so
-	 * only legacy boot loaders will execute this jmp.
-	 */
-	jmp	preferred_addr
-
-GLOBAL(efi_pe_entry)
-	movq	%rcx, efi64_config(%rip)	/* Handle */
-	movq	%rdx, efi64_config+8(%rip) /* EFI System table pointer */
-
-	leaq	efi64_config(%rip), %rax
-	movq	%rax, efi_config(%rip)
-
-	call	1f
-1:	popq	%rbp
-	subq	$1b, %rbp
-
-	/*
-	 * Relocate efi_config->call().
-	 */
-	addq	%rbp, efi64_config+32(%rip)
-
-	movq	%rax, %rdi
-	call	make_boot_params
-	cmpq	$0,%rax
-	je	fail
-	mov	%rax, %rsi
-	leaq	startup_32(%rip), %rax
-	movl	%eax, BP_code32_start(%rsi)
-	jmp	2f		/* Skip the relocation */
-
-handover_entry:
-	call	1f
-1:	popq	%rbp
-	subq	$1b, %rbp
-
-	/*
-	 * Relocate efi_config->call().
-	 */
-	movq	efi_config(%rip), %rax
-	addq	%rbp, 32(%rax)
-2:
-	movq	efi_config(%rip), %rdi
-	call	efi_main
-	movq	%rax,%rsi
-	cmpq	$0,%rax
-	jne	2f
-fail:
-	/* EFI init failed, so hang. */
-	hlt
-	jmp	fail
-2:
-	movl	BP_code32_start(%esi), %eax
-	leaq	preferred_addr(%rax), %rax
-	jmp	*%rax
-
-preferred_addr:
-#endif
 
 	/* Setup data segments. */
 	xorl	%eax, %eax
@@ -372,6 +313,59 @@ preferred_addr:
 ENDPROC(startup_64)
 
 #ifdef CONFIG_EFI_STUB
+
+/* The entry point for the PE/COFF executable is efi_pe_entry. */
+ENTRY(efi_pe_entry)
+	movq	%rcx, efi64_config(%rip)	/* Handle */
+	movq	%rdx, efi64_config+8(%rip) /* EFI System table pointer */
+
+	leaq	efi64_config(%rip), %rax
+	movq	%rax, efi_config(%rip)
+
+	call	1f
+1:	popq	%rbp
+	subq	$1b, %rbp
+
+	/*
+	 * Relocate efi_config->call().
+	 */
+	addq	%rbp, efi64_config+32(%rip)
+
+	movq	%rax, %rdi
+	call	make_boot_params
+	cmpq	$0,%rax
+	je	fail
+	mov	%rax, %rsi
+	leaq	startup_32(%rip), %rax
+	movl	%eax, BP_code32_start(%rsi)
+	jmp	2f		/* Skip the relocation */
+
+handover_entry:
+	call	1f
+1:	popq	%rbp
+	subq	$1b, %rbp
+
+	/*
+	 * Relocate efi_config->call().
+	 */
+	movq	efi_config(%rip), %rax
+	addq	%rbp, 32(%rax)
+2:
+	movq	efi_config(%rip), %rdi
+	call	efi_main
+	movq	%rax,%rsi
+	cmpq	$0,%rax
+	jne	2f
+fail:
+	/* EFI init failed, so hang. */
+	hlt
+	jmp	fail
+2:
+	movl	BP_code32_start(%esi), %eax
+	leaq	startup_64(%rax), %rax
+	jmp	*%rax
+ENDPROC(efi_pe_entry)
+
 	.org 0x390
 ENTRY(efi64_stub_entry)
 	movq	%rdi, efi64_config(%rip)	/* Handle */
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1583327 — [PATCH 03/10] x86: boot, annotate functions properly

1) GLOBAL is meant for global symbols, but not functions. Use ENTRY
   which is dedicated for global functions.
2) Finish every function with ENDPROC.

Note that efi_pe_entry is not a start of a function, it is the middle of
startup_64 -- annotate as such.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: <x86@kernel.org>

---

The alternative to the startup_64 change would be to move efi_pe_entry
out of startup_64 which actually makes more sense, but I am afraid I
cannot test the result properly. See what it is now:
	.org 0x200
	ENTRY(startup_64)
	#ifdef CONFIG_EFI_STUB
		jmp     preferred_addr
	GLOBAL(efi_pe_entry)
		... ; a lot of assembly (efi_pe_entry)
		leaq    preferred_addr(%rax), %rax
		jmp     *%rax
	preferred_addr:
	#endif
		... ; a lot of assembly (startup_64)
	ENDPROC(startup_64)

What about:
	.org 0x200
	ENTRY(startup_64)
		... ; a lot of assembly (startup_64)
	ENDPROC(startup_64)

	#ifdef CONFIG_EFI_STUB
	ENTRY(efi_pe_entry)
		... ; a lot of assembly (efi_pe_entry)
		leaq    startup_64(%rax), %rax
		jmp     *%rax
	ENDPROC(efi_pe_entry)
	#endif
This solution is at the end of the series as RFC.
---
 arch/x86/boot/compressed/head_64.S |  3 ++-
 arch/x86/boot/copy.S               | 12 ++++++------
 arch/x86/boot/pmjump.S             |  4 ++--
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 4d85e600db78..59eccbc46ad7 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -250,7 +250,7 @@ ENTRY(startup_64)
 	 */
 	jmp	preferred_addr
 
-ENTRY(efi_pe_entry)
+GLOBAL(efi_pe_entry)
 	movq	%rcx, efi64_config(%rip)	/* Handle */
 	movq	%rdx, efi64_config+8(%rip) /* EFI System table pointer */
 
@@ -369,6 +369,7 @@ preferred_addr:
  */
 	leaq	relocated(%rbx), %rax
 	jmp	*%rax
+ENDPROC(startup_64)
 
 #ifdef CONFIG_EFI_STUB
 	.org 0x390
diff --git a/arch/x86/boot/copy.S b/arch/x86/boot/copy.S
index 1eb7d298b47d..44133dd0218e 100644
--- a/arch/x86/boot/copy.S
+++ b/arch/x86/boot/copy.S
@@ -17,7 +17,7 @@
 	.code16
 	.text
 
-GLOBAL(memcpy)
+ENTRY(memcpy)
 	pushw	%si
 	pushw	%di
 	movw	%ax, %di
@@ -33,7 +33,7 @@ GLOBAL(memcpy)
 	retl
 ENDPROC(memcpy)
 
-GLOBAL(memset)
+ENTRY(memset)
 	pushw	%di
 	movw	%ax, %di
 	movzbl	%dl, %eax
@@ -48,7 +48,7 @@ GLOBAL(memset)
 	retl
 ENDPROC(memset)
 
-GLOBAL(copy_from_fs)
+ENTRY(copy_from_fs)
 	pushw	%ds
 	pushw	%fs
 	popw	%ds
@@ -57,7 +57,7 @@ GLOBAL(copy_from_fs)
 	retl
 ENDPROC(copy_from_fs)
 
-GLOBAL(copy_to_fs)
+ENTRY(copy_to_fs)
 	pushw	%es
 	pushw	%fs
 	popw	%es
@@ -67,7 +67,7 @@ GLOBAL(copy_to_fs)
 ENDPROC(copy_to_fs)
 
 #if 0 /* Not currently used, but can be enabled as needed */
-GLOBAL(copy_from_gs)
+ENTRY(copy_from_gs)
 	pushw	%ds
 	pushw	%gs
 	popw	%ds
@@ -76,7 +76,7 @@ GLOBAL(copy_from_gs)
 	retl
 ENDPROC(copy_from_gs)
 
-GLOBAL(copy_to_gs)
+ENTRY(copy_to_gs)
 	pushw	%es
 	pushw	%gs
 	popw	%es
diff --git a/arch/x86/boot/pmjump.S b/arch/x86/boot/pmjump.S
index 3e0edc6d2a20..6528f78a79b5 100644
--- a/arch/x86/boot/pmjump.S
+++ b/arch/x86/boot/pmjump.S
@@ -23,7 +23,7 @@
 /*
  * void protected_mode_jump(u32 entrypoint, u32 bootparams);
  */
-GLOBAL(protected_mode_jump)
+ENTRY(protected_mode_jump)
 	movl	%edx, %esi		# Pointer to boot_params table
 
 	xorl	%ebx, %ebx
@@ -48,7 +48,7 @@ ENDPROC(protected_mode_jump)
 
 	.code32
 	.section ".text32","ax"
-GLOBAL(in_pm32)
+ENTRY(in_pm32)
 	# Set up data segments for flat 32-bit mode
 	movl	%ecx, %ds
 	movl	%ecx, %es
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1583339

On 17/02/17 11:47, Jiri Slaby wrote:
> This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END,
> and other macros across x86. When we have all this sorted out, this will
> help to inject DWARF unwinding info by objtool later.
> 
> So, let us use the macros this way:
> * ENTRY -- start of a global function
> * ENDPROC -- end of a local/global function
> * GLOBAL -- start of a globally visible data symbol
> * END -- end of local/global data symbol
> 
> The goal is forcing ENTRY to emit .cfi_startproc and ENDPROC to emit
> .cfi_endproc.
> 
> This particular patch makes proper use of GLOBAL on data and ENTRY on a
> function which was not the case on 4 locations.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: <x86@kernel.org>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: <xen-devel@lists.xenproject.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: <linux-pm@vger.kernel.org>
> ---
>  arch/x86/kernel/acpi/wakeup_64.S | 14 +++++++-------
>  arch/x86/kernel/head_64.S        |  2 +-
>  arch/x86/kernel/mcount_64.S      |  2 +-
>  arch/x86/xen/xen-head.S          |  2 +-
>  4 files changed, 10 insertions(+), 10 deletions(-)

Xen parts:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[toc] | [prev] | [next] | [standalone]

#1590150

On 03/01/2017, 10:38 AM, Ingo Molnar wrote:
> Agreed?

Sure. I wanted to keep it minimal to see if you agree with this
direction at all. No problem to be more intrusive :).

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1590168

On Wed, 1 Mar 2017, Ingo Molnar wrote:
> 
> * Jiri Slaby <jslaby@suse.cz> wrote:
> 
> > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END,
> > and other macros across x86. When we have all this sorted out, this will
> > help to inject DWARF unwinding info by objtool later.
> > 
> > So, let us use the macros this way:
> > * ENTRY -- start of a global function
> > * ENDPROC -- end of a local/global function
> > * GLOBAL -- start of a globally visible data symbol
> > * END -- end of local/global data symbol
> 
> So how about using macro names that actually show the purpose, instead of 
> importing all the crappy, historic, essentially randomly chosen debug symbol macro 
> names from the binutils and older kernels?
> 
> Something sane, like:
> 
> 	SYM__FUNCTION_START

Sane would be:

     	SYM_FUNCTION_START

The double underscore is just not giving any value.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1590248

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 1 Mar 2017, Ingo Molnar wrote:
> > 
> > * Jiri Slaby <jslaby@suse.cz> wrote:
> > 
> > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END,
> > > and other macros across x86. When we have all this sorted out, this will
> > > help to inject DWARF unwinding info by objtool later.
> > > 
> > > So, let us use the macros this way:
> > > * ENTRY -- start of a global function
> > > * ENDPROC -- end of a local/global function
> > > * GLOBAL -- start of a globally visible data symbol
> > > * END -- end of local/global data symbol
> > 
> > So how about using macro names that actually show the purpose, instead of 
> > importing all the crappy, historic, essentially randomly chosen debug symbol macro 
> > names from the binutils and older kernels?
> > 
> > Something sane, like:
> > 
> > 	SYM__FUNCTION_START
> 
> Sane would be:
> 
>      	SYM_FUNCTION_START
> 
> The double underscore is just not giving any value.

So the double underscore (at least in my view) has two advantages:

1) it helps separate the prefix from the postfix.

I.e. it's a 'symbols' namespace, and a 'function start', not the 'start' of a 
'symbol function'.

2) It also helps easy greppability.

Try this in latest -tip:

  git grep e820__

To see all the E820 API calls - with no false positives!

'git grep e820_' on the other hand is a lot less reliable...

But no strong feelings either way, I just try to sneak in these small namespace 
structure tricks when nobody's looking! ;-)

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1591982

On 03/01/2017, 11:27 AM, Ingo Molnar wrote:
> But no strong feelings either way, I just try to sneak in these small namespace 
> structure tricks when nobody's looking! ;-)

So I assume to introduce two underscores.

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1592196

On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> On Wed, 1 Mar 2017, Ingo Molnar wrote:
>> > 
>> > * Jiri Slaby <jslaby@suse.cz> wrote:
>> > 
>> > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL,
>END,
>> > > and other macros across x86. When we have all this sorted out,
>this will
>> > > help to inject DWARF unwinding info by objtool later.
>> > > 
>> > > So, let us use the macros this way:
>> > > * ENTRY -- start of a global function
>> > > * ENDPROC -- end of a local/global function
>> > > * GLOBAL -- start of a globally visible data symbol
>> > > * END -- end of local/global data symbol
>> > 
>> > So how about using macro names that actually show the purpose,
>instead of 
>> > importing all the crappy, historic, essentially randomly chosen
>debug symbol macro 
>> > names from the binutils and older kernels?
>> > 
>> > Something sane, like:
>> > 
>> > 	SYM__FUNCTION_START
>> 
>> Sane would be:
>> 
>>      	SYM_FUNCTION_START
>> 
>> The double underscore is just not giving any value.
>
>So the double underscore (at least in my view) has two advantages:
>
>1) it helps separate the prefix from the postfix.
>
>I.e. it's a 'symbols' namespace, and a 'function start', not the
>'start' of a 
>'symbol function'.
>
>2) It also helps easy greppability.
>
>Try this in latest -tip:
>
>  git grep e820__
>
>To see all the E820 API calls - with no false positives!
>
>'git grep e820_' on the other hand is a lot less reliable...
>
>But no strong feelings either way, I just try to sneak in these small
>namespace 
>structure tricks when nobody's looking! ;-)
>
>Thanks,
>
>	Ingo

This seems needlessly verbose to me and clutters the code.

How about:

PROC..ENDPROC, LOCALPROC..ENDPROC and DATA..ENDDATA.  Clear, unambiguous and balanced.


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1593402

On 03/03/2017, 07:20 PM, hpa@zytor.com wrote:
> On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>>> On Wed, 1 Mar 2017, Ingo Molnar wrote:
>>>>
>>>> * Jiri Slaby <jslaby@suse.cz> wrote:
>>>>
>>>>> This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL,
>> END,
>>>>> and other macros across x86. When we have all this sorted out,
>> this will
>>>>> help to inject DWARF unwinding info by objtool later.
>>>>>
>>>>> So, let us use the macros this way:
>>>>> * ENTRY -- start of a global function
>>>>> * ENDPROC -- end of a local/global function
>>>>> * GLOBAL -- start of a globally visible data symbol
>>>>> * END -- end of local/global data symbol
>>>>
>>>> So how about using macro names that actually show the purpose,
>> instead of 
>>>> importing all the crappy, historic, essentially randomly chosen
>> debug symbol macro 
>>>> names from the binutils and older kernels?
>>>>
>>>> Something sane, like:
>>>>
>>>> 	SYM__FUNCTION_START
>>>
>>> Sane would be:
>>>
>>>      	SYM_FUNCTION_START
>>>
>>> The double underscore is just not giving any value.
>>
>> So the double underscore (at least in my view) has two advantages:
>>
>> 1) it helps separate the prefix from the postfix.
>>
>> I.e. it's a 'symbols' namespace, and a 'function start', not the
>> 'start' of a 
>> 'symbol function'.
>>
>> 2) It also helps easy greppability.
>>
>> Try this in latest -tip:
>>
>>  git grep e820__
>>
>> To see all the E820 API calls - with no false positives!
>>
>> 'git grep e820_' on the other hand is a lot less reliable...
>>
>> But no strong feelings either way, I just try to sneak in these small
>> namespace 
>> structure tricks when nobody's looking! ;-)
>>
>> Thanks,
>>
>> 	Ingo
> 
> This seems needlessly verbose to me and clutters the code.
> 
> How about:
> 
> PROC..ENDPROC, LOCALPROC..ENDPROC and DATA..ENDDATA.  Clear, unambiguous and balanced.

I tried this, but:
arch/x86/kernel/relocate_kernel_64.S:27:0: warning: "DATA" redefined
 #define DATA(offset)  (KEXEC_CONTROL_CODE_MAX_SIZE+(offset))


I am not saying that I cannot fix it up. I just want to say, that these
names might be too generic, especially "PROC" and "DATA". So should I
really stick to these?

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1593956

* hpa@zytor.com <hpa@zytor.com> wrote:

> On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >* Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> >> On Wed, 1 Mar 2017, Ingo Molnar wrote:
> >> > 
> >> > * Jiri Slaby <jslaby@suse.cz> wrote:
> >> > 
> >> > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL,
> >END,
> >> > > and other macros across x86. When we have all this sorted out,
> >this will
> >> > > help to inject DWARF unwinding info by objtool later.
> >> > > 
> >> > > So, let us use the macros this way:
> >> > > * ENTRY -- start of a global function
> >> > > * ENDPROC -- end of a local/global function
> >> > > * GLOBAL -- start of a globally visible data symbol
> >> > > * END -- end of local/global data symbol
> >> > 
> >> > So how about using macro names that actually show the purpose,
> >instead of 
> >> > importing all the crappy, historic, essentially randomly chosen
> >debug symbol macro 
> >> > names from the binutils and older kernels?
> >> > 
> >> > Something sane, like:
> >> > 
> >> > 	SYM__FUNCTION_START
> >> 
> >> Sane would be:
> >> 
> >>      	SYM_FUNCTION_START
> >> 
> >> The double underscore is just not giving any value.
> >
> >So the double underscore (at least in my view) has two advantages:
> >
> >1) it helps separate the prefix from the postfix.
> >
> >I.e. it's a 'symbols' namespace, and a 'function start', not the
> >'start' of a 
> >'symbol function'.
> >
> >2) It also helps easy greppability.
> >
> >Try this in latest -tip:
> >
> >  git grep e820__
> >
> >To see all the E820 API calls - with no false positives!
> >
> >'git grep e820_' on the other hand is a lot less reliable...
> >
> >But no strong feelings either way, I just try to sneak in these small
> >namespace 
> >structure tricks when nobody's looking! ;-)
> >
> >Thanks,
> >
> >	Ingo
> 
> This seems needlessly verbose to me and clutters the code.
> 
> How about:
> 
> PROC..ENDPROC, LOCALPROC..ENDPROC and DATA..ENDDATA.  Clear, unambiguous and balanced.

I'm sorry, but that naming scheme is disgusing, it reminds me of BASIC 
nomenclature ... did we run out of underscores or what?

Nor would clearly structured names clutter anything, this isn't going to be used 
deep inside nested code, it's going to be the single level syntactic term in 
addition to the symbol name itself:

	SYM__FUNCTION_START(some_kernel_asm_function)
	...
	SYM__FUNCTION_END(some_kernel_asm_function)

We could shorten it to 'FUNC' if length is really an issue:

	SYM__FUNC_START(some_kernel_asm_function)
	...
	SYM__FUNC_END(some_kernel_asm_function)

Also, 'PROC', presumably standing for 'procedure', is both ambiguous and a 
misnomer:

 - it's ambiguous with commonly used abbreviations of procfs and process

 - C functions are not actually 'procedures'. Procedures in PASCAL style languages
   denote functions that don't return any values. Most of the kernel asm functions
   actually do. I realize that even in C we sometimes talk about 'procedures' out
   of hysterical inertia, but if you check the C standards, most of them don't
   even use the term 'procedure'.

'function' on the other hand is totally unambiguous.

Plus the lack of START/STOP (or BEGIN/END) makes it easy to commit the mistake of 
forgetting to add the end marker, and your naming scheme preserves that!

So if we fix/extend these macros then _PLEASE_ we need to get this stupid, 
illogical, nonsensical, external tooling inherited naming craziness fixed first, 
not doubled down on...

</rant>

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1592252

On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> On Wed, 1 Mar 2017, Ingo Molnar wrote:
>> > 
>> > * Jiri Slaby <jslaby@suse.cz> wrote:
>> > 
>> > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL,
>END,
>> > > and other macros across x86. When we have all this sorted out,
>this will
>> > > help to inject DWARF unwinding info by objtool later.
>> > > 
>> > > So, let us use the macros this way:
>> > > * ENTRY -- start of a global function
>> > > * ENDPROC -- end of a local/global function
>> > > * GLOBAL -- start of a globally visible data symbol
>> > > * END -- end of local/global data symbol
>> > 
>> > So how about using macro names that actually show the purpose,
>instead of 
>> > importing all the crappy, historic, essentially randomly chosen
>debug symbol macro 
>> > names from the binutils and older kernels?
>> > 
>> > Something sane, like:
>> > 
>> > 	SYM__FUNCTION_START
>> 
>> Sane would be:
>> 
>>      	SYM_FUNCTION_START
>> 
>> The double underscore is just not giving any value.
>
>So the double underscore (at least in my view) has two advantages:
>
>1) it helps separate the prefix from the postfix.
>
>I.e. it's a 'symbols' namespace, and a 'function start', not the
>'start' of a 
>'symbol function'.
>
>2) It also helps easy greppability.
>
>Try this in latest -tip:
>
>  git grep e820__
>
>To see all the E820 API calls - with no false positives!
>
>'git grep e820_' on the other hand is a lot less reliable...
>
>But no strong feelings either way, I just try to sneak in these small
>namespace 
>structure tricks when nobody's looking! ;-)
>
>Thanks,
>
>	Ingo

IMO these little "namespace tricks" especially for small common macros like we are taking about here make the code very frustrating to read, and even more to write.  Noone would design a programming language that way, and things like PROC are really just substitutes for proper language features (and could even be as assembly rather than cpp macros.)

When I say frustrating I mean, at least for me, full-Ballmer launch-chair-at-monitor level frustrating.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1593988

* hpa@zytor.com <hpa@zytor.com> wrote:

> On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote:

> >> > So how about using macro names that actually show the purpose, instead of 
> >> > importing all the crappy, historic, essentially randomly chosen debug 
> >> > symbol macro names from the binutils and older kernels?
> >> > 
> >> > Something sane, like:
> >> > 
> >> > 	SYM__FUNCTION_START
> >> 
> >> Sane would be:
> >> 
> >>      	SYM_FUNCTION_START
> >> 
> >> The double underscore is just not giving any value.
> >
> > So the double underscore (at least in my view) has two advantages:
> >
> > 1) it helps separate the prefix from the postfix.
> >
> > I.e. it's a 'symbols' namespace, and a 'function start', not the 'start' of a 
> > 'symbol function'.
> >
> > 2) It also helps easy greppability.
> >
> > Try this in latest -tip:
> >
> >    git grep e820__
> >
> > To see all the E820 API calls - with no false positives!
> >
> > 'git grep e820_' on the other hand is a lot less reliable...
> 
> IMO these little "namespace tricks" especially for small common macros like we 
> are taking about here make the code very frustrating to read, and even more to 
> write.  Noone would design a programming language that way, and things like PROC 
> are really just substitutes for proper language features (and could even be as 
> assembly rather than cpp macros.)

This is a totally different thing from language keywords which needs to be short 
and concise for obvious reasons.

Keywords of languages get nested and are used all the time, and everyone needs to 
know them and they need to stay out of the way. The symbol start/end macros we are 
talking about here are _MUCH_ less common, and they are only ever used in a single 
nesting level:

        SYM__FUNC_START(some_kernel_asm_function)
        ...
        SYM__FUNC_END(some_kernel_asm_function)

Most kernel developers writing new assembly code rarely know these constructs by 
heart, they just look them up and carbon copy existing practices. And guess what, 
the 'looking them up' gets harder if the macro naming scheme is an idosyncratic 
leftover from long ago.

Kernel developers _reading_ assembly code will know the exact purpose of the 
macros even less, especially if they are named in an ambiguous, illogical fashion.

Furthermore, your suggestion of:

> PROC..ENDPROC, LOCALPROC..ENDPROC and DATA..ENDDATA.  Clear, unambiguous and 
> balanced.

Are neither clear, not unambiguous nor balanced! I mean, they are the _exact_ 
opposite:

 - 'PROC' is actually ambiguous in the kernel source code context, as it clashes 
   with common abbreviations of 'procfs' and 'process'.

   It's also an unnecessary abbreviation of a word ('procedure') that is not 
   actually used a _single time_ in the C ISO/IEC 9899:TC2 standard - in all half 
   thousand+ pages of it. (!) Why the hell does this have to be used in the 
   kernel?

 - It's visually and semantically imbalanced, because some terms have an 'END' 
   prefix, but there's no matching 'START' or 'BEGIN' prefix for their 
   counterparts. This makes it easy to commit various symbol definition 
   termination errors, like:

	PROC(some_kernel_asm_function)
	...

   Here it's not obvious that it needs an ENDPROC. While if it's written as:

        SYM__FUNC_START(some_kernel_asm_function)
        ...

   ... it's pretty obvious at first sight that an '_END' is missing!

 - What you suggest also has senselessly missing underscores, which makes it 
   _more_ cluttered and less clear. We clearly don't have addtowaitqueue() and
   removefromwaitqueue() function names in the kernel, right? Why should we have
   'ENDPROC' and 'ENDDATA' macro names?

 - Hierarchical naming schemes generally tend to put the more generic category
   name first, not last. So it's:

	mutex_init()
	mutex_lock()
	mutex_unlock()
	mutex_trylock()

   It's _NOT_ the other way around:

	init_mutex()
	lock_mutex()
	unlock_mutex()
	trylock_mutex()

   The prefix naming scheme is easier to read both visually/typographically 
   (because it aligns vertically in a natural fashion so it's easier to pattern 
   match), and also semantically: because when reading it it's easy to skip the 
   line once your brain reads the generic category of 'mutex'.

   But with 'ENDPROC' my brain both has to needlessly perform the following steps:

	- disambiguate the 'END' and the 'PROC'

	- fill in the missing underscore

	- and finally when arriving at the generic term 'PROC', discard it as
	  uninteresting

 - Short names have good use in programming languages, because everyone who uses
   that language knows what they are and they become a visual substitute for the
   language element.

   But assembly macros are _NOT_ a new language in this sense, they are actually 
   more similar to library function names: where brevity is actually
   counterintuitive and harmful, because they are ambiguous and pollute the 
   generic namespace. If you look at C library API function name best practices
   you'll see that the best ones are all hierarchically named and categorized,
   with the more generic category put first, they are unambiguously balanced even
   if that makes the names longer, they are clear and use underscores.

For all these reasons the naming scheme you suggest is one of the worst we could 
come up with! I mean, if I had to _intentionally_ engineer something as harmful as 
possible to readability and maintainability this would be pretty close to it...

I'm upset, because even a single minute of reflection should have told you all 
this. I mean, IMHO it's not even a close argument: your suggested naming scheme is 
bleeding from half a dozen of mortal wounds...

I can be convinced to drop the double underscores (I seem to be in the minority 
regard them), and I can be convinced that 'FUNC' is shorter and still easy to 
understand instead of 'FUNCTION', but other than that please stop the naming 
madness!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1594476 — [RFC] linkage: new macros for functions and data

SYM_LOCAL_ALIAS_START -- use where there are two local names for one
  code
SYM_ALIAS_START -- use where there are two global names for one code
SYM_LOCAL_FUNC_START -- use for local functions
SYM_FUNCTION_START -- use for global functions
SYM_WEAK_FUNC_START -- use for weak functions
SYM_ALIAS_END -- the end of LOCALALIASed or ALIASed code
SYM_FUNCTION_END -- the end of SYM_LOCAL_FUNC_START, SYM_FUNCTION_START,
  SYM_WEAK_FUNC_START, ...
SYM_DATA_START -- global data symbol
SYM_DATA_END -- the end of SYM_DATA_START symbol

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: hpa@zytor.com
Cc: Ingo Molnar <mingo@kernel.org>
Cc: jpoimboe@redhat.com
Cc: Juergen Gross <jgross@suse.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Cc: mingo@redhat.com
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: xen-devel@lists.xenproject.org
Cc: x86@kernel.org
---

So this is what I have ATM.

 include/linux/linkage.h | 87 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 72 insertions(+), 15 deletions(-)

diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index a6a42dd02466..79f634a57466 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -78,33 +78,90 @@
 #define ALIGN __ALIGN
 #define ALIGN_STR __ALIGN_STR
 
-#ifndef ENTRY
-#define ENTRY(name) \
-	.globl name ASM_NL \
+#ifndef ENTRY /* deprecated, use SYM_FUNCTION_START */
+#define ENTRY(name)	SYM_FUNCTION_START(name)
+#endif
+#endif /* LINKER_SCRIPT */
+
+/* === code annotations === */
+
+/* SYM_LOCAL_ALIAS_START -- use where there are two local names for one code */
+#ifndef SYM_LOCAL_ALIAS_START
+#define SYM_LOCAL_ALIAS_START(name) \
 	ALIGN ASM_NL \
 	name:
 #endif
-#endif /* LINKER_SCRIPT */
 
-#ifndef WEAK
-#define WEAK(name)	   \
+/* SYM_ALIAS_START -- use where there are two global names for one code */
+#ifndef SYM_ALIAS_START
+#define SYM_ALIAS_START(name) \
+	.globl name ASM_NL \
+	SYM_LOCAL_ALIAS_START(name)
+#endif
+
+/*
+ * so far the same as SYM_LOCAL_ALIAS_START, but we will need to distinguish
+ * them later
+ */
+/* SYM_LOCAL_FUNC_START -- use for local functions */
+#ifndef SYM_LOCAL_FUNC_START
+#define SYM_LOCAL_FUNC_START(name) \
+	SYM_LOCAL_ALIAS_START(name)
+#endif
+
+/* SYM_FUNCTION_START -- use for global functions */
+#ifndef SYM_FUNCTION_START
+#define SYM_FUNCTION_START(name) \
+	.globl name ASM_NL \
+	SYM_LOCAL_FUNC_START(name)
+#endif
+
+/* SYM_WEAK_FUNC_START -- use for weak functions */
+#ifndef SYM_WEAK_FUNC_START
+#define SYM_WEAK_FUNC_START(name) \
 	.weak name ASM_NL   \
 	name:
 #endif
 
-#ifndef END
-#define END(name) \
-	.size name, .-name
+/* SYM_ALIAS_END -- the end of LOCALALIASed or ALIASed code */
+#ifndef SYM_ALIAS_END
+#define SYM_ALIAS_END(name) \
+	.type name, @function ASM_NL \
+	SYM_DATA_END(name)
 #endif
 
 /* If symbol 'name' is treated as a subroutine (gets called, and returns)
- * then please use ENDPROC to mark 'name' as STT_FUNC for the benefit of
- * static analysis tools such as stack depth analyzer.
+ * then please use SYM_FUNCTION_END to mark 'name' as STT_FUNC for the benefit
+ * of static analysis tools such as stack depth analyzer.
  */
-#ifndef ENDPROC
-#define ENDPROC(name) \
-	.type name, @function ASM_NL \
-	END(name)
+/*
+ * SYM_FUNCTION_END -- the end of SYM_LOCAL_FUNC_START, SYM_FUNCTION_START,
+ * SYM_WEAK_FUNC_START, ...
+ */
+#ifndef SYM_FUNCTION_END
+#define SYM_FUNCTION_END(name) \
+	SYM_ALIAS_END(name) /* the same as for SYM_LOCAL_FUNC_START */
+#endif
+
+#ifndef WEAK /* deprecated, use SYM_WEAK_FUNC_START */
+#define WEAK(name)	SYM_WEAK_FUNC_START(name)
+#endif
+
+/* === data annotations === */
+
+/* SYM_DATA_START -- global data symbol */
+#ifndef SYM_DATA_START
+#define SYM_DATA_START(name)	SYM_FUNCTION_START(name)
+#endif
+
+/* SYM_DATA_END -- the end of SYM_DATA_START symbol */
+#ifndef SYM_DATA_END
+#define SYM_DATA_END(name) \
+	.size name, .-name
+#endif
+
+#ifndef END /* deprecated, use SYM_DATA_END */
+#define END(name) SYM_DATA_END(name)
 #endif
 
 #endif
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1590674

* Jiri Slaby <jslaby@suse.cz> wrote:

> This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END,
> and other macros across x86. When we have all this sorted out, this will
> help to inject DWARF unwinding info by objtool later.
> 
> So, let us use the macros this way:
> * ENTRY -- start of a global function
> * ENDPROC -- end of a local/global function
> * GLOBAL -- start of a globally visible data symbol
> * END -- end of local/global data symbol

So how about using macro names that actually show the purpose, instead of 
importing all the crappy, historic, essentially randomly chosen debug symbol macro 
names from the binutils and older kernels?

Something sane, like:

	SYM__FUNCTION_START
	SYM__FUNCTION_END

	SYM__DATA_START
	SYM__DATA_END

... and extend that macro namespace with any other variants we might need.

We can still keep the old macro names (for a short while) to ease the transition, 
but for heaven's sake, if we do "cleanups" before complicating the code let's make 
sure the result is actually readable!

Agreed?

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web