[PATCH 0/3] xen: Linker scripts synchronization

[PATCH 0/3] xen: Linker scripts synchronization

[Michal Orzel]

This patch series aims to do the first step towards linker scripts
synchronization. Linker scripts for arm and x86 share a lot of common
sections and in order to make the process of changing/improving/syncing
them, these sections shall be defined in just one place.

The first patch creates a header file to store the first portion of the
content mutual to both x86 and arm linker scripts. When populating this
file, we are taking an example from x86 script as it is more improved and
up-to-date.

The last two patches make use of the common macros in x86 and arm linker
scripts respectively.

Michal Orzel (3):
  xen: Introduce a header to store common linker scripts content
  x86: Make use of helpers defined in xen_lds.h
  xen/arm: Make use of helpers defined in xen_lds.h

 xen/arch/arm/xen.lds.S    |  37 ++++---------
 xen/arch/x86/xen.lds.S    |  86 +++-------------------------
 xen/include/xen/xen_lds.h | 114 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+), 103 deletions(-)
 create mode 100644 xen/include/xen/xen_lds.h

-- 
2.25.1

[PATCH 1/3] xen: Introduce a header to store common linker scripts content

[Michal Orzel]

Both x86 and arm linker scripts share quite a lot of common content.
It is difficult to keep syncing them up, thus introduce a new header
in include/xen called xen_lds.h to store the internals mutual to all
the linker scripts.

Populate xen_lds.h with the first portion of the common sections.
Some of them are not yet added/completed in arm linker script but they
definitely should be. Please note that this patch does not aim to
perform the full sync up between the linker scripts. It creates a base
for further work.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/include/xen/xen_lds.h | 114 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)
 create mode 100644 xen/include/xen/xen_lds.h

diff --git a/xen/include/xen/xen_lds.h b/xen/include/xen/xen_lds.h
new file mode 100644
index 0000000000..f1ca67ecfd
--- /dev/null
+++ b/xen/include/xen/xen_lds.h
@@ -0,0 +1,114 @@
+#ifndef __XEN_LDS_H__
+#define __XEN_LDS_H__
+
+/*
+ * 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                      \
+  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.
+ *
+ * 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 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) }   \
+  .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 CTORS_SECTION                           \
+       . = ALIGN(8);                            \
+       __ctors_start = .;                       \
+       *(SORT_BY_INIT_PRIORITY(.init_array.*))  \
+       *(SORT_BY_INIT_PRIORITY(.ctors.*))       \
+       *(.init_array)                           \
+       *(.ctors)                                \
+       __ctors_end = .;
+
+#define VPCI_SECTION             \
+       . = ALIGN(POINTER_ALIGN); \
+       __start_vpci_array = .;   \
+       *(SORT(.data.vpci.*))     \
+       __end_vpci_array = .;
+
+#define HYPFS_SECTION            \
+       . = ALIGN(8);             \
+       __paramhypfs_start = .;   \
+       *(.data.paramhypfs)       \
+       __paramhypfs_end = .;
+
+#define LOCK_PROFILE_SECTION     \
+       . = ALIGN(POINTER_ALIGN); \
+       __lock_profile_start = .; \
+       *(.lockprofile.data)      \
+       __lock_profile_end = .;
+
+#endif /* __XEN_LDS_H__ */
-- 
2.25.1

Re: [PATCH 1/3] xen: Introduce a header to store common linker scripts content

[Jan Beulich]

On 21.03.2022 09:21, Michal Orzel wrote:
> Both x86 and arm linker scripts share quite a lot of common content.
> It is difficult to keep syncing them up, thus introduce a new header
> in include/xen called xen_lds.h to store the internals mutual to all
> the linker scripts.
> 
> Populate xen_lds.h with the first portion of the common sections.
> Some of them are not yet added/completed in arm linker script but they
> definitely should be. Please note that this patch does not aim to
> perform the full sync up between the linker scripts. It creates a base
> for further work.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
>  xen/include/xen/xen_lds.h | 114 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
>  create mode 100644 xen/include/xen/xen_lds.h

While perhaps just minor, I'm not happy about new files added with underscores
in their names. Dashes are easier to type. Plus, looking at Linux, it may make
sense to name this xen.lds.h.

