[kvm-unit-tests PATCH 0/5] s390x: Snippet fixes

[kvm-unit-tests PATCH 0/5] s390x: Snippet fixes

[Janosch Frank]

A small set of fixes mostly related to the snippet support and error
management.

Janosch Frank (5):
  s390x: Add a linker script to assembly snippets
  s390x: snippets: Fix SET_PSW_NEW_ADDR macro
  lib: s390x: sie: Set guest memory pointer
  s390x: Clear first stack frame and end backtrace early
  lib: s390x: Handle debug prints for SIE exceptions correctly

 lib/s390x/interrupt.c       | 46 +++++++++++++++++++++++++++++++++----
 lib/s390x/sie.c             |  1 +
 lib/s390x/sie.h             |  2 ++
 lib/s390x/snippet.h         |  3 +--
 lib/s390x/stack.c           |  2 ++
 s390x/Makefile              |  5 ++--
 s390x/cpu.S                 |  6 +++--
 s390x/cstart64.S            |  2 ++
 s390x/mvpg-sie.c            |  2 +-
 s390x/pv-diags.c            |  6 ++---
 s390x/snippets/asm/flat.lds | 43 ++++++++++++++++++++++++++++++++++
 s390x/snippets/asm/macros.S |  2 +-
 12 files changed, 104 insertions(+), 16 deletions(-)
 create mode 100644 s390x/snippets/asm/flat.lds

-- 
2.34.1

[kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets

[Janosch Frank]

A linker script has a few benefits:
- Random data doesn't end up in the binary breaking tests
- We can easily define a lowcore and load the snippet from 0x0 instead
of 0x4000 which makes asm snippets behave like c snippets
- We can easily define an invalid PGM new PSW to ensure an exit on a
guest PGM

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/snippet.h         |  3 +--
 s390x/Makefile              |  5 +++--
 s390x/mvpg-sie.c            |  2 +-
 s390x/pv-diags.c            |  6 +++---
 s390x/snippets/asm/flat.lds | 43 +++++++++++++++++++++++++++++++++++++
 5 files changed, 51 insertions(+), 8 deletions(-)
 create mode 100644 s390x/snippets/asm/flat.lds

diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h
index b17b2a4c..57045994 100644
--- a/lib/s390x/snippet.h
+++ b/lib/s390x/snippet.h
@@ -32,8 +32,7 @@
 
 #define SNIPPET_PV_TWEAK0	0x42UL
 #define SNIPPET_PV_TWEAK1	0UL
-#define SNIPPET_OFF_C		0
-#define SNIPPET_OFF_ASM		0x4000
+#define SNIPPET_UNPACK_OFF	0
 
 
 /*
diff --git a/s390x/Makefile b/s390x/Makefile
index bf1504f9..bb0f9eb8 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -135,7 +135,8 @@ $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
 	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
 
 $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o
-	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
+	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $(patsubst %.gbin,%.o,$@)
+	$(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)
@@ -144,7 +145,7 @@ $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
 	truncate -s '%4096' $@
 
 $(SNIPPET_DIR)/asm/%.hdr: $(SNIPPET_DIR)/asm/%.gbin $(HOST_KEY_DOCUMENT)
-	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x4000,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
+	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
 
 $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
 	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c
index 46a2edb6..99f4859b 100644
--- a/s390x/mvpg-sie.c
+++ b/s390x/mvpg-sie.c
@@ -87,7 +87,7 @@ static void setup_guest(void)
 
 	snippet_setup_guest(&vm, false);
 	snippet_init(&vm, SNIPPET_NAME_START(c, mvpg_snippet),
-		     SNIPPET_LEN(c, mvpg_snippet), SNIPPET_OFF_C);
+		     SNIPPET_LEN(c, mvpg_snippet), SNIPPET_UNPACK_OFF);
 
 	/* Enable MVPG interpretation as we want to test KVM and not ourselves */
 	vm.sblk->eca = ECA_MVPGI;
diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
index 9ced68c7..5165937a 100644
--- a/s390x/pv-diags.c
+++ b/s390x/pv-diags.c
@@ -28,7 +28,7 @@ static void test_diag_500(void)
 
 	snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_500),
 			SNIPPET_HDR_START(asm, snippet_pv_diag_500),
-			size_gbin, size_hdr, SNIPPET_OFF_ASM);
+			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
 
 	sie(&vm);
 	report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 &&
@@ -83,7 +83,7 @@ static void test_diag_288(void)
 
 	snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_288),
 			SNIPPET_HDR_START(asm, snippet_pv_diag_288),
-			size_gbin, size_hdr, SNIPPET_OFF_ASM);
+			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
 
 	sie(&vm);
 	report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 &&
@@ -124,7 +124,7 @@ static void test_diag_yield(void)
 
 	snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_yield),
 			SNIPPET_HDR_START(asm, snippet_pv_diag_yield),
-			size_gbin, size_hdr, SNIPPET_OFF_ASM);
+			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
 
 	/* 0x44 */
 	report_prefix_push("0x44");
