[kvm-unit-tests PATCH 0/6] s390x: PV fixups

[kvm-unit-tests PATCH 0/6] s390x: PV fixups

[Janosch Frank]

A small set of patches that clean up the PV snippet handling.

Janosch Frank (6):
  s390x: snippets: asm: Add a macro to write an exception PSW
  s390x: MAKEFILE: Use $< instead of pathsubst
  s390x: Add a linker script to assembly snippets
  lib: s390x: sie: Improve validity handling and make it vm specific
  lib: s390x: Use a new asce for each PV guest
  lib: s390x: sie: Properly populate SCA

 lib/s390x/asm-offsets.c                  |  2 ++
 lib/s390x/sie.c                          | 36 +++++++++++++-------
 lib/s390x/sie.h                          | 43 ++++++++++++++++++++++--
 lib/s390x/snippet.h                      |  3 +-
 lib/s390x/uv.c                           | 35 +++++++++++++++++--
 lib/s390x/uv.h                           |  5 ++-
 s390x/Makefile                           | 18 +++++++---
 s390x/cpu.S                              |  6 ++++
 s390x/mvpg-sie.c                         |  2 +-
 s390x/pv-diags.c                         |  6 ++--
 s390x/snippets/asm/macros.S              | 28 +++++++++++++++
 s390x/snippets/asm/snippet-pv-diag-288.S |  4 +--
 s390x/snippets/asm/snippet-pv-diag-500.S |  6 ++--
 13 files changed, 157 insertions(+), 37 deletions(-)
 create mode 100644 s390x/snippets/asm/macros.S

-- 
2.34.1

[kvm-unit-tests PATCH 1/6] s390x: snippets: asm: Add a macro to write an exception PSW

[Janosch Frank]

Setting exception new PSWs is commonly needed so let's add a macro for
that.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/snippets/asm/macros.S              | 28 ++++++++++++++++++++++++
 s390x/snippets/asm/snippet-pv-diag-288.S |  4 ++--
 s390x/snippets/asm/snippet-pv-diag-500.S |  6 ++---
 3 files changed, 32 insertions(+), 6 deletions(-)
 create mode 100644 s390x/snippets/asm/macros.S

diff --git a/s390x/snippets/asm/macros.S b/s390x/snippets/asm/macros.S
new file mode 100644
index 00000000..667fb6dc
--- /dev/null
+++ b/s390x/snippets/asm/macros.S
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Commonly used assembly macros
+ *
+ * Copyright (c) 2022 IBM Corp
+ *
+ * Authors:
+ *  Janosch Frank <frankja@linux.ibm.com>
+ */
+#include <asm/asm-offsets.h>
+
+/*
+ * Writes a PSW to addr_psw, useful for exception PSWs in lowcore
+ *
+ * reg is the scratch register used for temporary storage, it's NOT restored
+ * The psw address part is defined via psw_new_addr
+ * The psw mask part is always 64 bit
+ */
+.macro SET_PSW_NEW_ADDR reg, psw_new_addr, addr_psw
+larl	\reg, psw_mask_64
+stg	\reg, \addr_psw
+larl	\reg, \psw_new_addr
+stg	\reg, \addr_psw + 8
+.endm
+
+.section .rodata
+psw_mask_64:
+	.quad	0x0000000180000000
diff --git a/s390x/snippets/asm/snippet-pv-diag-288.S b/s390x/snippets/asm/snippet-pv-diag-288.S
index aaee3cd1..63f2113b 100644
--- a/s390x/snippets/asm/snippet-pv-diag-288.S
+++ b/s390x/snippets/asm/snippet-pv-diag-288.S
@@ -8,6 +8,7 @@
  *  Janosch Frank <frankja@linux.ibm.com>
  */
 #include <asm/asm-offsets.h>
+#include "macros.S"
 .section .text
 
 /* Clean and pre-load registers that are used for diag 288 */
@@ -19,8 +20,7 @@ lghi	%r1, 2
 lghi	%r2, 3
 
 /* Let's jump to the pgm exit label on a PGM */
