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

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

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.

> Note that we'd need to split DATA() in order to separate r/w and r/o
> contributions. Further splitting might be needed to also support more
> advanced attributes (e.g. merge), hence why this isn't done right here.
> Sadly while a new section's name can be derived from the presently in
> use, its attributes cannot be. Perhaps the only thing we can do is give
> DATA() a 2nd mandatory parameter. Then again I guess most data
> definitions could be moved to C anyway.
> ---
> v5: Re-base over changes earlier in the series.
> v4: Re-base.
> v2: Make detection properly fail on old gas (by adjusting
>     cc-option-add-closure).
> 
> --- a/Config.mk
> +++ b/Config.mk
> @@ -102,7 +102,7 @@ cc-option = $(shell if $(1) $(2:-Wno-%=-
>  # Usage: $(call cc-option-add CFLAGS,CC,-march=winchip-c6)
>  cc-option-add = $(eval $(call cc-option-add-closure,$(1),$(2),$(3)))
>  define cc-option-add-closure
> -    ifneq ($$(call cc-option,$$($(2)),$(3),n),n)
> +    ifneq ($$(call cc-option,$$($(2)),$(firstword $(3)),n),n)
>          $(1) += $(3)
>      endif
>  endef
> --- 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.

Thanks, Roger.