> --- /dev/null
> +++ b/xen/include/xen/xen_lds.h
> @@ -0,0 +1,114 @@
> +#ifndef __XEN_LDS_H__
> +#define __XEN_LDS_H__
> +
> +/*
> + * 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                      \
> +  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.
> + *
> + * 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 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) }   \
> +  .comment 0 : { *(.comment) }               \
> +  .symtab 0 : { *(.symtab) }                 \
> +  .strtab 0 : { *(.strtab) }                 \
> +  .shstrtab 0 : { *(.shstrtab) }

Please don't add non-Stabs sections to this macro.

> +#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 CTORS_SECTION                           \
> +       . = ALIGN(8);                            \
> +       __ctors_start = .;                       \
> +       *(SORT_BY_INIT_PRIORITY(.init_array.*))  \
> +       *(SORT_BY_INIT_PRIORITY(.ctors.*))       \
> +       *(.init_array)                           \
> +       *(.ctors)                                \
> +       __ctors_end = .;
> +
> +#define VPCI_SECTION             \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __start_vpci_array = .;   \
> +       *(SORT(.data.vpci.*))     \
> +       __end_vpci_array = .;
> +
> +#define HYPFS_SECTION            \
> +       . = ALIGN(8);             \
> +       __paramhypfs_start = .;   \
> +       *(.data.paramhypfs)       \
> +       __paramhypfs_end = .;
> +
> +#define LOCK_PROFILE_SECTION     \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __lock_profile_start = .; \
> +       *(.lockprofile.data)      \
> +       __lock_profile_end = .;
> +
> +#endif /* __XEN_LDS_H__ */

I'm not sure _SECTION is a good suffix to use in the four names above:
These aren't individual sections in the output, and for CTORS_SECTION
it's also not even a single input section.

As to CTORS_SECTION - I'm unconvinced of generalizing this without
first getting it right.

Overall I think it would be better to introduce this header along
with actually using the macros. That way one can check within the
patch that what you move / replace actually matches on both sides
without needing to cross patch boundaries. If you wanted to introduce
(and then include right away) an empty header first, that would be an
acceptable intermediate approach afaic.

Jan

Re: [PATCH 1/3] xen: Introduce a header to store common linker scripts content

[Michal Orzel]

Hi Jan,
 
On 21.03.2022 09:21, Michal Orzel wrote:
> Both x86 and arm linker scripts share quite a lot of common content.
> It is difficult to keep syncing them up, thus introduce a new header
> in include/xen called xen_lds.h to store the internals mutual to all
> the linker scripts.
> 
> Populate xen_lds.h with the first portion of the common sections.
> Some of them are not yet added/completed in arm linker script but they
> definitely should be. Please note that this patch does not aim to
> perform the full sync up between the linker scripts. It creates a base
> for further work.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
>  xen/include/xen/xen_lds.h | 114 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
>  create mode 100644 xen/include/xen/xen_lds.h

While perhaps just minor, I'm not happy about new files added with underscores
in their names. Dashes are easier to type. Plus, looking at Linux, it may make
sense to name this xen.lds.h.

I'm ok to change the name to xen.lds.h.

> --- /dev/null
> +++ b/xen/include/xen/xen_lds.h
> @@ -0,0 +1,114 @@
> +#ifndef __XEN_LDS_H__
> +#define __XEN_LDS_H__
> +
> +/*
> + * 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                      \
> +  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.
> + *
> + * 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 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) }   \
> +  .comment 0 : { *(.comment) }               \
> +  .symtab 0 : { *(.symtab) }                 \
> +  .strtab 0 : { *(.strtab) }                 \
> +  .shstrtab 0 : { *(.shstrtab) }

Please don't add non-Stabs sections to this macro.

Ok, I will add a new macro storing the last 4 sections called ELF_DETAILS_SECTIONS,
to be coherent with what Linux does (ELF_DETAILS).

> +#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 CTORS_SECTION                           \
> +       . = ALIGN(8);                            \
> +       __ctors_start = .;                       \
> +       *(SORT_BY_INIT_PRIORITY(.init_array.*))  \
> +       *(SORT_BY_INIT_PRIORITY(.ctors.*))       \
> +       *(.init_array)                           \
> +       *(.ctors)                                \
> +       __ctors_end = .;
> +
> +#define VPCI_SECTION             \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __start_vpci_array = .;   \
> +       *(SORT(.data.vpci.*))     \
> +       __end_vpci_array = .;
> +
> +#define HYPFS_SECTION            \
> +       . = ALIGN(8);             \
> +       __paramhypfs_start = .;   \
> +       *(.data.paramhypfs)       \
> +       __paramhypfs_end = .;
> +
> +#define LOCK_PROFILE_SECTION     \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __lock_profile_start = .; \
> +       *(.lockprofile.data)      \
> +       __lock_profile_end = .;
> +
> +#endif /* __XEN_LDS_H__ */

