[Xen-devel] [PATCH v2 0/3] misc improvements

[Xen-devel] [PATCH v2 0/3] misc improvements

[Roger Pau Monne]

Hello,

Following patches are the general improvements that came out of my
attempt to add LLD 8 support.

I've now put on hold the addition of LLD 8 support, since according to
LLD documentation it should mimic GNU ld behaviour, but that's clearly
not the case with orphan sections in linker scripts. Until this is
resolved, either by fixing LLD 8 behavior or updating the documentation,
I don't foresee LLD 8 support in Xen.

Roger Pau Monne (3):
  x86/linker: use DECL_SECTION uniformly
  x86: check for multiboot{1,2} header presence
  x86/linker: add a reloc section to ELF binary

 xen/arch/x86/Makefile  |  7 ++++++-
 xen/arch/x86/xen.lds.S | 12 ++++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.20.1 (Apple Git-117)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 1/3] x86/linker: use DECL_SECTION uniformly

[Roger Pau Monne]

Replace the two open-coded EFI related section declarations with the
usage of DECL_SECTION. This is a preparatory change for also adding a
reloc section to the ELF binary.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 xen/arch/x86/xen.lds.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index cb42dc8fda..98a99444c2 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -298,12 +298,12 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(4);
-  .reloc : {
+  DECL_SECTION(.reloc) {
     *(.reloc)
   } :text
   /* Trick the linker into setting the image size to exactly 16Mb. */
   . = ALIGN(__section_alignment__);
-  .pad : {
+  DECL_SECTION(.pad) {
     . = ALIGN(MB(16));
   } :text
 #endif
-- 
2.20.1 (Apple Git-117)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/3] x86: check for multiboot{1, 2} header presence

[Roger Pau Monne]

After building the hypervisor binary. Note that the check is performed
by searching for the magic header value at the start of the binary.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
---
Changes since v1:
 - Use an intermediate file to perform the header checks.
---
 xen/arch/x86/Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 8a8d8f060f..5c908c49e3 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -100,8 +100,12 @@ syms-warn-dup-y := --warn-dup
 syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
 
 $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
-	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
+	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(@D)/.$(@F) $(XEN_IMG_OFFSET) \
 	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
+	# Check for multiboot{1,2} headers
+	od -t x4 -N 8192 $(@D)/.$(@F) | grep 1badb002 > /dev/null
+	od -t x4 -N 32768 $(@D)/.$(@F) | grep e85250d6 > /dev/null
+	mv $(@D)/.$(@F) $(TARGET)
 
 ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
 
-- 
2.20.1 (Apple Git-117)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] x86: check for multiboot{1, 2} header presence

[Andrew Cooper]

On 21/06/2019 17:38, Roger Pau Monne wrote:
> After building the hypervisor binary. Note that the check is performed
> by searching for the magic header value at the start of the binary.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>

While the change is ok, won't this break the gitlab CI which is
currently using the llvm-8 toolchain?

> ---
> Changes since v1:
>  - Use an intermediate file to perform the header checks.
> ---
>  xen/arch/x86/Makefile | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 8a8d8f060f..5c908c49e3 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -100,8 +100,12 @@ syms-warn-dup-y := --warn-dup
>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>  
>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> -	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
> +	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(@D)/.$(@F) $(XEN_IMG_OFFSET) \
>  	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
> +	# Check for multiboot{1,2} headers
> +	od -t x4 -N 8192 $(@D)/.$(@F) | grep 1badb002 > /dev/null
> +	od -t x4 -N 32768 $(@D)/.$(@F) | grep e85250d6 > /dev/null
> +	mv $(@D)/.$(@F) $(TARGET)

This might be a bit cleaner to read as

$(TARGET): tmp=$(@D)/.$(@F)
$(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32

rather than having $(@D)/.$(@F) spread throughout the rule.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] x86: check for multiboot{1, 2} header presence

[Roger Pau Monné]

On Fri, Jun 21, 2019 at 06:20:54PM +0100, Andrew Cooper wrote:
> On 21/06/2019 17:38, Roger Pau Monne wrote:
> > After building the hypervisor binary. Note that the check is performed
> > by searching for the magic header value at the start of the binary.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> 
> While the change is ok, won't this break the gitlab CI which is
> currently using the llvm-8 toolchain?

Yes, that's right. It's also going to break the FreeBSD Xen build on
osstest (this is not part of the xen- flights however).

ATM the gitlab llvm-8 builds don't seem to correctly pick up the LD
env variable, so they still pass with this change applied.

> > ---
> > Changes since v1:
> >  - Use an intermediate file to perform the header checks.
> > ---
> >  xen/arch/x86/Makefile | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index 8a8d8f060f..5c908c49e3 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -100,8 +100,12 @@ syms-warn-dup-y := --warn-dup
> >  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
> >  
> >  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> > -	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
> > +	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(@D)/.$(@F) $(XEN_IMG_OFFSET) \
> >  	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
> > +	# Check for multiboot{1,2} headers
> > +	od -t x4 -N 8192 $(@D)/.$(@F) | grep 1badb002 > /dev/null
> > +	od -t x4 -N 32768 $(@D)/.$(@F) | grep e85250d6 > /dev/null
> > +	mv $(@D)/.$(@F) $(TARGET)
> 
> This might be a bit cleaner to read as
> 
> $(TARGET): tmp=$(@D)/.$(@F)
> $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> 
> rather than having $(@D)/.$(@F) spread throughout the rule.

Ack, Thanks!

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 3/3] x86/linker: add a reloc section to ELF binary

[Roger Pau Monne]

if the hypervisor has been built with EFI support (ie: multiboot2).
This allows to position the .reloc section correctly in the output
binary.

Note that for the ELF output format the .reloc section is moved before
.bss for two reasons: in order for the resulting binary to not contain
any section with data after .bss, so that the file size can be smaller
than the loaded memory size, and because the data it contains is
read-only, so it belongs with the other sections containing read-only
data.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
---
changes sincce v1:
 - Move the .reloc section position in the output binary only for the
   ELF output format.
---
 xen/arch/x86/xen.lds.S | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 98a99444c2..19aa4332cf 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -175,6 +175,14 @@ SECTIONS
   } :text
 #endif
 #endif
+
+#if defined(XEN_BUILD_EFI) && !defined(EFI)
+  . = ALIGN(4);
+  DECL_SECTION(.reloc) {
+    *(.reloc)
+  } :text
+#endif
+
   _erodata = .;
 
   . = ALIGN(SECTION_ALIGN);
-- 
2.20.1 (Apple Git-117)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/3] x86/linker: add a reloc section to ELF binary

[Jan Beulich]

>>> On 21.06.19 at 18:38, <roger.pau@citrix.com> wrote:
> if the hypervisor has been built with EFI support (ie: multiboot2).
> This allows to position the .reloc section correctly in the output
> binary.

The title still says "add ... to ELF binary", when really such a section
is already there (and in fact that's the problem you're trying to work
around). As mentioned before, if anything you add mention of the
section to the linker script, but not to the ELF binary (afaict).

> Note that for the ELF output format the .reloc section is moved before
> .bss for two reasons: in order for the resulting binary to not contain
> any section with data after .bss, so that the file size can be smaller
> than the loaded memory size,

Sounds like another linker quirk then. I didn't think there was a
requirement for image and file offsets to go in lock step. There is
some correlation iirc, but the sentence may want weakening a little.

> and because the data it contains is
> read-only, so it belongs with the other sections containing read-only
> data.

I can accept this one (as a secondary argument).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel