[Xen-devel] [PATCH v3 1/3] x86/linker: add a reloc section to ELF linker script

[Xen-devel] [PATCH v3 1/3] x86/linker: add a reloc section to ELF linker script

[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 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 since v2:
 - Fix subject to mention the section is added to the linker script.
 - Fix commit message to note .reloc is added together with the rest
   of the read-only sections.

Changes since 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

[Xen-devel] [PATCH v3 2/3] xen/link: handle .init.rodata.cst* sections in the linker script

[Roger Pau Monne]

Note that those sections when not prefixed with .init are already
handled by the more general .rodata.* matching pattern in the .rodata
output section.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 xen/arch/arm/xen.lds.S | 1 +
 xen/arch/x86/xen.lds.S | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index e664c4441a..b636d9f223 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -156,6 +156,7 @@ SECTIONS
        *(.init.rodata)
        *(.init.rodata.rel)
        *(.init.rodata.str*)
+       *(.init.rodata.cst*)
 
        . = ALIGN(POINTER_ALIGN);
        __setup_start = .;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 19aa4332cf..d0c7fbc37b 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -216,6 +216,7 @@ SECTIONS
        *(.init.rodata)
        *(.init.rodata.rel)
        *(.init.rodata.str*)
+       *(.init.rodata.cst*)
 
        . = ALIGN(POINTER_ALIGN);
        __setup_start = .;
-- 
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 v3 3/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 v2:
 - Use a variable to store the intermediate file name.
 - Remove the intermediate file in the clean target.
 - Add intermediate file to gitignore.

Changes since v1:
 - Use an intermediate file to perform the header checks.
---
 .gitignore            | 1 +
 xen/arch/x86/Makefile | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index a77cbff02c..56bcb64837 100644
--- a/.gitignore
+++ b/.gitignore
@@ -278,6 +278,7 @@ tools/xentrace/xentrace
 xen/.banner
 xen/.config
 xen/.config.old
+xen/.xen
 xen/System.map
 xen/arch/x86/asm-macros.i
 xen/arch/x86/boot/mkelf32
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 8a8d8f060f..94e6c9aee3 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -99,9 +99,14 @@ endif
 syms-warn-dup-y := --warn-dup
 syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
 
+$(TARGET): TMP = $(@D)/.$(@F)
 $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
-	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
+	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null
+	od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null
+	mv $(TMP) $(TARGET)
 
 ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
 
@@ -249,7 +254,7 @@ efi/mkreloc: efi/mkreloc.c
 clean::
 	rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32
 	rm -f asm-macros.i $(BASEDIR)/include/asm-x86/asm-macros.*
-	rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d
+	rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d $(BASEDIR)/.xen
 	rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.efi efi/mkreloc
 	rm -f boot/cmdline.S boot/reloc.S boot/*.lnk boot/*.bin
 	rm -f note.o
-- 
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 v3 2/3] xen/link: handle .init.rodata.cst* sections in the linker script

[Andrew Cooper]

On 27/06/2019 10:33, Roger Pau Monne wrote:
> Note that those sections when not prefixed with .init are already
> handled by the more general .rodata.* matching pattern in the .rodata
> output section.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

So, in hindsight, we'll never get .cst in .data, because of the
"constant" in the name, which rules out a lot of my first attemt.

As this does resolve the problem, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>

However ...

> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
> ---
>  xen/arch/arm/xen.lds.S | 1 +
>  xen/arch/x86/xen.lds.S | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index e664c4441a..b636d9f223 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -156,6 +156,7 @@ SECTIONS
>         *(.init.rodata)
>         *(.init.rodata.rel)
>         *(.init.rodata.str*)
> +       *(.init.rodata.cst*)

... .init is just a grouping prefix, so I'd recommend that we treat
.init.rodata in exactly the same way as we treat .rodata, so I'd suggest
turning this into

*(.init.rodata)
*(.init.rodata.*)

to match the regular .rodata.

This can easily be adjusted on commit, to save yet-another-round.

~Andrew

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

Re: [Xen-devel] [PATCH v3 1/3] x86/linker: add a reloc section to ELF linker script

[Andrew Cooper]

On 27/06/2019 10:33, Roger Pau Monne wrote:
> 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 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>

I have to admit that I'm still confused as to why we need this in the
first place.

The ELF build is linked to a fixed virtual address, irrespective of
whether grub loads it via MB1 or MB2 and/or with EFI details.

i.e. the non-EFI build shouldn't have any remaining relocations by the
time it is fully linked.

Or am I missing something?

~Andrew

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

Re: [Xen-devel] [PATCH v3 1/3] x86/linker: add a reloc section to ELF linker script

[Roger Pau Monné]

On Thu, Jun 27, 2019 at 11:59:46AM +0100, Andrew Cooper wrote:
> On 27/06/2019 10:33, Roger Pau Monne wrote:
> > 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 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>
> 
> I have to admit that I'm still confused as to why we need this in the
> first place.
> 
> The ELF build is linked to a fixed virtual address, irrespective of
> whether grub loads it via MB1 or MB2 and/or with EFI details.
> 
> i.e. the non-EFI build shouldn't have any remaining relocations by the
> time it is fully linked.
> 
> Or am I missing something?

Right, but there's code that depends on the symbols defined in .reloc
(__base_relocs_start/__base_relocs_end), so unless those symbols are
defined the linker will throw a missing symbols error on the final
link step.

I could add .reloc to the discarded sections list and create the
__base_relocs_start and __base_relocs_end symbols on the linker script
maybe, but I'm not sure that's any better than just having the dummy
.reloc section.

Or another option would be to compile the units that use those symbols
twice, one for the ELF build and one for the PE build, but again that
doesn't seem much better IMO.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v3 1/3] x86/linker: add a reloc section to ELF linker script

[Jan Beulich]

>>> On 27.06.19 at 11:33, <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.
> 
> Note that for the ELF output format the .reloc section is moved before
> .bss 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>

Assuming you've addressed Andrew's concerns in your reply,
Acked-by: Jan Beulich <jbeulich@suse.com>
(But Andrew, please confirm.)

Jan


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

Re: [Xen-devel] [PATCH v3 2/3] xen/link: handle .init.rodata.cst* sections in the linker script

[Jan Beulich]

>>> On 27.06.19 at 12:53, <andrew.cooper3@citrix.com> wrote:
> On 27/06/2019 10:33, Roger Pau Monne wrote:
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -156,6 +156,7 @@ SECTIONS
>>         *(.init.rodata)
>>         *(.init.rodata.rel)
>>         *(.init.rodata.str*)
>> +       *(.init.rodata.cst*)
> 
> ... .init is just a grouping prefix, so I'd recommend that we treat
> .init.rodata in exactly the same way as we treat .rodata, so I'd suggest
> turning this into
> 
> *(.init.rodata)
> *(.init.rodata.*)
> 
> to match the regular .rodata.

Or, as suggested elsewhere, make .rodata use less wide matching,
like we do for .init.rodata.

Jan



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

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

[Jan Beulich]

>>> On 27.06.19 at 11:33, <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>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [Xen-devel] [PATCH v3 1/3] x86/linker: add a reloc section to ELF linker script

[Andrew Cooper]

On 27/06/2019 12:07, Roger Pau Monné wrote:
> On Thu, Jun 27, 2019 at 11:59:46AM +0100, Andrew Cooper wrote:
>> On 27/06/2019 10:33, Roger Pau Monne wrote:
>>> 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 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>
>> I have to admit that I'm still confused as to why we need this in the
>> first place.
>>
>> The ELF build is linked to a fixed virtual address, irrespective of
>> whether grub loads it via MB1 or MB2 and/or with EFI details.
>>
>> i.e. the non-EFI build shouldn't have any remaining relocations by the
>> time it is fully linked.
>>
>> Or am I missing something?
> Right, but there's code that depends on the symbols defined in .reloc
> (__base_relocs_start/__base_relocs_end), so unless those symbols are
> defined the linker will throw a missing symbols error on the final
> link step.

Ok.  I can certainly accept that this is how the code currently functions.

> I could add .reloc to the discarded sections list and create the
> __base_relocs_start and __base_relocs_end symbols on the linker script
> maybe, but I'm not sure that's any better than just having the dummy
> .reloc section.
>
> Or another option would be to compile the units that use those symbols
> twice, one for the ELF build and one for the PE build, but again that
> doesn't seem much better IMO.

So the bug here is that efi_arch_relocate_image() isn't inside an #ifdef
EFI (or is it XEN_BUILD_EFI?)

And the reason #ifdef-ing it won't work is because we have a single pass
of extra objects to link into the main Xen to add EFI support.

I think the proper longterm fix is to have CONFIG_EFI (suitably guarded
on toolchain support), seeing as it is common across our two binaries,
and the extra bits for the real EFI build then become rather smaller.

However, it is definitely not fair to lump this fix on you for this
series, so given a comment explaining that this isn't used in the ELF
build, but needs to be present for compatibility with the EFI build,
I'll be ok with the patch in this form.

Again, a comment can be fixed up on commit if everyone is happy with
this approach.

~Andrew

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

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

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2646 bytes --]

On 27/06/2019 10:33, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 8a8d8f060f..94e6c9aee3 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -99,9 +99,14 @@ endif
>  syms-warn-dup-y := --warn-dup
>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>  
> +$(TARGET): TMP = $(@D)/.$(@F)

I'd suggest giving this a .elf32 suffix to make it clear which pass of
the build it comes from.

>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> -	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
> +	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null ||
> +	od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null

This works, but

Makefile:104: recipe for target '/local/xen.git/xen/xen' failed

Isn't helpful to identify what went wrong.  Sadly, we can't use $(error
...) in a shell snippet, but:

andrewcoop@andrewcoop:/local/xen.git/xen$ git d
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 94e6c9aee3..a1d6605a8b 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -99,13 +99,14 @@ endif
 syms-warn-dup-y := --warn-dup
 syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
 
-$(TARGET): TMP = $(@D)/.$(@F)
+$(TARGET): TMP = $(@D)/.$(@F).elf32
 $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
        ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null
-       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null
+       od -t x4 -N 8192 $(TMP)  | grep 1badb002 > /dev/null || \
+               { echo "No Multiboot1 header found"; false; }
+       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null || \
+               { echo "No Multiboot2 header found"; false; }
        mv $(TMP) $(TARGET)
 
 ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)

results in:

No Multiboot1 header found
Makefile:104: recipe for target '/local/xen.git/xen/xen' failed
make[2]: *** [/local/xen.git/xen/xen] Error 1
Makefile:136: recipe for target '/local/xen.git/xen/xen' failed
make[1]: *** [/local/xen.git/xen/xen] Error 2
Makefile:45: recipe for target 'build' failed
make: *** [build] Error 2

Which is far more clear.

Thoughts?

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 3476 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

>>> On 27.06.19 at 13:51, <andrew.cooper3@citrix.com> wrote:
> On 27/06/2019 10:33, Roger Pau Monne wrote:
>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>> index 8a8d8f060f..94e6c9aee3 100644
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -99,9 +99,14 @@ endif
>>  syms-warn-dup-y := --warn-dup
>>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>>  
>> +$(TARGET): TMP = $(@D)/.$(@F)
> 
> I'd suggest giving this a .elf32 suffix to make it clear which pass of
> the build it comes from.
> 
>>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
>> -	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
>> +	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null ||
>> +	od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null
> 
> This works, but
> 
> Makefile:104: recipe for target '/local/xen.git/xen/xen' failed
> 
> Isn't helpful to identify what went wrong.  Sadly, we can't use $(error
> ...) in a shell snippet,

I think we could:

       $(if $(shell od -t x4 -N 8192 $(TMP) | grep 1badb002),,$(error ...)exit 1)

But I admit I didn't check whether it is well defined that only one of
the last two operands of $(if ) get actually evaluated.

> but:
> 
> andrewcoop@andrewcoop:/local/xen.git/xen$ git d
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 94e6c9aee3..a1d6605a8b 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -99,13 +99,14 @@ endif
>  syms-warn-dup-y := --warn-dup
>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>  
> -$(TARGET): TMP = $(@D)/.$(@F)
> +$(TARGET): TMP = $(@D)/.$(@F).elf32
>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
>         ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null
> -       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null
> +       od -t x4 -N 8192 $(TMP)  | grep 1badb002 > /dev/null || \
> +               { echo "No Multiboot1 header found"; false; }
> +       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null || \
> +               { echo "No Multiboot2 header found"; false; }
>         mv $(TMP) $(TARGET)
>  
>  ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o 
> $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
> 
> results in:
> 
> No Multiboot1 header found
> Makefile:104: recipe for target '/local/xen.git/xen/xen' failed
> make[2]: *** [/local/xen.git/xen/xen] Error 1
> Makefile:136: recipe for target '/local/xen.git/xen/xen' failed
> make[1]: *** [/local/xen.git/xen/xen] Error 2
> Makefile:45: recipe for target 'build' failed
> make: *** [build] Error 2
> 
> Which is far more clear.
> 
> Thoughts?

Good idea. The only further request I have is to add >&2 to the
echo commands (unless we go the $(error ) route anyway).

Jan



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

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

[Andrew Cooper]

On 27/06/2019 13:10, Jan Beulich wrote:
>>>> On 27.06.19 at 13:51, <andrew.cooper3@citrix.com> wrote:
>> On 27/06/2019 10:33, Roger Pau Monne wrote:
>>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>>> index 8a8d8f060f..94e6c9aee3 100644
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -99,9 +99,14 @@ endif
>>>  syms-warn-dup-y := --warn-dup
>>>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>>>  
>>> +$(TARGET): TMP = $(@D)/.$(@F)
>> I'd suggest giving this a .elf32 suffix to make it clear which pass of
>> the build it comes from.
>>
>>>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
>>> -	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
>>> +	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null ||
>>> +	od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null
>> This works, but
>>
>> Makefile:104: recipe for target '/local/xen.git/xen/xen' failed
>>
>> Isn't helpful to identify what went wrong.  Sadly, we can't use $(error
>> ...) in a shell snippet,
> I think we could:
>
>        $(if $(shell od -t x4 -N 8192 $(TMP) | grep 1badb002),,$(error ...)exit 1)
>
> But I admit I didn't check whether it is well defined that only one of
> the last two operands of $(if ) get actually evaluated.

Of the two options, I think the || { ; false } is clearer to follow.

>
>> but:
>>
>> andrewcoop@andrewcoop:/local/xen.git/xen$ git d
>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>> index 94e6c9aee3..a1d6605a8b 100644
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -99,13 +99,14 @@ endif
>>  syms-warn-dup-y := --warn-dup
>>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>>  
>> -$(TARGET): TMP = $(@D)/.$(@F)
>> +$(TARGET): TMP = $(@D)/.$(@F).elf32
>>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
>>         ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null
>> -       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null
>> +       od -t x4 -N 8192 $(TMP)  | grep 1badb002 > /dev/null || \
>> +               { echo "No Multiboot1 header found"; false; }
>> +       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null || \
>> +               { echo "No Multiboot2 header found"; false; }
>>         mv $(TMP) $(TARGET)
>>  
>>  ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o 
>> $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
>>
>> results in:
>>
>> No Multiboot1 header found
>> Makefile:104: recipe for target '/local/xen.git/xen/xen' failed
>> make[2]: *** [/local/xen.git/xen/xen] Error 1
>> Makefile:136: recipe for target '/local/xen.git/xen/xen' failed
>> make[1]: *** [/local/xen.git/xen/xen] Error 2
>> Makefile:45: recipe for target 'build' failed
>> make: *** [build] Error 2
>>
>> Which is far more clear.
>>
>> Thoughts?
> Good idea. The only further request I have is to add >&2 to the
> echo commands.

Done.

~Andrew

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

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

[Roger Pau Monné]

On Thu, Jun 27, 2019 at 12:51:05PM +0100, Andrew Cooper wrote:
> On 27/06/2019 10:33, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index 8a8d8f060f..94e6c9aee3 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -99,9 +99,14 @@ endif
> >  syms-warn-dup-y := --warn-dup
> >  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
> >  
> > +$(TARGET): TMP = $(@D)/.$(@F)
> 
> I'd suggest giving this a .elf32 suffix to make it clear which pass of
> the build it comes from.

That's fine, please also adjust the ignored list and the clean
target.

> >  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> > -	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) $(XEN_IMG_OFFSET) \
> > +	./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null ||
> > +	od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null
> 
> This works, but
> 
> Makefile:104: recipe for target '/local/xen.git/xen/xen' failed
> 
> Isn't helpful to identify what went wrong.  Sadly, we can't use $(error
> ...) in a shell snippet, but:
> 
> andrewcoop@andrewcoop:/local/xen.git/xen$ git d
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 94e6c9aee3..a1d6605a8b 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -99,13 +99,14 @@ endif
>  syms-warn-dup-y := --warn-dup
>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>  
> -$(TARGET): TMP = $(@D)/.$(@F)
> +$(TARGET): TMP = $(@D)/.$(@F).elf32
>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
>         ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(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 $(TMP) | grep 1badb002 > /dev/null
> -       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null
> +       od -t x4 -N 8192 $(TMP)  | grep 1badb002 > /dev/null || \
> +               { echo "No Multiboot1 header found"; false; }
> +       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null || \
> +               { echo "No Multiboot2 header found"; false; }
>         mv $(TMP) $(TARGET)
>  
>  ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
> 
> results in:
> 
> No Multiboot1 header found
> Makefile:104: recipe for target '/local/xen.git/xen/xen' failed
> make[2]: *** [/local/xen.git/xen/xen] Error 1
> Makefile:136: recipe for target '/local/xen.git/xen/xen' failed
> make[1]: *** [/local/xen.git/xen/xen] Error 2
> Makefile:45: recipe for target 'build' failed
> make: *** [build] Error 2
> 
> Which is far more clear.
> 
> Thoughts?

Thanks, is indeed better. I also agree with Jan on the
redirection to stderr.

Roger.

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

Re: [Xen-devel] [PATCH v3 2/3] xen/link: handle .init.rodata.cst* sections in the linker script

[Roger Pau Monné]

On Thu, Jun 27, 2019 at 05:25:06AM -0600, Jan Beulich wrote:
> >>> On 27.06.19 at 12:53, <andrew.cooper3@citrix.com> wrote:
> > On 27/06/2019 10:33, Roger Pau Monne wrote:
> >> --- a/xen/arch/arm/xen.lds.S
> >> +++ b/xen/arch/arm/xen.lds.S
> >> @@ -156,6 +156,7 @@ SECTIONS
> >>         *(.init.rodata)
> >>         *(.init.rodata.rel)
> >>         *(.init.rodata.str*)
> >> +       *(.init.rodata.cst*)
> > 
> > ... .init is just a grouping prefix, so I'd recommend that we treat
> > .init.rodata in exactly the same way as we treat .rodata, so I'd suggest
> > turning this into
> > 
> > *(.init.rodata)
> > *(.init.rodata.*)
> > 
> > to match the regular .rodata.
> 
> Or, as suggested elsewhere, make .rodata use less wide matching,
> like we do for .init.rodata.

I'm happy to handle .init.rodata subsections as Xen currently handles
.rodata subsections. There are no custom subsections explicitly added
to either .rodata or .init.rodata, and if we start adding such
subsections explicitly the linker script will likely need
modifications anyway to mark the start and end of those subsections in
the final binary, or the default placement of the wider wildcard will
be already fine.

Thanks, Roger.

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