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

[Michal Orzel <michal.orzel@xxxxxxx>]

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