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

[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

[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

[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

[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

[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

[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

[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

[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