* [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data @ 2017-02-17 10:47 Jiri Slaby 2017-02-17 10:47 ` [PATCH 02/10] x86: assembly, use ENDPROC for functions Jiri Slaby ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Jiri Slaby @ 2017-02-17 10:47 UTC (permalink / raw) To: mingo Cc: tglx, hpa, x86, jpoimboe, linux-kernel, Jiri Slaby, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 02/10] x86: assembly, use ENDPROC for functions 2017-02-17 10:47 [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Jiri Slaby @ 2017-02-17 10:47 ` Jiri Slaby 2017-02-17 11:08 ` Juergen Gross 2017-02-17 11:06 ` [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Juergen Gross 2017-03-01 9:38 ` Ingo Molnar 2 siblings, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-02-17 10:47 UTC (permalink / raw) To: mingo Cc: Juergen Gross, hpa, linux-pm, x86, Rafael J. Wysocki, linux-kernel, Pavel Machek, jpoimboe, xen-devel, tglx, Jiri Slaby, Boris Ostrovsky Somewhere END was used to end a function, elsewhere, nothing was used. So unify it and mark them all by ENDPROC. 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> Cc: <xen-devel@lists.xenproject.org> --- arch/x86/entry/entry_64.S | 40 ++++++++++++++++++------------------ arch/x86/entry/entry_64_compat.S | 4 ++-- arch/x86/kernel/mcount_64.S | 10 +++++---- 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, 40 insertions(+), 28 deletions(-) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 044d18ebc43c..2f06104e2b5e 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) +ENDPROC(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) +ENDPROC(stub_ptregs_64) .macro ptregs_stub func ENTRY(ptregs_\func) leaq \func(%rip), %rax jmp stub_ptregs_64 -END(ptregs_\func) +ENDPROC(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) +ENDPROC(__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) +ENDPROC(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) +ENDPROC(irq_entries_start) /* * Interrupt entry/exit. @@ -652,7 +652,7 @@ native_irq_return_ldt: */ jmp native_irq_return_iret #endif -END(common_interrupt) +ENDPROC(common_interrupt) /* * APIC interrupts. @@ -664,7 +664,7 @@ ENTRY(\sym) .Lcommon_\sym: interrupt \do_sym jmp ret_from_intr -END(\sym) +ENDPROC(\sym) .endm #ifdef CONFIG_TRACING @@ -830,7 +830,7 @@ ENTRY(\sym) jmp error_exit /* %ebx: no swapgs flag */ .endif -END(\sym) +ENDPROC(\sym) .endm #ifdef CONFIG_TRACING @@ -873,7 +873,7 @@ ENTRY(native_load_gs_index) SWAPGS popfq ret -END(native_load_gs_index) +ENDPROC(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) +ENDPROC(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) +ENDPROC(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) +ENDPROC(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) +ENDPROC(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) +ENDPROC(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) +ENDPROC(error_entry) /* @@ -1162,7 +1162,7 @@ ENTRY(error_exit) testl %eax, %eax jnz retint_kernel jmp retint_user -END(error_exit) +ENDPROC(error_exit) /* Runs on exception stack */ ENTRY(nmi) @@ -1510,12 +1510,12 @@ nmi_restore: * mode, so this cannot result in a fault. */ INTERRUPT_RETURN -END(nmi) +ENDPROC(nmi) ENTRY(ignore_sysret) mov $-ENOSYS, %eax sysret -END(ignore_sysret) +ENDPROC(ignore_sysret) ENTRY(rewind_stack_do_exit) /* Prevent any naive code from trying to unwind to our caller. */ @@ -1526,4 +1526,4 @@ ENTRY(rewind_stack_do_exit) call do_exit 1: jmp 1b -END(rewind_stack_do_exit) +ENDPROC(rewind_stack_do_exit) diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S index e1721dafbcb1..966c09ffd62d 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) +ENDPROC(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) +ENDPROC(entry_INT80_compat) ALIGN GLOBAL(stub32_clone) diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S index 3e35675e201e..5255c7888c50 100644 --- a/arch/x86/kernel/mcount_64.S +++ b/arch/x86/kernel/mcount_64.S @@ -145,7 +145,7 @@ ENTRY(fentry_hook) retq -END(fentry_hook) +ENDPROC(fentry_hook) ENTRY(ftrace_caller) /* save_mcount_regs fills in first two parameters */ @@ -177,6 +177,7 @@ GLOBAL(ftrace_epilogue) GLOBAL(ftrace_graph_call) jmp ftrace_stub #endif +ENDPROC(ftrace_caller) /* This is weak to keep gas from relaxing the jumps */ WEAK(ftrace_stub) @@ -252,7 +253,7 @@ GLOBAL(ftrace_regs_caller_end) jmp ftrace_epilogue -END(ftrace_regs_caller) +ENDPROC(ftrace_regs_caller) #else /* ! CONFIG_DYNAMIC_FTRACE */ @@ -288,7 +289,7 @@ trace: restore_mcount_regs jmp fgraph_trace -END(fentry_hook) +ENDPROC(fentry_hook) #endif /* CONFIG_DYNAMIC_FTRACE */ EXPORT_SYMBOL(fentry_hook) #endif /* CONFIG_FUNCTION_TRACER */ @@ -312,7 +313,7 @@ ENTRY(ftrace_graph_caller) restore_mcount_regs retq -END(ftrace_graph_caller) +ENDPROC(ftrace_graph_caller) ENTRY(return_to_handler) subq $24, %rsp @@ -329,4 +330,5 @@ ENTRY(return_to_handler) movq (%rsp), %rax addq $24, %rsp jmp *%rdi +ENDPROC(return_to_handler) #endif diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S index ce8da3a0412c..ec6b26fd3a7e 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 +ENDPROC(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 +ENDPROC(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..c8855d50f9c1 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 +ENDPROC(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..fe21a26a09fe 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 +ENDPROC(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 +ENDPROC(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) +ENDPROC(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..08203a187446 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 +ENDPROC(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..d617bea76039 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) +ENDPROC(xen_iret) ENTRY(xen_sysret64) /* @@ -68,6 +69,7 @@ ENTRY(xen_sysret64) 1: jmp hypercall_iret ENDPATCH(xen_sysret64) RELOC(xen_sysret64, 1b+1) +ENDPROC(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 39ea5484763a..0e793c1f0d45 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 - +ENDPROC(startup_xen) __FINIT .pushsection .text diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S index 5e246716d58f..512fda03c93f 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) +ENDPROC(pvh_start_xen) .section ".init.data","aw" .balign 8 -- 2.11.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 02/10] x86: assembly, use ENDPROC for functions 2017-02-17 10:47 ` [PATCH 02/10] x86: assembly, use ENDPROC for functions Jiri Slaby @ 2017-02-17 11:08 ` Juergen Gross 0 siblings, 0 replies; 42+ messages in thread From: Juergen Gross @ 2017-02-17 11:08 UTC (permalink / raw) To: Jiri Slaby, mingo Cc: tglx, hpa, x86, jpoimboe, linux-kernel, Rafael J. Wysocki, Pavel Machek, linux-pm, Boris Ostrovsky, xen-devel On 17/02/17 11:47, Jiri Slaby wrote: > Somewhere END was used to end a function, elsewhere, nothing was used. > So unify it and mark them all by ENDPROC. > > 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> > Cc: <xen-devel@lists.xenproject.org> > --- > arch/x86/entry/entry_64.S | 40 ++++++++++++++++++------------------ > arch/x86/entry/entry_64_compat.S | 4 ++-- > arch/x86/kernel/mcount_64.S | 10 +++++---- > 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, 40 insertions(+), 28 deletions(-) Xen parts: Reviewed-by: Juergen Gross <jgross@suse.com> Juergen ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-02-17 10:47 [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Jiri Slaby 2017-02-17 10:47 ` [PATCH 02/10] x86: assembly, use ENDPROC for functions Jiri Slaby @ 2017-02-17 11:06 ` Juergen Gross 2017-03-01 9:38 ` Ingo Molnar 2 siblings, 0 replies; 42+ messages in thread From: Juergen Gross @ 2017-02-17 11:06 UTC (permalink / raw) To: Jiri Slaby, mingo Cc: tglx, hpa, x86, jpoimboe, linux-kernel, Boris Ostrovsky, xen-devel, Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-02-17 10:47 [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Jiri Slaby 2017-02-17 10:47 ` [PATCH 02/10] x86: assembly, use ENDPROC for functions Jiri Slaby 2017-02-17 11:06 ` [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Juergen Gross @ 2017-03-01 9:38 ` Ingo Molnar 2017-03-01 9:50 ` Jiri Slaby 2017-03-01 10:09 ` Thomas Gleixner 2 siblings, 2 replies; 42+ messages in thread From: Ingo Molnar @ 2017-03-01 9:38 UTC (permalink / raw) To: Jiri Slaby Cc: Juergen Gross, Len Brown, hpa, Andrew Morton, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, tglx, Linus Torvalds, Boris Ostrovsky, Peter Zijlstra * 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-01 9:38 ` Ingo Molnar @ 2017-03-01 9:50 ` Jiri Slaby 2017-03-01 10:09 ` Thomas Gleixner 1 sibling, 0 replies; 42+ messages in thread From: Jiri Slaby @ 2017-03-01 9:50 UTC (permalink / raw) To: Ingo Molnar Cc: Juergen Gross, Len Brown, hpa, Andrew Morton, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, tglx, Linus Torvalds, Boris Ostrovsky, Peter Zijlstra 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-01 9:38 ` Ingo Molnar 2017-03-01 9:50 ` Jiri Slaby @ 2017-03-01 10:09 ` Thomas Gleixner 2017-03-01 10:27 ` Ingo Molnar 1 sibling, 1 reply; 42+ messages in thread From: Thomas Gleixner @ 2017-03-01 10:09 UTC (permalink / raw) To: Ingo Molnar Cc: Jiri Slaby, mingo, hpa, x86, jpoimboe, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm, Linus Torvalds, Andrew Morton, Peter Zijlstra 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-01 10:09 ` Thomas Gleixner @ 2017-03-01 10:27 ` Ingo Molnar 2017-03-03 12:22 ` Jiri Slaby ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Ingo Molnar @ 2017-03-01 10:27 UTC (permalink / raw) To: Thomas Gleixner Cc: Juergen Gross, Len Brown, hpa, Peter Zijlstra, linux-pm, Linus Torvalds, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, Boris Ostrovsky, Jiri Slaby, Andrew Morton * 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-01 10:27 ` Ingo Molnar @ 2017-03-03 12:22 ` Jiri Slaby 2017-03-03 18:20 ` hpa 2017-03-03 18:24 ` hpa 2 siblings, 0 replies; 42+ messages in thread From: Jiri Slaby @ 2017-03-03 12:22 UTC (permalink / raw) To: Ingo Molnar, Thomas Gleixner Cc: Juergen Gross, Len Brown, hpa, Peter Zijlstra, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, Boris Ostrovsky, Linus Torvalds, Andrew Morton 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-01 10:27 ` Ingo Molnar 2017-03-03 12:22 ` Jiri Slaby @ 2017-03-03 18:20 ` hpa 2017-03-06 14:09 ` Jiri Slaby 2017-03-07 7:57 ` Ingo Molnar 2017-03-03 18:24 ` hpa 2 siblings, 2 replies; 42+ messages in thread From: hpa @ 2017-03-03 18:20 UTC (permalink / raw) To: Ingo Molnar, Thomas Gleixner Cc: Juergen Gross, Len Brown, Peter Zijlstra, linux-pm, Linus Torvalds, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, Boris Ostrovsky, Jiri Slaby, Andrew Morton 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-03 18:20 ` hpa @ 2017-03-06 14:09 ` Jiri Slaby 2017-03-07 7:57 ` Ingo Molnar 1 sibling, 0 replies; 42+ messages in thread From: Jiri Slaby @ 2017-03-06 14:09 UTC (permalink / raw) To: hpa, Ingo Molnar, Thomas Gleixner Cc: Juergen Gross, Len Brown, Peter Zijlstra, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, Boris Ostrovsky, Linus Torvalds, Andrew Morton 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-03 18:20 ` hpa 2017-03-06 14:09 ` Jiri Slaby @ 2017-03-07 7:57 ` Ingo Molnar 1 sibling, 0 replies; 42+ messages in thread From: Ingo Molnar @ 2017-03-07 7:57 UTC (permalink / raw) To: hpa Cc: Juergen Gross, Len Brown, Andrew Morton, linux-pm, Linus Torvalds, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, Thomas Gleixner, Jiri Slaby, Boris Ostrovsky, Peter Zijlstra * 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-01 10:27 ` Ingo Molnar 2017-03-03 12:22 ` Jiri Slaby 2017-03-03 18:20 ` hpa @ 2017-03-03 18:24 ` hpa 2017-03-07 8:27 ` Ingo Molnar 2 siblings, 1 reply; 42+ messages in thread From: hpa @ 2017-03-03 18:24 UTC (permalink / raw) To: Ingo Molnar, Thomas Gleixner Cc: Juergen Gross, Len Brown, Peter Zijlstra, linux-pm, Linus Torvalds, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, Boris Ostrovsky, Jiri Slaby, Andrew Morton 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data 2017-03-03 18:24 ` hpa @ 2017-03-07 8:27 ` Ingo Molnar 2017-03-07 17:24 ` [RFC] linkage: new macros for functions and data Jiri Slaby 0 siblings, 1 reply; 42+ messages in thread From: Ingo Molnar @ 2017-03-07 8:27 UTC (permalink / raw) To: hpa Cc: Juergen Gross, Len Brown, Andrew Morton, linux-pm, Linus Torvalds, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, jpoimboe, xen-devel, Thomas Gleixner, Jiri Slaby, Boris Ostrovsky, Peter Zijlstra * 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* [RFC] linkage: new macros for functions and data 2017-03-07 8:27 ` Ingo Molnar @ 2017-03-07 17:24 ` Jiri Slaby 2017-03-16 8:02 ` Ingo Molnar 0 siblings, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-03-07 17:24 UTC (permalink / raw) To: Ingo Molnar Cc: Juergen Gross, Len Brown, x86, Peter Zijlstra, linux-pm, Linus Torvalds, hpa, Rafael J. Wysocki, linux-kernel, mingo, Thomas Gleixner, Pavel Machek, jpoimboe, xen-devel, Boris Ostrovsky, Jiri Slaby, Andrew Morton 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [RFC] linkage: new macros for functions and data 2017-03-07 17:24 ` [RFC] linkage: new macros for functions and data Jiri Slaby @ 2017-03-16 8:02 ` Ingo Molnar 2017-03-16 8:13 ` Jiri Slaby 0 siblings, 1 reply; 42+ messages in thread From: Ingo Molnar @ 2017-03-16 8:02 UTC (permalink / raw) To: Jiri Slaby Cc: Juergen Gross, Len Brown, x86, Peter Zijlstra, linux-pm, hpa, Rafael J. Wysocki, linux-kernel, mingo, Thomas Gleixner, Pavel Machek, jpoimboe, xen-devel, Boris Ostrovsky, Linus Torvalds, Andrew Morton * Jiri Slaby <jslaby@suse.cz> wrote: > 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 This looks mostly good to me, with minor details: - The mixed 'FUNC' and 'FUNCTION' naming looks a bit confusing - I'd pick one and use that consistently. - I'd also make the START/END constructs look more symmetric, i.e. make the attribute a postfix, not a prefix. - In fact I'd make the 'alias' notion an attribute as well - what matters mostly is that it's a function symbol, and that fact is lost from the SYM_ALIAS naming. I.e. something like this: SYM_FUNCTION_START SYM_FUNCTION_START_WEAK SYM_FUNCTION_START_LOCAL SYM_FUNCTION_START_ALIAS SYM_FUNCTION_START_LOCAL_ALIAS ... SYM_FUNCTION_END SYM_DATA_START SYM_DATA_END Note how simple and structured looking the nomenclature is when grouped in such a way, how easy it is to verify at a glance, and how easy it is to extend with new postfixes. ( In theory we could also make the attributes a real macro argument in the future, should the number of attributes increase significantly. ) Does this look good to everyone? Thanks, Ingo _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFC] linkage: new macros for functions and data 2017-03-16 8:02 ` Ingo Molnar @ 2017-03-16 8:13 ` Jiri Slaby 2017-03-20 12:32 ` [PATCH v2 01/10] linkage: new macros for assembler symbols Jiri Slaby 0 siblings, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-03-16 8:13 UTC (permalink / raw) To: Ingo Molnar Cc: linux-kernel, Andrew Morton, Boris Ostrovsky, hpa, jpoimboe, Juergen Gross, Len Brown, Linus Torvalds, linux-pm, mingo, Pavel Machek, Peter Zijlstra, Rafael J. Wysocki, Thomas Gleixner, xen-devel, x86 On 03/16/2017, 09:02 AM, Ingo Molnar wrote: > > * Jiri Slaby <jslaby@suse.cz> wrote: > >> 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 > > This looks mostly good to me, with minor details: > > - The mixed 'FUNC' and 'FUNCTION' naming looks a bit confusing - I'd pick one and > use that consistently. Of course, I did it later after sending the RFC. I stuck to FUNC. ... > Does this look good to everyone? Fine by me. I will send patches for that, so that you can comment on that on real uses. thanks, -- js suse labs ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 01/10] linkage: new macros for assembler symbols 2017-03-16 8:13 ` Jiri Slaby @ 2017-03-20 12:32 ` Jiri Slaby 2017-03-20 12:32 ` [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby 2017-03-20 12:32 ` [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Jiri Slaby 0 siblings, 2 replies; 42+ messages in thread From: Jiri Slaby @ 2017-03-20 12:32 UTC (permalink / raw) To: mingo Cc: tglx, hpa, x86, jpoimboe, linux-kernel, Jiri Slaby, Andrew Morton, Boris Ostrovsky, Ingo Molnar, Juergen Gross, Len Brown, Linus Torvalds, linux-pm, Pavel Machek, Peter Zijlstra, Rafael J. Wysocki, xen-devel 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-20 12:32 ` [PATCH v2 01/10] linkage: new macros for assembler symbols Jiri Slaby @ 2017-03-20 12:32 ` Jiri Slaby 2017-03-20 13:32 ` Josh Poimboeuf 2017-03-21 14:08 ` Pavel Machek 2017-03-20 12:32 ` [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Jiri Slaby 1 sibling, 2 replies; 42+ messages in thread From: Jiri Slaby @ 2017-03-20 12:32 UTC (permalink / raw) To: mingo Cc: Juergen Gross, Len Brown, hpa, linux-pm, x86, Rafael J. Wysocki, linux-kernel, Pavel Machek, jpoimboe, xen-devel, tglx, Jiri Slaby, Boris Ostrovsky 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-20 12:32 ` [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby @ 2017-03-20 13:32 ` Josh Poimboeuf 2017-03-20 15:32 ` Jiri Slaby 2017-03-21 14:08 ` Pavel Machek 1 sibling, 1 reply; 42+ messages in thread From: Josh Poimboeuf @ 2017-03-20 13:32 UTC (permalink / raw) To: Jiri Slaby Cc: Juergen Gross, Len Brown, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, hpa, xen-devel, Boris Ostrovsky, tglx 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-20 13:32 ` Josh Poimboeuf @ 2017-03-20 15:32 ` Jiri Slaby 2017-03-20 16:07 ` Josh Poimboeuf 0 siblings, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-03-20 15:32 UTC (permalink / raw) To: Josh Poimboeuf Cc: Juergen Gross, Len Brown, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, hpa, xen-devel, Boris Ostrovsky, tglx 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-20 15:32 ` Jiri Slaby @ 2017-03-20 16:07 ` Josh Poimboeuf 0 siblings, 0 replies; 42+ messages in thread From: Josh Poimboeuf @ 2017-03-20 16:07 UTC (permalink / raw) To: Jiri Slaby Cc: Juergen Gross, Len Brown, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, hpa, xen-devel, Boris Ostrovsky, tglx 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-20 12:32 ` [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby 2017-03-20 13:32 ` Josh Poimboeuf @ 2017-03-21 14:08 ` Pavel Machek 2017-03-22 7:25 ` Ingo Molnar 2017-03-22 12:06 ` Jiri Slaby 1 sibling, 2 replies; 42+ messages in thread From: Pavel Machek @ 2017-03-21 14:08 UTC (permalink / raw) To: Jiri Slaby Cc: Juergen Gross, Len Brown, hpa, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, jpoimboe, xen-devel, tglx, Boris Ostrovsky [-- 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-21 14:08 ` Pavel Machek @ 2017-03-22 7:25 ` Ingo Molnar 2017-03-22 7:39 ` Jiri Slaby 2017-03-22 12:06 ` Jiri Slaby 1 sibling, 1 reply; 42+ messages in thread From: Ingo Molnar @ 2017-03-22 7:25 UTC (permalink / raw) To: Pavel Machek Cc: Jiri Slaby, mingo, tglx, hpa, x86, jpoimboe, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm * 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-22 7:25 ` Ingo Molnar @ 2017-03-22 7:39 ` Jiri Slaby 2017-03-22 7:46 ` Ingo Molnar 0 siblings, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-03-22 7:39 UTC (permalink / raw) To: Ingo Molnar, Pavel Machek, Josh Poimboeuf Cc: mingo, tglx, hpa, x86, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-22 7:39 ` Jiri Slaby @ 2017-03-22 7:46 ` Ingo Molnar 2017-03-22 14:11 ` Josh Poimboeuf 0 siblings, 1 reply; 42+ messages in thread From: Ingo Molnar @ 2017-03-22 7:46 UTC (permalink / raw) To: Jiri Slaby Cc: Pavel Machek, Josh Poimboeuf, mingo, tglx, hpa, x86, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm * 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-22 7:46 ` Ingo Molnar @ 2017-03-22 14:11 ` Josh Poimboeuf 2017-03-22 15:01 ` Jiri Slaby 2017-03-23 7:38 ` Ingo Molnar 0 siblings, 2 replies; 42+ messages in thread From: Josh Poimboeuf @ 2017-03-22 14:11 UTC (permalink / raw) To: Ingo Molnar Cc: Jiri Slaby, Pavel Machek, mingo, tglx, hpa, x86, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-22 14:11 ` Josh Poimboeuf @ 2017-03-22 15:01 ` Jiri Slaby 2017-03-22 15:33 ` Josh Poimboeuf 2017-03-23 7:38 ` Ingo Molnar 1 sibling, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-03-22 15:01 UTC (permalink / raw) To: Josh Poimboeuf, Ingo Molnar Cc: Pavel Machek, mingo, tglx, hpa, x86, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-22 15:01 ` Jiri Slaby @ 2017-03-22 15:33 ` Josh Poimboeuf 0 siblings, 0 replies; 42+ messages in thread From: Josh Poimboeuf @ 2017-03-22 15:33 UTC (permalink / raw) To: Jiri Slaby Cc: Ingo Molnar, Pavel Machek, mingo, tglx, hpa, x86, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-22 14:11 ` Josh Poimboeuf 2017-03-22 15:01 ` Jiri Slaby @ 2017-03-23 7:38 ` Ingo Molnar 2017-03-23 13:24 ` Josh Poimboeuf 1 sibling, 1 reply; 42+ messages in thread From: Ingo Molnar @ 2017-03-23 7:38 UTC (permalink / raw) To: Josh Poimboeuf Cc: Jiri Slaby, Pavel Machek, mingo, tglx, hpa, x86, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm * 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-23 7:38 ` Ingo Molnar @ 2017-03-23 13:24 ` Josh Poimboeuf 0 siblings, 0 replies; 42+ messages in thread From: Josh Poimboeuf @ 2017-03-23 13:24 UTC (permalink / raw) To: Ingo Molnar Cc: Jiri Slaby, Pavel Machek, mingo, tglx, hpa, x86, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-21 14:08 ` Pavel Machek 2017-03-22 7:25 ` Ingo Molnar @ 2017-03-22 12:06 ` Jiri Slaby 2017-03-22 15:52 ` Pavel Machek 1 sibling, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-03-22 12:06 UTC (permalink / raw) To: Pavel Machek Cc: mingo, tglx, hpa, x86, jpoimboe, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm [-- Attachment #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 #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 854 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data 2017-03-22 12:06 ` Jiri Slaby @ 2017-03-22 15:52 ` Pavel Machek 0 siblings, 0 replies; 42+ messages in thread From: Pavel Machek @ 2017-03-22 15:52 UTC (permalink / raw) To: Jiri Slaby Cc: mingo, tglx, hpa, x86, jpoimboe, linux-kernel, Boris Ostrovsky, Juergen Gross, xen-devel, Rafael J. Wysocki, Len Brown, linux-pm [-- Attachment #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 #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-03-20 12:32 ` [PATCH v2 01/10] linkage: new macros for assembler symbols Jiri Slaby 2017-03-20 12:32 ` [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby @ 2017-03-20 12:32 ` Jiri Slaby 2017-03-21 14:48 ` Josh Poimboeuf 2017-03-22 14:26 ` Josh Poimboeuf 1 sibling, 2 replies; 42+ messages in thread From: Jiri Slaby @ 2017-03-20 12:32 UTC (permalink / raw) To: mingo Cc: tglx, hpa, x86, jpoimboe, linux-kernel, Jiri Slaby, Rafael J. Wysocki, Pavel Machek, linux-pm, Boris Ostrovsky, Juergen Gross, xen-devel 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-03-20 12:32 ` [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Jiri Slaby @ 2017-03-21 14:48 ` Josh Poimboeuf 2017-03-22 7:29 ` Ingo Molnar 2017-03-22 14:26 ` Josh Poimboeuf 1 sibling, 1 reply; 42+ messages in thread From: Josh Poimboeuf @ 2017-03-21 14:48 UTC (permalink / raw) To: Jiri Slaby Cc: mingo, tglx, hpa, x86, linux-kernel, Rafael J. Wysocki, Pavel Machek, linux-pm, Boris Ostrovsky, Juergen Gross, xen-devel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-03-21 14:48 ` Josh Poimboeuf @ 2017-03-22 7:29 ` Ingo Molnar 0 siblings, 0 replies; 42+ messages in thread From: Ingo Molnar @ 2017-03-22 7:29 UTC (permalink / raw) To: Josh Poimboeuf Cc: Juergen Gross, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, hpa, xen-devel, tglx, Jiri Slaby, Boris Ostrovsky * 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-03-20 12:32 ` [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Jiri Slaby 2017-03-21 14:48 ` Josh Poimboeuf @ 2017-03-22 14:26 ` Josh Poimboeuf 2017-03-22 15:44 ` Jiri Slaby 1 sibling, 1 reply; 42+ messages in thread From: Josh Poimboeuf @ 2017-03-22 14:26 UTC (permalink / raw) To: Jiri Slaby Cc: mingo, tglx, hpa, x86, linux-kernel, Rafael J. Wysocki, Pavel Machek, linux-pm, Boris Ostrovsky, Juergen Gross, xen-devel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-03-22 14:26 ` Josh Poimboeuf @ 2017-03-22 15:44 ` Jiri Slaby 2017-04-10 11:23 ` Jiri Slaby 0 siblings, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-03-22 15:44 UTC (permalink / raw) To: Josh Poimboeuf Cc: Juergen Gross, linux-pm, x86, Rafael J. Wysocki, linux-kernel, mingo, Pavel Machek, hpa, xen-devel, tglx, Boris Ostrovsky 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-03-22 15:44 ` Jiri Slaby @ 2017-04-10 11:23 ` Jiri Slaby 2017-04-10 19:35 ` Josh Poimboeuf 0 siblings, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-04-10 11:23 UTC (permalink / raw) To: Josh Poimboeuf Cc: mingo, tglx, hpa, x86, linux-kernel, Rafael J. Wysocki, Pavel Machek, linux-pm, Boris Ostrovsky, Juergen Gross, xen-devel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-04-10 11:23 ` Jiri Slaby @ 2017-04-10 19:35 ` Josh Poimboeuf 2017-04-12 6:24 ` Jiri Slaby 0 siblings, 1 reply; 42+ messages in thread From: Josh Poimboeuf @ 2017-04-10 19:35 UTC (permalink / raw) To: Jiri Slaby Cc: mingo, tglx, hpa, x86, linux-kernel, Rafael J. Wysocki, Pavel Machek, linux-pm, Boris Ostrovsky, Juergen Gross, xen-devel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-04-10 19:35 ` Josh Poimboeuf @ 2017-04-12 6:24 ` Jiri Slaby 2017-04-12 6:52 ` Ingo Molnar 0 siblings, 1 reply; 42+ messages in thread From: Jiri Slaby @ 2017-04-12 6:24 UTC (permalink / raw) To: Josh Poimboeuf Cc: mingo, tglx, hpa, x86, linux-kernel, Rafael J. Wysocki, Pavel Machek, linux-pm, Boris Ostrovsky, Juergen Gross, xen-devel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions 2017-04-12 6:24 ` Jiri Slaby @ 2017-04-12 6:52 ` Ingo Molnar 0 siblings, 0 replies; 42+ messages in thread From: Ingo Molnar @ 2017-04-12 6:52 UTC (permalink / raw) To: Jiri Slaby Cc: Josh Poimboeuf, mingo, tglx, hpa, x86, linux-kernel, Rafael J. Wysocki, Pavel Machek, linux-pm, Boris Ostrovsky, Juergen Gross, xen-devel * 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2017-04-12 6:52 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-17 10:47 [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Jiri Slaby 2017-02-17 10:47 ` [PATCH 02/10] x86: assembly, use ENDPROC for functions Jiri Slaby 2017-02-17 11:08 ` Juergen Gross 2017-02-17 11:06 ` [PATCH 01/10] x86: assembly, ENTRY for fn, GLOBAL for data Juergen Gross 2017-03-01 9:38 ` Ingo Molnar 2017-03-01 9:50 ` Jiri Slaby 2017-03-01 10:09 ` Thomas Gleixner 2017-03-01 10:27 ` Ingo Molnar 2017-03-03 12:22 ` Jiri Slaby 2017-03-03 18:20 ` hpa 2017-03-06 14:09 ` Jiri Slaby 2017-03-07 7:57 ` Ingo Molnar 2017-03-03 18:24 ` hpa 2017-03-07 8:27 ` Ingo Molnar 2017-03-07 17:24 ` [RFC] linkage: new macros for functions and data Jiri Slaby 2017-03-16 8:02 ` Ingo Molnar 2017-03-16 8:13 ` Jiri Slaby 2017-03-20 12:32 ` [PATCH v2 01/10] linkage: new macros for assembler symbols Jiri Slaby 2017-03-20 12:32 ` [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby 2017-03-20 13:32 ` Josh Poimboeuf 2017-03-20 15:32 ` Jiri Slaby 2017-03-20 16:07 ` Josh Poimboeuf 2017-03-21 14:08 ` Pavel Machek 2017-03-22 7:25 ` Ingo Molnar 2017-03-22 7:39 ` Jiri Slaby 2017-03-22 7:46 ` Ingo Molnar 2017-03-22 14:11 ` Josh Poimboeuf 2017-03-22 15:01 ` Jiri Slaby 2017-03-22 15:33 ` Josh Poimboeuf 2017-03-23 7:38 ` Ingo Molnar 2017-03-23 13:24 ` Josh Poimboeuf 2017-03-22 12:06 ` Jiri Slaby 2017-03-22 15:52 ` Pavel Machek 2017-03-20 12:32 ` [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Jiri Slaby 2017-03-21 14:48 ` Josh Poimboeuf 2017-03-22 7:29 ` Ingo Molnar 2017-03-22 14:26 ` Josh Poimboeuf 2017-03-22 15:44 ` Jiri Slaby 2017-04-10 11:23 ` Jiri Slaby 2017-04-10 19:35 ` Josh Poimboeuf 2017-04-12 6:24 ` Jiri Slaby 2017-04-12 6:52 ` Ingo Molnar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).