On 09/12/2025 2:28 pm, Jan Beulich wrote: > On 09.12.2025 15:21, Grygorii Strashko wrote: >> >> On 09.12.25 15:19, Andrew Cooper wrote: >>> On 08/12/2025 6:49 pm, Grygorii Strashko wrote: >>>> Hi Andrew, >>>> >>>> On 06.12.25 16:21, Andrew Cooper wrote: >>>>> On 06/12/2025 2:15 pm, Andrew Cooper wrote: >>>>>> On 06/12/2025 9:10 am, Grygorii Strashko wrote: >>>>>>> On 05.12.25 22:00, Andrew Cooper wrote: >>>>>>>> On 05/12/2025 7:34 pm, Grygorii Strashko wrote: >>>>>>>>> From: Grygorii Strashko <[email protected]> >>>>>>>>> >>>>>>>>> Extend coverage support on .init and lib code. >>>>>>>>> Add two hidden Kconfig options: >>>>>>>>> - RELAX_INIT_CHECK "Relax strict check for .init sections only in >>>>>>>>> %.init.o >>>>>>>>> files" >>>>>>>>> - DO_NOT_FREE_INIT_MEMORY "Prevent freeing of .init sections at the >>>>>>>>> end of >>>>>>>>> Xen boot." >>>>>>>>> >>>>>>>>> Both selected selected when COVERAGE=y, as getting coverage >>>>>>>>> report for >>>>>>>>> ".init" code is required: >>>>>>>>> - to bypass strict check for .init sections only in %.init.o files; >>>>>>>>> - the .init code stay in memory after Xen boot. >>>>>>>>> >>>>>>>>> RELAX_INIT_CHECK/DO_NOT_FREE_INIT_MEMORY could be used by other >>>>>>>>> debug >>>>>>>>> features in the future. >>>>>>>>> >>>>>>>>> Signed-off-by: Grygorii Strashko <[email protected]> >>>>>>>>> --- >>>>>>>>> changes in v2: >>>>>>>>> - add RELAX_INIT_CHECK and DO_NOT_FREE_INIT_MEMORY, those are two >>>>>>>>> different things, >>>>>>>>> both potentially reusable >>>>>>>>> - enable coverage for libfdt/libelf always >>>>>>>>> - enable colverage for .init always >>>>>>>> This is a lot nicer (i.e. more simple). >>>>>>>> >>>>>>>> But, I still don't know why we need to avoid freeing init memory >>>>>>>> to make >>>>>>>> this work. What explodes if we dont? >>>>>>>> >>>>>>> It will just crash when coverage data is collected. >>>>>>> >>>>>>> First I made changes in make file to get .init covered >>>>>>> then I hit a crash >>>>>>> then I checked %.init.o >>>>>>> conclusion was obvious. >>>>>>> >>>>>>> For example: >>>>>>> objdump -x bzimage.init.o | grep gcov >>>>>>> >>>>>>> 0000000000000010 l O .bss 0000000000000028 >>>>>>> __gcov0.bzimage_check >>>>>>> 0000000000000040 l O .bss 0000000000000040 >>>>>>> __gcov0.bzimage_headroom >>>>>>> 0000000000000000 l O .bss 0000000000000008 >>>>>>> __gcov0.output_length >>>>>>> 0000000000000080 l O .bss 0000000000000060 >>>>>>> __gcov0.bzimage_parse >>>>>>> 0000000000000098 l O .init.data.rel.local 0000000000000028 >>>>>>> __gcov_.bzimage_parse >>>>>>> 0000000000000070 l O .init.data.rel.local 0000000000000028 >>>>>>> __gcov_.bzimage_headroom >>>>>>> 0000000000000048 l O .init.data.rel.local 0000000000000028 >>>>>>> __gcov_.bzimage_check >>>>>>> 0000000000000020 l O .init.data.rel.local 0000000000000028 >>>>>>> __gcov_.output_length >>>>>>> 0000000000000000 *UND* 0000000000000000 __gcov_init >>>>>>> 0000000000000000 *UND* 0000000000000000 __gcov_exit >>>>>>> 0000000000000000 *UND* 0000000000000000 __gcov_merge_add >>>>>>> 0000000000000008 R_X86_64_PLT32 __gcov_init-0x0000000000000004 >>>>>>> 0000000000000012 R_X86_64_PLT32 __gcov_exit-0x0000000000000004 >>>>>>> 0000000000000020 R_X86_64_64 __gcov_merge_add >>>>>>> >>>>>> Aah, we should exclude the OJBCOPY too. That's what's moving >>>>>> .data.rel.local amongst other sections we target with attributes >>>>>> directly. >>>>> we can't target. >>>> I've come up with below diff - seems it's working without >>>> DO_NOT_FREE_INIT_MEMORY. >>>> Is this what you have in mind? >>>> >>>> diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug >>>> index 8fc201d12c2c..16b1a82db46e 100644 >>>> --- a/xen/Kconfig.debug >>>> +++ b/xen/Kconfig.debug >>>> @@ -40,7 +40,6 @@ config COVERAGE >>>> depends on SYSCTL && !LIVEPATCH >>>> select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS if >>>> !ENFORCE_UNIQUE_SYMBOLS >>>> select RELAX_INIT_CHECK >>>> - select DO_NOT_FREE_INIT_MEMORY >>>> help >>>> Enable code coverage support. >>>> >>>> diff --git a/xen/Rules.mk b/xen/Rules.mk >>>> index 8c4861a427e6..47fdcc1d23b5 100644 >>>> --- a/xen/Rules.mk >>>> +++ b/xen/Rules.mk >>>> @@ -33,11 +33,15 @@ cov-cflags-y := >>>> nocov-y := >>>> noubsan-y := >>>> >>>> +# when coverage is enabled the gcc internal section should stay in >>>> memory >>>> +# after Xen boot >>>> +ifneq ($(CONFIG_COVERAGE),y) >>>> SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \ >>>> $(foreach w,1 2 4, \ >>>> >>>> rodata.str$(w).$(a)) \ >>>> rodata.cst$(a)) \ >>>> $(foreach r,rel rel.ro,data.$(r).local) >>>> +endif >>>> >>>> # The filename build.mk has precedence over Makefile >>>> include $(firstword $(wildcard $(srcdir)/build.mk) $(srcdir)/Makefile) >>>> diff --git a/xen/common/libelf/Makefile b/xen/common/libelf/Makefile >>>> index 60b3ae40728f..8180c78f1510 100644 >>>> --- a/xen/common/libelf/Makefile >>>> +++ b/xen/common/libelf/Makefile >>>> @@ -1,8 +1,10 @@ >>>> obj-bin-y := libelf.o >>>> libelf-objs := libelf-tools.o libelf-loader.o libelf-dominfo.o >>>> >>>> +ifneq ($(CONFIG_COVERAGE),y) >>>> SECTIONS := text data $(SPECIAL_DATA_SECTIONS) >>>> OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section >>>> .$(s)=.init.$(s)) >>>> +endif >>>> >>>> CFLAGS-y += -Wno-pointer-sign >>>> >>>> diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile >>>> index ae0f69c01373..fb26e5bff0fd 100644 >>>> --- a/xen/common/libfdt/Makefile >>>> +++ b/xen/common/libfdt/Makefile >>>> @@ -4,7 +4,9 @@ SECTIONS := text data $(SPECIAL_DATA_SECTIONS) >>>> >>>> # For CONFIG_OVERLAY_DTB, libfdt functionalities will be needed >>>> during runtime. >>>> ifneq ($(CONFIG_OVERLAY_DTB),y) >>>> -OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section >>>> .$(s)=.init.$(s)) >>>> + ifneq ($(CONFIG_COVERAGE),y) >>>> + OBJCOPYFLAGS := $(foreach >>>> s,$(SECTIONS),--rename-section .$(s)=.init.$(s)) >>>> + endif >>>> endif >>> This is the (aforementioned) non-standard way of doing .init.o, which is >>> why it doesn't play nicely. >>> >>> I suggest that we first convert libelf and libfdt to the standard way of >>> doing .init. >> I assume the rest is ok. >> >>> For libelf this means we need regular __init annotations, but #undef'd >>> outside of __XEN__ (when we're doing the userspace build). >>> >> Need clarification here - this are imported libraries and changing their code >> directly was not welcome before. Therefore there is Xen specific magic in >> Makefiles. >> :( > I can't and won't speak for libfdt, but for libelf I think we should really > consider this ours (not imported) the latest as of the re-work for XSA-55.
Agreed. libelf was already modified for Xen, and then XSA-55 made it entirely unrecognisable. It's very much our code now. If we really don't want to modify libfdt's source, we should make a standard way of init-ing code like this, so we can move the custom logic out of libfdt's Makefile. The problem really is custom local logic; that's what we need to fix. ~Andrew