I'm not sure _SECTION is a good suffix to use in the four names above:
These aren't individual sections in the output, and for CTORS_SECTION
it's also not even a single input section.

How about _ENTRY suffix?
Otherwise we can do different suffixes depending on the content.
LOCK_PROFILE_DATA, HYPFS_PARAM, VPCI_ARRAY

As to CTORS_SECTION - I'm unconvinced of generalizing this without
first getting it right.

I will get rid of CTORS_SECTIONS then.

Overall I think it would be better to introduce this header along
with actually using the macros. That way one can check within the
patch that what you move / replace actually matches on both sides
without needing to cross patch boundaries. If you wanted to introduce
(and then include right away) an empty header first, that would be an
acceptable intermediate approach afaic.

I just wanted to split this into arch specific patches because maintainers are different.
I do not understand your second solution with empty header.
Do you mean that the first patch shall create an empty header (with just an intro comment)
and include it in arch specific linker scripts?

Anyway, I can merge these 3 patches into 1 if you want.

Jan

Cheers,
Michal

Re: [PATCH 1/3] xen: Introduce a header to store common linker scripts content

[Jan Beulich]

Note: please properly quote your replies. As you'll see what you said
in reply to my remarks is not properly separated from my remarks, and
hence hard to read.

On 21.03.2022 11:14, Michal Orzel wrote:
> On 21.03.2022 09:21, Michal Orzel wrote:
>> --- /dev/null
>> +++ b/xen/include/xen/xen_lds.h
>> @@ -0,0 +1,114 @@
>> +#ifndef __XEN_LDS_H__
>> +#define __XEN_LDS_H__
>> +
>> +/*
>> + * 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                      \
>> +  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.
>> + *
>> + * 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 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) }   \
>> +  .comment 0 : { *(.comment) }               \
>> +  .symtab 0 : { *(.symtab) }                 \
>> +  .strtab 0 : { *(.strtab) }                 \
>> +  .shstrtab 0 : { *(.shstrtab) }
> 
> Please don't add non-Stabs sections to this macro.
> 
> Ok, I will add a new macro storing the last 4 sections called ELF_DETAILS_SECTIONS,
> to be coherent with what Linux does (ELF_DETAILS).
> 
>> +#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 CTORS_SECTION                           \
>> +       . = ALIGN(8);                            \
>> +       __ctors_start = .;                       \
>> +       *(SORT_BY_INIT_PRIORITY(.init_array.*))  \
>> +       *(SORT_BY_INIT_PRIORITY(.ctors.*))       \
>> +       *(.init_array)                           \
>> +       *(.ctors)                                \
>> +       __ctors_end = .;
>> +
>> +#define VPCI_SECTION             \
>> +       . = ALIGN(POINTER_ALIGN); \
>> +       __start_vpci_array = .;   \
>> +       *(SORT(.data.vpci.*))     \
>> +       __end_vpci_array = .;
>> +
>> +#define HYPFS_SECTION            \
>> +       . = ALIGN(8);             \
>> +       __paramhypfs_start = .;   \
>> +       *(.data.paramhypfs)       \
>> +       __paramhypfs_end = .;
>> +
>> +#define LOCK_PROFILE_SECTION     \
>> +       . = ALIGN(POINTER_ALIGN); \
>> +       __lock_profile_start = .; \
>> +       *(.lockprofile.data)      \
>> +       __lock_profile_end = .;
>> +
>> +#endif /* __XEN_LDS_H__ */
> 
> I'm not sure _SECTION is a good suffix to use in the four names above:
> These aren't individual sections in the output, and for CTORS_SECTION
> it's also not even a single input section.
> 
> How about _ENTRY suffix?
> Otherwise we can do different suffixes depending on the content.
> LOCK_PROFILE_DATA, HYPFS_PARAM, VPCI_ARRAY

