[PATCH v2 01/10] linkage: new macros for assembler symbols

[PATCH v2 01/10] linkage: new macros for assembler symbols

[Jiri Slaby]

Introduce new C macros for annotations of functions and data in
assembly. There is a long-term mess in macros like ENTRY, END, ENDPROC
and similar. They are used in different manners and sometimes
incorrectly.

So introduce macros with clear use to annotate assembly as follows:

a) Support macros
   SYM_T_FUNC -- type used by assembler to mark functions
   SYM_T_OBJECT -- type used by assembler to mark data

   They are defined as STT_FUNC and STT_OBJECT respectively. According
   to the gas manual, this is the most portable way. I am not sure about
   other assemblers, so we can switch this back to %function and %object
   if this turns into a problem. Architectures can also override them by
   something like ", @function" if need be.

   SYM_A_ALIGN, SYM_A_NOALIGN -- should we align the symbol?
   SYM_V_GLOBAL, SYM_V_WEAK, SYM_V_LOCAL -- visibility of symbols
b) Mostly internal annotations, used by the ones below
   SYM_START -- use only if you have to
   SYM_END -- use only if you have to
c) Generic annotations
d) Annotations for code
   SYM_FUNC_START_LOCAL_ALIAS -- use where there are two local names for
	one code
   SYM_FUNC_START_ALIAS -- use where there are two global names for one
	code
   SYM_FUNC_END_ALIAS -- the end of LOCAL_ALIASed or ALIASed code

   SYM_FUNC_START -- use for global functions
   SYM_FUNC_START_LOCAL -- use for local functions
   SYM_FUNC_START_WEAK -- use for weak functions
   SYM_FUNC_END -- the end of SYM_FUNC_START_LOCAL, SYM_FUNC_START,
	SYM_FUNC_START_WEAK, ...

   SYM_FUNC_INNER_LABEL -- only for global labels in the middle of
	functions
d) For data
   SYM_DATA_START -- global data symbol
   SYM_DATA_END -- the end of SYM_DATA_START symbol

==========

Note that SYM_FUNC_START_WEAK aligns symbols now too.

The macros allow to pair starts and ends of functions and mark function
correctly in the output ELF objects. This will also help a lot to
generate DWARF information automatically during build of asm.

Finally, all users of the old macros will be converted to use these
later.

[v2]
* use SYM_ prefix and sane names
* add SYM_START and SYM_END and parametrize all the macros

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
---
 arch/x86/include/asm/linkage.h |   5 +-
 include/linux/linkage.h        | 131 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 126 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/linkage.h b/arch/x86/include/asm/linkage.h
index 0ccb26dda126..a96f6fc36011 100644
--- a/arch/x86/include/asm/linkage.h
+++ b/arch/x86/include/asm/linkage.h
@@ -12,9 +12,8 @@
 
 #ifdef __ASSEMBLY__
 
-#define GLOBAL(name)	\
-	.globl name;	\
-	name:
+/* deprecated, use SYM_DATA_START, SYM_FUNC_START, or SYM_FUNC_INNER_LABEL */
+#define GLOBAL(name)	SYM_DATA_START(name)
 
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_ALIGNMENT_16)
 #define __ALIGN		.p2align 4, 0x90
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index a6a42dd02466..c1dc824d2bc6 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -74,25 +74,46 @@
 
 #ifdef __ASSEMBLY__
 
+/* SYM_T_FUNC -- type used by assembler to mark functions */
+#ifndef SYM_T_FUNC
+#define SYM_T_FUNC				STT_FUNC
+#endif
+
+/* SYM_T_OBJECT -- type used by assembler to mark data */
+#ifndef SYM_T_OBJECT
+#define SYM_T_OBJECT				STT_OBJECT
+#endif
+
+/* SYM_A_* -- should we align the symbol? */
+#define SYM_A_ALIGN				ALIGN
+#define SYM_A_NOALIGN				/* nothing */
+
+/* SYM_V_* -- visibility of symbols */
+#define SYM_V_GLOBAL(name)			.globl name
+#define SYM_V_WEAK(name)			.weak name
+#define SYM_V_LOCAL(name)			/* nothing */
+
 #ifndef LINKER_SCRIPT
 #define ALIGN __ALIGN
 #define ALIGN_STR __ALIGN_STR
 
+/* === DEPRECATED annotations === */
+
 #ifndef ENTRY
+/* deprecated, use SYM_FUNC_START */
 #define ENTRY(name) \
-	.globl name ASM_NL \
-	ALIGN ASM_NL \
-	name:
+	SYM_FUNC_START(name)
 #endif
 #endif /* LINKER_SCRIPT */
 
 #ifndef WEAK
+/* deprecated, use SYM_FUNC_START_WEAK */
 #define WEAK(name)	   \
-	.weak name ASM_NL   \
-	name:
+	SYM_FUNC_START_WEAK(name)
 #endif
 
 #ifndef END
+/* deprecated, use SYM_FUNC_END, SYM_DATA_END, or SYM_END */
 #define END(name) \
 	.size name, .-name
 #endif
@@ -102,9 +123,105 @@
  * static analysis tools such as stack depth analyzer.
  */
 #ifndef ENDPROC
+/* deprecated, use SYM_FUNC_END */
 #define ENDPROC(name) \
-	.type name, @function ASM_NL \
-	END(name)
+	SYM_FUNC_END(name)
+#endif
+
+/* === generic annotations === */
+
+/* SYM_START -- use only if you have to */
+#ifndef SYM_START
+#define SYM_START(name, align, visibility)		\
+	visibility(name) ASM_NL				\
+	align ASM_NL					\
+	name:
+#endif
+
+/* SYM_END -- use only if you have to */
+#ifndef SYM_END
+#define SYM_END(name, sym_type)				\
+	.type name sym_type ASM_NL			\
+	.size name, .-name
+#endif
+
+/* === code annotations === */
+
+/* SYM_FUNC_START_LOCAL_ALIAS -- use where there are two local names for one code */
+#ifndef SYM_FUNC_START_LOCAL_ALIAS
+#define SYM_FUNC_START_LOCAL_ALIAS(name)		\
+	SYM_START(name, SYM_A_ALIGN, SYM_V_LOCAL)
+#endif
+
+/* SYM_FUNC_START_ALIAS -- use where there are two global names for one code */
+#ifndef SYM_FUNC_START_ALIAS
+#define SYM_FUNC_START_ALIAS(name)			\
+	SYM_START(name, SYM_A_ALIGN, SYM_V_GLOBAL)
+#endif
+
+/* SYM_FUNC_START -- use for global functions */
+#ifndef SYM_FUNC_START
+/*
+ * The same as SYM_FUNC_START_ALIAS, but we will need to distinguish these two
+ * later.
+ */
+#define SYM_FUNC_START(name)				\
+	SYM_START(name, SYM_A_ALIGN, SYM_V_GLOBAL)
+#endif
+
+/* SYM_FUNC_START_LOCAL -- use for local functions */
+#ifndef SYM_FUNC_START_LOCAL
+/* the same as SYM_FUNC_START_LOCAL_ALIAS, see comment near SYM_FUNC_START */
+#define SYM_FUNC_START_LOCAL(name)			\
+	SYM_START(name, SYM_A_ALIGN, SYM_V_LOCAL)
+#endif
+
+/* SYM_FUNC_START_WEAK -- use for weak functions */
+#ifndef SYM_FUNC_START_WEAK
+#define SYM_FUNC_START_WEAK(name)			\
+	SYM_START(name, SYM_A_ALIGN, SYM_V_WEAK)
+#endif
+
+/* SYM_FUNC_END_ALIAS -- the end of LOCAL_ALIASed or ALIASed code */
+#ifndef SYM_FUNC_END_ALIAS
+#define SYM_FUNC_END_ALIAS(name)			\
+	SYM_END(name, SYM_T_FUNC)
+#endif
+
+/*
+ * SYM_FUNC_END -- the end of SYM_FUNC_START_LOCAL, SYM_FUNC_START,
+ * SYM_FUNC_START_WEAK, ...
+ */
+#ifndef SYM_FUNC_END
+/* the same as SYM_FUNC_END_ALIAS, see comment near SYM_FUNC_START */
+#define SYM_FUNC_END(name)				\
+	SYM_END(name, SYM_T_FUNC)
+#endif
+
+/* SYM_FUNC_INNER_LABEL -- only for global labels to the middle of functions */
+#ifndef SYM_FUNC_INNER_LABEL
+#define SYM_FUNC_INNER_LABEL(name)			\
+	SYM_START(name, SYM_A_NOALIGN, SYM_V_GLOBAL)
+#endif
+
+/* === data annotations === */
+
+/* SYM_DATA_START -- global data symbol */
+#ifndef SYM_DATA_START
+#define SYM_DATA_START(name)				\
+	SYM_START(name, SYM_A_NOALIGN, SYM_V_GLOBAL)
+#endif
+
+/* SYM_DATA_START -- local data symbol */
+#ifndef SYM_DATA_START_LOCAL
+#define SYM_DATA_START_LOCAL(name)			\
+	SYM_START(name, SYM_A_NOALIGN, SYM_V_LOCAL)
+#endif
+
+/* SYM_DATA_END -- the end of SYM_DATA_START symbol */
+#ifndef SYM_DATA_END
+#define SYM_DATA_END(name)				\
+	SYM_END(name, SYM_T_OBJECT)
 #endif
 
 #endif
-- 
2.12.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

This is a start of series to cleanup macros used for starting functions,
data, globals etc. across x86. When we have all this sorted out, this
will help to inject DWARF unwinding info by objtool later.

The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
SYM_FUNC_END to emit .cfi_endproc. Automatically at best.

This particular patch makes proper use of SYM_DATA_START on data and
SYM_FUNC_START 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>
Reviewed-by: Juergen Gross <jgross@suse.com> [xen parts]
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/entry/entry_64_compat.S |  3 +--
 arch/x86/kernel/acpi/wakeup_64.S | 14 +++++++-------
 arch/x86/kernel/head_64.S        |  2 +-
 arch/x86/kernel/mcount_64.S      |  2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index e1721dafbcb1..73d7ff0b125c 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -342,8 +342,7 @@ ENTRY(entry_INT80_compat)
 	jmp	restore_regs_and_iret
 END(entry_INT80_compat)
 
-	ALIGN
-GLOBAL(stub32_clone)
+SYM_FUNC_START(stub32_clone)
 	/*
 	 * The 32-bit clone ABI is: clone(..., int tls_val, int *child_tidptr).
 	 * The 64-bit clone ABI is: clone(..., int *child_tidptr, int tls_val).
diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
index 50b8ed0317a3..0b5a5573f57d 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
+SYM_DATA_START(saved_rbp)		.quad	0
+SYM_DATA_START(saved_rsi)		.quad	0
+SYM_DATA_START(saved_rdi)		.quad	0
+SYM_DATA_START(saved_rbx)		.quad	0
 
-ENTRY(saved_rip)	.quad	0
-ENTRY(saved_rsp)	.quad	0
+SYM_DATA_START(saved_rip)		.quad	0
+SYM_DATA_START(saved_rsp)		.quad	0
 
-ENTRY(saved_magic)	.quad	0
+SYM_DATA_START(saved_magic)	.quad	0
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index ac9d327d2e42..e093a804f1fb 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -487,7 +487,7 @@ early_gdt_descr:
 early_gdt_descr_base:
 	.quad	INIT_PER_CPU_VAR(gdt_page)
 
-ENTRY(phys_base)
+SYM_DATA_START(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 7b0d3da52fb4..2b4d7045e823 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -320,7 +320,7 @@ ENTRY(ftrace_graph_caller)
 	retq
 END(ftrace_graph_caller)
 
-GLOBAL(return_to_handler)
+SYM_FUNC_START(return_to_handler)
 	subq  $24, %rsp
 
 	/* Save the return values */
-- 
2.12.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
> This is a start of series to cleanup macros used for starting functions,
> data, globals etc. across x86. When we have all this sorted out, this
> will help to inject DWARF unwinding info by objtool later.
> 
> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.

Do we still want to emit .cfi_startproc/endproc from the macro?  From
our last discussion, that seemed to be up in the air.

  https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble

What did you think about making CFI read-only for .c object files and
write-only for .S object files?

-- 
Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote:
> On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
>> This is a start of series to cleanup macros used for starting functions,
>> data, globals etc. across x86. When we have all this sorted out, this
>> will help to inject DWARF unwinding info by objtool later.
>>
>> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
>> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.
> 
> Do we still want to emit .cfi_startproc/endproc from the macro?  From
> our last discussion, that seemed to be up in the air.
> 
>   https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble

"Automatically at best" above means "completely from objtool". I am
still uncertain whether it will work 100% or we would have to help by
generating some pieces from the added macros. In particular, the ALIASes
are evil which cause harm here:

fun_alias:
fun:
<code>
.size fun, .-fun
.type fun STT_FUNC
.size fun_alias, .-fun_alias
.type fun_alias STT_FUNC

Both cannot create (overlapping) .cfi_startproc/endproc, only the inner
shall.

But it seems so far, that we might be able to deal with all of that from
objtool... (I have not been thinking about this particular thing deep
enough yet.) Some sort of "from the last label that is marked as
STT_FUNC till its .size" might work.

> What did you think about making CFI read-only for .c object files and
> write-only for .S object files?

There are those functions like sync_core() or native_save_fl() with
inline asm. And they seem to need a) read-write support, or b) manual
annotation. I would like to avoid b) for sure.

thanks,
-- 
js
suse labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Mon, Mar 20, 2017 at 04:32:09PM +0100, Jiri Slaby wrote:
> On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote:
> > On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
> >> This is a start of series to cleanup macros used for starting functions,
> >> data, globals etc. across x86. When we have all this sorted out, this
> >> will help to inject DWARF unwinding info by objtool later.
> >>
> >> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
> >> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.
> > 
> > Do we still want to emit .cfi_startproc/endproc from the macro?  From
> > our last discussion, that seemed to be up in the air.
> > 
> >   https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble
> 
> "Automatically at best" above means "completely from objtool". I am
> still uncertain whether it will work 100% or we would have to help by
> generating some pieces from the added macros. In particular, the ALIASes
> are evil which cause harm here:
> 
> fun_alias:
> fun:
> <code>
> .size fun, .-fun
> .type fun STT_FUNC
> .size fun_alias, .-fun_alias
> .type fun_alias STT_FUNC
> 
> Both cannot create (overlapping) .cfi_startproc/endproc, only the inner
> shall.
> 
> But it seems so far, that we might be able to deal with all of that from
> objtool... (I have not been thinking about this particular thing deep
> enough yet.) Some sort of "from the last label that is marked as
> STT_FUNC till its .size" might work.

Ok.

> > What did you think about making CFI read-only for .c object files and
> > write-only for .S object files?
> 
> There are those functions like sync_core() or native_save_fl() with
> inline asm. And they seem to need a) read-write support, or b) manual
> annotation. I would like to avoid b) for sure.

Ah, so I guess those inline asm functions cause problems because they
muck with the stack pointer with pushes and pops?

I don't think manual annotation of inline asm would be so bad.  IIUC, it
would only mean replacing the pushes and pops with a macro which does
the CFI-annotated version, like PUSH_CFI and POP_CFI.  And the benefit
would be that objtool doesn't have to try to rewrite a bunch of .c
object files.

Objtool read-write worries me because it gives more responsibility to
objtool.  It could be tricky to insert CFI instructions within the ones
already created by gcc.  Also, while unlikely, a bug in objtool could
theoretically corrupt an object file and brick the kernel.  Also I
wonder how all those extra file writes would affect build performance.

If at all possible, I would rather objtool stay out of the way of the
compiler and let gcc do its job of generating CFI.

-- 
Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 660 bytes --]

Hi!

> -ENTRY(saved_rbp)	.quad	0
> -ENTRY(saved_rsi)	.quad	0
> -ENTRY(saved_rdi)	.quad	0
> -ENTRY(saved_rbx)	.quad	0
> +SYM_DATA_START(saved_rbp)		.quad	0
> +SYM_DATA_START(saved_rsi)		.quad	0
> +SYM_DATA_START(saved_rdi)		.quad	0
> +SYM_DATA_START(saved_rbx)		.quad	0

Does it make sense to call it SYM_DATA_*START* when there's no
corresponding end?

Plus... it looks like saved_rsi (and friends) are only used inside
wakeup_64.S. Could we just delete the "ENTRY" annotations?

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

* Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > -ENTRY(saved_rbp)	.quad	0
> > -ENTRY(saved_rsi)	.quad	0
> > -ENTRY(saved_rdi)	.quad	0
> > -ENTRY(saved_rbx)	.quad	0
> > +SYM_DATA_START(saved_rbp)		.quad	0
> > +SYM_DATA_START(saved_rsi)		.quad	0
> > +SYM_DATA_START(saved_rdi)		.quad	0
> > +SYM_DATA_START(saved_rbx)		.quad	0
> 
> Does it make sense to call it SYM_DATA_*START* when there's no
> corresponding end?

That looks like a bug - I think we should strive for them to always be in pairs.

Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' 
SYM_*_START() uses? This could be done by emitting debug data into a special 
section and then analyzing that section for unpaired entries. The section can be 
discarded in the final link, it won't show up in the kernel image.

We don't ever nest symbols, right?

> Plus... it looks like saved_rsi (and friends) are only used inside
> wakeup_64.S. Could we just delete the "ENTRY" annotations?

That appears to make sense as well.

Thanks,

	Ingo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> 