diff --git a/s390x/snippets/asm/flat.lds b/s390x/snippets/asm/flat.lds
new file mode 100644
index 00000000..366d2d78
--- /dev/null
+++ b/s390x/snippets/asm/flat.lds
@@ -0,0 +1,43 @@
+SECTIONS
+{
+	.lowcore : {
+		/*
+		 * Initial short psw for disk boot, with 31 bit addressing for
+		 * non z/Arch environment compatibility and the instruction
+		 * address 0x4000.
+		 */
+		. = 0;
+		 LONG(0x00080000)
+		 LONG(0x80004000)
+		 /* Restart new PSW for booting via PSW restart. */
+		 . = 0x1a0;
+		 QUAD(0x0000000180000000)
+		 QUAD(0x0000000000004000)
+		 /*
+		  * Invalid PGM new PSW so we hopefully get a code 8
+		  * intercept on a PGM
+		  */
+		 . = 0x1d0;
+		 QUAD(0x0008000000000000)
+		 QUAD(0x0000000000000001)
+	}
+	. = 0x4000;
+	.text : {
+		*(.text)
+		*(.text.*)
+	}
+	. = ALIGN(64K);
+	etext = .;
+	. = ALIGN(16);
+	.data : {
+		*(.data)
+		*(.data.rel*)
+	}
+	. = ALIGN(16);
+	.rodata : { *(.rodata) *(.rodata.*) }
+	. = ALIGN(16);
+	__bss_start = .;
+	.bss : { *(.bss) }
+	__bss_end = .;
+	. = ALIGN(64K);
+}
-- 
2.34.1

[kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro

[Janosch Frank]

Let's store the psw mask instead of the address of the location where we
should load the mask from.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/snippets/asm/macros.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/snippets/asm/macros.S b/s390x/snippets/asm/macros.S
index 667fb6dc..09d7f5be 100644
--- a/s390x/snippets/asm/macros.S
+++ b/s390x/snippets/asm/macros.S
@@ -18,7 +18,7 @@
  */
 .macro SET_PSW_NEW_ADDR reg, psw_new_addr, addr_psw
 larl	\reg, psw_mask_64
-stg	\reg, \addr_psw
+mvc	\addr_psw(8,%r0), 0(\reg)
 larl	\reg, \psw_new_addr
 stg	\reg, \addr_psw + 8
 .endm
-- 
2.34.1

[kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer

[Janosch Frank]

Seems like it was introduced but never set. It's nicer to have a
pointer than to cast the MSO of a VM.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sie.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
index a71985b6..9241b4b4 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -93,6 +93,7 @@ void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len)
 
 	/* Guest memory chunks are always 1MB */
 	assert(!(guest_mem_len & ~HPAGE_MASK));
+	vm->guest_mem = (uint8_t *)guest_mem;
 	/* For non-PV guests we re-use the host's ASCE for ease of use */
 	vm->save_area.guest.asce = stctg(1);
 	/* Currently MSO/MSL is the easiest option */
-- 
2.34.1

[kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early

[Janosch Frank]

When setting the first stack frame to 0, we can check for a 0
backchain pointer when doing backtraces to know when to stop.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/stack.c | 2 ++
 s390x/cstart64.S  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c
index e714e07c..9f234a12 100644
--- a/lib/s390x/stack.c
+++ b/lib/s390x/stack.c
@@ -22,6 +22,8 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 	for (depth = 0; stack && depth < max_depth; depth++) {
 		return_addrs[depth] = (void *)stack->grs[8];
 		stack = stack->back_chain;
+		if (!stack)
+			break;
 	}
 
 	return depth;
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 666a9567..6f83da2a 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -37,6 +37,8 @@ start:
 	sam64				# Set addressing mode to 64 bit
 	/* setup stack */
 	larl	%r15, stackptr
+	/* Clear first stack frame */
+	xc      0(160,%r15), 0(%r15)
 	/* setup initial PSW mask + control registers*/
 	larl	%r1, initial_psw
 	lpswe	0(%r1)
-- 
2.34.1

[kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly

[Janosch Frank]

When we leave SIE due to an exception, we'll still have guest values
in registers 0 - 13 and that's not clearly portraied in our debug
prints. So let's fix that.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/interrupt.c | 46 ++++++++++++++++++++++++++++++++++++++-----
 lib/s390x/sie.h       |  2 ++
 s390x/cpu.S           |  6 ++++--
 3 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index dadb7415..ff47c2c2 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -9,6 +9,7 @@
  */
 #include <libcflat.h>
 #include <asm/barrier.h>
+#include <asm/asm-offsets.h>
 #include <sclp.h>
 #include <interrupt.h>
 #include <sie.h>
@@ -188,9 +189,12 @@ static void print_storage_exception_information(void)
 	}
 }
 
-static void print_int_regs(struct stack_frame_int *stack)
+static void print_int_regs(struct stack_frame_int *stack, bool sie)
 {
+	struct kvm_s390_sie_block *sblk;
+
 	printf("\n");
+	printf("%s\n", sie ? "Guest registers:" : "Host registers:");
 	printf("GPRS:\n");
 	printf("%016lx %016lx %016lx %016lx\n",
 	       stack->grs1[0], stack->grs1[1], stack->grs0[0], stack->grs0[1]);
@@ -198,24 +202,56 @@ static void print_int_regs(struct stack_frame_int *stack)
 	       stack->grs0[2], stack->grs0[3], stack->grs0[4], stack->grs0[5]);
 	printf("%016lx %016lx %016lx %016lx\n",
 	       stack->grs0[6], stack->grs0[7], stack->grs0[8], stack->grs0[9]);
-	printf("%016lx %016lx %016lx %016lx\n",
-	       stack->grs0[10], stack->grs0[11], stack->grs0[12], stack->grs0[13]);
+
+	if (sie) {
+		sblk = (struct kvm_s390_sie_block *)stack->grs0[12];
+		printf("%016lx %016lx %016lx %016lx\n",
+		       stack->grs0[10], stack->grs0[11], sblk->gg14, sblk->gg15);
+	} else {
+		printf("%016lx %016lx %016lx %016lx\n",
+		       stack->grs0[10], stack->grs0[11], stack->grs0[12], stack->grs0[13]);
+	}
+
 	printf("\n");
 }
 
 static void print_pgm_info(struct stack_frame_int *stack)
 
 {
-	bool in_sie;
+	bool in_sie, in_sie_gregs;
+	struct vm_save_area *vregs;
 
 	in_sie = (lowcore.pgm_old_psw.addr >= (uintptr_t)sie_entry &&
 		  lowcore.pgm_old_psw.addr <= (uintptr_t)sie_exit);
+	in_sie_gregs = (lowcore.pgm_old_psw.addr >= (uintptr_t)sie_entry_gregs &&
+			lowcore.pgm_old_psw.addr <= (uintptr_t)sie_exit_gregs);
 
 	printf("\n");
 	printf("Unexpected program interrupt %s: %#x on cpu %d at %#lx, ilen %d\n",
 	       in_sie ? "in SIE" : "",
 	       lowcore.pgm_int_code, stap(), lowcore.pgm_old_psw.addr, lowcore.pgm_int_id);
-	print_int_regs(stack);
+
+	/*
+	 * If we fall out of SIE before loading the host registers,
+	 * then we need to do it here so we print the host registers
+	 * and not the guest registers.
+	 *
+	 * Back tracing is actually not a problem since SIE restores gr15.
+	 */
+	if (in_sie_gregs) {
+		print_int_regs(stack, true);
+		vregs = *((struct vm_save_area **)(stack->grs0[13] + __SF_SIE_SAVEAREA));
+
+		/*
+		 * The grs are not linear on the interrupt stack frame.
+		 * We copy 0 and 1 here and 2 - 15 with the memcopy below.
+		 */
+		stack->grs1[0] = vregs->host.grs[0];
+		stack->grs1[1] = vregs->host.grs[1];
+		/*  2 - 15 */
+		memcpy(stack->grs0, &vregs->host.grs[2], sizeof(stack->grs0) - 8);
+	}
+	print_int_regs(stack, false);
 	dump_stack();
 
 	/* Dump stack doesn't end with a \n so we add it here instead */
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index a27a8401..147cb0f2 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -273,6 +273,8 @@ struct vm {
 
 extern void sie_entry(void);
 extern void sie_exit(void);
+extern void sie_entry_gregs(void);
+extern void sie_exit_gregs(void);
 extern void sie64a(struct kvm_s390_sie_block *sblk, struct vm_save_area *save_area);
 void sie(struct vm *vm);
 void sie_expect_validity(struct vm *vm);
diff --git a/s390x/cpu.S b/s390x/cpu.S
index 45bd551a..9155b044 100644
--- a/s390x/cpu.S
+++ b/s390x/cpu.S
@@ -82,7 +82,8 @@ sie64a:
 	# Store scb and save_area pointer into stack frame
 	stg	%r2,__SF_SIE_CONTROL(%r15)	# save control block pointer
 	stg	%r3,__SF_SIE_SAVEAREA(%r15)	# save guest register save area
-
+.globl sie_entry_gregs
+sie_entry_gregs:
 	# Load guest's gprs, fprs and fpc
 	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
 	ld	\i, \i * 8 + SIE_SAVEAREA_GUEST_FPRS(%r3)
@@ -121,7 +122,8 @@ sie_exit:
 	.endr
 	lfpc	SIE_SAVEAREA_HOST_FPC(%r14)
 	lmg	%r0,%r14,SIE_SAVEAREA_HOST_GRS(%r14)	# restore kernel registers
-
+.globl sie_exit_gregs
+sie_exit_gregs:
 	br	%r14
 
 	.align	8
-- 
2.34.1

Re: [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro

[Nico Boehr]

Quoting Janosch Frank (2022-11-23 09:46:53)
> Let's store the psw mask instead of the address of the location where we
> should load the mask from.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer

[Nico Boehr]

Quoting Janosch Frank (2022-11-23 09:46:54)
> Seems like it was introduced but never set. It's nicer to have a
> pointer than to cast the MSO of a VM.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early

[Nico Boehr]

Quoting Janosch Frank (2022-11-23 09:46:55)
> When setting the first stack frame to 0, we can check for a 0
> backchain pointer when doing backtraces to know when to stop.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets

[Claudio Imbrenda]

On Wed, 23 Nov 2022 08:46:52 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> A linker script has a few benefits:
> - Random data doesn't end up in the binary breaking tests
> - We can easily define a lowcore and load the snippet from 0x0 instead
> of 0x4000 which makes asm snippets behave like c snippets
> - We can easily define an invalid PGM new PSW to ensure an exit on a
> guest PGM
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

looks good in general, but I have a few questions

> ---
>  lib/s390x/snippet.h         |  3 +--
>  s390x/Makefile              |  5 +++--
>  s390x/mvpg-sie.c            |  2 +-
>  s390x/pv-diags.c            |  6 +++---
>  s390x/snippets/asm/flat.lds | 43 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 51 insertions(+), 8 deletions(-)
>  create mode 100644 s390x/snippets/asm/flat.lds
> 
> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h
> index b17b2a4c..57045994 100644
> --- a/lib/s390x/snippet.h
> +++ b/lib/s390x/snippet.h
> @@ -32,8 +32,7 @@
>  
>  #define SNIPPET_PV_TWEAK0	0x42UL
>  #define SNIPPET_PV_TWEAK1	0UL
> -#define SNIPPET_OFF_C		0
> -#define SNIPPET_OFF_ASM		0x4000
> +#define SNIPPET_UNPACK_OFF	0
>  
>  
>  /*
> diff --git a/s390x/Makefile b/s390x/Makefile
> index bf1504f9..bb0f9eb8 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -135,7 +135,8 @@ $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
>  	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
>  
>  $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o
> -	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $(patsubst %.gbin,%.o,$@)

I think you can simply use $< instead of the patsubst expression

> +	$(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)
> @@ -144,7 +145,7 @@ $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
>  	truncate -s '%4096' $@
>  
>  $(SNIPPET_DIR)/asm/%.hdr: $(SNIPPET_DIR)/asm/%.gbin $(HOST_KEY_DOCUMENT)
> -	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x4000,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
> +	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
>  
>  $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
>  	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
> diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c
> index 46a2edb6..99f4859b 100644
> --- a/s390x/mvpg-sie.c
> +++ b/s390x/mvpg-sie.c
> @@ -87,7 +87,7 @@ static void setup_guest(void)
>  
>  	snippet_setup_guest(&vm, false);
>  	snippet_init(&vm, SNIPPET_NAME_START(c, mvpg_snippet),
> -		     SNIPPET_LEN(c, mvpg_snippet), SNIPPET_OFF_C);
> +		     SNIPPET_LEN(c, mvpg_snippet), SNIPPET_UNPACK_OFF);
>  
>  	/* Enable MVPG interpretation as we want to test KVM and not ourselves */
>  	vm.sblk->eca = ECA_MVPGI;
> diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
> index 9ced68c7..5165937a 100644
> --- a/s390x/pv-diags.c
> +++ b/s390x/pv-diags.c
> @@ -28,7 +28,7 @@ static void test_diag_500(void)
>  
>  	snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_500),
>  			SNIPPET_HDR_START(asm, snippet_pv_diag_500),
> -			size_gbin, size_hdr, SNIPPET_OFF_ASM);
> +			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
>  
>  	sie(&vm);
>  	report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 &&
> @@ -83,7 +83,7 @@ static void test_diag_288(void)
>  
>  	snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_288),
>  			SNIPPET_HDR_START(asm, snippet_pv_diag_288),
> -			size_gbin, size_hdr, SNIPPET_OFF_ASM);
> +			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
>  
>  	sie(&vm);
>  	report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 &&
> @@ -124,7 +124,7 @@ static void test_diag_yield(void)
>  
>  	snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_yield),
>  			SNIPPET_HDR_START(asm, snippet_pv_diag_yield),
> -			size_gbin, size_hdr, SNIPPET_OFF_ASM);
> +			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
>  
>  	/* 0x44 */
>  	report_prefix_push("0x44");
> diff --git a/s390x/snippets/asm/flat.lds b/s390x/snippets/asm/flat.lds
> new file mode 100644
> index 00000000..366d2d78
> --- /dev/null
> +++ b/s390x/snippets/asm/flat.lds
> @@ -0,0 +1,43 @@
> +SECTIONS
> +{
> +	.lowcore : {
> +		/*
> +		 * Initial short psw for disk boot, with 31 bit addressing for
> +		 * non z/Arch environment compatibility and the instruction
> +		 * address 0x4000.
> +		 */
> +		. = 0;
> +		 LONG(0x00080000)
> +		 LONG(0x80004000)
> +		 /* Restart new PSW for booting via PSW restart. */
> +		 . = 0x1a0;
> +		 QUAD(0x0000000180000000)
> +		 QUAD(0x0000000000004000)
> +		 /*
> +		  * Invalid PGM new PSW so we hopefully get a code 8
> +		  * intercept on a PGM
> +		  */
> +		 . = 0x1d0;
> +		 QUAD(0x0008000000000000)
> +		 QUAD(0x0000000000000001)
> +	}
> +	. = 0x4000;
> +	.text : {
> +		*(.text)
> +		*(.text.*)
> +	}
> +	. = ALIGN(64K);

any reason to align to 64k? (instead of e.g. 4k)

> +	etext = .;
> +	. = ALIGN(16);

do you need the ALIGN? I would think we are already aligned here

> +	.data : {
> +		*(.data)
> +		*(.data.rel*)
> +	}
> +	. = ALIGN(16);
> +	.rodata : { *(.rodata) *(.rodata.*) }
> +	. = ALIGN(16);
> +	__bss_start = .;
> +	.bss : { *(.bss) }
> +	__bss_end = .;
> +	. = ALIGN(64K);

same question as above regarding 64k

> +}

