| Message ID | 5df1d87d-8e54-4e15-b1fb-46b274cb66ef@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | annotate entry points with type and size | expand |
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@suse.com> > --- > 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.
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@suse.com> >> --- >> 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. >> --- 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. 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. Jan
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@suse.com> > >> --- > >> 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.
--- 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) + LDFLAGS-$(call ld-option,--warn-rwx-segments) += --no-warn-rwx-segments CFLAGS += $(CFLAGS-y) --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -156,6 +156,9 @@ SECTIONS .init.text : { _sinittext = .; *(.init.text) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.init.text.*) +#endif _einittext = .; . = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */ *(.altinstr_replacement) --- a/xen/arch/ppc/xen.lds.S +++ b/xen/arch/ppc/xen.lds.S @@ -104,6 +104,9 @@ SECTIONS .init.text : { _sinittext = .; *(.init.text) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.init.text.*) +#endif _einittext = .; . = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */ } :text --- a/xen/arch/riscv/xen.lds.S +++ b/xen/arch/riscv/xen.lds.S @@ -104,6 +104,9 @@ SECTIONS .init.text : { _sinittext = .; *(.init.text) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.init.text.*) +#endif _einittext = .; . = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */ } :text --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -86,6 +86,9 @@ SECTIONS . = ALIGN(PAGE_SIZE); _stextentry = .; *(.text.entry) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.text.entry.*) +#endif . = ALIGN(PAGE_SIZE); _etextentry = .; @@ -214,6 +217,9 @@ SECTIONS #endif _sinittext = .; *(.init.text) +#ifdef CONFIG_CC_SPLIT_SECTIONS + *(.init.text.*) +#endif *(.text.startup) _einittext = .; /* --- a/xen/include/xen/linkage.h +++ b/xen/include/xen/linkage.h @@ -19,6 +19,14 @@ #define SYM_ALIGN(align...) .balign align +#if defined(HAVE_AS_SECTNAME_SUBST) && defined(CONFIG_CC_SPLIT_SECTIONS) +# define SYM_PUSH_SECTION(name, attr) \ + .pushsection %S.name, attr, %progbits; \ + .equ .Lsplit_section, 1 +#else +# define SYM_PUSH_SECTION(name, attr) +#endif + #define SYM_L_GLOBAL(name) .globl name; .hidden name #define SYM_L_WEAK(name) .weak name #define SYM_L_LOCAL(name) /* nothing */ @@ -33,7 +41,14 @@ SYM_ALIGN(align); \ name: -#define END(name) .size name, . - name +#define END(name) \ + .size name, . - name; \ + .ifdef .Lsplit_section; \ + .if .Lsplit_section; \ + .popsection; \ + .equ .Lsplit_section, 0; \ + .endif; \ + .endif /* * CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in @@ -47,6 +62,7 @@ #endif #define FUNC(name, align...) \ + SYM_PUSH_SECTION(name, "ax"); \ SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(align)) #define LABEL(name, align...) \ SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(align)) @@ -54,6 +70,7 @@ SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) #define FUNC_LOCAL(name, align...) \ + SYM_PUSH_SECTION(name, "ax"); \ SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(align)) #define LABEL_LOCAL(name, align...) \ SYM(name, NONE, LOCAL, DO_CODE_ALIGN(align))
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@suse.com> --- 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. 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)). TBD: On top of Roger's respective patch (for livepatch), also respect CONFIG_FUNCTION_ALIGNMENT. 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).