* [PATCH v2 0/2] xen: Linker scripts synchronization
@ 2022-03-22 8:02 Michal Orzel
2022-03-22 8:02 ` [PATCH v2 1/2] xen: Introduce a header to store common linker scripts content Michal Orzel
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Michal Orzel @ 2022-03-22 8:02 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné
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 an empty header file xen.lds.h to store the
constructs mutual to both x86 and arm linker scripts. It also includes
this header in the scripts.
The second patch populates xen.lds.h with the first portion of common
macros and replaces the original contructs with these helpers.
Michal Orzel (2):
xen: Introduce a header to store common linker scripts content
xen: Populate xen.lds.h and make use of its macros
xen/arch/arm/xen.lds.S | 38 +++++---------
xen/arch/x86/xen.lds.S | 79 +++-------------------------
xen/include/xen/xen.lds.h | 108 ++++++++++++++++++++++++++++++++++++++
3 files changed, 129 insertions(+), 96 deletions(-)
create mode 100644 xen/include/xen/xen.lds.h
--
2.25.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/2] xen: Introduce a header to store common linker scripts content
2022-03-22 8:02 [PATCH v2 0/2] xen: Linker scripts synchronization Michal Orzel
@ 2022-03-22 8:02 ` Michal Orzel
2022-03-29 9:06 ` Jan Beulich
2022-03-22 8:02 ` [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros Michal Orzel
2022-03-28 10:31 ` [PATCH v2 0/2] xen: Linker scripts synchronization Michal Orzel
2 siblings, 1 reply; 28+ messages in thread
From: Michal Orzel @ 2022-03-22 8:02 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné
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.
Include this header in linker scripts for x86 and arm.
This patch serves as an intermediate step before populating xen.lds.h
and making use of its content in the linker scripts later on.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v1:
-rename header to xen.lds.h to be coherent with Linux kernel
-include empty header in linker scripts
---
xen/arch/arm/xen.lds.S | 1 +
xen/arch/x86/xen.lds.S | 1 +
xen/include/xen/xen.lds.h | 8 ++++++++
3 files changed, 10 insertions(+)
create mode 100644 xen/include/xen/xen.lds.h
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 7921d8fa28..c666fc3e69 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
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d33e295320..4e3a9a2789 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
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
new file mode 100644
index 0000000000..dd292fa7dc
--- /dev/null
+++ b/xen/include/xen/xen.lds.h
@@ -0,0 +1,8 @@
+#ifndef __XEN_LDS_H__
+#define __XEN_LDS_H__
+
+/*
+ * Common macros to be used in architecture specific linker scripts.
+ */
+
+#endif /* __XEN_LDS_H__ */
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-22 8:02 [PATCH v2 0/2] xen: Linker scripts synchronization Michal Orzel
2022-03-22 8:02 ` [PATCH v2 1/2] xen: Introduce a header to store common linker scripts content Michal Orzel
@ 2022-03-22 8:02 ` Michal Orzel
2022-03-29 9:22 ` Jan Beulich
2022-03-29 9:54 ` Julien Grall
2022-03-28 10:31 ` [PATCH v2 0/2] xen: Linker scripts synchronization Michal Orzel
2 siblings, 2 replies; 28+ messages in thread
From: Michal Orzel @ 2022-03-22 8:02 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné
Populate header file xen.lds.h with the first portion of macros storing
constructs common to x86 and arm linker scripts. Replace the original
constructs with these helpers.
No functional improvements to x86 linker script.
Making use of common macros 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
-extended list of discarded section to include: .discard, desctructors
related sections, .fini_array which can reference .text.exit
-sections not related to debugging that are 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.
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>
---
Changes since v1:
-merge x86 and arm changes into single patch
-do not propagate issues by generalizing CTORS
-extract sections not related to debugging into separate macro
-get rid of _SECTION suffix in favor of using more meaningful suffixes
---
xen/arch/arm/xen.lds.S | 37 +++++---------
xen/arch/x86/xen.lds.S | 78 +++--------------------------
xen/include/xen/xen.lds.h | 100 ++++++++++++++++++++++++++++++++++++++
3 files changed, 119 insertions(+), 96 deletions(-)
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index c666fc3e69..e8ce7ad5f1 100644
--- 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
} :text
@@ -110,10 +107,7 @@ SECTIONS
__end_schedulers_array = .;
#ifdef CONFIG_HYPFS
- . = ALIGN(8);
- __paramhypfs_start = .;
- *(.data.paramhypfs)
- __paramhypfs_end = .;
+ HYPFS_PARAM
#endif
*(.data .data.*)
@@ -179,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_DATA
#endif
*(.init.data)
@@ -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
+
/* 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
+ ELF_DETAILS_SECTIONS
}
/*
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 4e3a9a2789..65efbf9d0c 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -13,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)
@@ -27,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)
@@ -160,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_ARRAY
#endif
} PHDR(text)
@@ -279,10 +267,7 @@ SECTIONS
__alt_instructions_end = .;
#ifdef CONFIG_DEBUG_LOCK_PROFILE
- . = ALIGN(POINTER_ALIGN);
- __lock_profile_start = .;
- *(.lockprofile.data)
- __lock_profile_end = .;
+ LOCK_PROFILE_DATA
#endif
. = ALIGN(8);
@@ -336,10 +321,7 @@ SECTIONS
__end_schedulers_array = .;
#ifdef CONFIG_HYPFS
- . = ALIGN(8);
- __paramhypfs_start = .;
- *(.data.paramhypfs)
- __paramhypfs_end = .;
+ HYPFS_PARAM
#endif
} PHDR(text)
@@ -396,24 +378,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. */
@@ -428,41 +393,12 @@ 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
+ ELF_DETAILS_SECTIONS
#endif
}
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index dd292fa7dc..ad1d199021 100644
--- 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 \
+ 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); \
+ __paramhypfs_start = .; \
+ *(.data.paramhypfs) \
+ __paramhypfs_end = .;
+
+#define LOCK_PROFILE_DATA \
+ . = ALIGN(POINTER_ALIGN); \
+ __lock_profile_start = .; \
+ *(.lockprofile.data) \
+ __lock_profile_end = .;
+
#endif /* __XEN_LDS_H__ */
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/2] xen: Linker scripts synchronization
2022-03-22 8:02 [PATCH v2 0/2] xen: Linker scripts synchronization Michal Orzel
2022-03-22 8:02 ` [PATCH v2 1/2] xen: Introduce a header to store common linker scripts content Michal Orzel
2022-03-22 8:02 ` [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros Michal Orzel
@ 2022-03-28 10:31 ` Michal Orzel
2022-03-28 11:21 ` Jan Beulich
2 siblings, 1 reply; 28+ messages in thread
From: Michal Orzel @ 2022-03-28 10:31 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
Hi Jan,
Could you please review this series as you did give some comments in v1?
On 22.03.2022 09:02, Michal Orzel wrote:
> 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 an empty header file xen.lds.h to store the
> constructs mutual to both x86 and arm linker scripts. It also includes
> this header in the scripts.
>
> The second patch populates xen.lds.h with the first portion of common
> macros and replaces the original contructs with these helpers.
>
> Michal Orzel (2):
> xen: Introduce a header to store common linker scripts content
> xen: Populate xen.lds.h and make use of its macros
>
> xen/arch/arm/xen.lds.S | 38 +++++---------
> xen/arch/x86/xen.lds.S | 79 +++-------------------------
> xen/include/xen/xen.lds.h | 108 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 129 insertions(+), 96 deletions(-)
> create mode 100644 xen/include/xen/xen.lds.h
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/2] xen: Linker scripts synchronization
2022-03-28 10:31 ` [PATCH v2 0/2] xen: Linker scripts synchronization Michal Orzel
@ 2022-03-28 11:21 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2022-03-28 11:21 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
On 28.03.2022 12:31, Michal Orzel wrote:
> Could you please review this series as you did give some comments in v1?
I have it on my list of things to look at, yes.
Jan
> On 22.03.2022 09:02, Michal Orzel wrote:
>> 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 an empty header file xen.lds.h to store the
>> constructs mutual to both x86 and arm linker scripts. It also includes
>> this header in the scripts.
>>
>> The second patch populates xen.lds.h with the first portion of common
>> macros and replaces the original contructs with these helpers.
>>
>> Michal Orzel (2):
>> xen: Introduce a header to store common linker scripts content
>> xen: Populate xen.lds.h and make use of its macros
>>
>> xen/arch/arm/xen.lds.S | 38 +++++---------
>> xen/arch/x86/xen.lds.S | 79 +++-------------------------
>> xen/include/xen/xen.lds.h | 108 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 129 insertions(+), 96 deletions(-)
>> create mode 100644 xen/include/xen/xen.lds.h
>>
>
> Cheers,
> Michal
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen: Introduce a header to store common linker scripts content
2022-03-22 8:02 ` [PATCH v2 1/2] xen: Introduce a header to store common linker scripts content Michal Orzel
@ 2022-03-29 9:06 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2022-03-29 9:06 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
On 22.03.2022 09:02, 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.
>
> Include this header in linker scripts for x86 and arm.
> This patch serves as an intermediate step before populating xen.lds.h
> and making use of its content in the linker scripts later on.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-22 8:02 ` [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros Michal Orzel
@ 2022-03-29 9:22 ` Jan Beulich
2022-03-29 9:37 ` Michal Orzel
2022-03-29 9:54 ` Julien Grall
1 sibling, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2022-03-29 9:22 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 9:22 ` Jan Beulich
@ 2022-03-29 9:37 ` Michal Orzel
2022-03-29 10:31 ` Jan Beulich
0 siblings, 1 reply; 28+ messages in thread
From: Michal Orzel @ 2022-03-29 9:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
Hi Jan,
Thanks for review.
On 29.03.2022 11:22, Jan Beulich wrote:
> 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.
>
I was thinking about it at the beginning so I'm ok with your solution.
>> @@ -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?
That makes sense. Ok.
>
>> --- 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)?
>
As this list is not full I thought we are going to add DWARF1 sections one day.
DWARF2_DEBUG_SECTIONS would mean that we list sections only from DWARF2 which is not true
as we have sections from DWARF3,5, etc.
Maybe we should leave it as it is but modify the comment to state:
/* DWARF2+ 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. */
>> +#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) } \
I also need to add *(.comment.*) due to:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=69e46280937526db9cf78259cd8a0a9ec62dc847
>> + .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.
>
Ok.
>> + __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.
>
Yes, name of the constructs is the good criteria.
I will do it in v3.
> Jan
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-22 8:02 ` [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros Michal Orzel
2022-03-29 9:22 ` Jan Beulich
@ 2022-03-29 9:54 ` Julien Grall
2022-03-29 10:12 ` Michal Orzel
2022-03-29 10:37 ` Jan Beulich
1 sibling, 2 replies; 28+ messages in thread
From: Julien Grall @ 2022-03-29 9:54 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné
Hi,
On 22/03/2022 08:02, Michal Orzel wrote:
> Populate header file xen.lds.h with the first portion of macros storing
> constructs common to x86 and arm linker scripts. Replace the original
> constructs with these helpers.
>
> No functional improvements to x86 linker script.
>
> Making use of common macros improves arm linker script with:
> -explicit list of debug sections that otherwise are seen as "orphans"
NIT: This is a bit confusing to see no space after -. Can you add one?
I would also recommend to start with (soft)tab to make clearer this is a
list.
Same goes for the other use below.
> by the linker. This will allow to fix issues after enabling linker
> option --orphan-handling one day
> -extended list of discarded section to include: .discard, desctructors
Typo: s/desctructors/destructors/
> related sections, .fini_array which can reference .text.exit
> -sections not related to debugging that are placed by ld.lld.
> Even though Xen on arm compilation with LLVM support is not ready yet,
Building natively Xen on Arm with Clang works. So do you mean you using
LLD?
NIT: Also, from the formatting it is not clear that the second sentence
is part of the last item. How about removing the newline just after the
first sentence?
> these sections do not cause problem to GNU ld.
>
> 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>
[...]
> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
> index dd292fa7dc..ad1d199021 100644
> --- 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
AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support
EFI on arm64.
As this #ifdef is now in generic code, can you explain how this is meant
to be used?
> +/*
> + * 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. */
> +#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) } \
This is a bit confusing. Here you seem to use the section .comment. But...
> + .symtab 0 : { *(.symtab) } \
> + .strtab 0 : { *(.strtab) } \
> + .shstrtab 0 : { *(.shstrtab) }
> +
> +#ifdef EFI
> +#define DISCARD_EFI_SECTIONS \
> + *(.comment) \
... here you will discard it if EFI is set. Which one take precedence if
the caller use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION?
Also, can you explain why we need to drop those sections when EFI is set?
> + *(.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); \
> + __paramhypfs_start = .; \
> + *(.data.paramhypfs) \
> + __paramhypfs_end = .;
> +
> +#define LOCK_PROFILE_DATA \
> + . = ALIGN(POINTER_ALIGN); \
> + __lock_profile_start = .; \
> + *(.lockprofile.data) \
> + __lock_profile_end = .;
> +
> #endif /* __XEN_LDS_H__ */
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 9:54 ` Julien Grall
@ 2022-03-29 10:12 ` Michal Orzel
2022-03-29 10:54 ` Julien Grall
2022-03-29 10:37 ` Jan Beulich
1 sibling, 1 reply; 28+ messages in thread
From: Michal Orzel @ 2022-03-29 10:12 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné
On 29.03.2022 11:54, Julien Grall wrote:
> Hi,
>
> On 22/03/2022 08:02, Michal Orzel wrote:
>> Populate header file xen.lds.h with the first portion of macros storing
>> constructs common to x86 and arm linker scripts. Replace the original
>> constructs with these helpers.
>>
>> No functional improvements to x86 linker script.
>>
>> Making use of common macros improves arm linker script with:
>> -explicit list of debug sections that otherwise are seen as "orphans"
>
> NIT: This is a bit confusing to see no space after -. Can you add one?
>
Ok.
> I would also recommend to start with (soft)tab to make clearer this is a list.
>
> Same goes for the other use below.
>
Ok.
>
>> by the linker. This will allow to fix issues after enabling linker
>> option --orphan-handling one day
>> -extended list of discarded section to include: .discard, desctructors
>
> Typo: s/desctructors/destructors/
>
Ok.
>> related sections, .fini_array which can reference .text.exit
>> -sections not related to debugging that are placed by ld.lld.
>> Even though Xen on arm compilation with LLVM support is not ready yet,
>
> Building natively Xen on Arm with Clang works. So do you mean you using LLD?
>
I mean using the LLVM replacements not only for CC + supporting cross-compilation.
As for the linker, Xen sets llvm-ld which is very very old and in fact README states
LLVM 3.5 or later but llvm-ld was removed before that. Thus IMO support for LLVM on arm
is not ready yet.
> NIT: Also, from the formatting it is not clear that the second sentence is part of the last item. How about removing the newline just after the first sentence?
>
Ok.
>> these sections do not cause problem to GNU ld.
>>
>> 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>
>
> [...]
>
>> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
>> index dd292fa7dc..ad1d199021 100644
>> --- 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
>
> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>
> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>
As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>> +/*
>> + * 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. */
>> +#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) } \
>
> This is a bit confusing. Here you seem to use the section .comment. But...
>
>> + .symtab 0 : { *(.symtab) } \
>> + .strtab 0 : { *(.strtab) } \
>> + .shstrtab 0 : { *(.shstrtab) }
>> +
>> +#ifdef EFI
>> +#define DISCARD_EFI_SECTIONS \
>> + *(.comment) \
>
> ... here you will discard it if EFI is set. Which one take precedence if the caller use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION?
>
ELF_DETAILS_SECTIONS is protected by #ifndef EFI and DISCARD_EFI_SECTION by #ifdef EFI
so the caller cannot use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION.
> Also, can you explain why we need to drop those sections when EFI is set?
>
This is related to x86. Please see the commit: 7844f90abd551f6d5cd9b670b5ed8a4683258a21
>> + *(.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); \
>> + __paramhypfs_start = .; \
>> + *(.data.paramhypfs) \
>> + __paramhypfs_end = .;
>> +
>> +#define LOCK_PROFILE_DATA \
>> + . = ALIGN(POINTER_ALIGN); \
>> + __lock_profile_start = .; \
>> + *(.lockprofile.data) \
>> + __lock_profile_end = .;
>> +
>> #endif /* __XEN_LDS_H__ */
>
> Cheers,
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 9:37 ` Michal Orzel
@ 2022-03-29 10:31 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2022-03-29 10:31 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
On 29.03.2022 11:37, Michal Orzel wrote:
> On 29.03.2022 11:22, Jan Beulich wrote:
>> On 22.03.2022 09:02, Michal Orzel wrote:
>>> --- 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)?
>>
> As this list is not full I thought we are going to add DWARF1 sections one day.
> DWARF2_DEBUG_SECTIONS would mean that we list sections only from DWARF2 which is not true
> as we have sections from DWARF3,5, etc.
> Maybe we should leave it as it is but modify the comment to state:
> /* DWARF2+ debug sections */
Well, yes, but only in a way. Dwarf3 and later are simply extensions
of Dwarf2, whereas Dwarf2 is not an extension of what originally was
called just Dwarf and now is commonly referred to as Dwarf1. I'm
fine with a comment saying Dwarf2+, but I'd like to not see the
construct be named just DWARF_*.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 9:54 ` Julien Grall
2022-03-29 10:12 ` Michal Orzel
@ 2022-03-29 10:37 ` Jan Beulich
2022-03-29 11:07 ` Julien Grall
1 sibling, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2022-03-29 10:37 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Michal Orzel, xen-devel
On 29.03.2022 11:54, Julien Grall wrote:
> On 22/03/2022 08:02, Michal Orzel wrote:
>> --- 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
>
> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support
> EFI on arm64.
>
> As this #ifdef is now in generic code, can you explain how this is meant
> to be used?
The identifier may now be somewhat misleading, yes - it has always meant
"linking a native EFI (i.e. PE/COFF) binary". The equivalence "EFI binary"
== "EFI support" has long been lost.
>> +/*
>> + * 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. */
>> +#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) } \
>
> This is a bit confusing. Here you seem to use the section .comment. But...
>
>> + .symtab 0 : { *(.symtab) } \
>> + .strtab 0 : { *(.strtab) } \
>> + .shstrtab 0 : { *(.shstrtab) }
>> +
>> +#ifdef EFI
>> +#define DISCARD_EFI_SECTIONS \
>> + *(.comment) \
>
> ... here you will discard it if EFI is set. Which one take precedence if
> the caller use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION?
Given the above explanation I think it's clear that only one of the
two may be used at a time: ELF_DETAILS_SECTIONS when linking an ELF
binary and DISCARD_EFI_SECTIONS when linking a PE/COFF binary.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 10:12 ` Michal Orzel
@ 2022-03-29 10:54 ` Julien Grall
2022-03-29 11:09 ` Michal Orzel
2022-03-29 11:42 ` Jan Beulich
0 siblings, 2 replies; 28+ messages in thread
From: Julien Grall @ 2022-03-29 10:54 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné
Hi,
On 29/03/2022 11:12, Michal Orzel wrote:
> On 29.03.2022 11:54, Julien Grall wrote:
>> Hi,
>>
>> On 22/03/2022 08:02, Michal Orzel wrote:
>>> Populate header file xen.lds.h with the first portion of macros storing
>>> constructs common to x86 and arm linker scripts. Replace the original
>>> constructs with these helpers.
>>>
>>> No functional improvements to x86 linker script.
>>>
>>> Making use of common macros improves arm linker script with:
>>> -explicit list of debug sections that otherwise are seen as "orphans"
>>
>> NIT: This is a bit confusing to see no space after -. Can you add one?
>>
> Ok.
>
>> I would also recommend to start with (soft)tab to make clearer this is a list.
>>
>> Same goes for the other use below.
>>
> Ok.
>
>>
>>> by the linker. This will allow to fix issues after enabling linker
>>> option --orphan-handling one day
>>> -extended list of discarded section to include: .discard, desctructors
>>
>> Typo: s/desctructors/destructors/
>>
> Ok.
>
>>> related sections, .fini_array which can reference .text.exit
>>> -sections not related to debugging that are placed by ld.lld.
>>> Even though Xen on arm compilation with LLVM support is not ready yet,
>>
>> Building natively Xen on Arm with Clang works. So do you mean you using LLD?
>>
> I mean using the LLVM replacements not only for CC + supporting cross-compilation.
> As for the linker, Xen sets llvm-ld which is very very old and in fact README states
> LLVM 3.5 or later but llvm-ld was removed before that.
I am confused. I looked at the llvm repo and lld is still there. So why
are you saying is lld is very old and removed?
> Thus IMO support for LLVM on arm
> is not ready yet.
I agree that building Xen on Arm only with LLVM tools is not possible
yet. But this statement seems to be a bit too broad here. I think what
matters is we don't support linking with LLD on Arm.
>>> these sections do not cause problem to GNU ld.
>>>
>>> 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>
>>
>> [...]
>>
>>> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
>>> index dd292fa7dc..ad1d199021 100644
>>> --- 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
>>
>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>
>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>
> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
I find the name "EFI" too generic to figure out that this code can only
be used by x86.
But, from my understanding, this header is meant to contain generic
code. It feels a bit odd that we are moving arch specific code.
To be honest, I don't quite understand why we need to make the
diffferentiation on x86. So I guess the first question is how this is
meant to be used on x86?
Once we answered that, we can decide whether this is correct to use EFI
in generic code. IOW, is thish going to be useful for other arch?
>
>>> +/*
>>> + * 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. */
>>> +#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) } \
>>
>> This is a bit confusing. Here you seem to use the section .comment. But...
>>
>>> + .symtab 0 : { *(.symtab) } \
>>> + .strtab 0 : { *(.strtab) } \
>>> + .shstrtab 0 : { *(.shstrtab) }
>>> +
>>> +#ifdef EFI
>>> +#define DISCARD_EFI_SECTIONS \
>>> + *(.comment) \
>>
>> ... here you will discard it if EFI is set. Which one take precedence if the caller use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION?
>>
> ELF_DETAILS_SECTIONS is protected by #ifndef EFI and DISCARD_EFI_SECTION by #ifdef EFI
> so the caller cannot use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION.
The caller will protect it. But it is not in the header. I don't think
we should expect the user to check x86 to understand how this is meant
to be used.
>
>> Also, can you explain why we need to drop those sections when EFI is set?
>>
> This is related to x86. Please see the commit: 7844f90abd551f6d5cd9b670b5ed8a4683258a21
Why is this in the generic header then?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 10:37 ` Jan Beulich
@ 2022-03-29 11:07 ` Julien Grall
2022-03-29 11:38 ` Jan Beulich
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-29 11:07 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Michal Orzel, xen-devel
Hi Jan,
On 29/03/2022 11:37, Jan Beulich wrote:
> On 29.03.2022 11:54, Julien Grall wrote:
>> On 22/03/2022 08:02, Michal Orzel wrote:
>>> --- 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
>>
>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support
>> EFI on arm64.
>>
>> As this #ifdef is now in generic code, can you explain how this is meant
>> to be used?
>
> The identifier may now be somewhat misleading, yes - it has always meant
> "linking a native EFI (i.e. PE/COFF) binary". The equivalence "EFI binary"
> == "EFI support" has long been lost.
On Arm, we will be generating a EFI binary (or better a Image/EFI). So
IIUC the description, we should in theory set EFI.
But I think it would do the wrong thing on Arm. Would you be able to
explain why you need to differentiate it on x86?
>>> +/*
>>> + * 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. */
>>> +#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) } \
>>
>> This is a bit confusing. Here you seem to use the section .comment. But...
>>
>>> + .symtab 0 : { *(.symtab) } \
>>> + .strtab 0 : { *(.strtab) } \
>>> + .shstrtab 0 : { *(.shstrtab) }
>>> +
>>> +#ifdef EFI
>>> +#define DISCARD_EFI_SECTIONS \
>>> + *(.comment) \
>>
>> ... here you will discard it if EFI is set. Which one take precedence if
>> the caller use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION?
>
> Given the above explanation I think it's clear that only one of the
> two may be used at a time: ELF_DETAILS_SECTIONS when linking an ELF
> binary and DISCARD_EFI_SECTIONS when linking a PE/COFF binary.
I guess this may be obvious on x86. But for Arm, we are generating the
ELF first and then extracting the information to generate the binary.
The end result will be a binary that is PE/COFF compatible.
So to me, it would make sense to include DISCARD_EFI_SECTIONS because we
going to create an EFI binary and also include EFI_DETAILS_SECTIONS
because we are building an ELF.
Overall it sounds like to me that it is too premature to move the
#if{,n}def EFI bits in the generic header.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 10:54 ` Julien Grall
@ 2022-03-29 11:09 ` Michal Orzel
2022-03-29 11:42 ` Jan Beulich
1 sibling, 0 replies; 28+ messages in thread
From: Michal Orzel @ 2022-03-29 11:09 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu,
Roger Pau Monné
On 29.03.2022 12:54, Julien Grall wrote:
> Hi,
>
> On 29/03/2022 11:12, Michal Orzel wrote:
>> On 29.03.2022 11:54, Julien Grall wrote:
>>> Hi,
>>>
>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>> Populate header file xen.lds.h with the first portion of macros storing
>>>> constructs common to x86 and arm linker scripts. Replace the original
>>>> constructs with these helpers.
>>>>
>>>> No functional improvements to x86 linker script.
>>>>
>>>> Making use of common macros improves arm linker script with:
>>>> -explicit list of debug sections that otherwise are seen as "orphans"
>>>
>>> NIT: This is a bit confusing to see no space after -. Can you add one?
>>>
>> Ok.
>>
>>> I would also recommend to start with (soft)tab to make clearer this is a list.
>>>
>>> Same goes for the other use below.
>>>
>> Ok.
>>
>>>
>>>> by the linker. This will allow to fix issues after enabling linker
>>>> option --orphan-handling one day
>>>> -extended list of discarded section to include: .discard, desctructors
>>>
>>> Typo: s/desctructors/destructors/
>>>
>> Ok.
>>
>>>> related sections, .fini_array which can reference .text.exit
>>>> -sections not related to debugging that are placed by ld.lld.
>>>> Even though Xen on arm compilation with LLVM support is not ready yet,
>>>
>>> Building natively Xen on Arm with Clang works. So do you mean you using LLD?
>>>
>> I mean using the LLVM replacements not only for CC + supporting cross-compilation.
>> As for the linker, Xen sets llvm-ld which is very very old and in fact README states
>> LLVM 3.5 or later but llvm-ld was removed before that.
>
> I am confused. I looked at the llvm repo and lld is still there. So why are you saying is lld is very old and removed?
>
lld is not llvm-ld. I'm talking about llvm-ld. lld is the current LLVM linker. Xen sets LD to llvm-ld which has been removed in 3.2:
See: https://releases.llvm.org/3.2/docs/ReleaseNotes.html
>> Thus IMO support for LLVM on arm
>> is not ready yet.
>
> I agree that building Xen on Arm only with LLVM tools is not possible yet. But this statement seems to be a bit too broad here. I think what matters is we don't support linking with LLD on Arm.
>
>>>> these sections do not cause problem to GNU ld.
>>>>
>>>> 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>
>>>
>>> [...]
>>>
>>>> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
>>>> index dd292fa7dc..ad1d199021 100644
>>>> --- 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
>>>
>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>>
>>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>>
>> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>
> I find the name "EFI" too generic to figure out that this code can only be used by x86.
>
> But, from my understanding, this header is meant to contain generic code. It feels a bit odd that we are moving arch specific code.
>
> To be honest, I don't quite understand why we need to make the diffferentiation on x86. So I guess the first question is how this is meant to be used on x86?
>
> Once we answered that, we can decide whether this is correct to use EFI in generic code. IOW, is thish going to be useful for other arch?
>
I think Jan needs to answer this question as I am not sure.
>>
>>>> +/*
>>>> + * 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. */
>>>> +#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) } \
>>>
>>> This is a bit confusing. Here you seem to use the section .comment. But...
>>>
>>>> + .symtab 0 : { *(.symtab) } \
>>>> + .strtab 0 : { *(.strtab) } \
>>>> + .shstrtab 0 : { *(.shstrtab) }
>>>> +
>>>> +#ifdef EFI
>>>> +#define DISCARD_EFI_SECTIONS \
>>>> + *(.comment) \
>>>
>>> ... here you will discard it if EFI is set. Which one take precedence if the caller use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION?
>>>
>> ELF_DETAILS_SECTIONS is protected by #ifndef EFI and DISCARD_EFI_SECTION by #ifdef EFI
>> so the caller cannot use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION.
>
> The caller will protect it. But it is not in the header. I don't think we should expect the user to check x86 to understand how this is meant to be used.
>
>>
>>> Also, can you explain why we need to drop those sections when EFI is set?
>>>
>> This is related to x86. Please see the commit: 7844f90abd551f6d5cd9b670b5ed8a4683258a21
>
> Why is this in the generic header then?
>
If we decide that EFI is not meant for anything else than x86, I will get rid of it completely from this header.
> Cheers,
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 11:07 ` Julien Grall
@ 2022-03-29 11:38 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2022-03-29 11:38 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Michal Orzel, xen-devel
On 29.03.2022 13:07, Julien Grall wrote:
> On 29/03/2022 11:37, Jan Beulich wrote:
>> On 29.03.2022 11:54, Julien Grall wrote:
>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>> --- 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
>>>
>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support
>>> EFI on arm64.
>>>
>>> As this #ifdef is now in generic code, can you explain how this is meant
>>> to be used?
>>
>> The identifier may now be somewhat misleading, yes - it has always meant
>> "linking a native EFI (i.e. PE/COFF) binary". The equivalence "EFI binary"
>> == "EFI support" has long been lost.
> On Arm, we will be generating a EFI binary (or better a Image/EFI). So
> IIUC the description, we should in theory set EFI.
Well, no - you're mixing up "generating" and "linking". What's of interest
here is what the linker is told to produce, not what may involved further
processing steps. We're talking about a linker script here, after all.
> But I think it would do the wrong thing on Arm. Would you be able to
> explain why you need to differentiate it on x86?
The differences aren't unique to x86; they all are related to how ELF and
PE/COFF differ from one another.
>>>> +/*
>>>> + * 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. */
>>>> +#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) } \
>>>
>>> This is a bit confusing. Here you seem to use the section .comment. But...
>>>
>>>> + .symtab 0 : { *(.symtab) } \
>>>> + .strtab 0 : { *(.strtab) } \
>>>> + .shstrtab 0 : { *(.shstrtab) }
>>>> +
>>>> +#ifdef EFI
>>>> +#define DISCARD_EFI_SECTIONS \
>>>> + *(.comment) \
>>>
>>> ... here you will discard it if EFI is set. Which one take precedence if
>>> the caller use both ELF_DETAILS_SECTIONS and DISCARD_EFI_SECTION?
>>
>> Given the above explanation I think it's clear that only one of the
>> two may be used at a time: ELF_DETAILS_SECTIONS when linking an ELF
>> binary and DISCARD_EFI_SECTIONS when linking a PE/COFF binary.
>
> I guess this may be obvious on x86. But for Arm, we are generating the
> ELF first and then extracting the information to generate the binary.
> The end result will be a binary that is PE/COFF compatible.
>
> So to me, it would make sense to include DISCARD_EFI_SECTIONS because we
> going to create an EFI binary and also include EFI_DETAILS_SECTIONS
> because we are building an ELF.
No - as per above, all we should be concerned about in the linker script
are requirements by the linker for linking a file in the request output
format.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 10:54 ` Julien Grall
2022-03-29 11:09 ` Michal Orzel
@ 2022-03-29 11:42 ` Jan Beulich
2022-03-30 10:32 ` Julien Grall
1 sibling, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2022-03-29 11:42 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Michal Orzel, xen-devel
On 29.03.2022 12:54, Julien Grall wrote:
> On 29/03/2022 11:12, Michal Orzel wrote:
>> On 29.03.2022 11:54, Julien Grall wrote:
>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>> --- 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
>>>
>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>>
>>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>>
>> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>
> I find the name "EFI" too generic to figure out that this code can only
> be used by x86.
>
> But, from my understanding, this header is meant to contain generic
> code. It feels a bit odd that we are moving arch specific code.
>
> To be honest, I don't quite understand why we need to make the
> diffferentiation on x86. So I guess the first question is how this is
> meant to be used on x86?
We produce two linker scripts from the single source file: One (with EFI
undefined) to link the ELF binary, and another (with EFI defined) to link
the PE/COFF output. If "EFI" is too imprecise as a name for the identifier,
I wouldn't mind renaming it (to PE_COFF?), but at the same time I'm not
convinced this is really necessary.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-29 11:42 ` Jan Beulich
@ 2022-03-30 10:32 ` Julien Grall
2022-03-30 10:42 ` Jan Beulich
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-30 10:32 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Michal Orzel, xen-devel
Hi Jan,
On 29/03/2022 12:42, Jan Beulich wrote:
> On 29.03.2022 12:54, Julien Grall wrote:
>> On 29/03/2022 11:12, Michal Orzel wrote:
>>> On 29.03.2022 11:54, Julien Grall wrote:
>>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>>> --- 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
>>>>
>>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>>>
>>>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>>>
>>> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>>
>> I find the name "EFI" too generic to figure out that this code can only
>> be used by x86.
>>
>> But, from my understanding, this header is meant to contain generic
>> code. It feels a bit odd that we are moving arch specific code.
>>
>> To be honest, I don't quite understand why we need to make the
>> diffferentiation on x86. So I guess the first question is how this is
>> meant to be used on x86?
>
> We produce two linker scripts from the single source file: One (with EFI
> undefined) to link the ELF binary, and another (with EFI defined) to link
> the PE/COFF output. If "EFI" is too imprecise as a name for the identifier,
> I wouldn't mind renaming it (to PE_COFF?), but at the same time I'm not
> convinced this is really necessary.
Thank for the explanation (and the other ones in this thread). You are
right the confusion arised from "generating" vs "linking".
Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
That said, it would possibly make more difficult to associate the flag
with "linking an EFI binary".
I think some documentaion about the define EFI would be help so there
are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
put it. Maybe at the top of the header?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 10:32 ` Julien Grall
@ 2022-03-30 10:42 ` Jan Beulich
2022-03-30 11:04 ` Michal Orzel
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2022-03-30 10:42 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
Michal Orzel, xen-devel
On 30.03.2022 12:32, Julien Grall wrote:
> On 29/03/2022 12:42, Jan Beulich wrote:
>> On 29.03.2022 12:54, Julien Grall wrote:
>>> On 29/03/2022 11:12, Michal Orzel wrote:
>>>> On 29.03.2022 11:54, Julien Grall wrote:
>>>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>>>> --- 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
>>>>>
>>>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>>>>
>>>>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>>>>
>>>> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>>>
>>> I find the name "EFI" too generic to figure out that this code can only
>>> be used by x86.
>>>
>>> But, from my understanding, this header is meant to contain generic
>>> code. It feels a bit odd that we are moving arch specific code.
>>>
>>> To be honest, I don't quite understand why we need to make the
>>> diffferentiation on x86. So I guess the first question is how this is
>>> meant to be used on x86?
>>
>> We produce two linker scripts from the single source file: One (with EFI
>> undefined) to link the ELF binary, and another (with EFI defined) to link
>> the PE/COFF output. If "EFI" is too imprecise as a name for the identifier,
>> I wouldn't mind renaming it (to PE_COFF?), but at the same time I'm not
>> convinced this is really necessary.
>
> Thank for the explanation (and the other ones in this thread). You are
> right the confusion arised from "generating" vs "linking".
>
> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
> That said, it would possibly make more difficult to associate the flag
> with "linking an EFI binary".
Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
> I think some documentaion about the define EFI would be help so there
> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
> put it. Maybe at the top of the header?
That's perhaps the best place, yes.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 10:42 ` Jan Beulich
@ 2022-03-30 11:04 ` Michal Orzel
2022-03-30 11:57 ` Jan Beulich
0 siblings, 1 reply; 28+ messages in thread
From: Michal Orzel @ 2022-03-30 11:04 UTC (permalink / raw)
To: Jan Beulich, Julien Grall
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel
Hello,
On 30.03.2022 12:42, Jan Beulich wrote:
> On 30.03.2022 12:32, Julien Grall wrote:
>> On 29/03/2022 12:42, Jan Beulich wrote:
>>> On 29.03.2022 12:54, Julien Grall wrote:
>>>> On 29/03/2022 11:12, Michal Orzel wrote:
>>>>> On 29.03.2022 11:54, Julien Grall wrote:
>>>>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>>>>> --- 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
>>>>>>
>>>>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>>>>>
>>>>>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>>>>>
>>>>> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>>>>
>>>> I find the name "EFI" too generic to figure out that this code can only
>>>> be used by x86.
>>>>
>>>> But, from my understanding, this header is meant to contain generic
>>>> code. It feels a bit odd that we are moving arch specific code.
>>>>
>>>> To be honest, I don't quite understand why we need to make the
>>>> diffferentiation on x86. So I guess the first question is how this is
>>>> meant to be used on x86?
>>>
>>> We produce two linker scripts from the single source file: One (with EFI
>>> undefined) to link the ELF binary, and another (with EFI defined) to link
>>> the PE/COFF output. If "EFI" is too imprecise as a name for the identifier,
>>> I wouldn't mind renaming it (to PE_COFF?), but at the same time I'm not
>>> convinced this is really necessary.
>>
>> Thank for the explanation (and the other ones in this thread). You are
>> right the confusion arised from "generating" vs "linking".
>>
>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>> That said, it would possibly make more difficult to associate the flag
>> with "linking an EFI binary".
>
> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>
>> I think some documentaion about the define EFI would be help so there
>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>> put it. Maybe at the top of the header?
>
> That's perhaps the best place, yes.
>
In this case how about the following comment at the top of xen.lds.h:
"To avoid any confusion about EFI macro used in this header vs EFI support,
the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
the latter means support for generating EFI binary. Currently EFI macro is
only defined by x86 to link PE/COFF output, however it is not unique to this
architecture."
> Jan
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 11:04 ` Michal Orzel
@ 2022-03-30 11:57 ` Jan Beulich
2022-03-30 12:13 ` Michal Orzel
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2022-03-30 11:57 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel, Julien Grall
On 30.03.2022 13:04, Michal Orzel wrote:
> Hello,
>
> On 30.03.2022 12:42, Jan Beulich wrote:
>> On 30.03.2022 12:32, Julien Grall wrote:
>>> On 29/03/2022 12:42, Jan Beulich wrote:
>>>> On 29.03.2022 12:54, Julien Grall wrote:
>>>>> On 29/03/2022 11:12, Michal Orzel wrote:
>>>>>> On 29.03.2022 11:54, Julien Grall wrote:
>>>>>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>>>>>> --- 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
>>>>>>>
>>>>>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>>>>>>
>>>>>>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>>>>>>
>>>>>> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>>>>>
>>>>> I find the name "EFI" too generic to figure out that this code can only
>>>>> be used by x86.
>>>>>
>>>>> But, from my understanding, this header is meant to contain generic
>>>>> code. It feels a bit odd that we are moving arch specific code.
>>>>>
>>>>> To be honest, I don't quite understand why we need to make the
>>>>> diffferentiation on x86. So I guess the first question is how this is
>>>>> meant to be used on x86?
>>>>
>>>> We produce two linker scripts from the single source file: One (with EFI
>>>> undefined) to link the ELF binary, and another (with EFI defined) to link
>>>> the PE/COFF output. If "EFI" is too imprecise as a name for the identifier,
>>>> I wouldn't mind renaming it (to PE_COFF?), but at the same time I'm not
>>>> convinced this is really necessary.
>>>
>>> Thank for the explanation (and the other ones in this thread). You are
>>> right the confusion arised from "generating" vs "linking".
>>>
>>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>>> That said, it would possibly make more difficult to associate the flag
>>> with "linking an EFI binary".
>>
>> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>>
>>> I think some documentaion about the define EFI would be help so there
>>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>>> put it. Maybe at the top of the header?
>>
>> That's perhaps the best place, yes.
>>
> In this case how about the following comment at the top of xen.lds.h:
>
> "To avoid any confusion about EFI macro used in this header vs EFI support,
> the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
> the latter means support for generating EFI binary.
No, that's the case on Arm only. As Julien suggested, it is perhaps best
to explain the difference between EFI and CONFIG_EFI, without going into
arch specifics.
Jan
> Currently EFI macro is
> only defined by x86 to link PE/COFF output, however it is not unique to this
> architecture."
>
> Cheers,
> Michal
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 11:57 ` Jan Beulich
@ 2022-03-30 12:13 ` Michal Orzel
2022-03-30 12:53 ` Jan Beulich
0 siblings, 1 reply; 28+ messages in thread
From: Michal Orzel @ 2022-03-30 12:13 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel, Julien Grall
On 30.03.2022 13:57, Jan Beulich wrote:
> On 30.03.2022 13:04, Michal Orzel wrote:
>> Hello,
>>
>> On 30.03.2022 12:42, Jan Beulich wrote:
>>> On 30.03.2022 12:32, Julien Grall wrote:
>>>> On 29/03/2022 12:42, Jan Beulich wrote:
>>>>> On 29.03.2022 12:54, Julien Grall wrote:
>>>>>> On 29/03/2022 11:12, Michal Orzel wrote:
>>>>>>> On 29.03.2022 11:54, Julien Grall wrote:
>>>>>>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>>>>>>> --- 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
>>>>>>>>
>>>>>>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>>>>>>>
>>>>>>>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>>>>>>>
>>>>>>> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>>>>>>
>>>>>> I find the name "EFI" too generic to figure out that this code can only
>>>>>> be used by x86.
>>>>>>
>>>>>> But, from my understanding, this header is meant to contain generic
>>>>>> code. It feels a bit odd that we are moving arch specific code.
>>>>>>
>>>>>> To be honest, I don't quite understand why we need to make the
>>>>>> diffferentiation on x86. So I guess the first question is how this is
>>>>>> meant to be used on x86?
>>>>>
>>>>> We produce two linker scripts from the single source file: One (with EFI
>>>>> undefined) to link the ELF binary, and another (with EFI defined) to link
>>>>> the PE/COFF output. If "EFI" is too imprecise as a name for the identifier,
>>>>> I wouldn't mind renaming it (to PE_COFF?), but at the same time I'm not
>>>>> convinced this is really necessary.
>>>>
>>>> Thank for the explanation (and the other ones in this thread). You are
>>>> right the confusion arised from "generating" vs "linking".
>>>>
>>>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>>>> That said, it would possibly make more difficult to associate the flag
>>>> with "linking an EFI binary".
>>>
>>> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>>>
>>>> I think some documentaion about the define EFI would be help so there
>>>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>>>> put it. Maybe at the top of the header?
>>>
>>> That's perhaps the best place, yes.
>>>
>> In this case how about the following comment at the top of xen.lds.h:
>>
>> "To avoid any confusion about EFI macro used in this header vs EFI support,
>> the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
>> the latter means support for generating EFI binary.
>
> No, that's the case on Arm only. As Julien suggested, it is perhaps best
> to explain the difference between EFI and CONFIG_EFI, without going into
> arch specifics.
Could you please tell me what you are reffering to as there is no such identifier
in Xen (as opposed to Linux) like CONFIG_EFI ?
>
> Jan
>
>> Currently EFI macro is
>> only defined by x86 to link PE/COFF output, however it is not unique to this
>> architecture."
>>
>> Cheers,
>> Michal
>>
>
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 12:13 ` Michal Orzel
@ 2022-03-30 12:53 ` Jan Beulich
2022-03-30 13:24 ` Michal Orzel
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2022-03-30 12:53 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel, Julien Grall
On 30.03.2022 14:13, Michal Orzel wrote:
> On 30.03.2022 13:57, Jan Beulich wrote:
>> On 30.03.2022 13:04, Michal Orzel wrote:
>>> On 30.03.2022 12:42, Jan Beulich wrote:
>>>> On 30.03.2022 12:32, Julien Grall wrote:
>>>>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>>>>> That said, it would possibly make more difficult to associate the flag
>>>>> with "linking an EFI binary".
>>>>
>>>> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>>>>
>>>>> I think some documentaion about the define EFI would be help so there
>>>>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>>>>> put it. Maybe at the top of the header?
>>>>
>>>> That's perhaps the best place, yes.
>>>>
>>> In this case how about the following comment at the top of xen.lds.h:
>>>
>>> "To avoid any confusion about EFI macro used in this header vs EFI support,
>>> the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
>>> the latter means support for generating EFI binary.
>>
>> No, that's the case on Arm only. As Julien suggested, it is perhaps best
>> to explain the difference between EFI and CONFIG_EFI, without going into
>> arch specifics.
> Could you please tell me what you are reffering to as there is no such identifier
> in Xen (as opposed to Linux) like CONFIG_EFI ?
Let's call it a "virtual" CONFIG_EFI then; I think we really should have
such an option. But yes, if you don't like referring to such a virtual
option, then describing what is meant verbally is certainly going to be
fine.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 12:53 ` Jan Beulich
@ 2022-03-30 13:24 ` Michal Orzel
2022-03-30 13:27 ` Jan Beulich
2022-03-30 13:30 ` Julien Grall
0 siblings, 2 replies; 28+ messages in thread
From: Michal Orzel @ 2022-03-30 13:24 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel, Julien Grall
On 30.03.2022 14:53, Jan Beulich wrote:
> On 30.03.2022 14:13, Michal Orzel wrote:
>> On 30.03.2022 13:57, Jan Beulich wrote:
>>> On 30.03.2022 13:04, Michal Orzel wrote:
>>>> On 30.03.2022 12:42, Jan Beulich wrote:
>>>>> On 30.03.2022 12:32, Julien Grall wrote:
>>>>>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>>>>>> That said, it would possibly make more difficult to associate the flag
>>>>>> with "linking an EFI binary".
>>>>>
>>>>> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>>>>>
>>>>>> I think some documentaion about the define EFI would be help so there
>>>>>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>>>>>> put it. Maybe at the top of the header?
>>>>>
>>>>> That's perhaps the best place, yes.
>>>>>
>>>> In this case how about the following comment at the top of xen.lds.h:
>>>>
>>>> "To avoid any confusion about EFI macro used in this header vs EFI support,
>>>> the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
>>>> the latter means support for generating EFI binary.
>>>
>>> No, that's the case on Arm only. As Julien suggested, it is perhaps best
>>> to explain the difference between EFI and CONFIG_EFI, without going into
>>> arch specifics.
>> Could you please tell me what you are reffering to as there is no such identifier
>> in Xen (as opposed to Linux) like CONFIG_EFI ?
>
> Let's call it a "virtual" CONFIG_EFI then; I think we really should have
> such an option. But yes, if you don't like referring to such a virtual
> option, then describing what is meant verbally is certainly going to be
> fine.
>
FWICS, there was an attempt done by Wei in his NUMA series to define CONFIG_EFI.
However as this is not yet merged and agreed, I would like not to refer to identifiers
not existing in the code, even though most people are familiar with this option from Linux.
So by taking an example from Linux I would verbally explain it like that:
"To avoid any confusion, please note that EFI macro does not correspond to EFI
runtime support and is used when linking a native EFI (i.e. PE/COFF) binary, hence its
usage in this header."
If that does not suite you, please help creating a better explanation.
> Jan
>
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 13:24 ` Michal Orzel
@ 2022-03-30 13:27 ` Jan Beulich
2022-03-30 13:30 ` Julien Grall
1 sibling, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2022-03-30 13:27 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel, Julien Grall
On 30.03.2022 15:24, Michal Orzel wrote:
> On 30.03.2022 14:53, Jan Beulich wrote:
>> On 30.03.2022 14:13, Michal Orzel wrote:
>>> On 30.03.2022 13:57, Jan Beulich wrote:
>>>> On 30.03.2022 13:04, Michal Orzel wrote:
>>>>> On 30.03.2022 12:42, Jan Beulich wrote:
>>>>>> On 30.03.2022 12:32, Julien Grall wrote:
>>>>>>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>>>>>>> That said, it would possibly make more difficult to associate the flag
>>>>>>> with "linking an EFI binary".
>>>>>>
>>>>>> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>>>>>>
>>>>>>> I think some documentaion about the define EFI would be help so there
>>>>>>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>>>>>>> put it. Maybe at the top of the header?
>>>>>>
>>>>>> That's perhaps the best place, yes.
>>>>>>
>>>>> In this case how about the following comment at the top of xen.lds.h:
>>>>>
>>>>> "To avoid any confusion about EFI macro used in this header vs EFI support,
>>>>> the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
>>>>> the latter means support for generating EFI binary.
>>>>
>>>> No, that's the case on Arm only. As Julien suggested, it is perhaps best
>>>> to explain the difference between EFI and CONFIG_EFI, without going into
>>>> arch specifics.
>>> Could you please tell me what you are reffering to as there is no such identifier
>>> in Xen (as opposed to Linux) like CONFIG_EFI ?
>>
>> Let's call it a "virtual" CONFIG_EFI then; I think we really should have
>> such an option. But yes, if you don't like referring to such a virtual
>> option, then describing what is meant verbally is certainly going to be
>> fine.
>>
> FWICS, there was an attempt done by Wei in his NUMA series to define CONFIG_EFI.
> However as this is not yet merged and agreed, I would like not to refer to identifiers
> not existing in the code, even though most people are familiar with this option from Linux.
>
> So by taking an example from Linux I would verbally explain it like that:
> "To avoid any confusion, please note that EFI macro does not correspond to EFI
> runtime support and is used when linking a native EFI (i.e. PE/COFF) binary, hence its
> usage in this header."
This reads okay to me (perhaps with "the" inserted before "EFI macro").
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 13:24 ` Michal Orzel
2022-03-30 13:27 ` Jan Beulich
@ 2022-03-30 13:30 ` Julien Grall
2022-03-30 13:36 ` Jan Beulich
1 sibling, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-30 13:30 UTC (permalink / raw)
To: Michal Orzel, Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel
Hi,
On 30/03/2022 14:24, Michal Orzel wrote:
>
>
> On 30.03.2022 14:53, Jan Beulich wrote:
>> On 30.03.2022 14:13, Michal Orzel wrote:
>>> On 30.03.2022 13:57, Jan Beulich wrote:
>>>> On 30.03.2022 13:04, Michal Orzel wrote:
>>>>> On 30.03.2022 12:42, Jan Beulich wrote:
>>>>>> On 30.03.2022 12:32, Julien Grall wrote:
>>>>>>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>>>>>>> That said, it would possibly make more difficult to associate the flag
>>>>>>> with "linking an EFI binary".
>>>>>>
>>>>>> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>>>>>>
>>>>>>> I think some documentaion about the define EFI would be help so there
>>>>>>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>>>>>>> put it. Maybe at the top of the header?
>>>>>>
>>>>>> That's perhaps the best place, yes.
>>>>>>
>>>>> In this case how about the following comment at the top of xen.lds.h:
>>>>>
>>>>> "To avoid any confusion about EFI macro used in this header vs EFI support,
>>>>> the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
>>>>> the latter means support for generating EFI binary.
>>>>
>>>> No, that's the case on Arm only. As Julien suggested, it is perhaps best
>>>> to explain the difference between EFI and CONFIG_EFI, without going into
>>>> arch specifics.
>>> Could you please tell me what you are reffering to as there is no such identifier
>>> in Xen (as opposed to Linux) like CONFIG_EFI ?
>>
>> Let's call it a "virtual" CONFIG_EFI then; I think we really should have
>> such an option. But yes, if you don't like referring to such a virtual
>> option, then describing what is meant verbally is certainly going to be
>> fine.
>>
> FWICS, there was an attempt done by Wei in his NUMA series to define CONFIG_EFI.
> However as this is not yet merged and agreed, I would like not to refer to identifiers
> not existing in the code, even though most people are familiar with this option from Linux.
>
> So by taking an example from Linux I would verbally explain it like that:
> "To avoid any confusion, please note that EFI macro does not correspond to EFI
> runtime support and is used when linking a native EFI (i.e. PE/COFF) binary, hence its
"EFI runtime support" can be mistakenly associated to EFI runtime
services (which BTW not supported on Arm). So I would suggest to
s/runtime/boot/.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 13:30 ` Julien Grall
@ 2022-03-30 13:36 ` Jan Beulich
2022-03-30 13:37 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2022-03-30 13:36 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel, Michal Orzel
On 30.03.2022 15:30, Julien Grall wrote:
> Hi,
>
> On 30/03/2022 14:24, Michal Orzel wrote:
>>
>>
>> On 30.03.2022 14:53, Jan Beulich wrote:
>>> On 30.03.2022 14:13, Michal Orzel wrote:
>>>> On 30.03.2022 13:57, Jan Beulich wrote:
>>>>> On 30.03.2022 13:04, Michal Orzel wrote:
>>>>>> On 30.03.2022 12:42, Jan Beulich wrote:
>>>>>>> On 30.03.2022 12:32, Julien Grall wrote:
>>>>>>>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>>>>>>>> That said, it would possibly make more difficult to associate the flag
>>>>>>>> with "linking an EFI binary".
>>>>>>>
>>>>>>> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>>>>>>>
>>>>>>>> I think some documentaion about the define EFI would be help so there
>>>>>>>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>>>>>>>> put it. Maybe at the top of the header?
>>>>>>>
>>>>>>> That's perhaps the best place, yes.
>>>>>>>
>>>>>> In this case how about the following comment at the top of xen.lds.h:
>>>>>>
>>>>>> "To avoid any confusion about EFI macro used in this header vs EFI support,
>>>>>> the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
>>>>>> the latter means support for generating EFI binary.
>>>>>
>>>>> No, that's the case on Arm only. As Julien suggested, it is perhaps best
>>>>> to explain the difference between EFI and CONFIG_EFI, without going into
>>>>> arch specifics.
>>>> Could you please tell me what you are reffering to as there is no such identifier
>>>> in Xen (as opposed to Linux) like CONFIG_EFI ?
>>>
>>> Let's call it a "virtual" CONFIG_EFI then; I think we really should have
>>> such an option. But yes, if you don't like referring to such a virtual
>>> option, then describing what is meant verbally is certainly going to be
>>> fine.
>>>
>> FWICS, there was an attempt done by Wei in his NUMA series to define CONFIG_EFI.
>> However as this is not yet merged and agreed, I would like not to refer to identifiers
>> not existing in the code, even though most people are familiar with this option from Linux.
>>
>> So by taking an example from Linux I would verbally explain it like that:
>> "To avoid any confusion, please note that EFI macro does not correspond to EFI
>> runtime support and is used when linking a native EFI (i.e. PE/COFF) binary, hence its
>
> "EFI runtime support" can be mistakenly associated to EFI runtime
> services (which BTW not supported on Arm). So I would suggest to
> s/runtime/boot/.
Or simply just "EFI support"?
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
2022-03-30 13:36 ` Jan Beulich
@ 2022-03-30 13:37 ` Julien Grall
0 siblings, 0 replies; 28+ messages in thread
From: Julien Grall @ 2022-03-30 13:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Roger Pau Monné,
xen-devel, Michal Orzel
On 30/03/2022 14:36, Jan Beulich wrote:
> On 30.03.2022 15:30, Julien Grall wrote:
>> Hi,
>>
>> On 30/03/2022 14:24, Michal Orzel wrote:
>>>
>>>
>>> On 30.03.2022 14:53, Jan Beulich wrote:
>>>> On 30.03.2022 14:13, Michal Orzel wrote:
>>>>> On 30.03.2022 13:57, Jan Beulich wrote:
>>>>>> On 30.03.2022 13:04, Michal Orzel wrote:
>>>>>>> On 30.03.2022 12:42, Jan Beulich wrote:
>>>>>>>> On 30.03.2022 12:32, Julien Grall wrote:
>>>>>>>>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI.
>>>>>>>>> That said, it would possibly make more difficult to associate the flag
>>>>>>>>> with "linking an EFI binary".
>>>>>>>>
>>>>>>>> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
>>>>>>>>
>>>>>>>>> I think some documentaion about the define EFI would be help so there
>>>>>>>>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to
>>>>>>>>> put it. Maybe at the top of the header?
>>>>>>>>
>>>>>>>> That's perhaps the best place, yes.
>>>>>>>>
>>>>>>> In this case how about the following comment at the top of xen.lds.h:
>>>>>>>
>>>>>>> "To avoid any confusion about EFI macro used in this header vs EFI support,
>>>>>>> the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
>>>>>>> the latter means support for generating EFI binary.
>>>>>>
>>>>>> No, that's the case on Arm only. As Julien suggested, it is perhaps best
>>>>>> to explain the difference between EFI and CONFIG_EFI, without going into
>>>>>> arch specifics.
>>>>> Could you please tell me what you are reffering to as there is no such identifier
>>>>> in Xen (as opposed to Linux) like CONFIG_EFI ?
>>>>
>>>> Let's call it a "virtual" CONFIG_EFI then; I think we really should have
>>>> such an option. But yes, if you don't like referring to such a virtual
>>>> option, then describing what is meant verbally is certainly going to be
>>>> fine.
>>>>
>>> FWICS, there was an attempt done by Wei in his NUMA series to define CONFIG_EFI.
>>> However as this is not yet merged and agreed, I would like not to refer to identifiers
>>> not existing in the code, even though most people are familiar with this option from Linux.
>>>
>>> So by taking an example from Linux I would verbally explain it like that:
>>> "To avoid any confusion, please note that EFI macro does not correspond to EFI
>>> runtime support and is used when linking a native EFI (i.e. PE/COFF) binary, hence its
>>
>> "EFI runtime support" can be mistakenly associated to EFI runtime
>> services (which BTW not supported on Arm). So I would suggest to
>> s/runtime/boot/.
>
> Or simply just "EFI support"?
That works for me.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-03-30 13:37 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-22 8:02 [PATCH v2 0/2] xen: Linker scripts synchronization Michal Orzel
2022-03-22 8:02 ` [PATCH v2 1/2] xen: Introduce a header to store common linker scripts content Michal Orzel
2022-03-29 9:06 ` Jan Beulich
2022-03-22 8:02 ` [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros Michal Orzel
2022-03-29 9:22 ` Jan Beulich
2022-03-29 9:37 ` Michal Orzel
2022-03-29 10:31 ` Jan Beulich
2022-03-29 9:54 ` Julien Grall
2022-03-29 10:12 ` Michal Orzel
2022-03-29 10:54 ` Julien Grall
2022-03-29 11:09 ` Michal Orzel
2022-03-29 11:42 ` Jan Beulich
2022-03-30 10:32 ` Julien Grall
2022-03-30 10:42 ` Jan Beulich
2022-03-30 11:04 ` Michal Orzel
2022-03-30 11:57 ` Jan Beulich
2022-03-30 12:13 ` Michal Orzel
2022-03-30 12:53 ` Jan Beulich
2022-03-30 13:24 ` Michal Orzel
2022-03-30 13:27 ` Jan Beulich
2022-03-30 13:30 ` Julien Grall
2022-03-30 13:36 ` Jan Beulich
2022-03-30 13:37 ` Julien Grall
2022-03-29 10:37 ` Jan Beulich
2022-03-29 11:07 ` Julien Grall
2022-03-29 11:38 ` Jan Beulich
2022-03-28 10:31 ` [PATCH v2 0/2] xen: Linker scripts synchronization Michal Orzel
2022-03-28 11:21 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.