[kvm-unit-tests PATCH v1 0/3] s390x: small Makefile improvements

[kvm-unit-tests PATCH v1 0/3] s390x: small Makefile improvements

[Marc Hartmayer]

The first patch is useful anyway, the third could be dropped to be consistent
with the other architectures.

Marc Hartmayer (2):
  s390x/Makefile: snippets: Add separate target for the ELF snippets
  s390x/Makefile: snippets: Avoid creation of .eh_frame and
    .eh_frame_hdr sections

Super User (1):
  Revert "s390x: Specify program headers with flags to avoid linker
    warnings"

 s390x/Makefile              | 16 ++++++++++------
 s390x/snippets/c/flat.lds.S | 12 +++---------
 2 files changed, 13 insertions(+), 15 deletions(-)


base-commit: 31f2cece1db4175869ca3fe4cbe229c0e15fdaf0
-- 
2.34.1

[kvm-unit-tests PATCH v1 1/3] s390x/Makefile: snippets: Add separate target for the ELF snippets

[Marc Hartmayer]

It's unusual to create multiple files in one target rule, and it's even more
unusual to create an ELF file with a `.gbin` file extension first, and then
overwrite it in the next step. It might even lead to errors as the input file
path is also used as the output file path - but this depends on the objcopy
implementation. Therefore, create an extra target for the ELF files and list it
as a prerequisite for the *.gbin targets.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 s390x/Makefile | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/s390x/Makefile b/s390x/Makefile
index 23342bd64f44..784818b2883e 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -153,14 +153,18 @@ $(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets)
 $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
 	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
 
-$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds
+$(SNIPPET_DIR)/asm/%.elf: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds
 	$(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/asm/flat.lds $<
-	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@
+
+$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.elf
+	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $< $@
 	truncate -s '%4096' $@
 
-$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) $(SNIPPET_DIR)/c/flat.lds
+$(SNIPPET_DIR)/c/%.elf: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) $(SNIPPET_DIR)/c/flat.lds
 	$(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/c/flat.lds $< $(snippet_lib) $(FLATLIBS)
-	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@
+
+$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.elf
+	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $< $@
 	truncate -s '%4096' $@
 
 %.hdr: %.gbin $(HOST_KEY_DOCUMENT)
-- 
2.34.1

Re: [kvm-unit-tests PATCH v1 1/3] s390x/Makefile: snippets: Add separate target for the ELF snippets

[Nicholas Piggin]

On Tue Jun 4, 2024 at 9:59 PM AEST, Marc Hartmayer wrote:
> It's unusual to create multiple files in one target rule, and it's even more
> unusual to create an ELF file with a `.gbin` file extension first, and then
> overwrite it in the next step. It might even lead to errors as the input file
> path is also used as the output file path - but this depends on the objcopy
> implementation. Therefore, create an extra target for the ELF files and list it
> as a prerequisite for the *.gbin targets.

I had some pain trying to figure out another ("pretty printing") patch
that changed some s390x/Makefile because of this. As far as I can tell
it looks good.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  s390x/Makefile | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 23342bd64f44..784818b2883e 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -153,14 +153,18 @@ $(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets)
>  $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
>  	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
>  
> -$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds
> +$(SNIPPET_DIR)/asm/%.elf: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds
>  	$(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/asm/flat.lds $<
> -	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@
> +
> +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.elf
> +	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $< $@
>  	truncate -s '%4096' $@
>  
> -$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) $(SNIPPET_DIR)/c/flat.lds
> +$(SNIPPET_DIR)/c/%.elf: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) $(SNIPPET_DIR)/c/flat.lds
>  	$(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/c/flat.lds $< $(snippet_lib) $(FLATLIBS)
> -	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@
> +
> +$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.elf
> +	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $< $@
>  	truncate -s '%4096' $@
>  
>  %.hdr: %.gbin $(HOST_KEY_DOCUMENT)

Re: [kvm-unit-tests PATCH v1 1/3] s390x/Makefile: snippets: Add separate target for the ELF snippets

[Marc Hartmayer]

On Wed, Jun 05, 2024 at 11:21 AM +1000, "Nicholas Piggin" <npiggin@gmail.com> wrote:
> On Tue Jun 4, 2024 at 9:59 PM AEST, Marc Hartmayer wrote:
>> It's unusual to create multiple files in one target rule, and it's even more
>> unusual to create an ELF file with a `.gbin` file extension first, and then
>> overwrite it in the next step. It might even lead to errors as the input file
>> path is also used as the output file path - but this depends on the objcopy
>> implementation. Therefore, create an extra target for the ELF files and list it
>> as a prerequisite for the *.gbin targets.
>
> I had some pain trying to figure out another ("pretty printing") patch
> that changed some s390x/Makefile because of this. As far as I can tell
> it looks good.

Hehe yes. Thomas sent me the following error message:

/usr/bin/s390x-linux-gnu-ld: warning: s390x/snippets/c/mvpg-snippet.gbin
has a LOAD segment with RWX permissions

…and at first this was totally confusing until I’ve looked at the code… :)

