[Xen-devel] [PATCH 0/4] x86: build with llvm 8 linker

[Xen-devel] [PATCH 0/4] x86: build with llvm 8 linker

[Roger Pau Monne]

Hello,

Current hypervisor code/build produces an invalid binary when linked
with lld 8 (llvm linker). This is because lld 8 places orphaned sections
at the beginning of the output file, thus displacing the multiboot
headers.

In order to build a correct image with lld 8 make sure there are no
orphaned sections, and also add a build-time check to assert the
multiboot headers are present between the offset boundary given in the
specification.

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

 xen/arch/x86/Makefile      |  3 +++
 xen/arch/x86/xen.lds.S     | 14 +++++++++-----
 xen/common/decompress.h    |  2 ++
 xen/common/xz/dec_bcj.c    |  6 +++---
 xen/common/xz/dec_stream.c |  2 +-
 5 files changed, 18 insertions(+), 9 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 1/4] xz: use initconst for hypervisor build

[Roger Pau Monne]

Or else clang adds a .init.rodata.cst8 section to the resulting object
file, which is not handled by the Xen linker script and can end up
before the text section which contains the headers, thus resulting in
a not usable binary.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>
---
 xen/common/decompress.h    | 2 ++
 xen/common/xz/dec_bcj.c    | 6 +++---
 xen/common/xz/dec_stream.c | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/common/decompress.h b/xen/common/decompress.h
index 647b7b1e83..4a429bca12 100644
--- a/xen/common/decompress.h
+++ b/xen/common/decompress.h
@@ -10,6 +10,7 @@
 #define STATIC
 #define INIT __init
 #define INITDATA __initdata
+#define INITCONST __initconst
 
 #define malloc xmalloc_bytes
 #define free xfree
@@ -22,6 +23,7 @@
 #define STATIC static
 #define INIT
 #define INITDATA
+#define INITCONST
 
 #define large_malloc malloc
 #define large_free free
