[kvm-unit-tests PATCH v3 0/9] s390x: Cleanup and maintenance 2

[kvm-unit-tests PATCH v3 0/9] s390x: Cleanup and maintenance 2

[Janosch Frank]

Here are a few more fixes and cleanups that improve readability and
architectural compliance.

v3:
	* Check CC == 1 for rc 0x100 query
	* ifndef the UV debug switch
	* Replaced tprot fix
	* Added a simple fix to the snippet lib linking problem

Janis Schoetterl-Glausch (1):
  lib: s390x: Add access key argument to tprot

Janosch Frank (8):
  s390x: uv: Tolerate 0x100 query return code
  s390x: uv-host: Fence a destroy cpu test on z15
  lib: s390x: uv: Fix share return value and print
  lib: s390x: uv: Add UVC_ERR_DEBUG switch
  lib: s390x: Print PGM code as hex
  s390x: Add sthyi cc==0 r2+1 verification
  s390x: snippets: Set stackptr and stacktop in cstart.S
  s390x: snippets: Define all things that are needed to link the lib

 lib/s390x/asm/arch_def.h  | 20 +++++++++++++++++---
 lib/s390x/asm/uv.h        | 21 +++++++++++++--------
 lib/s390x/interrupt.c     |  2 +-
 lib/s390x/sclp.c          |  2 +-
 s390x/skrf.c              |  3 +--
 s390x/snippets/c/cstart.S | 16 +++++++++++++++-
 s390x/snippets/c/flat.lds |  2 ++
 s390x/sthyi.c             | 20 +++++++++++---------
 s390x/uv-guest.c          |  4 +++-
 s390x/uv-host.c           | 19 ++++++++++++-------
 10 files changed, 76 insertions(+), 33 deletions(-)

-- 
2.30.2

[kvm-unit-tests PATCH v3 1/9] s390x: uv: Tolerate 0x100 query return code

[Janosch Frank]

RC 0x100 is not an error but a notice that we could have gotten more
data from the Ultravisor if we had asked for it. So let's tolerate
them in our tests.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-guest.c | 4 +++-
 s390x/uv-host.c  | 8 +++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
index f05ae4c3..44ad2154 100644
--- a/s390x/uv-guest.c
+++ b/s390x/uv-guest.c
@@ -71,7 +71,9 @@ static void test_query(void)
 
 	uvcb.header.len = sizeof(uvcb);
 	cc = uv_call(0, (u64)&uvcb);