>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks.

[…snip]

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [kvm-unit-tests PATCH v1 1/3] s390x/Makefile: snippets: Add separate target for the ELF snippets

[Janosch Frank]

On 6/4/24 1:59 PM, Marc Hartmayer wrote:
> It's unusual to create multiple files in one target rule, and it's even more
> unusual to create an ELF file with a `.gbin` file extension first, and then
> overwrite it in the next step. It might even lead to errors as the input file
> path is also used as the output file path - but this depends on the objcopy
> implementation. Therefore, create an extra target for the ELF files and list it
> as a prerequisite for the *.gbin targets.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Re: [kvm-unit-tests PATCH v1 1/3] s390x/Makefile: snippets: Add separate target for the ELF snippets

[Janosch Frank]

On 6/4/24 1:59 PM, Marc Hartmayer wrote:
> It's unusual to create multiple files in one target rule, and it's even more
> unusual to create an ELF file with a `.gbin` file extension first, and then
> overwrite it in the next step. It might even lead to errors as the input file
> path is also used as the output file path - but this depends on the objcopy
> implementation. Therefore, create an extra target for the ELF files and list it
> as a prerequisite for the *.gbin targets.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>

I've picked this one but it's unlikely that I'll pick the other patches 
in the series. Thanks for improving the makefile and fixing my mistakes :)

Re: [kvm-unit-tests PATCH v1 1/3] s390x/Makefile: snippets: Add separate target for the ELF snippets

[Marc Hartmayer]

On Mon, Jul 29, 2024 at 09:00 AM +0200, Janosch Frank <frankja@linux.ibm.com> wrote:
> On 6/4/24 1:59 PM, Marc Hartmayer wrote:
>> It's unusual to create multiple files in one target rule, and it's even more
>> unusual to create an ELF file with a `.gbin` file extension first, and then
>> overwrite it in the next step. It might even lead to errors as the input file
>> path is also used as the output file path - but this depends on the objcopy
>> implementation. Therefore, create an extra target for the ELF files and list it
>> as a prerequisite for the *.gbin targets.
>> 
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>
> I've picked this one but it's unlikely that I'll pick the other patches 
> in the series. Thanks for improving the makefile and fixing my
> mistakes :)

Thanks for the r-b and fine with me. The other two patches are not as
useful and would make s390x even more different from other architectures
with little to no real benefit.

>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[kvm-unit-tests PATCH v1 2/3] s390x/Makefile: snippets: Avoid creation of .eh_frame and .eh_frame_hdr sections

[Marc Hartmayer]

Use `-fno-asynchronous-unwind-tables` and `-fno-exceptions` to avoid creating
`.eh_frame` and `.eh_frame_hdr` sections. They are not used by the snippets and
the creation of the sections may result in the creation of an RWX
segment (depending on the toolchain used).

Before this change:

$ make -j s390x/snippets/c/spec_ex.elf >/dev/null && readelf -l s390x/snippets/c/spec_ex.elf
/usr/bin/s390x-linux-gnu-ld: warning: s390x/snippets/c/spec_ex.elf has a LOAD segment with RWX permissions

Elf file type is EXEC (Executable file)
Entry point 0x4000
There are 4 program headers, starting at offset 64

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  LOAD           0x0000000000001000 0x0000000000000000 0x0000000000000000
                 0x00000000000001b0 0x00000000000001b0  RW     0x1000
  LOAD           0x0000000000002000 0x0000000000004000 0x0000000000004000
                 0x000000000000104c 0x0000000000001060  RWE    0x1000
  GNU_EH_FRAME   0x0000000000003038 0x0000000000005038 0x0000000000005038
                 0x0000000000000014 0x0000000000000014  R      0x4
  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000  RW     0x10

 Section to Segment mapping:
  Segment Sections...
   00     .lowcore
   01     .text .eh_frame .eh_frame_hdr .bss
   02     .eh_frame_hdr
   03

After this change, there is no warning and no RWX ELF segment:

$ make -j s390x/snippets/c/spec_ex.elf >/dev/null && readelf -l s390x/snippets/c/spec_ex.elf
Elf file type is EXEC (Executable file)
Entry point 0x4000
There are 4 program headers, starting at offset 64

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  LOAD           0x0000000000001000 0x0000000000000000 0x0000000000000000
                 0x00000000000001b0 0x00000000000001b0  RW     0x1000
  LOAD           0x0000000000002000 0x0000000000004000 0x0000000000004000
                 0x00000000000000a8 0x00000000000000a8  R E    0x1000
  LOAD           0x0000000000000000 0x0000000000005000 0x0000000000005000
                 0x0000000000000000 0x0000000000000010  RW     0x1000
  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000  RW     0x10

 Section to Segment mapping:
  Segment Sections...
   00     .lowcore
   01     .text
   02     .bss
   03

Linker used:

$ s390x-linux-gnu-ld -v
GNU ld version 2.41-1.fc40

