* [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets
2022-11-23 8:46 [kvm-unit-tests PATCH 0/5] s390x: Snippet fixes Janosch Frank
@ 2022-11-23 8:46 ` Janosch Frank
2022-11-23 12:45 ` Claudio Imbrenda
2022-11-24 20:42 ` Janis Schoetterl-Glausch
2022-11-23 8:46 ` [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro Janosch Frank
` (3 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Janosch Frank @ 2022-11-23 8:46 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden, nrb
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets
2022-11-23 8:46 ` [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets Janosch Frank
@ 2022-11-23 12:45 ` Claudio Imbrenda
2022-11-24 14:28 ` Janosch Frank
2022-11-24 20:42 ` Janis Schoetterl-Glausch
1 sibling, 1 reply; 19+ messages in thread
From: Claudio Imbrenda @ 2022-11-23 12:45 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, linux-s390, david, thuth, seiden, nrb
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
> +}
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets
2022-11-23 12:45 ` Claudio Imbrenda
@ 2022-11-24 14:28 ` Janosch Frank
0 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2022-11-24 14:28 UTC (permalink / raw)
To: Claudio Imbrenda; +Cc: kvm, linux-s390, david, thuth, seiden, nrb
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
>
>> +}
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets
2022-11-23 8:46 ` [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets Janosch Frank
2022-11-23 12:45 ` Claudio Imbrenda
@ 2022-11-24 20:42 ` Janis Schoetterl-Glausch
2022-11-25 10:07 ` Janosch Frank
1 sibling, 1 reply; 19+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-11-24 20:42 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden, nrb
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);
> +}
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets
2022-11-24 20:42 ` Janis Schoetterl-Glausch
@ 2022-11-25 10:07 ` Janosch Frank
0 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2022-11-25 10:07 UTC (permalink / raw)
To: Janis Schoetterl-Glausch, kvm
Cc: linux-s390, imbrenda, david, thuth, seiden, nrb
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);
>> +}
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro
2022-11-23 8:46 [kvm-unit-tests PATCH 0/5] s390x: Snippet fixes Janosch Frank
2022-11-23 8:46 ` [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets Janosch Frank
@ 2022-11-23 8:46 ` Janosch Frank
2022-11-23 10:58 ` Nico Boehr
2022-11-23 13:05 ` Claudio Imbrenda
2022-11-23 8:46 ` [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer Janosch Frank
` (2 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Janosch Frank @ 2022-11-23 8:46 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden, nrb
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro
2022-11-23 8:46 ` [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro Janosch Frank
@ 2022-11-23 10:58 ` Nico Boehr
2022-11-23 13:05 ` Claudio Imbrenda
1 sibling, 0 replies; 19+ messages in thread
From: Nico Boehr @ 2022-11-23 10:58 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro
2022-11-23 8:46 ` [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro Janosch Frank
2022-11-23 10:58 ` Nico Boehr
@ 2022-11-23 13:05 ` Claudio Imbrenda
1 sibling, 0 replies; 19+ messages in thread
From: Claudio Imbrenda @ 2022-11-23 13:05 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, linux-s390, david, thuth, seiden, nrb
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer
2022-11-23 8:46 [kvm-unit-tests PATCH 0/5] s390x: Snippet fixes Janosch Frank
2022-11-23 8:46 ` [kvm-unit-tests PATCH 1/5] s390x: Add a linker script to assembly snippets Janosch Frank
2022-11-23 8:46 ` [kvm-unit-tests PATCH 2/5] s390x: snippets: Fix SET_PSW_NEW_ADDR macro Janosch Frank
@ 2022-11-23 8:46 ` Janosch Frank
2022-11-23 11:01 ` Nico Boehr
2022-11-23 12:46 ` Claudio Imbrenda
2022-11-23 8:46 ` [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early Janosch Frank
2022-11-23 8:46 ` [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly Janosch Frank
4 siblings, 2 replies; 19+ messages in thread
From: Janosch Frank @ 2022-11-23 8:46 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden, nrb
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer
2022-11-23 8:46 ` [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer Janosch Frank
@ 2022-11-23 11:01 ` Nico Boehr
2022-11-23 12:46 ` Claudio Imbrenda
1 sibling, 0 replies; 19+ messages in thread
From: Nico Boehr @ 2022-11-23 11:01 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer
2022-11-23 8:46 ` [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer Janosch Frank
2022-11-23 11:01 ` Nico Boehr
@ 2022-11-23 12:46 ` Claudio Imbrenda
1 sibling, 0 replies; 19+ messages in thread
From: Claudio Imbrenda @ 2022-11-23 12:46 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, linux-s390, david, thuth, seiden, nrb
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 */
^ permalink raw reply [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early
2022-11-23 8:46 [kvm-unit-tests PATCH 0/5] s390x: Snippet fixes Janosch Frank
` (2 preceding siblings ...)
2022-11-23 8:46 ` [kvm-unit-tests PATCH 3/5] lib: s390x: sie: Set guest memory pointer Janosch Frank
@ 2022-11-23 8:46 ` Janosch Frank
2022-11-23 11:05 ` Nico Boehr
2022-11-23 12:47 ` Claudio Imbrenda
2022-11-23 8:46 ` [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly Janosch Frank
4 siblings, 2 replies; 19+ messages in thread
From: Janosch Frank @ 2022-11-23 8:46 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden, nrb
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early
2022-11-23 8:46 ` [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early Janosch Frank
@ 2022-11-23 11:05 ` Nico Boehr
2022-11-23 12:47 ` Claudio Imbrenda
1 sibling, 0 replies; 19+ messages in thread
From: Nico Boehr @ 2022-11-23 11:05 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early
2022-11-23 8:46 ` [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early Janosch Frank
2022-11-23 11:05 ` Nico Boehr
@ 2022-11-23 12:47 ` Claudio Imbrenda
1 sibling, 0 replies; 19+ messages in thread
From: Claudio Imbrenda @ 2022-11-23 12:47 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, linux-s390, david, thuth, seiden, nrb
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)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly
2022-11-23 8:46 [kvm-unit-tests PATCH 0/5] s390x: Snippet fixes Janosch Frank
` (3 preceding siblings ...)
2022-11-23 8:46 ` [kvm-unit-tests PATCH 4/5] s390x: Clear first stack frame and end backtrace early Janosch Frank
@ 2022-11-23 8:46 ` Janosch Frank
2022-11-23 13:01 ` Claudio Imbrenda
4 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-11-23 8:46 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, imbrenda, david, thuth, seiden, nrb
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly
2022-11-23 8:46 ` [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly Janosch Frank
@ 2022-11-23 13:01 ` Claudio Imbrenda
2022-11-30 14:38 ` Janosch Frank
0 siblings, 1 reply; 19+ messages in thread
From: Claudio Imbrenda @ 2022-11-23 13:01 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, linux-s390, david, thuth, seiden, nrb
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly
2022-11-23 13:01 ` Claudio Imbrenda
@ 2022-11-30 14:38 ` Janosch Frank
2022-11-30 14:46 ` Claudio Imbrenda
0 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2022-11-30 14:38 UTC (permalink / raw)
To: Claudio Imbrenda; +Cc: kvm, linux-s390, david, thuth, seiden, nrb
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH 5/5] lib: s390x: Handle debug prints for SIE exceptions correctly
2022-11-30 14:38 ` Janosch Frank
@ 2022-11-30 14:46 ` Claudio Imbrenda
0 siblings, 0 replies; 19+ messages in thread
From: Claudio Imbrenda @ 2022-11-30 14:46 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, linux-s390, david, thuth, seiden, nrb
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 :)
^ permalink raw reply [flat|nested] 19+ messages in thread