-	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
+	report((!cc && uvcb.header.rc == UVC_RC_EXECUTED) ||
+	       (cc == 1 && uvcb.header.rc == 0x100),
+		"successful query");
 
 	/*
 	 * These bits have been introduced with the very first
diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 28035707..4b72c24d 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -385,7 +385,7 @@ static void test_init(void)
 
 static void test_query(void)
 {
-	int i = 0;
+	int i = 0, cc;
 
 	uvcb_qui.header.cmd = UVC_CMD_QUI;
 	uvcb_qui.header.len = sizeof(uvcb_qui);
@@ -400,8 +400,10 @@ static void test_query(void)
 	report(uvcb_qui.header.rc == 0x100, "insf length");
 
 	uvcb_qui.header.len = sizeof(uvcb_qui);
-	uv_call(0, (uint64_t)&uvcb_qui);
-	report(uvcb_qui.header.rc == UVC_RC_EXECUTED, "successful query");
+	cc = uv_call(0, (uint64_t)&uvcb_qui);
+	report((!cc && uvcb_qui.header.rc == UVC_RC_EXECUTED) ||
+	       (cc == 1 && uvcb_qui.header.rc == 0x100),
+		"successful query");
 
 	for (i = 0; cmds[i].name; i++)
 		report(uv_query_test_call(cmds[i].call_bit), "%s", cmds[i].name);
-- 
2.30.2

[kvm-unit-tests PATCH v3 2/9] s390x: uv-host: Fence a destroy cpu test on z15

[Janosch Frank]

Firmware will not give us the expected return code on z15 so let's
fence it for the z15 machine generation.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/asm/arch_def.h | 14 ++++++++++++++
 s390x/uv-host.c          | 11 +++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index aa80d840..c8d2722a 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -219,6 +219,20 @@ static inline unsigned short stap(void)
 	return cpu_address;
 }
 
+#define MACHINE_Z15A	0x8561
+#define MACHINE_Z15B	0x8562
+
+static inline uint16_t get_machine_id(void)
+{
+	uint64_t cpuid;
+
+	asm volatile("stidp %0" : "=Q" (cpuid));
+	cpuid = cpuid >> 16;
+	cpuid &= 0xffff;
+
+	return cpuid;
+}
+
 static inline int tprot(unsigned long addr)
 {
 	int cc;
diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 4b72c24d..92a41069 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -111,6 +111,7 @@ static void test_config_destroy(void)
 static void test_cpu_destroy(void)
 {
 	int rc;
+	uint16_t machineid = get_machine_id();
 	struct uv_cb_nodata uvcb = {
 		.header.len = sizeof(uvcb),
 		.header.cmd = UVC_CMD_DESTROY_SEC_CPU,
@@ -125,10 +126,12 @@ static void test_cpu_destroy(void)
 	       "hdr invalid length");
 	uvcb.header.len += 8;
 
-	uvcb.handle += 1;
-	rc = uv_call(0, (uint64_t)&uvcb);
-	report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
-	uvcb.handle -= 1;
+	if (machineid != MACHINE_Z15A && machineid != MACHINE_Z15B) {
+		uvcb.handle += 1;
+		rc = uv_call(0, (uint64_t)&uvcb);
+		report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
+		uvcb.handle -= 1;
+	}
 
 	rc = uv_call(0, (uint64_t)&uvcb);
 	report(rc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "success");
-- 
2.30.2

[kvm-unit-tests PATCH v3 3/9] lib: s390x: uv: Fix share return value and print

[Janosch Frank]

Let's only return 0/1 for success/failure respectively.
If needed we can later add rc/rrc pointers so we can check for the
reasons of cc==1 cases like we do in the kernel.

As share() might also be used in snippets it's best not to use prints
to avoid linking problems so lets remove the report_info().

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/asm/uv.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index ec10d1c4..2f099553 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -219,15 +219,8 @@ static inline int share(unsigned long addr, u16 cmd)
 		.header.len = sizeof(uvcb),
 		.paddr = addr
 	};
-	int cc;
 
-	cc = uv_call(0, (u64)&uvcb);
-	if (!cc && uvcb.header.rc == UVC_RC_EXECUTED)
-		return 0;
-
-	report_info("uv_call: cmd %04x cc %d response code: %04x", cc, cmd,
-		    uvcb.header.rc);
-	return -1;
+	return uv_call(0, (u64)&uvcb);
 }
 
 /*
-- 
2.30.2

[kvm-unit-tests PATCH v3 4/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch

[Janosch Frank]

Every time something goes wrong in a way we don't expect, we need to
add debug prints to some UVC to get the unexpected return code.

Let's just put the printing behind a macro so we can enable it if
needed via a simple switch.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/asm/uv.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index 2f099553..16db086d 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -12,6 +12,11 @@
 #ifndef _ASMS390X_UV_H_
 #define _ASMS390X_UV_H_
 
+/* Enables printing of command code and return codes for failed UVCs */
+#ifndef UVC_ERR_DEBUG
+#define UVC_ERR_DEBUG	0
+#endif
+
 #define UVC_RC_EXECUTED		0x0001
 #define UVC_RC_INV_CMD		0x0002
 #define UVC_RC_INV_STATE	0x0003
@@ -194,6 +199,13 @@ static inline int uv_call_once(unsigned long r1, unsigned long r2)
 		: [cc] "=d" (cc)
 		: [r1] "a" (r1), [r2] "a" (r2)
 		: "memory", "cc");
+
+	if (UVC_ERR_DEBUG && cc)
+		printf("UV call error: call %x rc %x rrc %x\n",
+		       ((struct uv_cb_header *)r2)->cmd,
+		       ((struct uv_cb_header *)r2)->rc,
+		       ((struct uv_cb_header *)r2)->rrc);
+
 	return cc;
 }
 
-- 
2.30.2

[kvm-unit-tests PATCH v3 5/9] lib: s390x: Add access key argument to tprot

[Janosch Frank]

From: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Currently there is only one callee passing a non zero key,
but having the argument will be useful in the future.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 6 +++---
 lib/s390x/sclp.c         | 2 +-
 s390x/skrf.c             | 3 +--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index c8d2722a..b34aa792 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -233,15 +233,15 @@ static inline uint16_t get_machine_id(void)
 	return cpuid;
 }
 
-static inline int tprot(unsigned long addr)
+static inline int tprot(unsigned long addr, char access_key)
 {
 	int cc;
 
 	asm volatile(
-		"	tprot	0(%1),0\n"
+		"	tprot	0(%1),0(%2)\n"
 		"	ipm	%0\n"
 		"	srl	%0,28\n"
-		: "=d" (cc) : "a" (addr) : "cc");
+		: "=d" (cc) : "a" (addr), "a" (access_key << 4) : "cc");
 	return cc;
 }
 
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 9502d161..02722498 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -217,7 +217,7 @@ void sclp_memory_setup(void)
 	/* probe for r/w memory up to max memory size */
 	while (ram_size < max_ram_size) {
 		expect_pgm_int();
-		cc = tprot(ram_size + storage_increment_size - 1);
+		cc = tprot(ram_size + storage_increment_size - 1, 0);
 		/* stop once we receive an exception or have protected memory */
 		if (clear_pgm_int() || cc != 0)
 			break;
diff --git a/s390x/skrf.c b/s390x/skrf.c
index 8ca7588c..ca4efbf1 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -103,8 +103,7 @@ static void test_tprot(void)
 {
 	report_prefix_push("tprot");
 	expect_pgm_int();
-	asm volatile("tprot	%[addr],0xf0(0)\n"
-		     : : [addr] "a" (pagebuf) : );
+	tprot((unsigned long)pagebuf, 0xf);
 	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
 	report_prefix_pop();
 }
-- 
2.30.2

[kvm-unit-tests PATCH v3 6/9] lib: s390x: Print PGM code as hex

[Janosch Frank]

We have them defined as hex constants in lib/s390x/asm/arch_def.h so
why not print them as hex values?

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 126d4c0a..27d3b767 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -169,7 +169,7 @@ static void print_pgm_info(struct stack_frame_int *stack)
 		  lc->pgm_old_psw.addr <= (uintptr_t)sie_exit);
 
 	printf("\n");