Therefore the commit 9801dbbe9ea4 ("s390x: Specify program headers with flags to
avoid linker warnings") can be reverted.
---
Note: we have to double check whether__builtin_frame_address() in s390x/stack.c
needs the .eh_frame or not.
---
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 s390x/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/s390x/Makefile b/s390x/Makefile
index 784818b2883e..f1bbf5e9f457 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -148,10 +148,10 @@ endif
 
 # the asm/c snippets %.o have additional generated files as dependencies
 $(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets)
-	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
+	$(CC) $(CFLAGS) -c -nostdlib -fno-asynchronous-unwind-tables -fno-exceptions -o $@ $<
 
 $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
-	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
+	$(CC) $(CFLAGS) -c -nostdlib -fno-asynchronous-unwind-tables -fno-exceptions -o $@ $<
 
 $(SNIPPET_DIR)/asm/%.elf: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds
 	$(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/asm/flat.lds $<
-- 
2.34.1

[kvm-unit-tests PATCH v1 3/3] Revert "s390x: Specify program headers with flags to avoid linker warnings"

[Marc Hartmayer]

From: Super User <root@t35lp69.lnxne.boe>

This reverts commit 9801dbbe9ea4591b2c32a51e5b29cb64502b93fb.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 s390x/snippets/c/flat.lds.S | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/s390x/snippets/c/flat.lds.S b/s390x/snippets/c/flat.lds.S
index 6b8ceb9e0dca..468b5f1eebe8 100644
--- a/s390x/snippets/c/flat.lds.S
+++ b/s390x/snippets/c/flat.lds.S
@@ -1,11 +1,5 @@
 #include <asm/asm-offsets.h>
 
-PHDRS
-{
-    text PT_LOAD FLAGS(5);
-    data PT_LOAD FLAGS(6);
-}
-
 SECTIONS
 {
 	.lowcore : {
@@ -35,7 +29,7 @@ SECTIONS
 		*(.init)
 		*(.text)
 		*(.text.*)
-	} :text
+	}
 	. = ALIGN(4K);
 	etext = .;
 	/* End text */
@@ -43,9 +37,9 @@ SECTIONS
 	.data : {
 		*(.data)
 		*(.data.rel*)
-	} :data
+	}
 	. = ALIGN(16);
-	.rodata : { *(.rodata) *(.rodata.*) } :data
+	.rodata : { *(.rodata) *(.rodata.*) }
 	. = ALIGN(16);
 	.bss : { *(.bss) }
 	/* End data */
-- 
2.34.1

Re: [kvm-unit-tests PATCH v1 3/3] Revert "s390x: Specify program headers with flags to avoid linker warnings"

[Marc Hartmayer]

On Tue, Jun 04, 2024 at 01:59 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
> From: Super User <root@t35lp69.lnxne.boe>

Oops, author must be fixed… :/

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [kvm-unit-tests PATCH v1 0/3] s390x: small Makefile improvements

[Nicholas Piggin]

On Tue Jun 4, 2024 at 9:59 PM AEST, Marc Hartmayer wrote:
> The first patch is useful anyway, the third could be dropped to be consistent
> with the other architectures.

Interesting. Is this the reason for the warning on all the other archs?
Maybe they should all use the same options and all remove the explicit
PHDR specification?

Thanks,
Nick

Re: [kvm-unit-tests PATCH v1 0/3] s390x: small Makefile improvements

[Marc Hartmayer]

On Wed, Jun 05, 2024 at 11:30 AM +1000, "Nicholas Piggin" <npiggin@gmail.com> wrote:
> On Tue Jun 4, 2024 at 9:59 PM AEST, Marc Hartmayer wrote:
>> The first patch is useful anyway, the third could be dropped to be consistent
>> with the other architectures.
>
> Interesting. Is this the reason for the warning on all the other
> archs?

Could be, but the .eh_frame and .eh_frame_hdr sections are sometimes
required, e.g for __builtin_return_address(n),…. Another fix would be to
specify the sections in the linker scripts explicitly - but I’ve to ask
whether this has other side effects…

> Maybe they should all use the same options and all remove the explicit
> PHDR specification?
>
> Thanks,
> Nick
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [kvm-unit-tests PATCH v1 0/3] s390x: small Makefile improvements

[Marc Hartmayer]

On Tue, Jun 04, 2024 at 01:59 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
> The first patch is useful anyway, the third could be dropped to be consistent
> with the other architectures.
>
> Marc Hartmayer (2):
>   s390x/Makefile: snippets: Add separate target for the ELF snippets
>   s390x/Makefile: snippets: Avoid creation of .eh_frame and
>     .eh_frame_hdr sections
>
> Super User (1):
>   Revert "s390x: Specify program headers with flags to avoid linker
>     warnings"
>
>  s390x/Makefile              | 16 ++++++++++------
>  s390x/snippets/c/flat.lds.S | 12 +++---------
>  2 files changed, 13 insertions(+), 15 deletions(-)
>
>
> base-commit: 31f2cece1db4175869ca3fe4cbe229c0e15fdaf0
> -- 
> 2.34.1
>
>

Polite ping.

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294