> * Pavel Machek <pavel@ucw.cz> wrote:
> 
>> Hi!
>>
>>> -ENTRY(saved_rbp)	.quad	0
>>> -ENTRY(saved_rsi)	.quad	0
>>> -ENTRY(saved_rdi)	.quad	0
>>> -ENTRY(saved_rbx)	.quad	0
>>> +SYM_DATA_START(saved_rbp)		.quad	0
>>> +SYM_DATA_START(saved_rsi)		.quad	0
>>> +SYM_DATA_START(saved_rdi)		.quad	0
>>> +SYM_DATA_START(saved_rbx)		.quad	0
>>
>> Does it make sense to call it SYM_DATA_*START* when there's no
>> corresponding end?
> 
> That looks like a bug - I think we should strive for them to always be in pairs.
> 
> Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' 
> SYM_*_START() uses? This could be done by emitting debug data into a special 
> section and then analyzing that section for unpaired entries. The section can be 
> discarded in the final link, it won't show up in the kernel image.

It should be easier than that. No introduction of other info needed --
every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should
be a bug now.

> We don't ever nest symbols, right?

AFAI could see so far, correct.

>> Plus... it looks like saved_rsi (and friends) are only used inside
>> wakeup_64.S. Could we just delete the "ENTRY" annotations?
> 
> That appears to make sense as well.

+1, will fix this.

thanks,
-- 
js
suse labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

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

> On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > 
> > * Pavel Machek <pavel@ucw.cz> wrote:
> > 
> >> Hi!
> >>
> >>> -ENTRY(saved_rbp)	.quad	0
> >>> -ENTRY(saved_rsi)	.quad	0
> >>> -ENTRY(saved_rdi)	.quad	0
> >>> -ENTRY(saved_rbx)	.quad	0
> >>> +SYM_DATA_START(saved_rbp)		.quad	0
> >>> +SYM_DATA_START(saved_rsi)		.quad	0
> >>> +SYM_DATA_START(saved_rdi)		.quad	0
> >>> +SYM_DATA_START(saved_rbx)		.quad	0
> >>
> >> Does it make sense to call it SYM_DATA_*START* when there's no
> >> corresponding end?
> > 
> > That looks like a bug - I think we should strive for them to always be in pairs.
> > 
> > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' 
> > SYM_*_START() uses? This could be done by emitting debug data into a special 
> > section and then analyzing that section for unpaired entries. The section can be 
> > discarded in the final link, it won't show up in the kernel image.
> 
> It should be easier than that. No introduction of other info needed --
> every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should
> be a bug now.

I'm all for that!

Can we detect double ends as well - i.e. do a build check of the full syntax of 
these symbol definition primitives?

Thanks,

	Ingo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> 
> * Jiri Slaby <jslaby@suse.cz> wrote:
> 
> > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > 
> > > * Pavel Machek <pavel@ucw.cz> wrote:
> > > 
> > >> Hi!
> > >>
> > >>> -ENTRY(saved_rbp)	.quad	0
> > >>> -ENTRY(saved_rsi)	.quad	0
> > >>> -ENTRY(saved_rdi)	.quad	0
> > >>> -ENTRY(saved_rbx)	.quad	0
> > >>> +SYM_DATA_START(saved_rbp)		.quad	0
> > >>> +SYM_DATA_START(saved_rsi)		.quad	0
> > >>> +SYM_DATA_START(saved_rdi)		.quad	0
> > >>> +SYM_DATA_START(saved_rbx)		.quad	0
> > >>
> > >> Does it make sense to call it SYM_DATA_*START* when there's no
> > >> corresponding end?
> > > 
> > > That looks like a bug - I think we should strive for them to always be in pairs.
> > > 
> > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' 
> > > SYM_*_START() uses? This could be done by emitting debug data into a special 
> > > section and then analyzing that section for unpaired entries. The section can be 
> > > discarded in the final link, it won't show up in the kernel image.
> > 
> > It should be easier than that. No introduction of other info needed --
> > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should
> > be a bug now.
> 
> I'm all for that!

It would be easy to add this checking to objtool since it already reads
the symbol table.  The hard part is figuring out the logistics. :-)

- Should the warnings be on by default?

- Part of the "objtool check" command or something else?

- Separate config option or just include it with
  CONFIG_STACK_VALIDATION?

- Should all asm files be checked, including those currently skipped by
  objtool with OBJECT_FILES_NON_STANDARD?

> Can we detect double ends as well - i.e. do a build check of the full syntax of 
> these symbol definition primitives?

Detecting double ends would be a little trickier.  The second SYM_*_END
supersedes the first, so that information isn't in the ELF symbol table.

We could use a special section to annotate all the macro uses and have
objtool do the checking, similar to what you suggested earlier.

Or, here's a much easier way to do it, without involving objtool:

--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -138,9 +138,17 @@
 	name:
 #endif
 
+#ifndef CHECK_DUP_SYM_END
+#define CHECK_DUP_SYM_END(name)				\
+	.pushsection .discard.sym_func_end ASM_NL	\
+	SYM_END_##name: .byte 0 ASM_NL			\
+	.popsection
+#endif
+
 /* SYM_END -- use only if you have to */
 #ifndef SYM_END
 #define SYM_END(name, sym_type)				\
+	CHECK_DUP_SYM_END(name) ASM_NL			\
 	.type name sym_type ASM_NL			\
 	.size name, .-name
 #endif


If there's an extra SYM_*_END, the build fails.  For example, if I add
an extra SYM_FUNC_END(\name) to the THUNK macro:

    AS      arch/x86/entry/thunk_64.o
  arch/x86/entry/thunk_64.S: Assembler messages:
  arch/x86/entry/thunk_64.S:42: Error: symbol `SYM_END_trace_hardirqs_on_thunk' is already defined
  arch/x86/entry/thunk_64.S:43: Error: symbol `SYM_END_trace_hardirqs_off_thunk' is already defined
  arch/x86/entry/thunk_64.S:47: Error: symbol `SYM_END_lockdep_sys_exit_thunk' is already defined
  arch/x86/entry/thunk_64.S:51: Error: symbol `SYM_END____preempt_schedule' is already defined
  arch/x86/entry/thunk_64.S:52: Error: symbol `SYM_END____preempt_schedule_notrace' is already defined
  scripts/Makefile.build:395: recipe for target 'arch/x86/entry/thunk_64.o' failed

-- 
Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

On 03/22/2017, 03:11 PM, Josh Poimboeuf wrote:
> Or, here's a much easier way to do it, without involving objtool:
> 
> --- a/include/linux/linkage.h
> +++ b/include/linux/linkage.h
> @@ -138,9 +138,17 @@
>  	name:
>  #endif
>  
> +#ifndef CHECK_DUP_SYM_END
> +#define CHECK_DUP_SYM_END(name)				\
> +	.pushsection .discard.sym_func_end ASM_NL	\
> +	SYM_END_##name: .byte 0 ASM_NL			\
> +	.popsection
> +#endif
> +
>  /* SYM_END -- use only if you have to */
>  #ifndef SYM_END
>  #define SYM_END(name, sym_type)				\
> +	CHECK_DUP_SYM_END(name) ASM_NL			\
>  	.type name sym_type ASM_NL			\
>  	.size name, .-name
>  #endif

I tried this approach and it didn't work for me inside .macros. Oh,
well, the name cannot be first, so now, we can have a check for both
correct pairing _and_ duplicate ends in one:

#define SYM_CHECK_START(name)                           \
        .pushsection .rodata.bubak ASM_NL               \
        .long has_no_SYM_END_##name - . ASM_NL          \
        .popsection

#define SYM_CHECK_END(name)                             \
        has_no_SYM_END_##name:

/* SYM_START -- use only if you have to */
#ifndef SYM_START
#define SYM_START(name, align, visibility, entry)       \
        SYM_CHECK_START(name) ASM_NL                    \
        visibility(name) ASM_NL                         \
        align ASM_NL                                    \
        name: ASM_NL                                    \
        entry
#endif

/* SYM_END -- use only if you have to */
#ifndef SYM_END
#define SYM_END(name, sym_type, exit)                   \
        exit ASM_NL                                     \
        SYM_CHECK_END(name) ASM_NL                      \
        .type name sym_type ASM_NL                      \
        .size name, .-name
#endif


So for the ftrace mistake I did:

  AS      arch/x86/kernel/mcount_64.o
/home/latest/linux/arch/x86/kernel/mcount_64.S: Assembler messages:
/home/latest/linux/arch/x86/kernel/mcount_64.S:192: Error: symbol
`has_no_SYM_END_ftrace_caller' is already defined


or if I remove SYM_END_FUNC completely:
  LD      vmlinux.o
  MODPOST vmlinux.o