-	printf("Unexpected program interrupt %s: %d on cpu %d at %#lx, ilen %d\n",
+	printf("Unexpected program interrupt %s: %#x on cpu %d at %#lx, ilen %d\n",
 	       in_sie ? "in SIE" : "",
 	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr, lc->pgm_int_id);
 	print_int_regs(stack);
-- 
2.30.2

[kvm-unit-tests PATCH v3 7/9] s390x: Add sthyi cc==0 r2+1 verification

[Janosch Frank]

On success r2 + 1 should be 0, let's also check for that.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/sthyi.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/s390x/sthyi.c b/s390x/sthyi.c
index db90b56f..4b153bf4 100644
--- a/s390x/sthyi.c
+++ b/s390x/sthyi.c
@@ -24,16 +24,16 @@ static inline int sthyi(uint64_t vaddr, uint64_t fcode, uint64_t *rc,
 {
 	register uint64_t code asm("0") = fcode;
 	register uint64_t addr asm("2") = vaddr;
-	register uint64_t rc3 asm("3") = 0;
+	register uint64_t rc3 asm("3") = 42;
 	int cc = 0;
 
-	asm volatile(".insn rre,0xB2560000,%[r1],%[r2]\n"
-		     "ipm	 %[cc]\n"
-		     "srl	 %[cc],28\n"
-		     : [cc] "=d" (cc)
-		     : [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1),
-		       [r2] "i" (r2)
-		     : "memory", "cc", "r3");
+	asm volatile(
+		".insn   rre,0xB2560000,%[r1],%[r2]\n"
+		"ipm     %[cc]\n"
+		"srl     %[cc],28\n"
+		: [cc] "=d" (cc), "+d" (rc3)
+		: [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1), [r2] "i" (r2)
+		: "memory", "cc");
 	if (rc)
 		*rc = rc3;
 	return cc;
@@ -139,16 +139,18 @@ static void test_fcode0(void)
 	struct sthyi_hdr_sctn *hdr;
 	struct sthyi_mach_sctn *mach;
 	struct sthyi_par_sctn *par;
+	uint64_t rc = 42;
 
 	/* Zero destination memory. */
 	memset(pagebuf, 0, PAGE_SIZE);
 
 	report_prefix_push("fcode 0");
-	sthyi((uint64_t)pagebuf, 0, NULL, 0, 2);
+	sthyi((uint64_t)pagebuf, 0, &rc, 0, 2);
 	hdr = (void *)pagebuf;
 	mach = (void *)pagebuf + hdr->INFMOFF;
 	par = (void *)pagebuf + hdr->INFPOFF;
 
+	report(!rc, "r2 + 1 == 0");
 	test_fcode0_hdr(hdr);
 	test_fcode0_mach(mach);
 	test_fcode0_par(par);
-- 
2.30.2

[kvm-unit-tests PATCH v3 8/9] s390x: snippets: Set stackptr and stacktop in cstart.S

[Janosch Frank]

We have a stack, so why not define it and be a step closer to include
the lib into the snippets.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/snippets/c/cstart.S | 2 +-
 s390x/snippets/c/flat.lds | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
index a1754808..031a6b83 100644
--- a/s390x/snippets/c/cstart.S
+++ b/s390x/snippets/c/cstart.S
@@ -17,7 +17,7 @@ start:
 	xgr \i,\i
 	.endr
 	/* 0x3000 is the stack page for now */
-	lghi	%r15, 0x4000 - 160
+	lghi	%r15, stackptr
 	sam64
 	brasl	%r14, main
 	/* For now let's only use cpu 0 in snippets so this will always work. */
diff --git a/s390x/snippets/c/flat.lds b/s390x/snippets/c/flat.lds
index ce3bfd69..59974b38 100644
--- a/s390x/snippets/c/flat.lds
+++ b/s390x/snippets/c/flat.lds
@@ -15,6 +15,8 @@ SECTIONS
 		 QUAD(0x0000000000004000)
 	}
 	. = 0x4000;
+	stackptr = . - 160;
+	stacktop = .;
 	.text : {
 		*(.init)
 		*(.text)
-- 
2.30.2

[kvm-unit-tests PATCH v3 9/9] s390x: snippets: Define all things that are needed to link the lib

[Janosch Frank]

Let's just define all of the needed things so we can link libcflat.

A significant portion of the lib won't work, like printing and
allocation but we can still use things like memset() which already
improves our lives significantly.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/snippets/c/cstart.S | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
index 031a6b83..2d397669 100644
--- a/s390x/snippets/c/cstart.S
+++ b/s390x/snippets/c/cstart.S
@@ -20,6 +20,20 @@ start:
 	lghi	%r15, stackptr
 	sam64
 	brasl	%r14, main
+/*
+ * Defining things that the linker needs to link in libcflat and make
+ * them result in sigp stop if called.
+ */
+.globl sie_exit
+.globl sie_entry
+.globl smp_cpu_setup_state
+.globl ipl_args
+.globl auxinfo
+sie_exit:
+sie_entry:
+smp_cpu_setup_state:
+ipl_args:
+auxinfo:
 	/* For now let's only use cpu 0 in snippets so this will always work. */
 	xgr	%r0, %r0
 	sigp    %r2, %r0, SIGP_STOP
-- 
2.30.2

Re: [kvm-unit-tests PATCH v3 1/9] s390x: uv: Tolerate 0x100 query return code

[Thomas Huth]

On 07/10/2021 10.50, Janosch Frank wrote:
> RC 0x100 is not an error but a notice that we could have gotten more
> data from the Ultravisor if we had asked for it. So let's tolerate
> them in our tests.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/uv-guest.c | 4 +++-
>   s390x/uv-host.c  | 8 +++++---
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> index f05ae4c3..44ad2154 100644
> --- a/s390x/uv-guest.c
> +++ b/s390x/uv-guest.c
> @@ -71,7 +71,9 @@ static void test_query(void)
>   
>   	uvcb.header.len = sizeof(uvcb);
>   	cc = uv_call(0, (u64)&uvcb);
> -	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
> +	report((!cc && uvcb.header.rc == UVC_RC_EXECUTED) ||
> +	       (cc == 1 && uvcb.header.rc == 0x100),
> +		"successful query");
>   
>   	/*
>   	 * These bits have been introduced with the very first
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 28035707..4b72c24d 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -385,7 +385,7 @@ static void test_init(void)
>   
>   static void test_query(void)
>   {
> -	int i = 0;
> +	int i = 0, cc;
>   
>   	uvcb_qui.header.cmd = UVC_CMD_QUI;
>   	uvcb_qui.header.len = sizeof(uvcb_qui);
> @@ -400,8 +400,10 @@ static void test_query(void)
>   	report(uvcb_qui.header.rc == 0x100, "insf length");
>   
>   	uvcb_qui.header.len = sizeof(uvcb_qui);
> -	uv_call(0, (uint64_t)&uvcb_qui);
> -	report(uvcb_qui.header.rc == UVC_RC_EXECUTED, "successful query");
> +	cc = uv_call(0, (uint64_t)&uvcb_qui);
> +	report((!cc && uvcb_qui.header.rc == UVC_RC_EXECUTED) ||
> +	       (cc == 1 && uvcb_qui.header.rc == 0x100),
> +		"successful query");
>   
>   	for (i = 0; cmds[i].name; i++)
>   		report(uv_query_test_call(cmds[i].call_bit), "%s", cmds[i].name);
> 

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v3 4/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch

[Thomas Huth]

On 07/10/2021 10.50, Janosch Frank wrote:
> Every time something goes wrong in a way we don't expect, we need to
> add debug prints to some UVC to get the unexpected return code.
> 
> Let's just put the printing behind a macro so we can enable it if
> needed via a simple switch.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   lib/s390x/asm/uv.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 2f099553..16db086d 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -12,6 +12,11 @@
>   #ifndef _ASMS390X_UV_H_
>   #define _ASMS390X_UV_H_
>   
> +/* Enables printing of command code and return codes for failed UVCs */
> +#ifndef UVC_ERR_DEBUG
> +#define UVC_ERR_DEBUG	0
> +#endif
> +
>   #define UVC_RC_EXECUTED		0x0001
>   #define UVC_RC_INV_CMD		0x0002
>   #define UVC_RC_INV_STATE	0x0003
> @@ -194,6 +199,13 @@ static inline int uv_call_once(unsigned long r1, unsigned long r2)
>   		: [cc] "=d" (cc)
>   		: [r1] "a" (r1), [r2] "a" (r2)
>   		: "memory", "cc");
> +
> +	if (UVC_ERR_DEBUG && cc)
> +		printf("UV call error: call %x rc %x rrc %x\n",
> +		       ((struct uv_cb_header *)r2)->cmd,
> +		       ((struct uv_cb_header *)r2)->rc,
> +		       ((struct uv_cb_header *)r2)->rrc);
> +
>   	return cc;
>   }
>   
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v3 5/9] lib: s390x: Add access key argument to tprot

[Thomas Huth]

On 07/10/2021 10.50, Janosch Frank wrote:
> From: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> 
> Currently there is only one callee passing a non zero key,
> but having the argument will be useful in the future.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   lib/s390x/asm/arch_def.h | 6 +++---
>   lib/s390x/sclp.c         | 2 +-
>   s390x/skrf.c             | 3 +--
>   3 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index c8d2722a..b34aa792 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -233,15 +233,15 @@ static inline uint16_t get_machine_id(void)
>   	return cpuid;
>   }
>   
> -static inline int tprot(unsigned long addr)
> +static inline int tprot(unsigned long addr, char access_key)
>   {
>   	int cc;
>   
>   	asm volatile(
> -		"	tprot	0(%1),0\n"
> +		"	tprot	0(%1),0(%2)\n"
>   		"	ipm	%0\n"
>   		"	srl	%0,28\n"
> -		: "=d" (cc) : "a" (addr) : "cc");
> +		: "=d" (cc) : "a" (addr), "a" (access_key << 4) : "cc");
>   	return cc;
>   }
>   
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 9502d161..02722498 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -217,7 +217,7 @@ void sclp_memory_setup(void)
>   	/* probe for r/w memory up to max memory size */
>   	while (ram_size < max_ram_size) {
>   		expect_pgm_int();
> -		cc = tprot(ram_size + storage_increment_size - 1);
> +		cc = tprot(ram_size + storage_increment_size - 1, 0);
>   		/* stop once we receive an exception or have protected memory */
>   		if (clear_pgm_int() || cc != 0)
>   			break;
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 8ca7588c..ca4efbf1 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -103,8 +103,7 @@ static void test_tprot(void)
>   {
>   	report_prefix_push("tprot");
>   	expect_pgm_int();
> -	asm volatile("tprot	%[addr],0xf0(0)\n"
> -		     : : [addr] "a" (pagebuf) : );
> +	tprot((unsigned long)pagebuf, 0xf);
>   	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>   	report_prefix_pop();
>   }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v3 7/9] s390x: Add sthyi cc==0 r2+1 verification

