Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.03.2022 09:02, Michal Orzel wrote:
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -69,10 +69,7 @@ SECTIONS
>         __proc_info_end = .;
>  
>  #ifdef CONFIG_HAS_VPCI
> -       . = ALIGN(POINTER_ALIGN);
> -       __start_vpci_array = .;
> -       *(SORT(.data.vpci.*))
> -       __end_vpci_array = .;
> +       VPCI_ARRAY
>  #endif

Here and elsewhere I think the #ifdef should move as well, or to be
precise VPCI_ARRAY (in the specific case here) should simply expand to
nothing when CONFIG_HAS_VPCI is not defined.

> @@ -222,22 +213,18 @@ SECTIONS
>    /* Section for the device tree blob (if any). */
>    .dtb : { *(.dtb) } :text
>  
> +  /*
> +   * Explicitly list debug sections, to avoid these sections being viewed as
> +   * "orphan" by the linker.
> +   */
> +  DWARF_DEBUG_SECTIONS

Considering the comment, perhaps better to move ...

>    /* Sections to be discarded */
> -  /DISCARD/ : {
> -       *(.exit.text)
> -       *(.exit.data)
> -       *(.exitcall.exit)
> -       *(.eh_frame)
> -  }
> +  DISCARD_SECTIONS
>  
>    /* Stabs debugging sections.  */
> -  .stab 0 : { *(.stab) }
> -  .stabstr 0 : { *(.stabstr) }
> -  .stab.excl 0 : { *(.stab.excl) }
> -  .stab.exclstr 0 : { *(.stab.exclstr) }
> -  .stab.index 0 : { *(.stab.index) }
> -  .stab.indexstr 0 : { *(.stab.indexstr) }
> -  .comment 0 : { *(.comment) }
> +  STABS_DEBUG_SECTIONS

... this up there?

> --- a/xen/include/xen/xen.lds.h
> +++ b/xen/include/xen/xen.lds.h
> @@ -5,4 +5,104 @@
>   * Common macros to be used in architecture specific linker scripts.
>   */
>  
> +/* Macros to declare debug sections. */
> +#ifdef EFI
> +/*
> + * Use the NOLOAD directive, despite currently ignored by (at least) GNU ld
> + * for PE output, in order to record that we'd prefer these sections to not
> + * be loaded into memory.
> + */
> +#define DECL_DEBUG(x, a) #x ALIGN(a) (NOLOAD) : { *(x) }
> +#define DECL_DEBUG2(x, y, a) #x ALIGN(a) (NOLOAD) : { *(x) *(y) }
> +#else
> +#define DECL_DEBUG(x, a) #x 0 : { *(x) }
> +#define DECL_DEBUG2(x, y, a) #x 0 : { *(x) *(y) }
> +#endif
> +
> +/* DWARF debug sections. */
> +#define DWARF_DEBUG_SECTIONS                      \

May I suggest to call this DWARF2_DEBUG_SECTIONS, to make clear no
Dwarf1 section is included here (and we also don't mean to support
such debug info)?

> +  DECL_DEBUG(.debug_abbrev, 1)                    \
> +  DECL_DEBUG2(.debug_info, .gnu.linkonce.wi.*, 1) \
> +  DECL_DEBUG(.debug_types, 1)                     \
> +  DECL_DEBUG(.debug_str, 1)                       \
> +  DECL_DEBUG2(.debug_line, .debug_line.*, 1)      \
> +  DECL_DEBUG(.debug_line_str, 1)                  \
> +  DECL_DEBUG(.debug_names, 4)                     \
> +  DECL_DEBUG(.debug_frame, 4)                     \
> +  DECL_DEBUG(.debug_loc, 1)                       \
> +  DECL_DEBUG(.debug_loclists, 4)                  \
> +  DECL_DEBUG(.debug_macinfo, 1)                   \
> +  DECL_DEBUG(.debug_macro, 1)                     \
> +  DECL_DEBUG(.debug_ranges, 8)                    \
> +  DECL_DEBUG(.debug_rnglists, 4)                  \
> +  DECL_DEBUG(.debug_addr, 8)                      \
> +  DECL_DEBUG(.debug_aranges, 1)                   \
> +  DECL_DEBUG(.debug_pubnames, 1)                  \
> +  DECL_DEBUG(.debug_pubtypes, 1)
> +
> +/* Stabs debug sections. */
> +#define STABS_DEBUG_SECTIONS                 \
> +  .stab 0 : { *(.stab) }                     \
> +  .stabstr 0 : { *(.stabstr) }               \
> +  .stab.excl 0 : { *(.stab.excl) }           \
> +  .stab.exclstr 0 : { *(.stab.exclstr) }     \
> +  .stab.index 0 : { *(.stab.index) }         \
> +  .stab.indexstr 0 : { *(.stab.indexstr) }
> +
> +/*
> + * Required sections not related to debugging.
> + *
> + * LLVM ld also wants .symtab, .strtab, and .shstrtab placed. These look to
> + * be benign to GNU ld, so we can have them here unconditionally.
> + */
> +#define ELF_DETAILS_SECTIONS     \
> +  .comment 0 : { *(.comment) }   \
> +  .symtab 0 : { *(.symtab) }     \
> +  .strtab 0 : { *(.strtab) }     \
> +  .shstrtab 0 : { *(.shstrtab) }
> +
> +#ifdef EFI
> +#define DISCARD_EFI_SECTIONS \
> +       *(.comment)   \
> +       *(.comment.*) \
> +       *(.note.*)
> +#else
> +#define DISCARD_EFI_SECTIONS
> +#endif
> +
> +/* Sections to be discarded. */
> +#define DISCARD_SECTIONS     \
> +  /DISCARD/ : {              \
> +       *(.text.exit)         \
> +       *(.exit.text)         \
> +       *(.exit.data)         \
> +       *(.exitcall.exit)     \
> +       *(.discard)           \
> +       *(.discard.*)         \
> +       *(.eh_frame)          \
> +       *(.dtors)             \
> +       *(.dtors.*)           \
> +       *(.fini_array)        \
> +       *(.fini_array.*)      \
> +       DISCARD_EFI_SECTIONS  \
> +  }
> +
> +#define VPCI_ARRAY               \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __start_vpci_array = .;   \
> +       *(SORT(.data.vpci.*))     \
> +       __end_vpci_array = .;
> +
> +#define HYPFS_PARAM              \
> +       . = ALIGN(8);             \

Since you're generalizing this, you mean POINTER_ALIGN here, not 8.

> +       __paramhypfs_start = .;   \
> +       *(.data.paramhypfs)       \
> +       __paramhypfs_end = .;
> +
> +#define LOCK_PROFILE_DATA        \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __lock_profile_start = .; \
> +       *(.lockprofile.data)      \
> +       __lock_profile_end = .;

While for *_SECTIONS I don't care as much, for these last three items
I think it would be good if we (maybe just informally) established an
ordering rule, such that we can ask further additions here to not occur
randomly. Once we've grown a few more of these, this would also help
quickly locating the perhaps just one construct of interest when
looking up things. Personally I think the only sensible ordering
criteria is the name of the construct being defined. This could be
mentioned in a comment ahead of them, and that comment would then at
the same time serve as separator between *_SECTIONS and any constructs
also defining (enclosing) symbols.

Jan