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

2017-04-12 Thread Ingo Molnar 

* Jiri Slaby  wrote:

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

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

Thanks,

Ingo

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

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

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

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

thanks,
-- 
js
suse labs

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

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

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

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

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

-- 
Josh

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

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

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

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

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

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

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

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

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

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

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

thanks,
-- 
js
suse labs

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

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

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

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

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

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

-- 
Josh

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

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

2017-03-22 Thread Ingo Molnar 

* Josh Poimboeuf  wrote:

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

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

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

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

Thanks,

Ingo

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

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

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

This gives ftrace_caller() two ends.

-- 
Josh

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

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

2017-03-20 Thread Jiri Slaby 
Somewhere END was used to end a function, elsewhere, nothing was used.
So unify it and mark them all by SYM_FUNC_END.

Signed-off-by: Jiri Slaby 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: 
Cc: "Rafael J. Wysocki" 
Cc: Pavel Machek 
Cc: 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Reviewed-by: Juergen Gross  [xen parts]
Cc: 
---
 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
declPER_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) */
callxen_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