diff --git a/xen/common/xz/dec_bcj.c b/xen/common/xz/dec_bcj.c
index 86c1192199..0a9a45de2b 100644
--- a/xen/common/xz/dec_bcj.c
+++ b/xen/common/xz/dec_bcj.c
@@ -87,10 +87,10 @@ static inline int INIT bcj_x86_test_msbyte(uint8_t b)
 
 static size_t INIT bcj_x86(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 {
-	static const bool_t mask_to_allowed_status[8]
+	static const bool_t INITCONST mask_to_allowed_status[8]
 		= { true, true, true, false, true, false, false, false };
 
-	static const uint8_t mask_to_bit_num[8] = { 0, 1, 2, 2, 3, 3, 3, 3 };
+	static const uint8_t INITCONST mask_to_bit_num[8] = { 0, 1, 2, 2, 3, 3, 3, 3 };
 
 	size_t i;
 	size_t prev_pos = (size_t)-1;
@@ -180,7 +180,7 @@ static size_t INIT bcj_powerpc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 #ifdef XZ_DEC_IA64
 static size_t INIT bcj_ia64(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
 {
-	static const uint8_t branch_table[32] = {
+	static const uint8_t INITCONST branch_table[32] = {
 		0, 0, 0, 0, 0, 0, 0, 0,
 		0, 0, 0, 0, 0, 0, 0, 0,
 		4, 4, 6, 6, 0, 0, 7, 7,
diff --git a/xen/common/xz/dec_stream.c b/xen/common/xz/dec_stream.c
index b8b566307c..61eb2ffb55 100644
--- a/xen/common/xz/dec_stream.c
+++ b/xen/common/xz/dec_stream.c
@@ -138,7 +138,7 @@ struct xz_dec {
 
 #ifdef XZ_DEC_ANY_CHECK
 /* Sizes of the Check field with different Check IDs */
-static const uint8_t check_sizes[16] = {
+static const uint8_t INITCONST check_sizes[16] = {
 	0,
 	4, 4, 4,
 	8, 8, 8,
-- 
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 1/4] xz: use initconst for hypervisor build

[Andrew Cooper]

On 19/06/2019 12:02, Roger Pau Monne wrote:
> Or else clang adds a .init.rodata.cst8 section to the resulting object
> file, which is not handled by the Xen linker script and can end up
> before the text section which contains the headers, thus resulting in
> a not usable binary.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I think Clang is actually adding a .rodata.cst8 section, and the bulk
objcopy turns it into .init.rodata.cst8.

This is a good change so Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com> (subject to some clarity over the exact
section), but I also think this is needed as well:

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index cb42dc8..4f23059 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -206,8 +206,7 @@ SECTIONS
 #endif
 
        *(.init.rodata)
-       *(.init.rodata.rel)
-       *(.init.rodata.str*)
+       *(.init.rodata.*)
 
        . = ALIGN(POINTER_ALIGN);
        __setup_start = .;
@@ -221,8 +220,7 @@ SECTIONS
        __initcall_end = .;
 
        *(.init.data)
-       *(.init.data.rel)
-       *(.init.data.rel.*)
+       *(.init.data.*)
        . = ALIGN(4);
        __trampoline_rel_start = .;
        *(.trampoline_rel)


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

Re: [Xen-devel] [PATCH 1/4] xz: use initconst for hypervisor build

[Jan Beulich]

>>> On 19.06.19 at 13:09, <andrew.cooper3@citrix.com> wrote:
> On 19/06/2019 12:02, Roger Pau Monne wrote:
>> Or else clang adds a .init.rodata.cst8 section to the resulting object
>> file, which is not handled by the Xen linker script and can end up
>> before the text section which contains the headers, thus resulting in
>> a not usable binary.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I think Clang is actually adding a .rodata.cst8 section, and the bulk
> objcopy turns it into .init.rodata.cst8.
> 
> This is a good change so Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com> (subject to some clarity over the exact
> section), but I also think this is needed as well:
> 
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index cb42dc8..4f23059 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -206,8 +206,7 @@ SECTIONS
>  #endif
>  
>         *(.init.rodata)
> -       *(.init.rodata.rel)
> -       *(.init.rodata.str*)
> +       *(.init.rodata.*)
>  
>         . = ALIGN(POINTER_ALIGN);
>         __setup_start = .;
> @@ -221,8 +220,7 @@ SECTIONS
>         __initcall_end = .;
>  
>         *(.init.data)
> -       *(.init.data.rel)
> -       *(.init.data.rel.*)
> +       *(.init.data.*)
>         . = ALIGN(4);
>         __trampoline_rel_start = .;
>         *(.trampoline_rel)

And the same thing then also for Arm.

Jan


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

Re: [Xen-devel] [PATCH 1/4] xz: use initconst for hypervisor build

[Jan Beulich]

>>> On 19.06.19 at 13:02, <roger.pau@citrix.com> wrote:
> Or else clang adds a .init.rodata.cst8 section to the resulting object
> file, which is not handled by the Xen linker script and can end up
> before the text section which contains the headers, thus resulting in
> a not usable binary.

To be honest I'd prefer if we went with just the change suggested
by Andrew, getting the linker script back in line with
SPECIAL_DATA_SECTIONS. The static const items in the
decompressors were left un-annotated intentionally, since the
.rodata.* thingies want/need taking care of anyway. After all you
won't (I hope) suggest also annotating the various string literals.

Jan



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

Re: [Xen-devel] [PATCH 1/4] xz: use initconst for hypervisor build

[Roger Pau Monné]

On Wed, Jun 19, 2019 at 05:50:52AM -0600, Jan Beulich wrote:
> >>> On 19.06.19 at 13:02, <roger.pau@citrix.com> wrote:
> > Or else clang adds a .init.rodata.cst8 section to the resulting object
> > file, which is not handled by the Xen linker script and can end up
> > before the text section which contains the headers, thus resulting in
> > a not usable binary.
> 
> To be honest I'd prefer if we went with just the change suggested
> by Andrew, getting the linker script back in line with
> SPECIAL_DATA_SECTIONS. The static const items in the
> decompressors were left un-annotated intentionally, since the
> .rodata.* thingies want/need taking care of anyway. After all you
> won't (I hope) suggest also annotating the various string literals.

OK, I think regardless of the rest of the lld 8 series it is worth
sending the linker script change in order to prevent having this
orphaned section.

Andrew, since you where the one to propose it, could you please send a
formal patch?

Thanks, Roger.

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

[Xen-devel] [PATCH 2/4] 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>
---
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

Re: [Xen-devel] [PATCH 2/4] x86/linker: use DECL_SECTION uniformly

[Andrew Cooper]

On 19/06/2019 12:02, Roger Pau Monne wrote:
> 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>

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

[Xen-devel] [PATCH 3/4] 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, or else the linker might place .reloc before the .text
section.

Note that 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>
---
 xen/arch/x86/xen.lds.S | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 98a99444c2..82103ef1da 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -175,6 +175,14 @@ SECTIONS
   } :text
 #endif
 #endif
+
+#ifdef XEN_BUILD_EFI
+  . = ALIGN(4);
+  DECL_SECTION(.reloc) {
+    *(.reloc)
+  } :text
+#endif
+
   _erodata = .;
 
   . = ALIGN(SECTION_ALIGN);
@@ -297,10 +305,6 @@ SECTIONS
   __2M_rwdata_end = .;
 
 #ifdef EFI
-  . = ALIGN(4);
-  DECL_SECTION(.reloc) {
-    *(.reloc)
-  } :text
   /* Trick the linker into setting the image size to exactly 16Mb. */
   . = ALIGN(__section_alignment__);
   DECL_SECTION(.pad) {
-- 
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 3/4] x86/linker: add a reloc section to ELF binary

[Andrew Cooper]

On 19/06/2019 12:02, Roger Pau Monne wrote:
> If the hypervisor has been built with EFI support (ie: multiboot2).

Seeing as this continues the sentence from the subject, it should start
without a capital.  Otherwise the result is werd to read.

> This allows to position the .reloc section correctly in the output
> binary, or else the linker might place .reloc before the .text
> section.

Really?  How can this be a legitimate transformation for the linker to make?

>
> Note that 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.

The content of .relocs is transformed via mkreloc to become
__base_relocs_{start,end} and shouldn't exist into the final linked
image AFAICT.

Since the MB1/MB2 builds aren't relocatable, I think we might be able to
get away with simply excluding them in the non-EFI build.

~Andrew

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

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

[Jan Beulich]

>>> On 19.06.19 at 13:20, <andrew.cooper3@citrix.com> wrote:
> On 19/06/2019 12:02, Roger Pau Monne wrote:
>> Note that 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.
> 
> The content of .relocs is transformed via mkreloc to become
> __base_relocs_{start,end} and shouldn't exist into the final linked
> image AFAICT.

The labels are referenced from code, and hence ...

> Since the MB1/MB2 builds aren't relocatable, I think we might be able to
> get away with simply excluding them in the non-EFI build.

... this won't be possible without further trickery, I'm afraid.

Jan



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

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

[Roger Pau Monné]

On Wed, Jun 19, 2019 at 12:20:40PM +0100, Andrew Cooper wrote:
> On 19/06/2019 12:02, Roger Pau Monne wrote:
> > If the hypervisor has been built with EFI support (ie: multiboot2).
> 
> Seeing as this continues the sentence from the subject, it should start
> without a capital.  Otherwise the result is werd to read.
> 
> > This allows to position the .reloc section correctly in the output
> > binary, or else the linker might place .reloc before the .text
> > section.
> 
> Really?  How can this be a legitimate transformation for the linker to make?

I've already submitted a bug report:

https://bugs.llvm.org/show_bug.cgi?id=42327

GNU ld behaviour is to place orphaned sections at the end.

> >
> > Note that 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.
> 
> The content of .relocs is transformed via mkreloc to become
> __base_relocs_{start,end} and shouldn't exist into the final linked
> image AFAICT.

__base_relocs_{start/end} is actually what's contained in the .relocs
section, or at least that was mny impression based on the contents of
xen/arch/x86/efi/relocs-dummy.S

> Since the MB1/MB2 builds aren't relocatable, I think we might be able to
> get away with simply excluding them in the non-EFI build.

Hm, OK. I'm slightly loss then. I've taken a look at the history of
xen/arch/x86/efi/relocs-dummy.S and it's not clear to me why such a
dummy file was added. My guess is that it's done in order to prevent
missing symbols errors. If that's the case I guess the code that makes
use of such symbols can be guarded, and the dummy file removed?

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 19.06.19 at 16:30, <roger.pau@citrix.com> wrote:
> On Wed, Jun 19, 2019 at 12:20:40PM +0100, Andrew Cooper wrote:
>> Since the MB1/MB2 builds aren't relocatable, I think we might be able to
>> get away with simply excluding them in the non-EFI build.
> 
> Hm, OK. I'm slightly loss then. I've taken a look at the history of
> xen/arch/x86/efi/relocs-dummy.S and it's not clear to me why such a
> dummy file was added. My guess is that it's done in order to prevent
> missing symbols errors. If that's the case I guess the code that makes
> use of such symbols can be guarded, and the dummy file removed?

Missing symbols errors - yes. But how would you remove the dummy
one, which is a place holder in stage 1 linking for what will be real
data in stages 2 and 3? I don't think we should ignore unresolved
symbols in stage 1.

Jan



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

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

[Jan Beulich]

>>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
> section.
> 
> Note that 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.

While this may be fine for ELF, I'm afraid it would be calling for
subtle issues with xen.efi (i.e. the PE binary): There a .reloc
section is generally expected to come after "normal" data
sections.

On the whole, seeing all these adjustments for a linker behavior
I'm tempted to call buggy, I'm not sure whether we shouldn't
instead declare this linker version as unusable with Xen. I can't
imagine that they're going to leave it as it is right now.

Jan



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

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

[Roger Pau Monné]

On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
> > section.
> > 
> > Note that 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.
> 
> While this may be fine for ELF, I'm afraid it would be calling for
> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
> section is generally expected to come after "normal" data
> sections.

OK, would you like me to leave the .reloc section at the previous
position for EFI builds then?

Or do we prefer to leave .reloc orphaned in the ELF build?

> On the whole, seeing all these adjustments for a linker behavior
> I'm tempted to call buggy, I'm not sure whether we shouldn't
> instead declare this linker version as unusable with Xen. I can't
> imagine that they're going to leave it as it is right now.

I've already submitted a bug report, let's see if this can be fixed
and backported to 8. It's a shame because previous versions of lld
worked just fine, and I would consider this a lld regression.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
> On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
>> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
>> > section.
>> > 
>> > Note that 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.
>> 
>> While this may be fine for ELF, I'm afraid it would be calling for
>> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
>> section is generally expected to come after "normal" data
>> sections.
> 
> OK, would you like me to leave the .reloc section at the previous
> position for EFI builds then?

Well, this part is a requirement, not a question of me liking you
to do so.

> Or do we prefer to leave .reloc orphaned in the ELF build?

Daniel might have an opinion here with his plans to actually
add relocations there in the non-linker-generated-PE build. I
don't have a strong opinion either way, as long as the
current method of building gets left as is (or even simplified).

Also a remark regarding the title - in my builds there already is
a .reloc section in the ELF images, so "add" doesn't really seem
correct to me. It sits right after .rodata, and I would it doesn't
get folded into there because - for some reason - .rodata is
actually marked writable.

Jan



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

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

[Roger Pau Monné]

On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
> >>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
> > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
> >> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
> >> > section.
> >> > 
> >> > Note that 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.
> >> 
> >> While this may be fine for ELF, I'm afraid it would be calling for
> >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
> >> section is generally expected to come after "normal" data
> >> sections.
> > 
> > OK, would you like me to leave the .reloc section at the previous
> > position for EFI builds then?
> 
> Well, this part is a requirement, not a question of me liking you
> to do so.
> 
> > Or do we prefer to leave .reloc orphaned in the ELF build?
> 
> Daniel might have an opinion here with his plans to actually
> add relocations there in the non-linker-generated-PE build. I
> don't have a strong opinion either way, as long as the
> current method of building gets left as is (or even simplified).
> 
> Also a remark regarding the title - in my builds there already is
> a .reloc section in the ELF images, so "add" doesn't really seem
> correct to me. It sits right after .rodata, and I would it doesn't
> get folded into there because - for some reason - .rodata is
> actually marked writable.

AFAICT .rodata is marked writable because it contains .data.param and
.data.rel.ro. I'm unsure why we need .data.rel.ro, I would assume that
once the final binary has been linked .data.rel.ro would be empty,
since there's no run time linking or relocation as Xen is a standalone
binary.

Regarding .data.param it should be renamed to .rodata.param, and I
should take a look at why it's marked as 'WA' instead of 'A'.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 21.06.19 at 13:46, <roger.pau@citrix.com> wrote:
> On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
>> >>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
>> > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
>> >> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
>> >> > section.
>> >> > 
>> >> > Note that 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.
>> >> 
>> >> While this may be fine for ELF, I'm afraid it would be calling for
>> >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
>> >> section is generally expected to come after "normal" data
>> >> sections.
>> > 
>> > OK, would you like me to leave the .reloc section at the previous
>> > position for EFI builds then?
>> 
>> Well, this part is a requirement, not a question of me liking you
>> to do so.
>> 
>> > Or do we prefer to leave .reloc orphaned in the ELF build?
>> 
>> Daniel might have an opinion here with his plans to actually
>> add relocations there in the non-linker-generated-PE build. I
>> don't have a strong opinion either way, as long as the
>> current method of building gets left as is (or even simplified).
>> 
>> Also a remark regarding the title - in my builds there already is
>> a .reloc section in the ELF images, so "add" doesn't really seem
>> correct to me. It sits right after .rodata, and I would it doesn't
>> get folded into there because - for some reason - .rodata is
>> actually marked writable.
> 
> AFAICT .rodata is marked writable because it contains .data.param and
> .data.rel.ro. I'm unsure why we need .data.rel.ro, I would assume that
> once the final binary has been linked .data.rel.ro would be empty,
> since there's no run time linking or relocation as Xen is a standalone
> binary.

No - contents of sections don't get moved to other sections while
linking, unless instructed so by the linker script. In all the
relocatable objects there's going to be .data.rel.ro, and hence the
linker script has to put them somewhere (or leave it to default
placement by the linker).

Hmm, thinking about it - are you perhaps mixing up .data.rel /
.data.rel.ro with .rel.data / .rela.data?

> Regarding .data.param it should be renamed to .rodata.param, and I
> should take a look at why it's marked as 'WA' instead of 'A'.

Well, there's no "const" on the structure instantiations.

Jan



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

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

[Roger Pau Monné]

On Fri, Jun 21, 2019 at 06:07:25AM -0600, Jan Beulich wrote:
> >>> On 21.06.19 at 13:46, <roger.pau@citrix.com> wrote:
> > On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
> >> >>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
> >> > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
> >> >> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
> >> >> > section.
> >> >> > 
> >> >> > Note that 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.
> >> >> 
> >> >> While this may be fine for ELF, I'm afraid it would be calling for
> >> >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
> >> >> section is generally expected to come after "normal" data
> >> >> sections.
> >> > 
> >> > OK, would you like me to leave the .reloc section at the previous
> >> > position for EFI builds then?
> >> 
> >> Well, this part is a requirement, not a question of me liking you
> >> to do so.
> >> 
> >> > Or do we prefer to leave .reloc orphaned in the ELF build?
> >> 
> >> Daniel might have an opinion here with his plans to actually
> >> add relocations there in the non-linker-generated-PE build. I
> >> don't have a strong opinion either way, as long as the
> >> current method of building gets left as is (or even simplified).
> >> 
> >> Also a remark regarding the title - in my builds there already is
> >> a .reloc section in the ELF images, so "add" doesn't really seem
> >> correct to me. It sits right after .rodata, and I would it doesn't
> >> get folded into there because - for some reason - .rodata is
> >> actually marked writable.
> > 
> > AFAICT .rodata is marked writable because it contains .data.param and
> > .data.rel.ro. I'm unsure why we need .data.rel.ro, I would assume that
> > once the final binary has been linked .data.rel.ro would be empty,
> > since there's no run time linking or relocation as Xen is a standalone
> > binary.
> 
> No - contents of sections don't get moved to other sections while
> linking, unless instructed so by the linker script. In all the
> relocatable objects there's going to be .data.rel.ro, and hence the
> linker script has to put them somewhere (or leave it to default
> placement by the linker).

Right, so as long as we place .data.rel.ro inside of .rodata the
resulting section is always going to be writable, due to the input
.data.rel.ro being writable.

> Hmm, thinking about it - are you perhaps mixing up .data.rel /
> .data.rel.ro with .rel.data / .rela.data?

Yes, I think I messed up. As you say, contents of sections don't move
unless explicitly done by the linker script.

> > Regarding .data.param it should be renamed to .rodata.param, and I
> > should take a look at why it's marked as 'WA' instead of 'A'.
> 
> Well, there's no "const" on the structure instantiations.

I think there is indeed a const on the instantiation, see __param
macro in init.h which is used in the declarations done with
__rtparam.

Thanks, Roger.

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

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

[Daniel Kiper]

On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
> >>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
> > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
> >> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
> >> > section.
> >> >
> >> > Note that 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.
> >>
> >> While this may be fine for ELF, I'm afraid it would be calling for
> >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
> >> section is generally expected to come after "normal" data
> >> sections.
> >
> > OK, would you like me to leave the .reloc section at the previous
> > position for EFI builds then?
>
> Well, this part is a requirement, not a question of me liking you
> to do so.
>
> > Or do we prefer to leave .reloc orphaned in the ELF build?
>
> Daniel might have an opinion here with his plans to actually
> add relocations there in the non-linker-generated-PE build. I
> don't have a strong opinion either way, as long as the
> current method of building gets left as is (or even simplified).

I would not drop .reloc section from xen-syms because it can be useful
for "manual" EFI image relocs generation. However, I am not strongly
tied to it. If you wish to drop it go ahead. I can readd it latter if
I get back to my new PE build work.

Daniel

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

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

[Roger Pau Monné]

On Mon, Jun 24, 2019 at 01:24:02PM +0200, Daniel Kiper wrote:
> On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
> > >>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
> > > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
> > >> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
> > >> > section.
> > >> >
> > >> > Note that 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.
> > >>
> > >> While this may be fine for ELF, I'm afraid it would be calling for
> > >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
> > >> section is generally expected to come after "normal" data
> > >> sections.
> > >
> > > OK, would you like me to leave the .reloc section at the previous
> > > position for EFI builds then?
> >
> > Well, this part is a requirement, not a question of me liking you
> > to do so.
> >
> > > Or do we prefer to leave .reloc orphaned in the ELF build?
> >
> > Daniel might have an opinion here with his plans to actually
> > add relocations there in the non-linker-generated-PE build. I
> > don't have a strong opinion either way, as long as the
> > current method of building gets left as is (or even simplified).
> 
> I would not drop .reloc section from xen-syms because it can be useful
> for "manual" EFI image relocs generation. However, I am not strongly
> tied to it. If you wish to drop it go ahead. I can readd it latter if
> I get back to my new PE build work.

Do you mean that the dummy .reloc section added to non-PE builds can
be dropped? (ie: remove xen/arch/x86/efi/relocs-dummy.S from the build)

I'm slightly lost, .reloc begin a section that's explicitly added to
non-PE builds by relocs-dummy.S I assumed it was needed for some
reason.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 25.06.19 at 10:10, <roger.pau@citrix.com> wrote:
> On Mon, Jun 24, 2019 at 01:24:02PM +0200, Daniel Kiper wrote:
>> On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
>> > >>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
>> > > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
>> > >> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
>> > >> > section.
>> > >> >
>> > >> > Note that 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.
>> > >>
>> > >> While this may be fine for ELF, I'm afraid it would be calling for
>> > >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
>> > >> section is generally expected to come after "normal" data
>> > >> sections.
>> > >
>> > > OK, would you like me to leave the .reloc section at the previous
>> > > position for EFI builds then?
>> >
>> > Well, this part is a requirement, not a question of me liking you
>> > to do so.
>> >
>> > > Or do we prefer to leave .reloc orphaned in the ELF build?
>> >
>> > Daniel might have an opinion here with his plans to actually
>> > add relocations there in the non-linker-generated-PE build. I
>> > don't have a strong opinion either way, as long as the
>> > current method of building gets left as is (or even simplified).
>> 
>> I would not drop .reloc section from xen-syms because it can be useful
>> for "manual" EFI image relocs generation. However, I am not strongly
>> tied to it. If you wish to drop it go ahead. I can readd it latter if
>> I get back to my new PE build work.
> 
> Do you mean that the dummy .reloc section added to non-PE builds can
> be dropped? (ie: remove xen/arch/x86/efi/relocs-dummy.S from the build)

Given my earlier reply it's not clear to me what you mean by "remove"
here. As a result ...

> I'm slightly lost, .reloc begin a section that's explicitly added to
> non-PE builds by relocs-dummy.S I assumed it was needed for some
> reason.

... it's also not clear what exactly you mean here, and hence whether
there's any reason needed beyond the reference to the two bounding
symbols by efi_arch_relocate_image().

Jan



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

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

[Roger Pau Monné]

On Tue, Jun 25, 2019 at 03:18:14AM -0600, Jan Beulich wrote:
> >>> On 25.06.19 at 10:10, <roger.pau@citrix.com> wrote:
> > On Mon, Jun 24, 2019 at 01:24:02PM +0200, Daniel Kiper wrote:
> >> On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
> >> > >>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
> >> > > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
> >> > >> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
> >> > >> > section.
> >> > >> >
> >> > >> > Note that 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.
> >> > >>
> >> > >> While this may be fine for ELF, I'm afraid it would be calling for
> >> > >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
> >> > >> section is generally expected to come after "normal" data
> >> > >> sections.
> >> > >
> >> > > OK, would you like me to leave the .reloc section at the previous
> >> > > position for EFI builds then?
> >> >
> >> > Well, this part is a requirement, not a question of me liking you
> >> > to do so.
> >> >
> >> > > Or do we prefer to leave .reloc orphaned in the ELF build?
> >> >
> >> > Daniel might have an opinion here with his plans to actually
> >> > add relocations there in the non-linker-generated-PE build. I
> >> > don't have a strong opinion either way, as long as the
> >> > current method of building gets left as is (or even simplified).
> >> 
> >> I would not drop .reloc section from xen-syms because it can be useful
> >> for "manual" EFI image relocs generation. However, I am not strongly
> >> tied to it. If you wish to drop it go ahead. I can readd it latter if
> >> I get back to my new PE build work.
> > 
> > Do you mean that the dummy .reloc section added to non-PE builds can
> > be dropped? (ie: remove xen/arch/x86/efi/relocs-dummy.S from the build)
> 
> Given my earlier reply it's not clear to me what you mean by "remove"
> here. As a result ...
> 
> > I'm slightly lost, .reloc begin a section that's explicitly added to
> > non-PE builds by relocs-dummy.S I assumed it was needed for some
> > reason.
> 
> ... it's also not clear what exactly you mean here, and hence whether
> there's any reason needed beyond the reference to the two bounding
> symbols by efi_arch_relocate_image().

Sorry for not being clear. By remove I mean `git rm
xen/arch/x86/efi/relocs-dummy.S` and fix the build, like the diff
appended below.

Is there any reason we should keep the dummy .reloc in the ELF
output?

Thanks, Roger.
---8<---
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 4bc0a196e9..5849604766 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -11,6 +11,6 @@ $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
 $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary)
 
 obj-y := stub.o
-obj-$(XEN_BUILD_EFI) := $(EFIOBJ) relocs-dummy.o
+obj-$(XEN_BUILD_EFI) := $(EFIOBJ)
 extra-$(XEN_BUILD_EFI) += buildid.o
 nocov-$(XEN_BUILD_EFI) += stub.o
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7a13a30bc0..2cf440e2ae 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -39,6 +39,7 @@ extern const intpte_t __page_tables_start[], __page_tables_end[];
 #define PE_BASE_RELOC_HIGHLOW  3
 #define PE_BASE_RELOC_DIR64   10
 
+#ifdef XEN_BUILD_PE
 extern const struct pe_base_relocs {
     u32 rva;
     u32 size;
@@ -97,6 +98,12 @@ static void __init efi_arch_relocate_image(unsigned long delta)
         base_relocs = (const void *)(base_relocs->entries + i + (i & 1));
     }
 }
+#else /* !XEN_BUILD_PE */
+static void __init efi_arch_relocate_image(unsigned long delta)
+{
+    ASSERT_UNREACHABLE();
+}
+#endif /* XEN_BUILD_PE */
 
 extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
 extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
diff --git a/xen/arch/x86/efi/relocs-dummy.S b/xen/arch/x86/efi/relocs-dummy.S
deleted file mode 100644
index d928a82d53..0000000000
--- a/xen/arch/x86/efi/relocs-dummy.S
+++ /dev/null
@@ -1,11 +0,0 @@
-
-	.section .reloc, "a", @progbits
-	.balign 4
-GLOBAL(__base_relocs_start)
-	.long 0
-	.long 8
-GLOBAL(__base_relocs_end)
-
-	.globl VIRT_START, ALT_START
-	.equ VIRT_START, XEN_VIRT_START
-	.equ ALT_START, XEN_VIRT_END


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

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

[Roger Pau Monné]

On Tue, Jun 25, 2019 at 01:08:49PM +0200, Roger Pau Monné wrote:
> On Tue, Jun 25, 2019 at 03:18:14AM -0600, Jan Beulich wrote:
> > >>> On 25.06.19 at 10:10, <roger.pau@citrix.com> wrote:
> > > On Mon, Jun 24, 2019 at 01:24:02PM +0200, Daniel Kiper wrote:
> > >> On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
> > >> > >>> On 19.06.19 at 17:06, <roger.pau@citrix.com> wrote:
> > >> > > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
> > >> > >> >>> On 19.06.19 at 13:02, <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, or else the linker might place .reloc before the .text
> > >> > >> > section.
> > >> > >> >
> > >> > >> > Note that 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.
> > >> > >>
> > >> > >> While this may be fine for ELF, I'm afraid it would be calling for
> > >> > >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
> > >> > >> section is generally expected to come after "normal" data
> > >> > >> sections.
> > >> > >
> > >> > > OK, would you like me to leave the .reloc section at the previous
> > >> > > position for EFI builds then?
> > >> >
> > >> > Well, this part is a requirement, not a question of me liking you
> > >> > to do so.
> > >> >
> > >> > > Or do we prefer to leave .reloc orphaned in the ELF build?
> > >> >
> > >> > Daniel might have an opinion here with his plans to actually
> > >> > add relocations there in the non-linker-generated-PE build. I
> > >> > don't have a strong opinion either way, as long as the
> > >> > current method of building gets left as is (or even simplified).
> > >> 
> > >> I would not drop .reloc section from xen-syms because it can be useful
> > >> for "manual" EFI image relocs generation. However, I am not strongly
> > >> tied to it. If you wish to drop it go ahead. I can readd it latter if
> > >> I get back to my new PE build work.
> > > 
> > > Do you mean that the dummy .reloc section added to non-PE builds can
> > > be dropped? (ie: remove xen/arch/x86/efi/relocs-dummy.S from the build)
> > 
> > Given my earlier reply it's not clear to me what you mean by "remove"
> > here. As a result ...
> > 
> > > I'm slightly lost, .reloc begin a section that's explicitly added to
> > > non-PE builds by relocs-dummy.S I assumed it was needed for some
> > > reason.
> > 
> > ... it's also not clear what exactly you mean here, and hence whether
> > there's any reason needed beyond the reference to the two bounding
> > symbols by efi_arch_relocate_image().
> 
> Sorry for not being clear. By remove I mean `git rm
> xen/arch/x86/efi/relocs-dummy.S` and fix the build, like the diff
> appended below.

The chunk below will not work because relocs-dummy is also needed
by the EFI build. I'm however lost at why this is required, and the
commit message that introduced the file (bf6501a62e) doesn't add any
reasoning.

Is maybe .reloc mandatory for PE format?

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 25.06.19 at 14:48, <roger.pau@citrix.com> wrote:
> On Tue, Jun 25, 2019 at 01:08:49PM +0200, Roger Pau Monné wrote:
>> Sorry for not being clear. By remove I mean `git rm
>> xen/arch/x86/efi/relocs-dummy.S` and fix the build, like the diff
>> appended below.
> 
> The chunk below will not work because relocs-dummy is also needed
> by the EFI build. I'm however lost at why this is required, and the
> commit message that introduced the file (bf6501a62e) doesn't add any
> reasoning.
> 
> Is maybe .reloc mandatory for PE format?

Yes, almost. You _can_ have one without .reloc, but then you're tied
to it loading at the linked address. That's fine with an ordinary boot
loader, but it's not an option when this is to get loaded just like a
normal binary, without knowing at which address it'll be placed.
Remember that the EFI boot environment runs in (pseudo)physical
mode, i.e. there's a 1:1 mapping between linear and physical
addresses. Therefore there's no way to predict a memory range
that's always going to be available (and that hence xen.efi could be
linked to load at).

Jan


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

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

[Jan Beulich]

>>> On 25.06.19 at 13:08, <roger.pau@citrix.com> wrote:
> Sorry for not being clear. By remove I mean `git rm
> xen/arch/x86/efi/relocs-dummy.S` and fix the build, like the diff
> appended below.
> 
> Is there any reason we should keep the dummy .reloc in the ELF
> output?

Yes, there is. And yes, I was afraid you'd mean this.

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -39,6 +39,7 @@ extern const intpte_t __page_tables_start[], __page_tables_end[];
>  #define PE_BASE_RELOC_HIGHLOW  3
>  #define PE_BASE_RELOC_DIR64   10
>  
> +#ifdef XEN_BUILD_PE

This is an identifier available to Makefiles only. You also can't propagate
it to .c files, as these get compiled just once to produce _both_ PE and
ELF binary. So while what you suggest may build, it'll result in a broken
xen.efi.

Jan



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

[Xen-devel] [PATCH 4/4] 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>
---
 xen/arch/x86/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 8a8d8f060f..9bb3bf6e6c 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -102,6 +102,9 @@ 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) \
 	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
+	# Check for multiboot{1,2} headers
+	od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null
+	od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null
 
 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 4/4] x86: check for multiboot{1, 2} header presence

[Andrew Cooper]

On 19/06/2019 12:02, 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>
> ---
>  xen/arch/x86/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 8a8d8f060f..9bb3bf6e6c 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -102,6 +102,9 @@ 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) \
>  	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
> +	# Check for multiboot{1,2} headers
> +	od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null
> +	od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null

Neat solution.  Is `grep -q` portable ?

~Andrew

>  
>  ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
>  


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

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

[Roger Pau Monné]

On Wed, Jun 19, 2019 at 12:11:43PM +0100, Andrew Cooper wrote:
> On 19/06/2019 12:02, 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>
> > ---
> >  xen/arch/x86/Makefile | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index 8a8d8f060f..9bb3bf6e6c 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -102,6 +102,9 @@ 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) \
> >  	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
> > +	# Check for multiboot{1,2} headers
> > +	od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null
> > +	od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null
> 
> Neat solution.  Is `grep -q` portable ?