arch/x86/built-in.o:(.rodata.bubak+0x130): undefined reference to
`has_no_SYM_END_ftrace_stub'


Sad is that this occurs only during linking, so I cannot put it in the
.discard section -- ideas?

thanks,
-- 
js
suse labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Wed, Mar 22, 2017 at 04:01:08PM +0100, Jiri Slaby wrote:
> On 03/22/2017, 03:11 PM, Josh Poimboeuf wrote:
> > Or, here's a much easier way to do it, without involving objtool:
> > 
> > --- a/include/linux/linkage.h
> > +++ b/include/linux/linkage.h
> > @@ -138,9 +138,17 @@
> >  	name:
> >  #endif
> >  
> > +#ifndef CHECK_DUP_SYM_END
> > +#define CHECK_DUP_SYM_END(name)				\
> > +	.pushsection .discard.sym_func_end ASM_NL	\
> > +	SYM_END_##name: .byte 0 ASM_NL			\
> > +	.popsection
> > +#endif
> > +
> >  /* SYM_END -- use only if you have to */
> >  #ifndef SYM_END
> >  #define SYM_END(name, sym_type)				\
> > +	CHECK_DUP_SYM_END(name) ASM_NL			\
> >  	.type name sym_type ASM_NL			\
> >  	.size name, .-name
> >  #endif
> 
> I tried this approach and it didn't work for me inside .macros. Oh,
> well, the name cannot be first, so now, we can have a check for both
> correct pairing _and_ duplicate ends in one:
> 
> #define SYM_CHECK_START(name)                           \
>         .pushsection .rodata.bubak ASM_NL               \
>         .long has_no_SYM_END_##name - . ASM_NL          \
>         .popsection
> 
> #define SYM_CHECK_END(name)                             \
>         has_no_SYM_END_##name:
> 
> /* SYM_START -- use only if you have to */
> #ifndef SYM_START
> #define SYM_START(name, align, visibility, entry)       \
>         SYM_CHECK_START(name) ASM_NL                    \
>         visibility(name) ASM_NL                         \
>         align ASM_NL                                    \
>         name: ASM_NL                                    \
>         entry
> #endif
> 
> /* SYM_END -- use only if you have to */
> #ifndef SYM_END
> #define SYM_END(name, sym_type, exit)                   \
>         exit ASM_NL                                     \
>         SYM_CHECK_END(name) ASM_NL                      \
>         .type name sym_type ASM_NL                      \
>         .size name, .-name
> #endif
> 
> 
> So for the ftrace mistake I did:
> 
>   AS      arch/x86/kernel/mcount_64.o
> /home/latest/linux/arch/x86/kernel/mcount_64.S: Assembler messages:
> /home/latest/linux/arch/x86/kernel/mcount_64.S:192: Error: symbol
> `has_no_SYM_END_ftrace_caller' is already defined
> 
> 
> or if I remove SYM_END_FUNC completely:
>   LD      vmlinux.o
>   MODPOST vmlinux.o
> arch/x86/built-in.o:(.rodata.bubak+0x130): undefined reference to
> `has_no_SYM_END_ftrace_stub'
> 
> 
> Sad is that this occurs only during linking, so I cannot put it in the
> .discard section -- ideas?

Ah, interesting idea but I can't think of a way to do the missing end
check before link time.

But it would be easy for objtool to check for a missing end because the
symbol would have a zero size.

-- 
Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> > 
> > * Jiri Slaby <jslaby@suse.cz> wrote:
> > 
> > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > > 
> > > > * Pavel Machek <pavel@ucw.cz> wrote:
> > > > 
> > > >> Hi!
> > > >>
> > > >>> -ENTRY(saved_rbp)	.quad	0
> > > >>> -ENTRY(saved_rsi)	.quad	0
> > > >>> -ENTRY(saved_rdi)	.quad	0
> > > >>> -ENTRY(saved_rbx)	.quad	0
> > > >>> +SYM_DATA_START(saved_rbp)		.quad	0
> > > >>> +SYM_DATA_START(saved_rsi)		.quad	0
> > > >>> +SYM_DATA_START(saved_rdi)		.quad	0
> > > >>> +SYM_DATA_START(saved_rbx)		.quad	0
> > > >>
> > > >> Does it make sense to call it SYM_DATA_*START* when there's no
> > > >> corresponding end?
> > > > 
> > > > That looks like a bug - I think we should strive for them to always be in pairs.
> > > > 
> > > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' 
> > > > SYM_*_START() uses? This could be done by emitting debug data into a special 
> > > > section and then analyzing that section for unpaired entries. The section can be 
> > > > discarded in the final link, it won't show up in the kernel image.
> > > 
> > > It should be easier than that. No introduction of other info needed --
> > > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should
> > > be a bug now.
> > 
> > I'm all for that!
> 
> It would be easy to add this checking to objtool since it already reads
> the symbol table.  The hard part is figuring out the logistics. :-)
> 
> - Should the warnings be on by default?

Yes, if objtool is running. Keep it simple.

> - Part of the "objtool check" command or something else?

Yes - I think it's still within the 'object file check' functionality.

> - Separate config option or just include it with
>   CONFIG_STACK_VALIDATION?

Yeah, but I'd rename CONFIG_STACK_VALIDATION to CONFIG_OBJ_VALIDATION or such. As 
I predicted early on, objtool will go beyond stack checking! ;-)

> - Should all asm files be checked, including those currently skipped by
>   objtool with OBJECT_FILES_NON_STANDARD?

The symbol syntax check should definitely be for all files, yes.

Could we perhaps emit 'non-standard stack frames' information into the .o itself 
(via a flag or a special section?), so that objtool can decide on its own whether 
to complain about any weirdnesses there?

> > Can we detect double ends as well - i.e. do a build check of the full syntax of 
> > these symbol definition primitives?
> 
> Detecting double ends would be a little trickier.  The second SYM_*_END
> supersedes the first, so that information isn't in the ELF symbol table.

Indeed.

> We could use a special section to annotate all the macro uses and have
> objtool do the checking, similar to what you suggested earlier.

That might be useful for other purposes as well - such as the non-standard stack 
frame annotations?

But it's your call really: I'm principally fine with any of the solutions, as long 
as the checking is done.

Thanks,

	Ingo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Thu, Mar 23, 2017 at 08:38:20AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> > > 
> > > * Jiri Slaby <jslaby@suse.cz> wrote:
> > > 
> > > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > > > 
> > > > > * Pavel Machek <pavel@ucw.cz> wrote:
> > > > > 
> > > > >> Hi!
> > > > >>
> > > > >>> -ENTRY(saved_rbp)	.quad	0
> > > > >>> -ENTRY(saved_rsi)	.quad	0
> > > > >>> -ENTRY(saved_rdi)	.quad	0
> > > > >>> -ENTRY(saved_rbx)	.quad	0
> > > > >>> +SYM_DATA_START(saved_rbp)		.quad	0
> > > > >>> +SYM_DATA_START(saved_rsi)		.quad	0
> > > > >>> +SYM_DATA_START(saved_rdi)		.quad	0
> > > > >>> +SYM_DATA_START(saved_rbx)		.quad	0
> > > > >>
> > > > >> Does it make sense to call it SYM_DATA_*START* when there's no
> > > > >> corresponding end?
> > > > > 
> > > > > That looks like a bug - I think we should strive for them to always be in pairs.
> > > > > 
> > > > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' 
> > > > > SYM_*_START() uses? This could be done by emitting debug data into a special 
> > > > > section and then analyzing that section for unpaired entries. The section can be 
> > > > > discarded in the final link, it won't show up in the kernel image.
> > > > 
> > > > It should be easier than that. No introduction of other info needed --
> > > > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should
> > > > be a bug now.
> > > 
> > > I'm all for that!
> > 
> > It would be easy to add this checking to objtool since it already reads
> > the symbol table.  The hard part is figuring out the logistics. :-)
> > 
> > - Should the warnings be on by default?
> 
> Yes, if objtool is running. Keep it simple.
> 
> > - Part of the "objtool check" command or something else?
> 
> Yes - I think it's still within the 'object file check' functionality.
> 
> > - Separate config option or just include it with
> >   CONFIG_STACK_VALIDATION?
> 
> Yeah, but I'd rename CONFIG_STACK_VALIDATION to CONFIG_OBJ_VALIDATION or such. As 
> I predicted early on, objtool will go beyond stack checking! ;-)
> 
> > - Should all asm files be checked, including those currently skipped by
> >   objtool with OBJECT_FILES_NON_STANDARD?
> 
> The symbol syntax check should definitely be for all files, yes.

That all sounds reasonable.  I'll work something up.

> Could we perhaps emit 'non-standard stack frames' information into the .o itself 
> (via a flag or a special section?), so that objtool can decide on its own whether 
> to complain about any weirdnesses there?

For the OBJECT_FILES_NON_STANDARD case, where the whole file is
"special", we can just provide a flag to "objtool check" to tell it to
skip stack checking for that file, but still do the symbol checks.

> > > Can we detect double ends as well - i.e. do a build check of the full syntax of 
> > > these symbol definition primitives?
> > 
> > Detecting double ends would be a little trickier.  The second SYM_*_END
> > supersedes the first, so that information isn't in the ELF symbol table.
> 
> Indeed.
> 
> > We could use a special section to annotate all the macro uses and have
> > objtool do the checking, similar to what you suggested earlier.
> 
> That might be useful for other purposes as well - such as the non-standard stack 
> frame annotations?

To start with we can try going without all the special sections (other
than the SYM_END double end check).  If we end up finding another case
which isn't covered then we can always add the special sections later.

-- 
Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

[-- Attachment #1.1.1: Type: text/plain, Size: 2806 bytes --]

Hi,

On 03/21/2017, 03:08 PM, Pavel Machek wrote:
>> -ENTRY(saved_rbp)	.quad	0
>> -ENTRY(saved_rsi)	.quad	0
>> -ENTRY(saved_rdi)	.quad	0
>> -ENTRY(saved_rbx)	.quad	0
>> +SYM_DATA_START(saved_rbp)		.quad	0
>> +SYM_DATA_START(saved_rsi)		.quad	0
>> +SYM_DATA_START(saved_rdi)		.quad	0
>> +SYM_DATA_START(saved_rbx)		.quad	0
> 
> Does it make sense to call it SYM_DATA_*START* when there's no
> corresponding end?
> 
> Plus... it looks like saved_rsi (and friends) are only used inside
> wakeup_64.S. Could we just delete the "ENTRY" annotations?


So, now I have:

=== linkage.h ===

/* SYM_DATA_SIMPLE -- start+end wrapper around simple global data */
#ifndef SYM_DATA_SIMPLE
#define SYM_DATA_SIMPLE(name, data)                             \
        SYM_DATA_START(name) ASM_NL                             \
        data ASM_NL                                             \
        SYM_DATA_END(name)
#endif

/* SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data */
#ifndef SYM_DATA_SIMPLE_LOCAL
#define SYM_DATA_SIMPLE_LOCAL(name, data)                       \
        SYM_DATA_START_LOCAL(name) ASM_NL                       \
        data ASM_NL                                             \
        SYM_DATA_END(name)
#endif

=== wakeup_64.S ===

SYM_DATA_SIMPLE_LOCAL(saved_rbp, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rsi, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rdi, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rbx, .quad 0)

SYM_DATA_SIMPLE_LOCAL(saved_rip, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rsp, .quad 0)

SYM_DATA_SIMPLE_LOCAL(saved_magic, .quad 0)

=== original ===

    10: 0000000000000060     0 NOTYPE  GLOBAL DEFAULT    3 saved_magic
    11: 0000000000000050     0 NOTYPE  GLOBAL DEFAULT    3 saved_rsp
    12: 0000000000000030     0 NOTYPE  GLOBAL DEFAULT    3 saved_rbx
    13: 0000000000000020     0 NOTYPE  GLOBAL DEFAULT    3 saved_rdi
    14: 0000000000000010     0 NOTYPE  GLOBAL DEFAULT    3 saved_rsi
    15: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT    3 saved_rbp
    16: 0000000000000040     0 NOTYPE  GLOBAL DEFAULT    3 saved_rip

=== new ===

     4: 0000000000000030     8 OBJECT  LOCAL  DEFAULT    3 saved_magic
     6: 0000000000000028     8 OBJECT  LOCAL  DEFAULT    3 saved_rsp
     7: 0000000000000018     8 OBJECT  LOCAL  DEFAULT    3 saved_rbx
     8: 0000000000000010     8 OBJECT  LOCAL  DEFAULT    3 saved_rdi
     9: 0000000000000008     8 OBJECT  LOCAL  DEFAULT    3 saved_rsi
    10: 0000000000000000     8 OBJECT  LOCAL  DEFAULT    3 saved_rbp
    11: 0000000000000020     8 OBJECT  LOCAL  DEFAULT    3 saved_rip

=== EOF ===

BTW, ENTRY() aligned the data to 2^4 = 16 as we can see in the original.
But I see no point aligning data like this.

thanks,
-- 
js
suse labs


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 3081 bytes --]

On Wed 2017-03-22 13:06:54, Jiri Slaby wrote:
> Hi,
> 
> On 03/21/2017, 03:08 PM, Pavel Machek wrote:
> >> -ENTRY(saved_rbp)	.quad	0
> >> -ENTRY(saved_rsi)	.quad	0
> >> -ENTRY(saved_rdi)	.quad	0
> >> -ENTRY(saved_rbx)	.quad	0
> >> +SYM_DATA_START(saved_rbp)		.quad	0
> >> +SYM_DATA_START(saved_rsi)		.quad	0
> >> +SYM_DATA_START(saved_rdi)		.quad	0
> >> +SYM_DATA_START(saved_rbx)		.quad	0
> > 
> > Does it make sense to call it SYM_DATA_*START* when there's no
> > corresponding end?
> > 
> > Plus... it looks like saved_rsi (and friends) are only used inside
> > wakeup_64.S. Could we just delete the "ENTRY" annotations?
> 
> 
> So, now I have:
> 
> === linkage.h ===
> 
> /* SYM_DATA_SIMPLE -- start+end wrapper around simple global data */
> #ifndef SYM_DATA_SIMPLE
> #define SYM_DATA_SIMPLE(name, data)                             \
>         SYM_DATA_START(name) ASM_NL                             \
>         data ASM_NL                                             \
>         SYM_DATA_END(name)
> #endif
> 
> /* SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data */
> #ifndef SYM_DATA_SIMPLE_LOCAL
> #define SYM_DATA_SIMPLE_LOCAL(name, data)                       \
>         SYM_DATA_START_LOCAL(name) ASM_NL                       \
>         data ASM_NL                                             \
>         SYM_DATA_END(name)
> #endif
> 
> === wakeup_64.S ===
> 
> SYM_DATA_SIMPLE_LOCAL(saved_rbp, .quad 0)
> SYM_DATA_SIMPLE_LOCAL(saved_rsi, .quad 0)
> SYM_DATA_SIMPLE_LOCAL(saved_rdi, .quad 0)
> SYM_DATA_SIMPLE_LOCAL(saved_rbx, .quad 0)
> 
> SYM_DATA_SIMPLE_LOCAL(saved_rip, .quad 0)
> SYM_DATA_SIMPLE_LOCAL(saved_rsp, .quad 0)
> 
> SYM_DATA_SIMPLE_LOCAL(saved_magic, .quad 0)
> 
> === original ===
> 
>     10: 0000000000000060     0 NOTYPE  GLOBAL DEFAULT    3 saved_magic
>     11: 0000000000000050     0 NOTYPE  GLOBAL DEFAULT    3 saved_rsp
>     12: 0000000000000030     0 NOTYPE  GLOBAL DEFAULT    3 saved_rbx
>     13: 0000000000000020     0 NOTYPE  GLOBAL DEFAULT    3 saved_rdi
>     14: 0000000000000010     0 NOTYPE  GLOBAL DEFAULT    3 saved_rsi
>     15: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT    3 saved_rbp
>     16: 0000000000000040     0 NOTYPE  GLOBAL DEFAULT    3 saved_rip
> 
> === new ===
> 
>      4: 0000000000000030     8 OBJECT  LOCAL  DEFAULT    3 saved_magic
>      6: 0000000000000028     8 OBJECT  LOCAL  DEFAULT    3 saved_rsp
>      7: 0000000000000018     8 OBJECT  LOCAL  DEFAULT    3 saved_rbx
>      8: 0000000000000010     8 OBJECT  LOCAL  DEFAULT    3 saved_rdi
>      9: 0000000000000008     8 OBJECT  LOCAL  DEFAULT    3 saved_rsi
>     10: 0000000000000000     8 OBJECT  LOCAL  DEFAULT    3 saved_rbp
>     11: 0000000000000020     8 OBJECT  LOCAL  DEFAULT    3 saved_rip
> 
> === EOF ===
> 
> BTW, ENTRY() aligned the data to 2^4 = 16 as we can see in the original.
> But I see no point aligning data like this.

Yep, that's a bug, too. I guess it hurts even more on 32-bit....

Thanks for fixing this,
									Pavel

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Jiri Slaby]

Somewhere END was used to end a function, elsewhere, nothing was used.
So unify it and mark them all by SYM_FUNC_END.

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: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: <linux-pm@vger.kernel.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Reviewed-by: Juergen Gross <jgross@suse.com> [xen parts]
Cc: <xen-devel@lists.xenproject.org>
---
 arch/x86/entry/entry_64.S            | 40 ++++++++++++++++++------------------
 arch/x86/entry/entry_64_compat.S     |  5 +++--
 arch/x86/kernel/mcount_64.S          | 12 ++++++-----
 arch/x86/power/hibernate_asm_64.S    |  2 ++
 arch/x86/realmode/rm/reboot.S        |  1 +
 arch/x86/realmode/rm/trampoline_64.S |  4 ++++
 arch/x86/realmode/rm/wakeup_asm.S    |  1 +
 arch/x86/xen/xen-asm_64.S            |  2 ++
 arch/x86/xen/xen-head.S              |  2 +-
 arch/x86/xen/xen-pvh.S               |  2 +-
 10 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index d2b2a2948ffe..3e523f8d7e7f 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -324,7 +324,7 @@ syscall_return_via_sysret:
 opportunistic_sysret_failed:
 	SWAPGS
 	jmp	restore_c_regs_and_iret
-END(entry_SYSCALL_64)
+SYM_FUNC_END(entry_SYSCALL_64)
 
 ENTRY(stub_ptregs_64)
 	/*
@@ -350,13 +350,13 @@ ENTRY(stub_ptregs_64)
 
 1:
 	jmp	*%rax				/* Called from C */
-END(stub_ptregs_64)
+SYM_FUNC_END(stub_ptregs_64)
 
 .macro ptregs_stub func
 ENTRY(ptregs_\func)
 	leaq	\func(%rip), %rax
 	jmp	stub_ptregs_64
-END(ptregs_\func)
+SYM_FUNC_END(ptregs_\func)
 .endm
 
 /* Instantiate ptregs_stub for each ptregs-using syscall */
@@ -399,7 +399,7 @@ ENTRY(__switch_to_asm)
 	popq	%rbp
 
 	jmp	__switch_to
-END(__switch_to_asm)
+SYM_FUNC_END(__switch_to_asm)
 
 /*
  * A newly forked process directly context switches into this address.
@@ -435,7 +435,7 @@ ENTRY(ret_from_fork)
 	 */
 	movq	$0, RAX(%rsp)
 	jmp	2b
-END(ret_from_fork)
+SYM_FUNC_END(ret_from_fork)
 
 /*
  * Build the entry stubs with some assembler magic.
@@ -450,7 +450,7 @@ ENTRY(irq_entries_start)
 	jmp	common_interrupt
 	.align	8
     .endr
-END(irq_entries_start)
+SYM_FUNC_END(irq_entries_start)
 
 /*
  * Interrupt entry/exit.
@@ -652,7 +652,7 @@ native_irq_return_ldt:
 	 */
 	jmp	native_irq_return_iret
 #endif
-END(common_interrupt)
+SYM_FUNC_END(common_interrupt)
 
 /*
  * APIC interrupts.
@@ -664,7 +664,7 @@ ENTRY(\sym)
 .Lcommon_\sym:
 	interrupt \do_sym
 	jmp	ret_from_intr
-END(\sym)
+SYM_FUNC_END(\sym)
 .endm
 
 #ifdef CONFIG_TRACING
@@ -830,7 +830,7 @@ ENTRY(\sym)
 
 	jmp	error_exit			/* %ebx: no swapgs flag */
 	.endif
-END(\sym)
+SYM_FUNC_END(\sym)
 .endm
 
 #ifdef CONFIG_TRACING
@@ -873,7 +873,7 @@ ENTRY(native_load_gs_index)
 	SWAPGS
 	popfq
 	ret
-END(native_load_gs_index)
+SYM_FUNC_END(native_load_gs_index)
 EXPORT_SYMBOL(native_load_gs_index)
 
 	_ASM_EXTABLE(.Lgs_change, bad_gs)
@@ -903,7 +903,7 @@ ENTRY(do_softirq_own_stack)
 	leaveq
 	decl	PER_CPU_VAR(irq_count)
 	ret
-END(do_softirq_own_stack)
+SYM_FUNC_END(do_softirq_own_stack)
 
 #ifdef CONFIG_XEN
 idtentry xen_hypervisor_callback xen_do_hypervisor_callback has_error_code=0
@@ -939,7 +939,7 @@ ENTRY(xen_do_hypervisor_callback)		/* do_hypervisor_callback(struct *pt_regs) */
 	call	xen_maybe_preempt_hcall
 #endif
 	jmp	error_exit
-END(xen_do_hypervisor_callback)
+SYM_FUNC_END(xen_do_hypervisor_callback)
 
 /*
  * Hypervisor uses this for application faults while it executes.
@@ -985,7 +985,7 @@ ENTRY(xen_failsafe_callback)
 	SAVE_EXTRA_REGS
 	ENCODE_FRAME_POINTER
 	jmp	error_exit
-END(xen_failsafe_callback)
+SYM_FUNC_END(xen_failsafe_callback)
 
 apicinterrupt3 HYPERVISOR_CALLBACK_VECTOR \
 	xen_hvm_callback_vector xen_evtchn_do_upcall
@@ -1036,7 +1036,7 @@ ENTRY(paranoid_entry)
 	SWAPGS
 	xorl	%ebx, %ebx
 1:	ret
-END(paranoid_entry)
+SYM_FUNC_END(paranoid_entry)
 
 /*
  * "Paranoid" exit path from exception stack.  This is invoked
@@ -1065,7 +1065,7 @@ paranoid_exit_restore:
 	RESTORE_C_REGS
 	REMOVE_PT_GPREGS_FROM_STACK 8
 	INTERRUPT_RETURN
-END(paranoid_exit)
+SYM_FUNC_END(paranoid_exit)
 
 /*
  * Save all registers in pt_regs, and switch gs if needed.
@@ -1147,7 +1147,7 @@ ENTRY(error_entry)
 	mov	%rax, %rsp
 	decl	%ebx
 	jmp	.Lerror_entry_from_usermode_after_swapgs
-END(error_entry)
+SYM_FUNC_END(error_entry)
 
 
 /*
@@ -1161,7 +1161,7 @@ ENTRY(error_exit)
 	testl	%ebx, %ebx
 	jnz	retint_kernel
 	jmp	retint_user
-END(error_exit)
+SYM_FUNC_END(error_exit)
 
 /* Runs on exception stack */
 ENTRY(nmi)
@@ -1509,12 +1509,12 @@ nmi_restore:
 	 * mode, so this cannot result in a fault.
 	 */
 	INTERRUPT_RETURN
-END(nmi)
+SYM_FUNC_END(nmi)
 
 ENTRY(ignore_sysret)
 	mov	$-ENOSYS, %eax
 	sysret
-END(ignore_sysret)
+SYM_FUNC_END(ignore_sysret)
 
 ENTRY(rewind_stack_do_exit)
 	/* Prevent any naive code from trying to unwind to our caller. */
@@ -1525,4 +1525,4 @@ ENTRY(rewind_stack_do_exit)
 
 	call	do_exit
 1:	jmp 1b
-END(rewind_stack_do_exit)
+SYM_FUNC_END(rewind_stack_do_exit)
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 73d7ff0b125c..9239f80e11ce 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -262,7 +262,7 @@ sysret32_from_system_call:
 	movq	RSP-ORIG_RAX(%rsp), %rsp
 	swapgs
 	sysretl
-END(entry_SYSCALL_compat)
+SYM_FUNC_END(entry_SYSCALL_compat)
 
 /*
  * 32-bit legacy system call entry.
@@ -340,7 +340,7 @@ ENTRY(entry_INT80_compat)
 	TRACE_IRQS_ON
 	SWAPGS
 	jmp	restore_regs_and_iret
-END(entry_INT80_compat)
+SYM_FUNC_END(entry_INT80_compat)
 
 SYM_FUNC_START(stub32_clone)
 	/*
@@ -352,3 +352,4 @@ SYM_FUNC_START(stub32_clone)
 	 */
 	xchg	%r8, %rcx
 	jmp	sys_clone
+SYM_FUNC_END(stub32_clone)
diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index 2b4d7045e823..f3b0cb57a8c7 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -152,7 +152,7 @@ EXPORT_SYMBOL(mcount)
 
 ENTRY(function_hook)
 	retq
-END(function_hook)
+SYM_FUNC_END(function_hook)
 
 ENTRY(ftrace_caller)
 	/* save_mcount_regs fills in first two parameters */
@@ -184,11 +184,12 @@ GLOBAL(ftrace_epilogue)
 GLOBAL(ftrace_graph_call)
 	jmp ftrace_stub
 #endif
+SYM_FUNC_END(ftrace_caller)
 
 /* This is weak to keep gas from relaxing the jumps */
 WEAK(ftrace_stub)
 	retq
-END(ftrace_caller)
+SYM_FUNC_END(ftrace_caller)
 
 ENTRY(ftrace_regs_caller)
 	/* Save the current flags before any operations that can change them */
@@ -259,7 +260,7 @@ GLOBAL(ftrace_regs_caller_end)
 
 	jmp ftrace_epilogue
 
-END(ftrace_regs_caller)
+SYM_FUNC_END(ftrace_regs_caller)
 
 
 #else /* ! CONFIG_DYNAMIC_FTRACE */
@@ -295,7 +296,7 @@ trace:
 	restore_mcount_regs
 
 	jmp fgraph_trace
-END(function_hook)
+SYM_FUNC_END(function_hook)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 #endif /* CONFIG_FUNCTION_TRACER */
 
@@ -318,7 +319,7 @@ ENTRY(ftrace_graph_caller)
 	restore_mcount_regs
 
 	retq
-END(ftrace_graph_caller)
+SYM_FUNC_END(ftrace_graph_caller)
 
 SYM_FUNC_START(return_to_handler)
 	subq  $24, %rsp
@@ -335,4 +336,5 @@ SYM_FUNC_START(return_to_handler)
 	movq (%rsp), %rax
 	addq $24, %rsp
 	jmp *%rdi
+SYM_FUNC_END(return_to_handler)
 #endif
diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S
index ce8da3a0412c..5b6eb95e1b7b 100644
--- a/arch/x86/power/hibernate_asm_64.S
+++ b/arch/x86/power/hibernate_asm_64.S
@@ -68,6 +68,7 @@ ENTRY(restore_image)
 	/* jump to relocated restore code */
 	movq	relocated_restore_code(%rip), %rcx
 	jmpq	*%rcx
+SYM_FUNC_END(restore_image)
 
 	/* code below has been relocated to a safe page */
 ENTRY(core_restore_code)
@@ -98,6 +99,7 @@ ENTRY(core_restore_code)
 .Ldone:
 	/* jump to the restore_registers address from the image header */
 	jmpq	*%r8
+SYM_FUNC_END(core_restore_code)
 
 	 /* code below belongs to the image kernel */
 	.align PAGE_SIZE
diff --git a/arch/x86/realmode/rm/reboot.S b/arch/x86/realmode/rm/reboot.S
index d66c607bdc58..3dec2ba4adc5 100644
--- a/arch/x86/realmode/rm/reboot.S
+++ b/arch/x86/realmode/rm/reboot.S
@@ -62,6 +62,7 @@ GLOBAL(machine_real_restart_paging_off)
 	movl	%ecx, %gs
 	movl	%ecx, %ss
 	ljmpw	$8, $1f
+SYM_FUNC_END(machine_real_restart_asm)
 
 /*
  * This is 16-bit protected mode code to disable paging and the cache,
diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
index dac7b20d2f9d..6869f1229c3a 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -79,6 +79,8 @@ ENTRY(trampoline_start)
 no_longmode:
 	hlt
 	jmp no_longmode
+SYM_FUNC_END(trampoline_start)
+
 #include "../kernel/verify_cpu.S"
 
 	.section ".text32","ax"
@@ -116,6 +118,7 @@ ENTRY(startup_32)
 	 * the new gdt/idt that has __KERNEL_CS with CS.L = 1.
 	 */
 	ljmpl	$__KERNEL_CS, $pa_startup_64
+SYM_FUNC_END(startup_32)
 
 	.section ".text64","ax"
 	.code64
@@ -123,6 +126,7 @@ ENTRY(startup_32)
 ENTRY(startup_64)
 	# Now jump into the kernel using virtual addresses
 	jmpq	*tr_start(%rip)
+SYM_FUNC_END(startup_64)
 
 	.section ".rodata","a"
 	# Duplicate the global descriptor table
diff --git a/arch/x86/realmode/rm/wakeup_asm.S b/arch/x86/realmode/rm/wakeup_asm.S
index 9e7e14797a72..b5f711327aef 100644
--- a/arch/x86/realmode/rm/wakeup_asm.S
+++ b/arch/x86/realmode/rm/wakeup_asm.S
@@ -134,6 +134,7 @@ ENTRY(wakeup_start)
 #else
 	jmp	trampoline_start
 #endif
+SYM_FUNC_END(wakeup_start)
 
 bogus_real_magic:
 1:
diff --git a/arch/x86/xen/xen-asm_64.S b/arch/x86/xen/xen-asm_64.S
index c3df43141e70..2a968c087269 100644
--- a/arch/x86/xen/xen-asm_64.S
+++ b/arch/x86/xen/xen-asm_64.S
@@ -49,6 +49,7 @@ ENTRY(xen_iret)
 1:	jmp hypercall_iret
 ENDPATCH(xen_iret)
 RELOC(xen_iret, 1b+1)
+SYM_FUNC_END(xen_iret)
 
 ENTRY(xen_sysret64)
 	/*
@@ -68,6 +69,7 @@ ENTRY(xen_sysret64)
 1:	jmp hypercall_iret
 ENDPATCH(xen_sysret64)
 RELOC(xen_sysret64, 1b+1)
+SYM_FUNC_END(xen_sysret64)
 
 /*
  * Xen handles syscall callbacks much like ordinary exceptions, which
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 37794e42b67d..f8b2371faf62 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -32,7 +32,7 @@ ENTRY(startup_xen)
 	mov $init_thread_union+THREAD_SIZE, %_ASM_SP
 
 	jmp xen_start_kernel
-
+SYM_FUNC_END(startup_xen)
 	__FINIT
 
 .pushsection .text
diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index 5e246716d58f..7dc332435c14 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -133,7 +133,7 @@ ENTRY(pvh_start_xen)
 
 	ljmp $__BOOT_CS, $_pa(startup_32)
 #endif
-END(pvh_start_xen)
+SYM_FUNC_END(pvh_start_xen)
 
 	.section ".init.data","aw"
 	.balign 8
-- 
2.12.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 07/10] x86: assembly, annotate aliases

[Jiri Slaby]

_key_expansion_128 is an alias to _key_expansion_256a, __memcpy to
memcpy, xen_syscall32_target to xen_sysenter_target, and so on. Annotate
them all using the new SYM_FUNC_START_ALIAS, SYM_FUNC_START_LOCAL_ALIAS,
and SYM_FUNC_END_ALIAS. This will make the tools generating the
debuginfo happy.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
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>
Reviewed-by: Juergen Gross <jgross@suse.com> [xen parts]
Cc: <linux-crypto@vger.kernel.org>
Cc: <xen-devel@lists.xenproject.org>
---
 arch/x86/crypto/aesni-intel_asm.S | 5 ++---
 arch/x86/lib/memcpy_64.S          | 4 ++--
 arch/x86/lib/memmove_64.S         | 4 ++--
 arch/x86/lib/memset_64.S          | 4 ++--
 arch/x86/xen/xen-asm_64.S         | 4 ++--
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_asm.S b/arch/x86/crypto/aesni-intel_asm.S
index 8913ea70323c..ea247bfed89d 100644
--- a/arch/x86/crypto/aesni-intel_asm.S
+++ b/arch/x86/crypto/aesni-intel_asm.S
@@ -1744,8 +1744,7 @@ ENDPROC(aesni_gcm_enc)
 #endif
 
 
-.align 4
-_key_expansion_128:
+SYM_FUNC_START_LOCAL_ALIAS(_key_expansion_128)
 SYM_FUNC_START_LOCAL(_key_expansion_256a)
 	pshufd $0b11111111, %xmm1, %xmm1
 	shufps $0b00010000, %xmm0, %xmm4
@@ -1756,8 +1755,8 @@ SYM_FUNC_START_LOCAL(_key_expansion_256a)
 	movaps %xmm0, (TKEYP)
 	add $0x10, TKEYP
 	ret
-ENDPROC(_key_expansion_128)
 ENDPROC(_key_expansion_256a)
+SYM_FUNC_END_ALIAS(_key_expansion_128)
 
 SYM_FUNC_START_LOCAL(_key_expansion_192a)
 	pshufd $0b01010101, %xmm1, %xmm1
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 779782f58324..2d6518b4dbd6 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -26,7 +26,7 @@
  * Output:
  * rax original destination
  */
-ENTRY(__memcpy)
+SYM_FUNC_START_ALIAS(__memcpy)
 ENTRY(memcpy)
 	ALTERNATIVE_2 "jmp memcpy_orig", "", X86_FEATURE_REP_GOOD, \
 		      "jmp memcpy_erms", X86_FEATURE_ERMS
@@ -40,7 +40,7 @@ ENTRY(memcpy)
 	rep movsb
 	ret
 ENDPROC(memcpy)
-ENDPROC(__memcpy)
+SYM_FUNC_END_ALIAS(__memcpy)
 EXPORT_SYMBOL(memcpy)
 EXPORT_SYMBOL(__memcpy)
 
diff --git a/arch/x86/lib/memmove_64.S b/arch/x86/lib/memmove_64.S
index 15de86cd15b0..d22af97e5b27 100644
--- a/arch/x86/lib/memmove_64.S
+++ b/arch/x86/lib/memmove_64.S
@@ -25,7 +25,7 @@
  */
 .weak memmove
 
-ENTRY(memmove)
+SYM_FUNC_START_ALIAS(memmove)
 ENTRY(__memmove)
 
 	/* Handle more 32 bytes in loop */
@@ -207,6 +207,6 @@ ENTRY(__memmove)
 13:
 	retq
 ENDPROC(__memmove)
-ENDPROC(memmove)
+SYM_FUNC_END_ALIAS(memmove)
 EXPORT_SYMBOL(__memmove)
 EXPORT_SYMBOL(memmove)
diff --git a/arch/x86/lib/memset_64.S b/arch/x86/lib/memset_64.S
index 55b95db30a61..0d3a1d341e60 100644
--- a/arch/x86/lib/memset_64.S
+++ b/arch/x86/lib/memset_64.S
@@ -18,7 +18,7 @@
  *
  * rax   original destination
  */
-ENTRY(memset)
+SYM_FUNC_START_ALIAS(memset)
 ENTRY(__memset)
 	/*
 	 * Some CPUs support enhanced REP MOVSB/STOSB feature. It is recommended
@@ -42,8 +42,8 @@ ENTRY(__memset)
 	rep stosb
 	movq %r9,%rax
 	ret
-ENDPROC(memset)
 ENDPROC(__memset)
+SYM_FUNC_END_ALIAS(memset)
 EXPORT_SYMBOL(memset)
 EXPORT_SYMBOL(__memset)
 
diff --git a/arch/x86/xen/xen-asm_64.S b/arch/x86/xen/xen-asm_64.S
index 2a968c087269..b3bf662a4f6a 100644
--- a/arch/x86/xen/xen-asm_64.S
+++ b/arch/x86/xen/xen-asm_64.S
@@ -117,13 +117,13 @@ ENDPROC(xen_sysenter_target)
 
 #else /* !CONFIG_IA32_EMULATION */
 
-ENTRY(xen_syscall32_target)
+SYM_FUNC_START_ALIAS(xen_syscall32_target)
 ENTRY(xen_sysenter_target)
 	lea 16(%rsp), %rsp	/* strip %rcx, %r11 */
 	mov $-ENOSYS, %rax
 	pushq $0
 	jmp hypercall_iret
-ENDPROC(xen_syscall32_target)
 ENDPROC(xen_sysenter_target)
+SYM_FUNC_END_ALIAS(xen_syscall32_target)
 
 #endif	/* CONFIG_IA32_EMULATION */
-- 
2.12.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Josh Poimboeuf]

On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
>  ENTRY(ftrace_caller)
>  	/* save_mcount_regs fills in first two parameters */
> @@ -184,11 +184,12 @@ GLOBAL(ftrace_epilogue)
>  GLOBAL(ftrace_graph_call)
>  	jmp ftrace_stub
>  #endif
> +SYM_FUNC_END(ftrace_caller)
>  
>  /* This is weak to keep gas from relaxing the jumps */
>  WEAK(ftrace_stub)
>  	retq
> -END(ftrace_caller)
> +SYM_FUNC_END(ftrace_caller)

This gives ftrace_caller() two ends.

-- 
Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
> >  ENTRY(ftrace_caller)
> >  	/* save_mcount_regs fills in first two parameters */
> > @@ -184,11 +184,12 @@ GLOBAL(ftrace_epilogue)
> >  GLOBAL(ftrace_graph_call)
> >  	jmp ftrace_stub
> >  #endif
> > +SYM_FUNC_END(ftrace_caller)
> >  
> >  /* This is weak to keep gas from relaxing the jumps */
> >  WEAK(ftrace_stub)
> >  	retq
> > -END(ftrace_caller)
> > +SYM_FUNC_END(ftrace_caller)
> 
> This gives ftrace_caller() two ends.

Such errors too could be detected automatically via objtool or some other symbol 
debug mechanism.

The reason it might be a good fit for objtool is to make the checking optional (no 
need to burden a regular build with it), plus objtool already looks at the .o from 
first principles - a symbol checking sub-functionality could analyze the symbol 
names in the same pass.

If we want to complicate things with CFI then we absolutely should increase the 
quality of our symbol names space.

Thanks,

	Ingo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Josh Poimboeuf]

On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
> Somewhere END was used to end a function, elsewhere, nothing was used.
> So unify it and mark them all by SYM_FUNC_END.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

For me these patches would be easier to review if the SYM_FUNC_START and
SYM_FUNC_END pairs for a given function are done in the same patch.

Also I noticed several cases in entry_64.S where the old ENTRY macro is
still used, and paired with SYM_FUNC_END.

Maybe there should be an x86 version of the deprecated ENTRY/ENDPROC/etc
macros which throw a warning or an error?

-- 
Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Jiri Slaby]

On 03/22/2017, 03:26 PM, Josh Poimboeuf wrote:
> On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
>> Somewhere END was used to end a function, elsewhere, nothing was used.
>> So unify it and mark them all by SYM_FUNC_END.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> 
> For me these patches would be easier to review if the SYM_FUNC_START and
> SYM_FUNC_END pairs for a given function are done in the same patch.

This patchset was intended to make everything paired with minimum
changes. I certainly can change also counter-elements of each
added/changed one if you prefer.

> Also I noticed several cases in entry_64.S where the old ENTRY macro is
> still used, and paired with SYM_FUNC_END.
> 
> Maybe there should be an x86 version of the deprecated ENTRY/ENDPROC/etc
> macros which throw a warning or an error?

Yes, my plan is to throw ENTRY/ENDPROC on the floor from x86 completely.
And I will do it after this patchset settles down by sed or something in
one shot (per directory or something).

thanks,
-- 
js
suse labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Jiri Slaby]

On 03/22/2017, 04:44 PM, Jiri Slaby wrote:
> On 03/22/2017, 03:26 PM, Josh Poimboeuf wrote:
>> On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
>>> Somewhere END was used to end a function, elsewhere, nothing was used.
>>> So unify it and mark them all by SYM_FUNC_END.
>>>
>>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>>
>> For me these patches would be easier to review if the SYM_FUNC_START and
>> SYM_FUNC_END pairs for a given function are done in the same patch.
> 
> This patchset was intended to make everything paired with minimum
> changes. I certainly can change also counter-elements of each
> added/changed one if you prefer.

So do really you want me to use the new macros while I am
adding/changing the counter-macro? Is there anything else blocking the
merge of the patches?

>> Also I noticed several cases in entry_64.S where the old ENTRY macro is
>> still used, and paired with SYM_FUNC_END.
>>
>> Maybe there should be an x86 version of the deprecated ENTRY/ENDPROC/etc
>> macros which throw a warning or an error?
> 
> Yes, my plan is to throw ENTRY/ENDPROC on the floor from x86 completely.
> And I will do it after this patchset settles down by sed or something in
> one shot (per directory or something).
> 
> thanks,
-- 
js
suse labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Josh Poimboeuf]

On Mon, Apr 10, 2017 at 01:23:46PM +0200, Jiri Slaby wrote:
> On 03/22/2017, 04:44 PM, Jiri Slaby wrote:
> > On 03/22/2017, 03:26 PM, Josh Poimboeuf wrote:
> >> On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
> >>> Somewhere END was used to end a function, elsewhere, nothing was used.
> >>> So unify it and mark them all by SYM_FUNC_END.
> >>>
> >>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> >>
> >> For me these patches would be easier to review if the SYM_FUNC_START and
> >> SYM_FUNC_END pairs for a given function are done in the same patch.
> > 
> > This patchset was intended to make everything paired with minimum
> > changes. I certainly can change also counter-elements of each
> > added/changed one if you prefer.
> 
> So do really you want me to use the new macros while I am
> adding/changing the counter-macro? Is there anything else blocking the
> merge of the patches?

The code should be in a mergeable state after each patch.  If only
patches 1-3 were merged, the code would be in an inconsistent state,
with some functions having confusing ENTRY/SYM_FUNC_END pairs.  That
complicates git history and also makes it harder to review each patch.

It would be cleaner to separate things out.  First, convert ENTRY/END
functions to use ENDPROC, which is a minor bug fix.  Then they can be
converted to the new SYM_FUNC_START/END macros in a separate patch.

-- 
Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Jiri Slaby]

On 04/10/2017, 09:35 PM, Josh Poimboeuf wrote:
> The code should be in a mergeable state after each patch.  If only
> patches 1-3 were merged, the code would be in an inconsistent state,
> with some functions having confusing ENTRY/SYM_FUNC_END pairs.  That
> complicates git history and also makes it harder to review each patch.
> 
> It would be cleaner to separate things out.  First, convert ENTRY/END
> functions to use ENDPROC, which is a minor bug fix.  Then they can be
> converted to the new SYM_FUNC_START/END macros in a separate patch.

OTOH I don't think touching and reviewing the same place twice is what
actually maintainers would want to see. But as I wrote earlier, I can do
whatever is preferred -- therefore I am asking before I start reworking
the patches: maintainers, what do you prefer?

thanks,
-- 
js
suse labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

[Ingo Molnar]

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

> On 04/10/2017, 09:35 PM, Josh Poimboeuf wrote:
> > The code should be in a mergeable state after each patch.  If only
> > patches 1-3 were merged, the code would be in an inconsistent state,
> > with some functions having confusing ENTRY/SYM_FUNC_END pairs.  That
> > complicates git history and also makes it harder to review each patch.
> > 
> > It would be cleaner to separate things out.  First, convert ENTRY/END
> > functions to use ENDPROC, which is a minor bug fix.  Then they can be
> > converted to the new SYM_FUNC_START/END macros in a separate patch.
> 
> OTOH I don't think touching and reviewing the same place twice is what
> actually maintainers would want to see. But as I wrote earlier, I can do
> whatever is preferred -- therefore I am asking before I start reworking
> the patches: maintainers, what do you prefer?

I'd lean towards Josh's suggestion of a more granular series. Having to review 
more is sometimes less, if the patches are more focused.

Thanks,

	Ingo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel