Re: [PATCH v5 8/8] common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly functions

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 22, 2024 at 11:50:08AM +0100, Jan Beulich wrote:
> On 19.01.2024 11:36, Roger Pau Monné wrote:
> > On Mon, Jan 15, 2024 at 03:40:19PM +0100, Jan Beulich wrote:
> >> Leverage the new infrastructure in xen/linkage.h to also switch to per-
> >> function sections (when configured), deriving the specific name from the
> >> "base" section in use at the time FUNC() is invoked.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really
> >>      wanted side effect of this change is that respective out-of-line
> >>      code now moves much closer to its original (invoking) code.
> > 
> > Hm, I'm afraid I don't have much useful suggestions here.
> > 
> >> TBD: Of course something with the same overall effect, but less
> >>      impactful might do in Config.mk. E.g. $(filter-out -D%,$(3))
> >>      instead of $(firstword (3)).
> > 
> > I don't have a strong opinion re those two options  My preference
> > however (see below for reasoning) would be to put this detection in
> > Kconfig.
> > 
> >> TBD: On top of Roger's respective patch (for livepatch), also respect
> >>      CONFIG_FUNCTION_ALIGNMENT.
> > 
> > I think you can drop that, as the series is blocked.
> 
> Considering the series here has been pending for quite some time, too,
> I guess I'd like to keep it just in case that other functionality
> becomes unblocked or available by some other means, even if only to
> remind myself.

So as you have seen I've posted a new version of just the function
alignment patch, that I expected wasn't controversial.

> >> --- a/xen/Makefile
> >> +++ b/xen/Makefile
> >> @@ -409,6 +409,9 @@ AFLAGS += -D__ASSEMBLY__
> >>  
> >>  $(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack)
> >>  
> >> +# Check to see whether the assmbler supports the --sectname-subst option.
> >> +$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst 
> >> -DHAVE_AS_SECTNAME_SUBST)
> > 
> > I guess you already know what I'm going to comment on.  I think this
> > would be clearer if it was a Kconfig option.  For once because I think
> > we should gate livapatch support on the option being available, and
> > secondly it would avoid having to pass the extra -D around.
> > 
> > I think it's relevant to have a consistent set of build tool options
> > requirements for livepatch support, so that when enabled the support
> > is consistent across builds.  With this approach livepatch could be
> > enabled in Kconfig, but depending on the tools support the resulting
> > binary might or might not support live patching of assembly code.
> > Such behavior is IMO unhelpful from a user PoV, and can lead to
> > surprises down the road.
> 
> I can see the desire to have LIVEPATCH grow such a dependency. Yet there
> is the bigger still open topic of the criteria towards what, if anything,
> to probe for in Kconfig, what, if anything, to probe for in Makefile, and
> which of the probing perhaps do in both places. I'm afraid my attempts to
> move us closer to a resolution (topic on summit, making proposals on
> list) have utterly failed. IOW I don't currently see how the existing
> disagreement there can be resolved, which will result in me to continue
> following the (traditional) Makefile approach unless I clearly view
> Kconfig superior in a particular case. I could perhaps be talked into
> following a "mixed Kconfig/Makefile model", along the lines of "x86:
> convert CET tool chain feature checks to mixed Kconfig/Makefile model",
> albeit I'm sure you're aware there are issues to sort out there, which I
> see no value in putting time into as long as I can't expect things to
> make progress subsequently.

I think there are more subtle cases where it's indeed arguable that
putting it in Kconfig or a Makefile makes no difference from a user
experience PoV, hence in the context here I do believe it wants to be
in Kconfig as LIVEPATCH can be make dependent on it.

> Dropping this patch, while an option, would seem undesirable to me, since
> even without Kconfig probing using new enough tool chains the splitting
> would yield reliable results, and provide - imo - an improvement over
> what we have right now.

I could send a followup afterwards to arrange for the check to be in
Kconfig, but it would feel a bit odd to me this is not done in the
first place.

I don't want to block the patch because I think it's useful, so worst
case I'm willing to give my Ack and provide an alternative Kconfig
based patch afterwards.

Thanks, Roger.