It is, but grep -q closes the pipe on the first match, and then od
will return an error. Note sure whether there's a way to workaround
this issue, but I think the above is simple enough.

Thanks, Roger.

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

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

[Andrew Cooper]

On 19/06/2019 12:20, Roger Pau Monné wrote:
> On Wed, Jun 19, 2019 at 12:11:43PM +0100, Andrew Cooper wrote:
>> On 19/06/2019 12:02, 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>
>>> ---
>>>  xen/arch/x86/Makefile | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>>> index 8a8d8f060f..9bb3bf6e6c 100644
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -102,6 +102,9 @@ 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) \
>>>  	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
>>> +	# Check for multiboot{1,2} headers
>>> +	od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null
>>> +	od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null
>> Neat solution.  Is `grep -q` portable ?
> It is, but grep -q closes the pipe on the first match, and then od
> will return an error. Note sure whether there's a way to workaround
> this issue, but I think the above is simple enough.

In which case, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

[Jan Beulich]

>>> On 19.06.19 at 13:02, <roger.pau@citrix.com> 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>
> ---
>  xen/arch/x86/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 8a8d8f060f..9bb3bf6e6c 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -102,6 +102,9 @@ 
> 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) \
>  	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
> +	# Check for multiboot{1,2} headers
> +	od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null
> +	od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null