I'd prefer the latter.

> As to CTORS_SECTION - I'm unconvinced of generalizing this without
> first getting it right.
> 
> I will get rid of CTORS_SECTIONS then.
> 
> Overall I think it would be better to introduce this header along
> with actually using the macros. That way one can check within the
> patch that what you move / replace actually matches on both sides
> without needing to cross patch boundaries. If you wanted to introduce
> (and then include right away) an empty header first, that would be an
> acceptable intermediate approach afaic.
> 
> I just wanted to split this into arch specific patches because maintainers are different.
> I do not understand your second solution with empty header.
> Do you mean that the first patch shall create an empty header (with just an intro comment)
> and include it in arch specific linker scripts?

Yes, I view this as one possible option.

> Anyway, I can merge these 3 patches into 1 if you want.

Well, at least part of the Arm changes can likely remain separate.
But where you abstract things by introducing a macro in the header,
it would be better if the original (supposedly functionally identical)
construct(s) was (were) also replaced at the same time.

Jan

Re: [PATCH 1/3] xen: Introduce a header to store common linker scripts content

[Michal Orzel]

Hi Jan,

On 21.03.2022 11:23, Jan Beulich wrote:
> Note: please properly quote your replies. As you'll see what you said
> in reply to my remarks is not properly separated from my remarks, and
> hence hard to read.
> 
Sorry about that. I had some issues with my e-mail client and had to use the non-default one.

> On 21.03.2022 11:14, Michal Orzel wrote:
>> On 21.03.2022 09:21, Michal Orzel wrote:
>>> --- /dev/null
>>> +++ b/xen/include/xen/xen_lds.h
>>> @@ -0,0 +1,114 @@
>>> +#ifndef __XEN_LDS_H__
>>> +#define __XEN_LDS_H__
>>> +
>>> +/*
>>> + * 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                      \
>>> +  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.
>>> + *
>>> + * 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 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) }   \
>>> +  .comment 0 : { *(.comment) }               \
>>> +  .symtab 0 : { *(.symtab) }                 \
>>> +  .strtab 0 : { *(.strtab) }                 \
>>> +  .shstrtab 0 : { *(.shstrtab) }
>>
>> Please don't add non-Stabs sections to this macro.
>>
>> Ok, I will add a new macro storing the last 4 sections called ELF_DETAILS_SECTIONS,
>> to be coherent with what Linux does (ELF_DETAILS).
>>
>>> +#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 CTORS_SECTION                           \
>>> +       . = ALIGN(8);                            \
>>> +       __ctors_start = .;                       \
>>> +       *(SORT_BY_INIT_PRIORITY(.init_array.*))  \
>>> +       *(SORT_BY_INIT_PRIORITY(.ctors.*))       \
>>> +       *(.init_array)                           \
>>> +       *(.ctors)                                \
>>> +       __ctors_end = .;
>>> +
>>> +#define VPCI_SECTION             \
>>> +       . = ALIGN(POINTER_ALIGN); \
>>> +       __start_vpci_array = .;   \
>>> +       *(SORT(.data.vpci.*))     \
>>> +       __end_vpci_array = .;
>>> +
>>> +#define HYPFS_SECTION            \
>>> +       . = ALIGN(8);             \
>>> +       __paramhypfs_start = .;   \
>>> +       *(.data.paramhypfs)       \
>>> +       __paramhypfs_end = .;
>>> +
>>> +#define LOCK_PROFILE_SECTION     \
>>> +       . = ALIGN(POINTER_ALIGN); \
>>> +       __lock_profile_start = .; \
>>> +       *(.lockprofile.data)      \
>>> +       __lock_profile_end = .;
>>> +
>>> +#endif /* __XEN_LDS_H__ */
>>
>> I'm not sure _SECTION is a good suffix to use in the four names above:
>> These aren't individual sections in the output, and for CTORS_SECTION
>> it's also not even a single input section.
>>
>> How about _ENTRY suffix?
>> Otherwise we can do different suffixes depending on the content.
>> LOCK_PROFILE_DATA, HYPFS_PARAM, VPCI_ARRAY
> 
> I'd prefer the latter.
> 
Ok.

