Re: [XEN][PATCH v2] coverage: extend coverage on .init and lib code

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/12/2025 7:34 pm, Grygorii Strashko wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>
> 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 <grygorii_strashko@xxxxxxxx>
> ---
> 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?

> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index 24f447b95734..c884a4199dc2 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -145,10 +145,10 @@ endif
>  $(call 
> cc-option-add,cov-cflags-$(CONFIG_COVERAGE),CC,-fprofile-update=atomic)
>  
>  # Reset cov-cflags-y in cases where an objects has another one as 
> prerequisite
> -$(nocov-y) $(filter %.init.o, $(obj-y) $(obj-bin-y) $(extra-y)): \
> +$(nocov-y) $(extra-y): \
>      cov-cflags-y :=

This could become a single line now.

> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 22e929aac778..2a0b322445cd 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -69,10 +69,12 @@ static __used void noreturn init_done(void)
>  {
>      int rc;
>  
> +#ifndef CONFIG_DO_NOT_FREE_INIT_MEMORY
>      /* Must be done past setting system_state. */
>      unregister_init_virtual_region();
>  
>      free_init_memory();
> +#endif
>  
>      /*
>       * We have finished booting. Mark the section .data.ro_after_init
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 7bbba76a92f8..280085c206a7 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -815,6 +815,7 @@ static void noreturn init_done(void)
>      for ( unsigned int i = 0; i < bi->nr_domains; i++ )
>          domain_unpause_by_systemcontroller(bi->domains[i].d);
>  
> +#ifndef CONFIG_DO_NOT_FREE_INIT_MEMORY
>      /* MUST be done prior to removing .init data. */
>      unregister_init_virtual_region();
>  
> @@ -837,6 +838,9 @@ static void noreturn init_done(void)
>      destroy_xen_mappings(start, end);
>      init_xenheap_pages(__pa(start), __pa(end));
>      printk("Freed %lukB init memory\n", (end - start) >> 10);
> +#else
> +    (void) end, (void) start, (void)va;
> +#endif

For both of these, the preferred way would be to use if (
IS_ENABLED(CONFIG_DO_NOT_FREE_INIT_MEMORY) ), which removes the need for
the else clause in x86.

Also, you make free_init_memory() un-called on ARM with this change. 
Depending on how the freeing-init question lands, you either want some
extra ifdefary, or the --gc-sections option that we discussed before.

~Andrew