What's the behavior when a signature is _not _ found? Will
$(TARGET) get deleted (by make)? I don't think it would (as we
don't specific .DELETE_ON_ERROR anywhere), so a subsequent
rebuild may not even execute this rule, and hence may look to be
successful despite it not actually having been.

Jan


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

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

[Roger Pau Monné]

On Wed, Jun 19, 2019 at 07:08:52AM -0600, Jan Beulich wrote:
> >>> On 19.06.19 at 13:02, <roger.pau@citrix.com> 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>
> > ---
> >  xen/arch/x86/Makefile | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index 8a8d8f060f..9bb3bf6e6c 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -102,6 +102,9 @@ 
> > 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) \
> >  	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$$/0x\1/p'`
> > +	# Check for multiboot{1,2} headers
> > +	od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null
> > +	od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null
> 
> What's the behavior when a signature is _not _ found? Will
> $(TARGET) get deleted (by make)? I don't think it would (as we
> don't specific .DELETE_ON_ERROR anywhere), so a subsequent
> rebuild may not even execute this rule, and hence may look to be
> successful despite it not actually having been.

Oh, right. It should be:

od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null || (rm -rf $(TARGET); exit 1)
od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null || (rm -rf $(TARGET); exit 1)

Would you be OK with the above runes?

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 19.06.19 at 16:40, <roger.pau@citrix.com> wrote:
> On Wed, Jun 19, 2019 at 07:08:52AM -0600, Jan Beulich wrote:
>> >>> On 19.06.19 at 13:02, <roger.pau@citrix.com> 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>
>> > ---
>> >  xen/arch/x86/Makefile | 3 +++
>> >  1 file changed, 3 insertions(+)
>> > 
>> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>> > index 8a8d8f060f..9bb3bf6e6c 100644
>> > --- a/xen/arch/x86/Makefile
>> > +++ b/xen/arch/x86/Makefile
>> > @@ -102,6 +102,9 @@ 
>> > 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) \
>> >  	               `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . 
> __2M_rwdata_end$$/0x\1/p'`
>> > +	# Check for multiboot{1,2} headers
>> > +	od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null
>> > +	od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null
>> 
>> What's the behavior when a signature is _not _ found? Will
>> $(TARGET) get deleted (by make)? I don't think it would (as we
>> don't specific .DELETE_ON_ERROR anywhere), so a subsequent
>> rebuild may not even execute this rule, and hence may look to be
>> successful despite it not actually having been.
> 
> Oh, right. It should be:
> 
> od -t x4 -N 8192 $(TARGET) | grep 1badb002 > /dev/null || (rm -rf $(TARGET); exit 1)
> od -t x4 -N 32768 $(TARGET) | grep e85250d6 > /dev/null || (rm -rf $(TARGET); exit 1)
> 
> Would you be OK with the above runes?

Not really, I'm afraid. This still wouldn't cope with an interrupted
build. I think it needs to be made work via an intermediate target,
or some other make trickery.

Jan


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