>> As to CTORS_SECTION - I'm unconvinced of generalizing this without
>> first getting it right.
>>
>> I will get rid of CTORS_SECTIONS then.
>>
>> Overall I think it would be better to introduce this header along
>> with actually using the macros. That way one can check within the
>> patch that what you move / replace actually matches on both sides
>> without needing to cross patch boundaries. If you wanted to introduce
>> (and then include right away) an empty header first, that would be an
>> acceptable intermediate approach afaic.
>>
>> I just wanted to split this into arch specific patches because maintainers are different.
>> I do not understand your second solution with empty header.
>> Do you mean that the first patch shall create an empty header (with just an intro comment)
>> and include it in arch specific linker scripts?
> 
> Yes, I view this as one possible option.
> 
>> Anyway, I can merge these 3 patches into 1 if you want.
> 
> Well, at least part of the Arm changes can likely remain separate.
> But where you abstract things by introducing a macro in the header,
> it would be better if the original (supposedly functionally identical)
> construct(s) was (were) also replaced at the same time.
> 
Hmm, I think I would go with the empty header solution.
So in v2 I would do the following:
-first patch introducing empty header and including it in linker scripts
-second patch making use of common macros in x86 and arm linker scripts

> Jan
> 

Cheers,
Michal

[PATCH 2/3] xen/x86: Make use of helpers defined in xen_lds.h

[Michal Orzel]

Header file xen_lds.h defines common macros to be used in arch specific
linker scripts. Include this header and make use of its helpers.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/x86/xen.lds.S | 86 ++++--------------------------------------
 1 file changed, 8 insertions(+), 78 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d33e295320..e82a148e08 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -2,6 +2,7 @@
 /* Modified for i386/x86-64 Xen by Keir Fraser */
 
 #include <xen/cache.h>
+#include <xen/xen_lds.h>
 #include <asm/page.h>
 #undef ENTRY
 #undef ALIGN
@@ -12,13 +13,6 @@
 #undef __XEN_VIRT_START
 #define __XEN_VIRT_START __image_base__
 #define DECL_SECTION(x) x :
-/*
- * 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) }
 
 ENTRY(efi_start)
 
@@ -26,8 +20,6 @@ ENTRY(efi_start)
 
 #define FORMAT "elf64-x86-64"
 #define DECL_SECTION(x) #x : AT(ADDR(#x) - __XEN_VIRT_START)
-#define DECL_DEBUG(x, a) #x 0 : { *(x) }
-#define DECL_DEBUG2(x, y, a) #x 0 : { *(x) *(y) }
 
 ENTRY(start_pa)
 
@@ -159,10 +151,7 @@ SECTIONS
        __note_gnu_build_id_end = .;
 #endif
 #ifdef CONFIG_HAS_VPCI
-       . = ALIGN(POINTER_ALIGN);
-       __start_vpci_array = .;
-       *(SORT(.data.vpci.*))
-       __end_vpci_array = .;
+       VPCI_SECTION
 #endif
   } PHDR(text)
 
@@ -278,19 +267,10 @@ SECTIONS
         __alt_instructions_end = .;
 
 #ifdef CONFIG_DEBUG_LOCK_PROFILE
-       . = ALIGN(POINTER_ALIGN);
-       __lock_profile_start = .;
-       *(.lockprofile.data)
-       __lock_profile_end = .;
+       LOCK_PROFILE_SECTION
 #endif
 
-       . = ALIGN(8);
-       __ctors_start = .;
-       *(SORT_BY_INIT_PRIORITY(.init_array.*))
-       *(SORT_BY_INIT_PRIORITY(.ctors.*))
-       *(.init_array)
-       *(.ctors)
-       __ctors_end = .;
+       CTORS_SECTION
   } PHDR(text)
 
 #ifndef EFI
@@ -335,10 +315,7 @@ SECTIONS
        __end_schedulers_array = .;
 
 #ifdef CONFIG_HYPFS
-       . = ALIGN(8);
-       __paramhypfs_start = .;
-       *(.data.paramhypfs)
-       __paramhypfs_end = .;
+       HYPFS_SECTION
 #endif
   } PHDR(text)
 
@@ -395,24 +372,7 @@ SECTIONS
    * _end here, so if these sections get loaded they'll be discarded at runtime
    * anyway.
    */
-  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)
+  DWARF_DEBUG_SECTIONS
 
 #ifdef EFI
   /* Trick the linker into setting the image size to no less than 16Mb. */