-larl	%r4, exit_pgm
-stg     %r4, GEN_LC_PGM_NEW_PSW + 8
+SET_PSW_NEW_ADDR 4, exit_pgm, GEN_LC_PGM_NEW_PSW
 
 /* Execute the diag288 */
 diag	%r0, %r2, 0x288
diff --git a/s390x/snippets/asm/snippet-pv-diag-500.S b/s390x/snippets/asm/snippet-pv-diag-500.S
index 8dd66bd9..f4d75388 100644
--- a/s390x/snippets/asm/snippet-pv-diag-500.S
+++ b/s390x/snippets/asm/snippet-pv-diag-500.S
@@ -8,6 +8,7 @@
  *  Janosch Frank <frankja@linux.ibm.com>
  */
 #include <asm/asm-offsets.h>
+#include "macros.S"
 .section .text
 
 /* Clean and pre-load registers that are used for diag 500 */
@@ -21,10 +22,7 @@ lghi	%r3, 3
 lghi	%r4, 4
 
 /* Let's jump to the next label on a PGM */
-xgr	%r5, %r5
-stg	%r5, GEN_LC_PGM_NEW_PSW
-larl	%r5, next
-stg	%r5, GEN_LC_PGM_NEW_PSW + 8
+SET_PSW_NEW_ADDR 5, next, GEN_LC_PGM_NEW_PSW
 
 /* Execute the diag500 */
 diag	0, 0, 0x500
-- 
2.34.1

Re: [kvm-unit-tests PATCH 1/6] s390x: snippets: asm: Add a macro to write an exception PSW

[Nico Boehr]

Quoting Janosch Frank (2022-07-29 10:26:28)
[...]
> diff --git a/s390x/snippets/asm/snippet-pv-diag-500.S b/s390x/snippets/asm/snippet-pv-diag-500.S
> index 8dd66bd9..f4d75388 100644
> --- a/s390x/snippets/asm/snippet-pv-diag-500.S
> +++ b/s390x/snippets/asm/snippet-pv-diag-500.S
> @@ -8,6 +8,7 @@
>   *  Janosch Frank <frankja@linux.ibm.com>
>   */
>  #include <asm/asm-offsets.h>
> +#include "macros.S"
>  .section .text
>  
>  /* Clean and pre-load registers that are used for diag 500 */
> @@ -21,10 +22,7 @@ lghi %r3, 3
>  lghi   %r4, 4
>  
>  /* Let's jump to the next label on a PGM */
> -xgr    %r5, %r5
> -stg    %r5, GEN_LC_PGM_NEW_PSW

So previously the PSW mask was zero and hence we had 24-bit addressing, no? Now, we have bits 31 and 32 one and hence 64 bit addressing.