[Thomas Huth]

On 07/10/2021 10.50, Janosch Frank wrote:
> On success r2 + 1 should be 0, let's also check for that.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   s390x/sthyi.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/s390x/sthyi.c b/s390x/sthyi.c
> index db90b56f..4b153bf4 100644
> --- a/s390x/sthyi.c
> +++ b/s390x/sthyi.c
> @@ -24,16 +24,16 @@ static inline int sthyi(uint64_t vaddr, uint64_t fcode, uint64_t *rc,
>   {
>   	register uint64_t code asm("0") = fcode;
>   	register uint64_t addr asm("2") = vaddr;
> -	register uint64_t rc3 asm("3") = 0;
> +	register uint64_t rc3 asm("3") = 42;
>   	int cc = 0;
>   
> -	asm volatile(".insn rre,0xB2560000,%[r1],%[r2]\n"
> -		     "ipm	 %[cc]\n"
> -		     "srl	 %[cc],28\n"
> -		     : [cc] "=d" (cc)
> -		     : [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1),
> -		       [r2] "i" (r2)
> -		     : "memory", "cc", "r3");
> +	asm volatile(
> +		".insn   rre,0xB2560000,%[r1],%[r2]\n"
> +		"ipm     %[cc]\n"
> +		"srl     %[cc],28\n"
> +		: [cc] "=d" (cc), "+d" (rc3)
> +		: [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1), [r2] "i" (r2)
> +		: "memory", "cc");
>   	if (rc)
>   		*rc = rc3;
>   	return cc;
> @@ -139,16 +139,18 @@ static void test_fcode0(void)
>   	struct sthyi_hdr_sctn *hdr;
>   	struct sthyi_mach_sctn *mach;
>   	struct sthyi_par_sctn *par;
> +	uint64_t rc = 42;
>   
>   	/* Zero destination memory. */
>   	memset(pagebuf, 0, PAGE_SIZE);
>   
>   	report_prefix_push("fcode 0");
> -	sthyi((uint64_t)pagebuf, 0, NULL, 0, 2);
> +	sthyi((uint64_t)pagebuf, 0, &rc, 0, 2);
>   	hdr = (void *)pagebuf;
>   	mach = (void *)pagebuf + hdr->INFMOFF;
>   	par = (void *)pagebuf + hdr->INFPOFF;
>   
> +	report(!rc, "r2 + 1 == 0");

Could you please check for "rc == CODE_SUCCES" (since we've got that for 
this purpose)?

With that change:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v3 8/9] s390x: snippets: Set stackptr and stacktop in cstart.S

[Thomas Huth]

On 07/10/2021 10.50, Janosch Frank wrote:
> We have a stack, so why not define it and be a step closer to include
> the lib into the snippets.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/snippets/c/cstart.S | 2 +-
>   s390x/snippets/c/flat.lds | 2 ++
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
> index a1754808..031a6b83 100644
> --- a/s390x/snippets/c/cstart.S
> +++ b/s390x/snippets/c/cstart.S
> @@ -17,7 +17,7 @@ start:
>   	xgr \i,\i
>   	.endr
>   	/* 0x3000 is the stack page for now */
> -	lghi	%r15, 0x4000 - 160
> +	lghi	%r15, stackptr

I already wanted to ask you to replace the magic value 0x4000 here ... great 
to see that you already did it :-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v3 8/9] s390x: snippets: Set stackptr and stacktop in cstart.S