@@ -427,41 +387,11 @@ SECTIONS
 #endif
 
   /* Sections to be discarded */
-  /DISCARD/ : {
-       *(.text.exit)
-       *(.exit.text)
-       *(.exit.data)
-       *(.exitcall.exit)
-       *(.discard)
-       *(.discard.*)
-       *(.eh_frame)
-       *(.dtors)
-       *(.dtors.*)
-       *(.fini_array)
-       *(.fini_array.*)
-#ifdef EFI
-       *(.comment)
-       *(.comment.*)
-       *(.note.*)
-#endif
-  }
+  DISCARD_SECTIONS
 
 #ifndef EFI
   /* 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) }
-  /*
-   * LLVM ld also wants .symtab, .strtab, and .shstrtab placed. These look to
-   * be benign to GNU ld, so we can have them here unconditionally.
-   */
-  .symtab 0 : { *(.symtab) }
-  .strtab 0 : { *(.strtab) }
-  .shstrtab 0 : { *(.shstrtab) }
+  STABS_DEBUG_SECTIONS
 #endif
 }
 
-- 
2.25.1

[PATCH 3/3] xen/arm: Make use of helpers defined in xen_lds.h

[Michal Orzel]

Header file xen_lds.h defines common macros to be used in arch specific
linker scripts. Include this header and make use of its helpers.

Making use of common helpers defined based on x86 linker script
improves arm linker script with:
-explicit list of debug sections that otherwise are seen as "orphans"
by the linker. This will allow to fix issues after enabling linker
option --orphan-handling one day
-re-arrangement of ordering/sorting in constructors section to match the
default linker script
-extended list of discarded section to include: .discard, desctructors
related sections, .fini_array which can reference .text.exit
-extended list of stabs section to include sections placed by ld.lld.
Even though Xen on arm compilation with LLVM support is not ready yet,
these sections do not cause problem to GNU ld

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/xen.lds.S | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 7921d8fa28..9e1832e94c 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -3,6 +3,7 @@
 /* Modified for ARM Xen by Ian Campbell */
 
 #include <xen/cache.h>
+#include <xen/xen_lds.h>
 #include <asm/page.h>
 #undef ENTRY
 #undef ALIGN
@@ -68,10 +69,7 @@ SECTIONS
        __proc_info_end = .;
 
 #ifdef CONFIG_HAS_VPCI
-       . = ALIGN(POINTER_ALIGN);
-       __start_vpci_array = .;
-       *(SORT(.data.vpci.*))
-       __end_vpci_array = .;
+       VPCI_SECTION
 #endif
   } :text
 
@@ -109,10 +107,7 @@ SECTIONS
        __end_schedulers_array = .;
 
 #ifdef CONFIG_HYPFS
-       . = ALIGN(8);
-       __paramhypfs_start = .;
-       *(.data.paramhypfs)
-       __paramhypfs_end = .;
+       HYPFS_SECTION
 #endif
 
        *(.data .data.*)
@@ -178,10 +173,7 @@ SECTIONS
        __alt_instructions_end = .;
 
 #ifdef CONFIG_DEBUG_LOCK_PROFILE
-       . = ALIGN(POINTER_ALIGN);
-       __lock_profile_start = .;
-       *(.lockprofile.data)
-       __lock_profile_end = .;
+       LOCK_PROFILE_SECTION
 #endif
 
        *(.init.data)
@@ -221,22 +213,17 @@ 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
+
   /* 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
 }
 
 /*
-- 
2.25.1

Re: [PATCH 3/3] xen/arm: Make use of helpers defined in xen_lds.h

[Jan Beulich]

On 21.03.2022 09:21, Michal Orzel wrote:
> Header file xen_lds.h defines common macros to be used in arch specific
> linker scripts. Include this header and make use of its helpers.
> 
> Making use of common helpers defined based on x86 linker script
> improves arm linker script with:
> -explicit list of debug sections that otherwise are seen as "orphans"
> by the linker. This will allow to fix issues after enabling linker
> option --orphan-handling one day
> -re-arrangement of ordering/sorting in constructors section to match the
> default linker script

As said in reply to patch 1 - I don't think this is correct on x86 right
now, and hence I don't think you want to propagate the same (at least
latent) issue to Arm.

Jan