I guess 24-bit addressing is not appropriate here (or at least doesn't matter too much), so I guess this is intended, isn't it?

Re: [kvm-unit-tests PATCH 1/6] s390x: snippets: asm: Add a macro to write an exception PSW

[Janosch Frank]

On 8/2/22 09:10, Nico Boehr wrote:
> Quoting Janosch Frank (2022-07-29 10:26:28)
> [...]
>> diff --git a/s390x/snippets/asm/snippet-pv-diag-500.S b/s390x/snippets/asm/snippet-pv-diag-500.S
>> index 8dd66bd9..f4d75388 100644
>> --- a/s390x/snippets/asm/snippet-pv-diag-500.S
>> +++ b/s390x/snippets/asm/snippet-pv-diag-500.S
>> @@ -8,6 +8,7 @@
>>    *  Janosch Frank <frankja@linux.ibm.com>
>>    */
>>   #include <asm/asm-offsets.h>
>> +#include "macros.S"
>>   .section .text
>>   
>>   /* Clean and pre-load registers that are used for diag 500 */
>> @@ -21,10 +22,7 @@ lghi %r3, 3
>>   lghi   %r4, 4
>>   
>>   /* Let's jump to the next label on a PGM */
>> -xgr    %r5, %r5
>> -stg    %r5, GEN_LC_PGM_NEW_PSW
> 
> So previously the PSW mask was zero and hence we had 24-bit addressing, no? Now, we have bits 31 and 32 one and hence 64 bit addressing.

Yes
Also the linker script patch will exchange the mask for an invalid one 
so we need to replace both the mask and the address.

> 
> I guess 24-bit addressing is not appropriate here (or at least doesn't matter too much), so I guess this is intended, isn't it?


Claudio complained about the addressing change so I moved it over to 
full 64 bit.

Re: [kvm-unit-tests PATCH 1/6] s390x: snippets: asm: Add a macro to write an exception PSW

[Nico Boehr]

Quoting Janosch Frank (2022-08-02 09:56:12)
> On 8/2/22 09:10, Nico Boehr wrote:
> > Quoting Janosch Frank (2022-07-29 10:26:28)
> > [...]
> >> diff --git a/s390x/snippets/asm/snippet-pv-diag-500.S b/s390x/snippets/asm/snippet-pv-diag-500.S
> >> index 8dd66bd9..f4d75388 100644
> >> --- a/s390x/snippets/asm/snippet-pv-diag-500.S
> >> +++ b/s390x/snippets/asm/snippet-pv-diag-500.S
> >> @@ -8,6 +8,7 @@
> >>    *  Janosch Frank <frankja@linux.ibm.com>
> >>    */
> >>   #include <asm/asm-offsets.h>
> >> +#include "macros.S"
> >>   .section .text
> >>   
> >>   /* Clean and pre-load registers that are used for diag 500 */
> >> @@ -21,10 +22,7 @@ lghi %r3, 3
> >>   lghi   %r4, 4
> >>   
> >>   /* Let's jump to the next label on a PGM */
> >> -xgr    %r5, %r5
> >> -stg    %r5, GEN_LC_PGM_NEW_PSW
> > 
> > So previously the PSW mask was zero and hence we had 24-bit addressing, no? Now, we have bits 31 and 32 one and hence 64 bit addressing.
> 
> Yes
> Also the linker script patch will exchange the mask for an invalid one 
> so we need to replace both the mask and the address.
> 
> > 
> > I guess 24-bit addressing is not appropriate here (or at least doesn't matter too much), so I guess this is intended, isn't it?

Allrighty, then:

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

[kvm-unit-tests PATCH 2/6] s390x: MAKEFILE: Use $< instead of pathsubst

[Janosch Frank]

No need to mangle strings if we already have the value at hand.

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

diff --git a/s390x/Makefile b/s390x/Makefile
index efd5e0c1..ee34a1d7 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -134,7 +134,7 @@ $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o
 	truncate -s '%4096' $@
 
 $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
-	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_lib) $(FLATLIBS)
+	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/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 $@ $@
 	truncate -s '%4096' $@
 
-- 
2.34.1

Re: [kvm-unit-tests PATCH 2/6] s390x: MAKEFILE: Use $< instead of pathsubst

[Steffen Eiden]

On 7/29/22 10:26, Janosch Frank wrote:
> No need to mangle strings if we already have the value at hand.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   s390x/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index efd5e0c1..ee34a1d7 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -134,7 +134,7 @@ $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o
>   	truncate -s '%4096' $@
>   
>   $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
> -	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_lib) $(FLATLIBS)
> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/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 $@ $@
>   	truncate -s '%4096' $@
>   

Re: [kvm-unit-tests PATCH 2/6] s390x: MAKEFILE: Use $< instead of pathsubst

[Nico Boehr]

Quoting Janosch Frank (2022-07-29 10:26:29)
> No need to mangle strings if we already have the value at hand.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

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

[Janosch Frank]

A linker script has a few benefits:
- 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

As we gain another step and file extension by linking, a comment
explains which file extensions are generated and why.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/snippet.h |  3 +--
 s390x/Makefile      | 16 +++++++++++++---
 s390x/mvpg-sie.c    |  2 +-
 s390x/pv-diags.c    |  6 +++---
 4 files changed, 18 insertions(+), 9 deletions(-)

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 ee34a1d7..9a0b48e2 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -122,6 +122,13 @@ else
 snippet-hdr-obj =
 endif
 
+# Each snippet will generate the following files (in order): \
+  *.o is a snippet that has been compiled \
+  *.ol is a snippet that has been linked \
+  *.gbin is a snippet that has been converted to binary \
+  *.gobj is the final format after converting the binary into a elf file again, \
+  it will be linked into the tests
+
 # 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 $@ $<
@@ -129,8 +136,11 @@ $(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
-	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
+$(SNIPPET_DIR)/asm/%.ol: $(SNIPPET_DIR)/asm/%.o
+	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $<
+
+$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.ol
+	$(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)
@@ -139,7 +149,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");
-- 
2.34.1

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

[Steffen Eiden]

On 7/29/22 10:26, Janosch Frank wrote:
> A linker script has a few benefits:
> - 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
> 
> As we gain another step and file extension by linking, a comment
> explains which file extensions are generated and why.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/snippet.h |  3 +--
>   s390x/Makefile      | 16 +++++++++++++---
>   s390x/mvpg-sie.c    |  2 +-
>   s390x/pv-diags.c    |  6 +++---
>   4 files changed, 18 insertions(+), 9 deletions(-)
> 
> 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 ee34a1d7..9a0b48e2 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -122,6 +122,13 @@ else
>   snippet-hdr-obj =
>   endif
>   
> +# Each snippet will generate the following files (in order): \
> +  *.o is a snippet that has been compiled \
> +  *.ol is a snippet that has been linked \
> +  *.gbin is a snippet that has been converted to binary \
> +  *.gobj is the final format after converting the binary into a elf file again, \
> +  it will be linked into the tests
> +
>   # 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 $@ $<
> @@ -129,8 +136,11 @@ $(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
> -	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
> +$(SNIPPET_DIR)/asm/%.ol: $(SNIPPET_DIR)/asm/%.o
> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $<

I think you forgot to include that linker script in the patch.
Or did I miss anything?

you can also use the $(SNIPPET_DIR) variable instead of searching for
that directory yourself.

> +
> +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.ol
> +	$(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)
> @@ -139,7 +149,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");

Steffen Eiden

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

[Janosch Frank]

On 7/29/22 16:17, Steffen Eiden wrote:
> 
> 
> On 7/29/22 10:26, Janosch Frank wrote:
>> A linker script has a few benefits:
>> - 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
>>
>> As we gain another step and file extension by linking, a comment
>> explains which file extensions are generated and why.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    lib/s390x/snippet.h |  3 +--
>>    s390x/Makefile      | 16 +++++++++++++---
>>    s390x/mvpg-sie.c    |  2 +-
>>    s390x/pv-diags.c    |  6 +++---
>>    4 files changed, 18 insertions(+), 9 deletions(-)
>>
>> 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 ee34a1d7..9a0b48e2 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -122,6 +122,13 @@ else
>>    snippet-hdr-obj =
>>    endif
>>    
>> +# Each snippet will generate the following files (in order): \
>> +  *.o is a snippet that has been compiled \
>> +  *.ol is a snippet that has been linked \
>> +  *.gbin is a snippet that has been converted to binary \
>> +  *.gobj is the final format after converting the binary into a elf file again, \
>> +  it will be linked into the tests
>> +
>>    # 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 $@ $<
>> @@ -129,8 +136,11 @@ $(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
>> -	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
>> +$(SNIPPET_DIR)/asm/%.ol: $(SNIPPET_DIR)/asm/%.o
>> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $<
> 
> I think you forgot to include that linker script in the patch.
> Or did I miss anything?
> 
> you can also use the $(SNIPPET_DIR) variable instead of searching for
> that directory yourself.

Right on both cases, I'll fix that...

> 
>> +
>> +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.ol
>> +	$(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)
>> @@ -139,7 +149,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");
> 
> Steffen Eiden

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

[Nico Boehr]

Quoting Janosch Frank (2022-07-29 10:26:30)
[...]
> 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);

Does it even make sense to pass SNIPPET_UNPACK_OFF? Since this is now the same for C and ASM we could completely get rid of this argument, couldn't we?

[kvm-unit-tests PATCH 4/6] lib: s390x: sie: Improve validity handling and make it vm specific

[Janosch Frank]

The current library doesn't support running multiple vms at once as it
stores the validity once and not per vm. Let's move the validity
handling into the vm and introduce a new function to retrieve the vir.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sie.c | 26 +++++++++++++-------------
 lib/s390x/sie.h |  6 ++++--
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
index 00aff713..c3a53ad6 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -15,19 +15,21 @@
 #include <libcflat.h>
 #include <alloc_page.h>
 
-static bool validity_expected;
-static uint16_t vir;		/* Validity interception reason */
-
-void sie_expect_validity(void)
+void sie_expect_validity(struct vm *vm)
 {
-	validity_expected = true;
-	vir = 0;
+	vm->validity_expected = true;
 }
 
-void sie_check_validity(uint16_t vir_exp)
+uint16_t sie_get_validity(struct vm *vm)
 {
+	return vm->sblk->ipb >> 16;
+}
+
+void sie_check_validity(struct vm *vm, uint16_t vir_exp)
+{
+	uint16_t vir = sie_get_validity(vm);
+
 	report(vir_exp == vir, "VALIDITY: %x", vir);
-	vir = 0;
 }
 
 void sie_handle_validity(struct vm *vm)
@@ -35,11 +37,9 @@ void sie_handle_validity(struct vm *vm)
 	if (vm->sblk->icptcode != ICPT_VALIDITY)
 		return;
 
-	vir = vm->sblk->ipb >> 16;
-
-	if (!validity_expected)
-		report_abort("VALIDITY: %x", vir);
-	validity_expected = false;
+	if (!vm->validity_expected)
+		report_abort("VALIDITY: %x", sie_get_validity(vm));
+	vm->validity_expected = false;
 }
 
 void sie(struct vm *vm)
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index de91ea5a..320c4218 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -233,14 +233,16 @@ struct vm {
 	struct vm_uv uv;			/* PV UV information */
 	/* Ptr to first guest page */
 	uint8_t *guest_mem;
+	bool validity_expected;
 };
 
 extern void sie_entry(void);
 extern void sie_exit(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(void);
-void sie_check_validity(uint16_t vir_exp);
+void sie_expect_validity(struct vm *vm);
+uint16_t sie_get_validity(struct vm *vm);
+void sie_check_validity(struct vm *vm, uint16_t vir_exp);
 void sie_handle_validity(struct vm *vm);
 void sie_guest_sca_create(struct vm *vm);
 void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
-- 
2.34.1

Re: [kvm-unit-tests PATCH 4/6] lib: s390x: sie: Improve validity handling and make it vm specific

[Steffen Eiden]

On 7/29/22 10:26, Janosch Frank wrote:
> The current library doesn't support running multiple vms at once as it
> stores the validity once and not per vm. Let's move the validity
> handling into the vm and introduce a new function to retrieve the vir.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

Re: [kvm-unit-tests PATCH 4/6] lib: s390x: sie: Improve validity handling and make it vm specific

[Nico Boehr]

Quoting Janosch Frank (2022-07-29 10:26:31)
> The current library doesn't support running multiple vms at once as it
> stores the validity once and not per vm. Let's move the validity
> handling into the vm and introduce a new function to retrieve the vir.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

One tiny suggestion below.

[...]
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index 00aff713..c3a53ad6 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -15,19 +15,21 @@
>  #include <libcflat.h>
>  #include <alloc_page.h>
>  
> -static bool validity_expected;
> -static uint16_t vir;           /* Validity interception reason */
> -
> -void sie_expect_validity(void)
> +void sie_expect_validity(struct vm *vm)
>  {
> -       validity_expected = true;
> -       vir = 0;
> +       vm->validity_expected = true;
>  }
>  
> -void sie_check_validity(uint16_t vir_exp)
> +uint16_t sie_get_validity(struct vm *vm)
>  {
> +       return vm->sblk->ipb >> 16;
> +}

Since one should only call this function when we had a validity, maybe add:

assert (vm->sblk->icptcode == ICPT_VALIDITY);

[kvm-unit-tests PATCH 5/6] lib: s390x: Use a new asce for each PV guest

[Janosch Frank]

Every PV guest needs its own ASCE so let's copy the topmost table
designated by CR1 to create a new ASCE for the PV guest. Before and
after SIE we now need to switch ASCEs to and from the PV guest / test
ASCE. The SIE assembly function does that automatically.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm-offsets.c |  2 ++
 lib/s390x/sie.c         |  2 ++
 lib/s390x/sie.h         |  2 ++
 lib/s390x/uv.c          | 35 +++++++++++++++++++++++++++++++++--
 lib/s390x/uv.h          |  5 ++---
 s390x/cpu.S             |  6 ++++++
 6 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
index fbea3278..f612f327 100644
--- a/lib/s390x/asm-offsets.c
+++ b/lib/s390x/asm-offsets.c
@@ -75,9 +75,11 @@ int main(void)
 	OFFSET(SIE_SAVEAREA_HOST_GRS, vm_save_area, host.grs[0]);
 	OFFSET(SIE_SAVEAREA_HOST_FPRS, vm_save_area, host.fprs[0]);
 	OFFSET(SIE_SAVEAREA_HOST_FPC, vm_save_area, host.fpc);
+	OFFSET(SIE_SAVEAREA_HOST_ASCE, vm_save_area, host.asce);
 	OFFSET(SIE_SAVEAREA_GUEST_GRS, vm_save_area, guest.grs[0]);
 	OFFSET(SIE_SAVEAREA_GUEST_FPRS, vm_save_area, guest.fprs[0]);
 	OFFSET(SIE_SAVEAREA_GUEST_FPC, vm_save_area, guest.fpc);
+	OFFSET(SIE_SAVEAREA_GUEST_ASCE, vm_save_area, guest.asce);
 	OFFSET(STACK_FRAME_INT_BACKCHAIN, stack_frame_int, back_chain);
 	OFFSET(STACK_FRAME_INT_FPC, stack_frame_int, fpc);
 	OFFSET(STACK_FRAME_INT_FPRS, stack_frame_int, fprs);
diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
index c3a53ad6..5ba669f1 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -84,6 +84,8 @@ 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));
+	/* 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 */
 	vm->sblk->mso = (uint64_t)guest_mem;
 	vm->sblk->msl = (uint64_t)guest_mem + ((guest_mem_len - 1) & HPAGE_MASK);
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index 320c4218..3e3605c9 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -205,12 +205,14 @@ union {
 struct vm_uv {
 	uint64_t vm_handle;
 	uint64_t vcpu_handle;
+	uint64_t asce;
 	void *conf_base_stor;
 	void *conf_var_stor;
 	void *cpu_stor;
 };
 
 struct vm_save_regs {
+	uint64_t asce;
 	uint64_t grs[16];
 	uint64_t fprs[16];
 	uint32_t fpc;
diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
index 3b4cafa9..99775929 100644
--- a/lib/s390x/uv.c
+++ b/lib/s390x/uv.c
@@ -76,7 +76,8 @@ void uv_init(void)
 	int cc;
 
 	/* Let's not do this twice */
-	assert(!initialized);
+	if (initialized)
+		return;
 	/* Query is done on initialization but let's check anyway */
 	assert(uvcb_qui.header.rc == 1 || uvcb_qui.header.rc == 0x100);
 
@@ -90,6 +91,25 @@ void uv_init(void)
 	initialized = true;
 }
 
+/*
+ * Create a new ASCE for the UV config because they can't be shared
+ * for security reasons. We just simply copy the top most table into a
+ * fresh set of allocated pages and use those pages as the asce.
+ */
+static uint64_t create_asce(void)
+{
+	void *pgd_new, *pgd_old;
+	uint64_t asce = stctg(1);
+
+	pgd_new = memalign_pages_flags(PAGE_SIZE, PAGE_SIZE * 4, 0);
+	pgd_old = (void *)(asce & PAGE_MASK);
+
+	memcpy(pgd_new, pgd_old, PAGE_SIZE * 4);
+
+	asce = __pa(pgd_new) | ASCE_DT_REGION1 | REGION_TABLE_LENGTH | ASCE_P;
+	return asce;
+}
+
 void uv_create_guest(struct vm *vm)
 {
 	struct uv_cb_cgc uvcb_cgc = {
@@ -125,7 +145,8 @@ void uv_create_guest(struct vm *vm)
 	vm->uv.cpu_stor = memalign_pages_flags(PAGE_SIZE, uvcb_qui.cpu_stor_len, 0);
 	uvcb_csc.stor_origin = (uint64_t)vm->uv.cpu_stor;
 
-	uvcb_cgc.guest_asce = (uint64_t)stctg(1);
+	uvcb_cgc.guest_asce = create_asce();
+	vm->save_area.guest.asce = uvcb_cgc.guest_asce;
 	uvcb_cgc.guest_sca = (uint64_t)vm->sca;
 
 	cc = uv_call(0, (uint64_t)&uvcb_cgc);
@@ -166,6 +187,16 @@ void uv_destroy_guest(struct vm *vm)
 	assert(cc == 0);
 	free_pages(vm->uv.conf_base_stor);
 	free_pages(vm->uv.conf_var_stor);
+
+	free_pages((void *)(vm->uv.asce & PAGE_MASK));
+	memset(&vm->uv, 0, sizeof(vm->uv));
+
+	/* Convert the sblk back to non-PV */
+	vm->save_area.guest.asce = stctg(1);
+	vm->sblk->sdf = 0;
+	vm->sblk->sidad = 0;
+	vm->sblk->pv_handle_cpu = 0;
+	vm->sblk->pv_handle_config = 0;
 }
 
 int uv_unpack(struct vm *vm, uint64_t addr, uint64_t len, uint64_t tweak)
diff --git a/lib/s390x/uv.h b/lib/s390x/uv.h
index 44264861..5fe29bda 100644
--- a/lib/s390x/uv.h
+++ b/lib/s390x/uv.h
@@ -28,9 +28,8 @@ static inline void uv_setup_asces(void)
 	/* We need to have a valid primary ASCE to run guests. */
 	setup_vm();
 
-	/* Set P bit in ASCE as it is required for PV guests */
-	asce = stctg(1) | ASCE_P;
-	lctlg(1, asce);
+	/* Grab the ASCE which setup_vm() just set up */
+	asce = stctg(1);
 
 	/* Copy ASCE into home space CR */
 	lctlg(13, asce);
diff --git a/s390x/cpu.S b/s390x/cpu.S
index 82b5e25d..45bd551a 100644
--- a/s390x/cpu.S
+++ b/s390x/cpu.S
@@ -76,6 +76,9 @@ sie64a:
 	.endr
 	stfpc	SIE_SAVEAREA_HOST_FPC(%r3)
 
+	stctg	%c1, %c1, SIE_SAVEAREA_HOST_ASCE(%r3)
+	lctlg	%c1, %c1, SIE_SAVEAREA_GUEST_ASCE(%r3)
+
 	# 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
@@ -102,6 +105,9 @@ sie_exit:
 	# Load guest register save area
 	lg	%r14,__SF_SIE_SAVEAREA(%r15)
 
+	# Restore the host asce
+	lctlg	%c1, %c1, SIE_SAVEAREA_HOST_ASCE(%r14)
+
 	# Store guest's gprs, fprs and fpc
 	stmg	%r0,%r13,SIE_SAVEAREA_GUEST_GRS(%r14)	# save guest gprs 0-13
 	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-- 
2.34.1

Re: [kvm-unit-tests PATCH 5/6] lib: s390x: Use a new asce for each PV guest

[Nico Boehr]

Quoting Janosch Frank (2022-07-29 10:26:32)
> Every PV guest needs its own ASCE so let's copy the topmost table
> designated by CR1 to create a new ASCE for the PV guest. Before and
> after SIE we now need to switch ASCEs to and from the PV guest / test
> ASCE. The SIE assembly function does that automatically.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

with two tiny things for you to consider.

[...]
> diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
> index 3b4cafa9..99775929 100644
> --- a/lib/s390x/uv.c
> +++ b/lib/s390x/uv.c
> @@ -76,7 +76,8 @@ void uv_init(void)
>         int cc;
>  
>         /* Let's not do this twice */
> -       assert(!initialized);
> +       if (initialized)
> +               return;

Caught me a bit by surprise, maybe you want to point out this change in the commit message.

[...]
> +/*
> + * Create a new ASCE for the UV config because they can't be shared
> + * for security reasons. We just simply copy the top most table into a
> + * fresh set of allocated pages and use those pages as the asce.
> + */
> +static uint64_t create_asce(void)
> +{
> +       void *pgd_new, *pgd_old;
> +       uint64_t asce = stctg(1);
> +
> +       pgd_new = memalign_pages_flags(PAGE_SIZE, PAGE_SIZE * 4, 0);
> +       pgd_old = (void *)(asce & PAGE_MASK);
> +
> +       memcpy(pgd_new, pgd_old, PAGE_SIZE * 4);
> +
> +       asce = __pa(pgd_new) | ASCE_DT_REGION1 | REGION_TABLE_LENGTH | ASCE_P;

I am wondering whether it might make sense to copy over the flags from cr1 so we don't have to worry about keeping them in sync?

But possibly it is sufficiently unlikely that these will ever change...

[kvm-unit-tests PATCH 6/6] lib: s390x: sie: Properly populate SCA

[Janosch Frank]

CPU0 is the only cpu that's being used but we should still mark it as
online and set the SDA in the SCA.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sie.c |  8 ++++++++
 lib/s390x/sie.h | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
index 5ba669f1..acfdff99 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -9,6 +9,7 @@
  */
 
 #include <asm/barrier.h>
+#include <bitops.h>
 #include <libcflat.h>
 #include <sie.h>
 #include <asm/page.h>
@@ -71,6 +72,13 @@ void sie_guest_sca_create(struct vm *vm)
 	vm->sblk->scaoh = ((uint64_t)vm->sca >> 32);
 	vm->sblk->scaol = (uint64_t)vm->sca & ~0x3fU;
 	vm->sblk->ecb2 |= ECB2_ESCA;
+
+	/* Enable SIGP sense running interpretation */
+	vm->sblk->ecb |= ECB_SRSI;
+
+	/* We assume that cpu 0 is always part of the vm */
+	vm->sca->mcn[0] = BIT(63);
+	vm->sca->cpu[0].sda = (uint64_t)vm->sblk;
 }
 
 /* Initializes the struct vm members like the SIE control block. */
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index 3e3605c9..a27a8401 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -202,6 +202,39 @@ union {
 	uint64_t	pv_grregs[16];		/* 0x0380 */
 } __attribute__((packed));
 
+union esca_sigp_ctrl {
+	uint16_t value;
+	struct {
+		uint8_t c : 1;
+		uint8_t reserved: 7;
+		uint8_t scn;
+	};
+};
+
+struct esca_entry {
+	union esca_sigp_ctrl sigp_ctrl;
+	uint16_t   reserved1[3];
+	uint64_t   sda;
+	uint64_t   reserved2[6];
+};
+
+union ipte_control {
+	unsigned long val;
+	struct {
+		unsigned long k  : 1;
+		unsigned long kh : 31;
+		unsigned long kg : 32;
+	};
+};
+
+struct esca_block {
+	union ipte_control ipte_control;
+	uint64_t   reserved1[7];
+	uint64_t   mcn[4];
+	uint64_t   reserved2[20];
+	struct esca_entry cpu[256];
+};
+
 struct vm_uv {
 	uint64_t vm_handle;
 	uint64_t vcpu_handle;
@@ -230,7 +263,7 @@ struct vm_save_area {
 struct vm {
 	struct kvm_s390_sie_block *sblk;
 	struct vm_save_area save_area;
-	void *sca;				/* System Control Area */
+	struct esca_block *sca;			/* System Control Area */
 	uint8_t *crycb;				/* Crypto Control Block */
 	struct vm_uv uv;			/* PV UV information */
 	/* Ptr to first guest page */
-- 
2.34.1

Re: [kvm-unit-tests PATCH 6/6] lib: s390x: sie: Properly populate SCA

[Nico Boehr]

Quoting Janosch Frank (2022-07-29 10:26:33)
> CPU0 is the only cpu that's being used but we should still mark it as
> online and set the SDA in the SCA.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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