[Janosch Frank]

On 10/7/21 11:13, Thomas Huth wrote:
> On 07/10/2021 10.50, Janosch Frank wrote:
>> We have a stack, so why not define it and be a step closer to include
>> the lib into the snippets.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    s390x/snippets/c/cstart.S | 2 +-
>>    s390x/snippets/c/flat.lds | 2 ++
>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
>> index a1754808..031a6b83 100644
>> --- a/s390x/snippets/c/cstart.S
>> +++ b/s390x/snippets/c/cstart.S
>> @@ -17,7 +17,7 @@ start:
>>    	xgr \i,\i
>>    	.endr
>>    	/* 0x3000 is the stack page for now */
>> -	lghi	%r15, 0x4000 - 160
>> +	lghi	%r15, stackptr
> 
> I already wanted to ask you to replace the magic value 0x4000 here ... great
> to see that you already did it :-)

Magic mind reading :-)

> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
Thanks!

Re: [kvm-unit-tests PATCH v3 9/9] s390x: snippets: Define all things that are needed to link the lib

[Thomas Huth]

On 07/10/2021 10.50, Janosch Frank wrote:
> Let's just define all of the needed things so we can link libcflat.
> 
> A significant portion of the lib won't work, like printing and
> allocation but we can still use things like memset() which already
> improves our lives significantly.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/snippets/c/cstart.S | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
> index 031a6b83..2d397669 100644
> --- a/s390x/snippets/c/cstart.S
> +++ b/s390x/snippets/c/cstart.S
> @@ -20,6 +20,20 @@ start:
>   	lghi	%r15, stackptr
>   	sam64
>   	brasl	%r14, main
> +/*
> + * Defining things that the linker needs to link in libcflat and make
> + * them result in sigp stop if called.
> + */
> +.globl sie_exit
> +.globl sie_entry
> +.globl smp_cpu_setup_state
> +.globl ipl_args
> +.globl auxinfo
> +sie_exit:
> +sie_entry:
> +smp_cpu_setup_state:
> +ipl_args:
> +auxinfo:

I think this likely could be done in a somewhat nicer way, e.g. by moving 
mem_init() and sclp_memory_setup() into a separate .c file in the lib, and 
by moving expect_pgm_int(), fixup_pgm_int() and friends into another 
separate .c file, too, so that we e.g. do not need to link against the code 
that uses sie_entry and sie_exit ... but that's a major rework on its own, 
so for the time being:

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v3 9/9] s390x: snippets: Define all things that are needed to link the lib

[Janosch Frank]

On 10/7/21 11:44, Thomas Huth wrote:
> On 07/10/2021 10.50, Janosch Frank wrote:
>> Let's just define all of the needed things so we can link libcflat.
>>
>> A significant portion of the lib won't work, like printing and
>> allocation but we can still use things like memset() which already
>> improves our lives significantly.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    s390x/snippets/c/cstart.S | 14 ++++++++++++++
>>    1 file changed, 14 insertions(+)
>>
>> diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
>> index 031a6b83..2d397669 100644
>> --- a/s390x/snippets/c/cstart.S
>> +++ b/s390x/snippets/c/cstart.S
>> @@ -20,6 +20,20 @@ start:
>>    	lghi	%r15, stackptr
>>    	sam64
>>    	brasl	%r14, main
>> +/*
>> + * Defining things that the linker needs to link in libcflat and make
>> + * them result in sigp stop if called.
>> + */
>> +.globl sie_exit
>> +.globl sie_entry
>> +.globl smp_cpu_setup_state
>> +.globl ipl_args
>> +.globl auxinfo
>> +sie_exit:
>> +sie_entry:
>> +smp_cpu_setup_state:
>> +ipl_args:
>> +auxinfo:
> 
> I think this likely could be done in a somewhat nicer way, e.g. by moving

Definitely, as I said, it's a simple fix

> mem_init() and sclp_memory_setup() into a separate .c file in the lib, and
> by moving expect_pgm_int(), fixup_pgm_int() and friends into another
> separate .c file, too, so that we e.g. do not need to link against the code
> that uses sie_entry and sie_exit ... but that's a major rework on its own,
> so for the time being:
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 
Thanks

Re: [kvm-unit-tests PATCH v3 7/9] s390x: Add sthyi cc==0 r2+1 verification

[Janosch Frank]