Re: [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer

[Claudio Imbrenda]

On Wed, 23 Nov 2022 08:46:54 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Seems like it was introduced but never set. It's nicer to have a
> pointer than to cast the MSO of a VM.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/s390x/sie.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index a71985b6..9241b4b4 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -93,6 +93,7 @@ void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len)
>  
>  	/* Guest memory chunks are always 1MB */
>  	assert(!(guest_mem_len & ~HPAGE_MASK));
> +	vm->guest_mem = (uint8_t *)guest_mem;
>  	/* For non-PV guests we re-use the host's ASCE for ease of use */
>  	vm->save_area.guest.asce = stctg(1);
>  	/* Currently MSO/MSL is the easiest option */

Re: [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early

[Claudio Imbrenda]

On Wed, 23 Nov 2022 08:46:55 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> When setting the first stack frame to 0, we can check for a 0
> backchain pointer when doing backtraces to know when to stop.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/s390x/stack.c | 2 ++
>  s390x/cstart64.S  | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c
> index e714e07c..9f234a12 100644
> --- a/lib/s390x/stack.c
> +++ b/lib/s390x/stack.c
> @@ -22,6 +22,8 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	for (depth = 0; stack && depth < max_depth; depth++) {
>  		return_addrs[depth] = (void *)stack->grs[8];
>  		stack = stack->back_chain;
> +		if (!stack)
> +			break;
>  	}
>  
>  	return depth;
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 666a9567..6f83da2a 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -37,6 +37,8 @@ start:
>  	sam64				# Set addressing mode to 64 bit
>  	/* setup stack */
>  	larl	%r15, stackptr
> +	/* Clear first stack frame */
> +	xc      0(160,%r15), 0(%r15)
>  	/* setup initial PSW mask + control registers*/
>  	larl	%r1, initial_psw
>  	lpswe	0(%r1)

Re: [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly

[Claudio Imbrenda]

On Wed, 23 Nov 2022 08:46:56 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> When we leave SIE due to an exception, we'll still have guest values
> in registers 0 - 13 and that's not clearly portraied in our debug
> prints. So let's fix that.

wouldn't it be cleaner to restore the registers in the interrupt
handler? (I thought we were already doing it)

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/interrupt.c | 46 ++++++++++++++++++++++++++++++++++++++-----
>  lib/s390x/sie.h       |  2 ++
>  s390x/cpu.S           |  6 ++++--
>  3 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index dadb7415..ff47c2c2 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -9,6 +9,7 @@
>   */
>  #include <libcflat.h>
>  #include <asm/barrier.h>
> +#include <asm/asm-offsets.h>
>  #include <sclp.h>
>  #include <interrupt.h>
>  #include <sie.h>
> @@ -188,9 +189,12 @@ static void print_storage_exception_information(void)
>  	}
>  }
>  
> -static void print_int_regs(struct stack_frame_int *stack)
> +static void print_int_regs(struct stack_frame_int *stack, bool sie)
>  {
> +	struct kvm_s390_sie_block *sblk;
> +
>  	printf("\n");
> +	printf("%s\n", sie ? "Guest registers:" : "Host registers:");
>  	printf("GPRS:\n");
>  	printf("%016lx %016lx %016lx %016lx\n",
>  	       stack->grs1[0], stack->grs1[1], stack->grs0[0], stack->grs0[1]);
> @@ -198,24 +202,56 @@ static void print_int_regs(struct stack_frame_int *stack)
>  	       stack->grs0[2], stack->grs0[3], stack->grs0[4], stack->grs0[5]);
>  	printf("%016lx %016lx %016lx %016lx\n",
>  	       stack->grs0[6], stack->grs0[7], stack->grs0[8], stack->grs0[9]);
> -	printf("%016lx %016lx %016lx %016lx\n",
> -	       stack->grs0[10], stack->grs0[11], stack->grs0[12], stack->grs0[13]);
> +
> +	if (sie) {
> +		sblk = (struct kvm_s390_sie_block *)stack->grs0[12];
> +		printf("%016lx %016lx %016lx %016lx\n",
> +		       stack->grs0[10], stack->grs0[11], sblk->gg14, sblk->gg15);
> +	} else {
> +		printf("%016lx %016lx %016lx %016lx\n",
> +		       stack->grs0[10], stack->grs0[11], stack->grs0[12], stack->grs0[13]);
> +	}
> +
>  	printf("\n");
>  }
>  
>  static void print_pgm_info(struct stack_frame_int *stack)
>  
>  {
> -	bool in_sie;
> +	bool in_sie, in_sie_gregs;
> +	struct vm_save_area *vregs;
>  
>  	in_sie = (lowcore.pgm_old_psw.addr >= (uintptr_t)sie_entry &&
>  		  lowcore.pgm_old_psw.addr <= (uintptr_t)sie_exit);
> +	in_sie_gregs = (lowcore.pgm_old_psw.addr >= (uintptr_t)sie_entry_gregs &&
> +			lowcore.pgm_old_psw.addr <= (uintptr_t)sie_exit_gregs);
>  
>  	printf("\n");
>  	printf("Unexpected program interrupt %s: %#x on cpu %d at %#lx, ilen %d\n",
>  	       in_sie ? "in SIE" : "",
>  	       lowcore.pgm_int_code, stap(), lowcore.pgm_old_psw.addr, lowcore.pgm_int_id);
> -	print_int_regs(stack);
> +
> +	/*
> +	 * If we fall out of SIE before loading the host registers,
> +	 * then we need to do it here so we print the host registers
> +	 * and not the guest registers.
> +	 *
> +	 * Back tracing is actually not a problem since SIE restores gr15.
> +	 */
> +	if (in_sie_gregs) {
> +		print_int_regs(stack, true);
> +		vregs = *((struct vm_save_area **)(stack->grs0[13] + __SF_SIE_SAVEAREA));
> +
> +		/*
> +		 * The grs are not linear on the interrupt stack frame.
> +		 * We copy 0 and 1 here and 2 - 15 with the memcopy below.
> +		 */
> +		stack->grs1[0] = vregs->host.grs[0];
> +		stack->grs1[1] = vregs->host.grs[1];
> +		/*  2 - 15 */
> +		memcpy(stack->grs0, &vregs->host.grs[2], sizeof(stack->grs0) - 8);
> +	}
> +	print_int_regs(stack, false);
>  	dump_stack();
>  
>  	/* Dump stack doesn't end with a \n so we add it here instead */
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index a27a8401..147cb0f2 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -273,6 +273,8 @@ struct vm {
>  
>  extern void sie_entry(void);
>  extern void sie_exit(void);
> +extern void sie_entry_gregs(void);
> +extern void sie_exit_gregs(void);
>  extern void sie64a(struct kvm_s390_sie_block *sblk, struct vm_save_area *save_area);
>  void sie(struct vm *vm);
>  void sie_expect_validity(struct vm *vm);
> diff --git a/s390x/cpu.S b/s390x/cpu.S
> index 45bd551a..9155b044 100644
> --- a/s390x/cpu.S
> +++ b/s390x/cpu.S
> @@ -82,7 +82,8 @@ sie64a:
>  	# Store scb and save_area pointer into stack frame
>  	stg	%r2,__SF_SIE_CONTROL(%r15)	# save control block pointer
>  	stg	%r3,__SF_SIE_SAVEAREA(%r15)	# save guest register save area
> -
> +.globl sie_entry_gregs
> +sie_entry_gregs:
>  	# Load guest's gprs, fprs and fpc
>  	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
>  	ld	\i, \i * 8 + SIE_SAVEAREA_GUEST_FPRS(%r3)
> @@ -121,7 +122,8 @@ sie_exit:
>  	.endr
>  	lfpc	SIE_SAVEAREA_HOST_FPC(%r14)
>  	lmg	%r0,%r14,SIE_SAVEAREA_HOST_GRS(%r14)	# restore kernel registers
> -
> +.globl sie_exit_gregs
> +sie_exit_gregs:
>  	br	%r14
>  
>  	.align	8

Re: [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro

[Claudio Imbrenda]

On Wed, 23 Nov 2022 08:46:53 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's store the psw mask instead of the address of the location where we
> should load the mask from.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  s390x/snippets/asm/macros.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/snippets/asm/macros.S b/s390x/snippets/asm/macros.S
> index 667fb6dc..09d7f5be 100644
> --- a/s390x/snippets/asm/macros.S
> +++ b/s390x/snippets/asm/macros.S
> @@ -18,7 +18,7 @@
>   */
>  .macro SET_PSW_NEW_ADDR reg, psw_new_addr, addr_psw
>  larl	\reg, psw_mask_64
> -stg	\reg, \addr_psw
> +mvc	\addr_psw(8,%r0), 0(\reg)
>  larl	\reg, \psw_new_addr
>  stg	\reg, \addr_psw + 8
>  .endm

Re: [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets

[Janosch Frank]

On 11/23/22 13:45, Claudio Imbrenda wrote:
> On Wed, 23 Nov 2022 08:46:52 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> A linker script has a few benefits:
>> - Random data doesn't end up in the binary breaking tests
>> - We can easily define a lowcore and load the snippet from 0x0 instead
>> of 0x4000 which makes asm snippets behave like c snippets
>> - We can easily define an invalid PGM new PSW to ensure an exit on a
>> guest PGM
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> 
> looks good in general, but I have a few questions
> 
>> ---
>>   lib/s390x/snippet.h         |  3 +--
>>   s390x/Makefile              |  5 +++--
>>   s390x/mvpg-sie.c            |  2 +-
>>   s390x/pv-diags.c            |  6 +++---
>>   s390x/snippets/asm/flat.lds | 43 +++++++++++++++++++++++++++++++++++++
>>   5 files changed, 51 insertions(+), 8 deletions(-)
>>   create mode 100644 s390x/snippets/asm/flat.lds
>>
>> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h
>> index b17b2a4c..57045994 100644
>> --- a/lib/s390x/snippet.h
>> +++ b/lib/s390x/snippet.h
>> @@ -32,8 +32,7 @@
>>   
>>   #define SNIPPET_PV_TWEAK0	0x42UL
>>   #define SNIPPET_PV_TWEAK1	0UL
>> -#define SNIPPET_OFF_C		0
>> -#define SNIPPET_OFF_ASM		0x4000
>> +#define SNIPPET_UNPACK_OFF	0
>>   
>>   
>>   /*
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index bf1504f9..bb0f9eb8 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -135,7 +135,8 @@ $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
>>   	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
>>   
>>   $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o
>> -	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
>> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $(patsubst %.gbin,%.o,$@)
> 
> I think you can simply use $< instead of the patsubst expression

Right, I'll fix that momentarily.

[...]

>> +	.text : {
>> +		*(.text)
>> +		*(.text.*)
>> +	}
>> +	. = ALIGN(64K);
> 
> any reason to align to 64k? (instead of e.g. 4k)

Well, I've copied that from s390x/flat.lds...
I'll have a look at the required alignments when I find time.

> 
>> +	etext = .;
>> +	. = ALIGN(16);
> 
> do you need the ALIGN? I would think we are already aligned here
> 
>> +	.data : {
>> +		*(.data)
>> +		*(.data.rel*)
>> +	}
>> +	. = ALIGN(16);
>> +	.rodata : { *(.rodata) *(.rodata.*) }
>> +	. = ALIGN(16);
>> +	__bss_start = .;
>> +	.bss : { *(.bss) }
>> +	__bss_end = .;
>> +	. = ALIGN(64K);
> 
> same question as above regarding 64k
> 
>> +}
> 

Re: [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets

[Janis Schoetterl-Glausch]

On Wed, 2022-11-23 at 08:46 +0000, Janosch Frank wrote:
> A linker script has a few benefits:
> - Random data doesn't end up in the binary breaking tests
> - We can easily define a lowcore and load the snippet from 0x0 instead
> of 0x4000 which makes asm snippets behave like c snippets
> - We can easily define an invalid PGM new PSW to ensure an exit on a
> guest PGM
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/snippet.h         |  3 +--
>  s390x/Makefile              |  5 +++--
>  s390x/mvpg-sie.c            |  2 +-
>  s390x/pv-diags.c            |  6 +++---
>  s390x/snippets/asm/flat.lds | 43 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 51 insertions(+), 8 deletions(-)
>  create mode 100644 s390x/snippets/asm/flat.lds
> 
> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h
> index b17b2a4c..57045994 100644
> --- a/lib/s390x/snippet.h
> +++ b/lib/s390x/snippet.h
> @@ -32,8 +32,7 @@
> 
>  #define SNIPPET_PV_TWEAK0	0x42UL
>  #define SNIPPET_PV_TWEAK1	0UL
> -#define SNIPPET_OFF_C		0
> -#define SNIPPET_OFF_ASM		0x4000
> +#define SNIPPET_UNPACK_OFF	0

You could also get rid of the offset parameter, couldn't you?
> 
> 
>  /*
> diff --git a/s390x/Makefile b/s390x/Makefile
> index bf1504f9..bb0f9eb8 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -135,7 +135,8 @@ $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
>  	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
> 
>  $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o
> -	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $(patsubst %.gbin,%.o,$@)
> +	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@

I assume .bss=alloc allocates the bss in the binary...

>  	truncate -s '%4096' $@
> 
>  $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
> @@ -144,7 +145,7 @@ $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
>  	truncate -s '%4096' $@
> 
>  $(SNIPPET_DIR)/asm/%.hdr: $(SNIPPET_DIR)/asm/%.gbin $(HOST_KEY_DOCUMENT)
> -	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x4000,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
> +	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
> 
>  $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
>  	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
> 
[...]

> diff --git a/s390x/snippets/asm/flat.lds b/s390x/snippets/asm/flat.lds
> new file mode 100644
> index 00000000..366d2d78
> --- /dev/null
> +++ b/s390x/snippets/asm/flat.lds
> @@ -0,0 +1,43 @@
> +SECTIONS
> +{
> +	.lowcore : {
> +		/*
> +		 * Initial short psw for disk boot, with 31 bit addressing for
> +		 * non z/Arch environment compatibility and the instruction
> +		 * address 0x4000.
> +		 */
> +		. = 0;
> +		 LONG(0x00080000)
> +		 LONG(0x80004000)
> +		 /* Restart new PSW for booting via PSW restart. */
> +		 . = 0x1a0;
> +		 QUAD(0x0000000180000000)
> +		 QUAD(0x0000000000004000)
> +		 /*
> +		  * Invalid PGM new PSW so we hopefully get a code 8
> +		  * intercept on a PGM
> +		  */
> +		 . = 0x1d0;
> +		 QUAD(0x0008000000000000)
> +		 QUAD(0x0000000000000001)
> +	}
> +	. = 0x4000;
> +	.text : {
> +		*(.text)
> +		*(.text.*)
> +	}
> +	. = ALIGN(64K);
> +	etext = .;
> +	. = ALIGN(16);
> +	.data : {
> +		*(.data)
> +		*(.data.rel*)
> +	}
> +	. = ALIGN(16);
> +	.rodata : { *(.rodata) *(.rodata.*) }
> +	. = ALIGN(16);
> +	__bss_start = .;

.. so the __bss symbols are not necessary.
But then, the c flat.lds has them too.
> +	.bss : { *(.bss) }
> +	__bss_end = .;
> +	. = ALIGN(64K);
> +}

Re: [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets

[Janosch Frank]

On 11/24/22 21:42, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-11-23 at 08:46 +0000, Janosch Frank wrote:
>> A linker script has a few benefits:
>> - Random data doesn't end up in the binary breaking tests
>> - We can easily define a lowcore and load the snippet from 0x0 instead
>> of 0x4000 which makes asm snippets behave like c snippets
>> - We can easily define an invalid PGM new PSW to ensure an exit on a
>> guest PGM
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/snippet.h         |  3 +--
>>   s390x/Makefile              |  5 +++--
>>   s390x/mvpg-sie.c            |  2 +-
>>   s390x/pv-diags.c            |  6 +++---
>>   s390x/snippets/asm/flat.lds | 43 +++++++++++++++++++++++++++++++++++++
>>   5 files changed, 51 insertions(+), 8 deletions(-)
>>   create mode 100644 s390x/snippets/asm/flat.lds
>>
>> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h
>> index b17b2a4c..57045994 100644
>> --- a/lib/s390x/snippet.h
>> +++ b/lib/s390x/snippet.h
>> @@ -32,8 +32,7 @@
>>
>>   #define SNIPPET_PV_TWEAK0	0x42UL
>>   #define SNIPPET_PV_TWEAK1	0UL
>> -#define SNIPPET_OFF_C		0
>> -#define SNIPPET_OFF_ASM		0x4000
>> +#define SNIPPET_UNPACK_OFF	0
> 
> You could also get rid of the offset parameter, couldn't you?

Right

>>
>>
>>   /*
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index bf1504f9..bb0f9eb8 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -135,7 +135,8 @@ $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
>>   	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
>>
>>   $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o
>> -	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
>> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $(patsubst %.gbin,%.o,$@)
>> +	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@
> 
> I assume .bss=alloc allocates the bss in the binary...

And that's fine since I don't want to handle 0x3E PGMs/faults.

If bss is in the binary then it'll be made secure on initial image 
unpack, if it isn't, then we need a handler to import pages on a 0x3E. 
And I don't really want to do that since a 0x3E could also mean that we 
have an issue with the test or HW/FW.

> 
>>   	truncate -s '%4096' $@
>>
>>   $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
>> @@ -144,7 +145,7 @@ $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
>>   	truncate -s '%4096' $@
>>
>>   $(SNIPPET_DIR)/asm/%.hdr: $(SNIPPET_DIR)/asm/%.gbin $(HOST_KEY_DOCUMENT)
>> -	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x4000,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
>> +	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
>>
>>   $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
>>   	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
>>
> [...]
> 
>> diff --git a/s390x/snippets/asm/flat.lds b/s390x/snippets/asm/flat.lds
>> new file mode 100644
>> index 00000000..366d2d78
>> --- /dev/null
>> +++ b/s390x/snippets/asm/flat.lds
>> @@ -0,0 +1,43 @@
>> +SECTIONS
>> +{
>> +	.lowcore : {
>> +		/*
>> +		 * Initial short psw for disk boot, with 31 bit addressing for
>> +		 * non z/Arch environment compatibility and the instruction
>> +		 * address 0x4000.
>> +		 */
>> +		. = 0;
>> +		 LONG(0x00080000)
>> +		 LONG(0x80004000)
>> +		 /* Restart new PSW for booting via PSW restart. */
>> +		 . = 0x1a0;
>> +		 QUAD(0x0000000180000000)
>> +		 QUAD(0x0000000000004000)
>> +		 /*
>> +		  * Invalid PGM new PSW so we hopefully get a code 8
>> +		  * intercept on a PGM
>> +		  */
>> +		 . = 0x1d0;
>> +		 QUAD(0x0008000000000000)
>> +		 QUAD(0x0000000000000001)
>> +	}
>> +	. = 0x4000;
>> +	.text : {
>> +		*(.text)
>> +		*(.text.*)
>> +	}
>> +	. = ALIGN(64K);
>> +	etext = .;
>> +	. = ALIGN(16);
>> +	.data : {
>> +		*(.data)
>> +		*(.data.rel*)
>> +	}
>> +	. = ALIGN(16);
>> +	.rodata : { *(.rodata) *(.rodata.*) }
>> +	. = ALIGN(16);
>> +	__bss_start = .;
> 
> .. so the __bss symbols are not necessary.
> But then, the c flat.lds has them too.
>> +	.bss : { *(.bss) }
>> +	__bss_end = .;
>> +	. = ALIGN(64K);
>> +}
> 

Re: [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly

[Janosch Frank]

On 11/23/22 14:01, Claudio Imbrenda wrote:
> On Wed, 23 Nov 2022 08:46:56 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> When we leave SIE due to an exception, we'll still have guest values
>> in registers 0 - 13 and that's not clearly portraied in our debug
>> prints. So let's fix that.
> 
> wouldn't it be cleaner to restore the registers in the interrupt
> handler? (I thought we were already doing it)

You mean RESTORE_REGS_STACK? Please don't make me write this in assembly...

RESTORE_REGS_STACK is doing test/pgm register swapping, it doesn't care 
if the test registers are "host" or "guest" registers.

Re: [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly

[Claudio Imbrenda]

On Wed, 30 Nov 2022 15:38:53 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/23/22 14:01, Claudio Imbrenda wrote:
> > On Wed, 23 Nov 2022 08:46:56 +0000
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> When we leave SIE due to an exception, we'll still have guest values
> >> in registers 0 - 13 and that's not clearly portraied in our debug
> >> prints. So let's fix that.  
> > 
> > wouldn't it be cleaner to restore the registers in the interrupt
> > handler? (I thought we were already doing it)  
> 
> You mean RESTORE_REGS_STACK? Please don't make me write this in assembly...
> 
> RESTORE_REGS_STACK is doing test/pgm register swapping, it doesn't care 
> if the test registers are "host" or "guest" registers.

fair enough :)