On 10/7/21 11:11, Thomas Huth wrote:
> On 07/10/2021 10.50, Janosch Frank wrote:
>> On success r2 + 1 should be 0, let's also check for that.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> ---
>>    s390x/sthyi.c | 20 +++++++++++---------
>>    1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/s390x/sthyi.c b/s390x/sthyi.c
>> index db90b56f..4b153bf4 100644
>> --- a/s390x/sthyi.c
>> +++ b/s390x/sthyi.c
>> @@ -24,16 +24,16 @@ static inline int sthyi(uint64_t vaddr, uint64_t fcode, uint64_t *rc,
>>    {
>>    	register uint64_t code asm("0") = fcode;
>>    	register uint64_t addr asm("2") = vaddr;
>> -	register uint64_t rc3 asm("3") = 0;
>> +	register uint64_t rc3 asm("3") = 42;
>>    	int cc = 0;
>>    
>> -	asm volatile(".insn rre,0xB2560000,%[r1],%[r2]\n"
>> -		     "ipm	 %[cc]\n"
>> -		     "srl	 %[cc],28\n"
>> -		     : [cc] "=d" (cc)
>> -		     : [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1),
>> -		       [r2] "i" (r2)
>> -		     : "memory", "cc", "r3");
>> +	asm volatile(
>> +		".insn   rre,0xB2560000,%[r1],%[r2]\n"
>> +		"ipm     %[cc]\n"
>> +		"srl     %[cc],28\n"
>> +		: [cc] "=d" (cc), "+d" (rc3)
>> +		: [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1), [r2] "i" (r2)
>> +		: "memory", "cc");
>>    	if (rc)
>>    		*rc = rc3;
>>    	return cc;
>> @@ -139,16 +139,18 @@ static void test_fcode0(void)
>>    	struct sthyi_hdr_sctn *hdr;
>>    	struct sthyi_mach_sctn *mach;
>>    	struct sthyi_par_sctn *par;
>> +	uint64_t rc = 42;
>>    
>>    	/* Zero destination memory. */
>>    	memset(pagebuf, 0, PAGE_SIZE);
>>    
>>    	report_prefix_push("fcode 0");
>> -	sthyi((uint64_t)pagebuf, 0, NULL, 0, 2);
>> +	sthyi((uint64_t)pagebuf, 0, &rc, 0, 2);
>>    	hdr = (void *)pagebuf;
>>    	mach = (void *)pagebuf + hdr->INFMOFF;
>>    	par = (void *)pagebuf + hdr->INFPOFF;
>>    
>> +	report(!rc, "r2 + 1 == 0");
> 
> Could you please check for "rc == CODE_SUCCES" (since we've got that for
> this purpose)?

I'll do one better and also check for !cc

> 
> With that change:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks!

Re: [kvm-unit-tests PATCH v3 9/9] s390x: snippets: Define all things that are needed to link the lib

[Thomas Huth]

On 07/10/2021 12.44, Janosch Frank wrote:
> On 10/7/21 11:44, Thomas Huth wrote:
>> On 07/10/2021 10.50, Janosch Frank wrote:
>>> Let's just define all of the needed things so we can link libcflat.
>>>
>>> A significant portion of the lib won't work, like printing and
>>> allocation but we can still use things like memset() which already
>>> improves our lives significantly.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    s390x/snippets/c/cstart.S | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
>>> index 031a6b83..2d397669 100644
>>> --- a/s390x/snippets/c/cstart.S
>>> +++ b/s390x/snippets/c/cstart.S
>>> @@ -20,6 +20,20 @@ start:
>>>        lghi    %r15, stackptr
>>>        sam64
>>>        brasl    %r14, main
>>> +/*
>>> + * Defining things that the linker needs to link in libcflat and make
>>> + * them result in sigp stop if called.
>>> + */
>>> +.globl sie_exit
>>> +.globl sie_entry
>>> +.globl smp_cpu_setup_state
>>> +.globl ipl_args
>>> +.globl auxinfo
>>> +sie_exit:
>>> +sie_entry:
>>> +smp_cpu_setup_state:
>>> +ipl_args:
>>> +auxinfo:
>>
>> I think this likely could be done in a somewhat nicer way, e.g. by moving
> 
> Definitely, as I said, it's a simple fix

Alternatively, something like this might work, too:

diff --git a/s390x/Makefile b/s390x/Makefile
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -80,7 +80,7 @@ asmlib = $(TEST_DIR)/cstart64.o $(TEST_DIR)/cpu.o
  FLATLIBS = $(libcflat)
  
  SNIPPET_DIR = $(TEST_DIR)/snippets
-snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o
+snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o lib/auxinfo.o
  
  # perquisites (=guests) for the snippet hosts.
  # $(TEST_DIR)/<snippet-host>.elf: snippets = $(SNIPPET_DIR)/<c/asm>/<snippet>.gbin
diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
--- a/s390x/snippets/c/cstart.S
+++ b/s390x/snippets/c/cstart.S
@@ -21,5 +21,9 @@ start:
         sam64
         brasl   %r14, main
         /* For now let's only use cpu 0 in snippets so this will always work. */
+.global puts
+.global exit
+puts:
+exit:
         xgr     %r0, %r0
         sigp    %r2, %r0, SIGP_STOP

I think that's more clear this way, since we're fencing the
functions that caused the dependencies to the other functions
from your patch. What do you think?

  Thomas

Re: [kvm-unit-tests PATCH v3 9/9] s390x: snippets: Define all things that are needed to link the lib

[Janosch Frank]

On 10/8/21 09:20, Thomas Huth wrote:
> On 07/10/2021 12.44, Janosch Frank wrote:
>> On 10/7/21 11:44, Thomas Huth wrote:
>>> On 07/10/2021 10.50, Janosch Frank wrote:
>>>> Let's just define all of the needed things so we can link libcflat.
>>>>
>>>> A significant portion of the lib won't work, like printing and
>>>> allocation but we can still use things like memset() which already
>>>> improves our lives significantly.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>     s390x/snippets/c/cstart.S | 14 ++++++++++++++
>>>>     1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
>>>> index 031a6b83..2d397669 100644
>>>> --- a/s390x/snippets/c/cstart.S
>>>> +++ b/s390x/snippets/c/cstart.S
>>>> @@ -20,6 +20,20 @@ start:
>>>>         lghi    %r15, stackptr
>>>>         sam64
>>>>         brasl    %r14, main
>>>> +/*
>>>> + * Defining things that the linker needs to link in libcflat and make
>>>> + * them result in sigp stop if called.
>>>> + */
>>>> +.globl sie_exit
>>>> +.globl sie_entry
>>>> +.globl smp_cpu_setup_state
>>>> +.globl ipl_args
>>>> +.globl auxinfo
>>>> +sie_exit:
>>>> +sie_entry:
>>>> +smp_cpu_setup_state:
>>>> +ipl_args:
>>>> +auxinfo:
>>>
>>> I think this likely could be done in a somewhat nicer way, e.g. by moving
>>
>> Definitely, as I said, it's a simple fix
> 
> Alternatively, something like this might work, too:

Seems like it works for the two tests that I checked.
Would you mind sending a proper patch?

I'd like to send out a pull today or on Monday.

> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -80,7 +80,7 @@ asmlib = $(TEST_DIR)/cstart64.o $(TEST_DIR)/cpu.o
>    FLATLIBS = $(libcflat)
>    
>    SNIPPET_DIR = $(TEST_DIR)/snippets
> -snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o
> +snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o lib/auxinfo.o
>    
>    # perquisites (=guests) for the snippet hosts.
>    # $(TEST_DIR)/<snippet-host>.elf: snippets = $(SNIPPET_DIR)/<c/asm>/<snippet>.gbin
> diff --git a/s390x/snippets/c/cstart.S b/s390x/snippets/c/cstart.S
> --- a/s390x/snippets/c/cstart.S
> +++ b/s390x/snippets/c/cstart.S
> @@ -21,5 +21,9 @@ start:
>           sam64
>           brasl   %r14, main
>           /* For now let's only use cpu 0 in snippets so this will always work. */
> +.global puts
> +.global exit
> +puts:
> +exit:
>           xgr     %r0, %r0
>           sigp    %r2, %r0, SIGP_STOP
> 
> I think that's more clear this way, since we're fencing the
> functions that caused the dependencies to the other functions
> from your patch. What do you think?
> 
>    Thomas
> 

Re: [kvm-unit-tests PATCH v3 4/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch

[Janosch Frank]

On 10/7/21 10:50, Janosch Frank wrote:
> Every time something goes wrong in a way we don't expect, we need to
> add debug prints to some UVC to get the unexpected return code.
> 
> Let's just put the printing behind a macro so we can enable it if
> needed via a simple switch.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   lib/s390x/asm/uv.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 2f099553..16db086d 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -12,6 +12,11 @@
>   #ifndef _ASMS390X_UV_H_
>   #define _ASMS390X_UV_H_
>   
> +/* Enables printing of command code and return codes for failed UVCs */
> +#ifndef UVC_ERR_DEBUG
> +#define UVC_ERR_DEBUG	0
> +#endif
> +
>   #define UVC_RC_EXECUTED		0x0001
>   #define UVC_RC_INV_CMD		0x0002
>   #define UVC_RC_INV_STATE	0x0003
> @@ -194,6 +199,13 @@ static inline int uv_call_once(unsigned long r1, unsigned long r2)
>   		: [cc] "=d" (cc)
>   		: [r1] "a" (r1), [r2] "a" (r2)
>   		: "memory", "cc");
> +
> +	if (UVC_ERR_DEBUG && cc)

That needs to check cc == 1...

> +		printf("UV call error: call %x rc %x rrc %x\n",
> +		       ((struct uv_cb_header *)r2)->cmd,
> +		       ((struct uv_cb_header *)r2)->rc,
> +		       ((struct uv_cb_header *)r2)->rrc);
> +
>   	return cc;
>   }
>   
>