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

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

[Janosch Frank]

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

Janosch Frank (9):
  s390x: uv: Tolerate 0x100 query return code
  s390x: pfmf: Fix 1MB handling
  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: Makefile: Remove snippet flatlib linking
  s390x: Add sthyi cc==0 r2+1 verification
  s390x: skrf: Fix tprot assembly

 lib/s390x/asm/arch_def.h | 14 ++++++++++++++
 lib/s390x/asm/uv.h       | 21 +++++++++++++--------
 lib/s390x/interrupt.c    |  2 +-
 s390x/Makefile           |  2 +-
 s390x/pfmf.c             | 10 ++++++++--
 s390x/skrf.c             |  2 +-
 s390x/sthyi.c            | 20 +++++++++++---------
 s390x/uv-guest.c         |  4 ++--
 s390x/uv-host.c          | 13 ++++++++-----
 9 files changed, 59 insertions(+), 29 deletions(-)

-- 
2.30.2

[kvm-unit-tests PATCH 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  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
index f05ae4c3..e7446e03 100644
--- a/s390x/uv-guest.c
+++ b/s390x/uv-guest.c
@@ -70,8 +70,8 @@ static void test_query(void)
 	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
 
 	uvcb.header.len = sizeof(uvcb);
-	cc = uv_call(0, (u64)&uvcb);
-	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
+	uv_call(0, (u64)&uvcb);
+	report(uvcb.header.rc == UVC_RC_EXECUTED || 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..66a11160 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -401,7 +401,7 @@ static void test_query(void)
 
 	uvcb_qui.header.len = sizeof(uvcb_qui);
 	uv_call(0, (uint64_t)&uvcb_qui);
-	report(uvcb_qui.header.rc == UVC_RC_EXECUTED, "successful query");
+	report(uvcb_qui.header.rc == UVC_RC_EXECUTED || 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 2/9] s390x: pfmf: Fix 1MB handling

[Janosch Frank]

On everything larger than 4k pfmf will update the address in GR2 when
it's interrupted so we should loop on pfmf and not trust that it
doesn't get interrupted.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/pfmf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/s390x/pfmf.c b/s390x/pfmf.c
index 2f3cb110..b0095bd7 100644
--- a/s390x/pfmf.c
+++ b/s390x/pfmf.c
@@ -54,6 +54,7 @@ static void test_1m_key(void)
 	bool rp = true;
 	union pfmf_r1 r1;
 	union skey skey;
+	void *addr = pagebuf;
 
 	report_prefix_push("1M");
 	if (test_facility(169)) {
@@ -64,7 +65,9 @@ static void test_1m_key(void)
 	r1.reg.sk = 1;
 	r1.reg.fsc = PFMF_FSC_1M;
 	r1.reg.key = 0x30;
-	pfmf(r1.val, pagebuf);
+	do {
+		addr = pfmf(r1.val, addr);
+	} while ((uintptr_t)addr != (uintptr_t)pagebuf + HPAGE_SIZE);
 	for (i = 0; i < 256; i++) {
 		skey.val = get_storage_key(pagebuf + i * PAGE_SIZE);
 		skey.val &= SKEY_ACC | SKEY_FP;
@@ -99,6 +102,7 @@ static void test_1m_clear(void)
 	int i;
 	union pfmf_r1 r1;
 	unsigned long sum = 0;
+	void *addr = pagebuf;
 
 	r1.val = 0;
 	r1.reg.cf = 1;
@@ -106,7 +110,9 @@ static void test_1m_clear(void)
 
 	report_prefix_push("1M");
 	memset(pagebuf, 42, PAGE_SIZE * 256);
-	pfmf(r1.val, pagebuf);
+	do {
+		addr = pfmf(r1.val, addr);
+	} while ((uintptr_t)addr != (uintptr_t)pagebuf + HPAGE_SIZE);
 	for (i = 0; i < PAGE_SIZE * 256; i++)
 		sum |= pagebuf[i];
 	report(!sum, "clear memory");
-- 
2.30.2

[kvm-unit-tests PATCH 3/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>
---
 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 66a11160..5e351120 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 4/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>
---
 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 5/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>
---
 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..0e958ad7 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -12,6 +12,9 @@
 #ifndef _ASMS390X_UV_H_
 #define _ASMS390X_UV_H_
 
+/* Enables printing of command code and return codes for failed UVCs */
+#define UVC_ERR_DEBUG	0
+
 #define UVC_RC_EXECUTED		0x0001
 #define UVC_RC_INV_CMD		0x0002
 #define UVC_RC_INV_STATE	0x0003
@@ -194,6 +197,15 @@ 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
+	if (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);
+#endif
+
 	return cc;
 }
 
-- 
2.30.2

[kvm-unit-tests PATCH 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>
---
 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 7/9] s390x: Makefile: Remove snippet flatlib linking

[Janosch Frank]

We can't link the flatlib as we do not export everything that it needs
and we don't (want to) call the init functions.

In the future we might implement a tiny lib that uses select lib
object files and re-implements functions like assert() and
test_facility() to be able to use some parts of the lib.

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 5d1a33a0..d09c0a17 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -92,7 +92,7 @@ $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
 	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
 
 $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_asmlib) $(FLATLIBS)
-	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib) $(FLATLIBS)
+	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib)
 	$(OBJCOPY) -O binary $@ $@
 	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
 
-- 
2.30.2

[kvm-unit-tests PATCH 8/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>
---
 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 9/9] s390x: skrf: Fix tprot assembly

[Janosch Frank]

It's a base + displacement address so we need to address it via 0(%[addr]).

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

diff --git a/s390x/skrf.c b/s390x/skrf.c
index 8ca7588c..84fb762c 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -103,7 +103,7 @@ static void test_tprot(void)
 {
 	report_prefix_push("tprot");
 	expect_pgm_int();
-	asm volatile("tprot	%[addr],0xf0(0)\n"
+	asm volatile("tprot	0(%[addr]),0xf0(0)\n"
 		     : : [addr] "a" (pagebuf) : );
 	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
 	report_prefix_pop();
-- 
2.30.2

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

[Claudio Imbrenda]

On Wed, 22 Sep 2021 07:18:03 +0000
Janosch Frank <frankja@linux.ibm.com> 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  | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> index f05ae4c3..e7446e03 100644
> --- a/s390x/uv-guest.c
> +++ b/s390x/uv-guest.c
> @@ -70,8 +70,8 @@ static void test_query(void)
>  	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>  
>  	uvcb.header.len = sizeof(uvcb);
> -	cc = uv_call(0, (u64)&uvcb);
> -	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
> +	uv_call(0, (u64)&uvcb);
> +	report(uvcb.header.rc == UVC_RC_EXECUTED || uvcb.header.rc
> == 0x100, "successful query");

if you want to be even more pedantic:
	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED ||
		cc == 1 && uvcb.header.rc == 0x100, ... 

>  
>  	/*
>  	 * These bits have been introduced with the very first
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 28035707..66a11160 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -401,7 +401,7 @@ static void test_query(void)
>  
>  	uvcb_qui.header.len = sizeof(uvcb_qui);
>  	uv_call(0, (uint64_t)&uvcb_qui);
> -	report(uvcb_qui.header.rc == UVC_RC_EXECUTED, "successful query");
> +	report(uvcb_qui.header.rc == UVC_RC_EXECUTED || uvcb_qui.header.rc == 0x100, "successful query");

same here

>  
>  	for (i = 0; cmds[i].name; i++)
>  		report(uv_query_test_call(cmds[i].call_bit), "%s", cmds[i].name);

Re: [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling

[Claudio Imbrenda]

On Wed, 22 Sep 2021 07:18:04 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> On everything larger than 4k pfmf will update the address in GR2 when
> it's interrupted so we should loop on pfmf and not trust that it
> doesn't get interrupted.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/pfmf.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/pfmf.c b/s390x/pfmf.c
> index 2f3cb110..b0095bd7 100644
> --- a/s390x/pfmf.c
> +++ b/s390x/pfmf.c
> @@ -54,6 +54,7 @@ static void test_1m_key(void)
>  	bool rp = true;
>  	union pfmf_r1 r1;
>  	union skey skey;
> +	void *addr = pagebuf;
>  
>  	report_prefix_push("1M");
>  	if (test_facility(169)) {
> @@ -64,7 +65,9 @@ static void test_1m_key(void)
>  	r1.reg.sk = 1;
>  	r1.reg.fsc = PFMF_FSC_1M;
>  	r1.reg.key = 0x30;
> -	pfmf(r1.val, pagebuf);
> +	do {
> +		addr = pfmf(r1.val, addr);
> +	} while ((uintptr_t)addr != (uintptr_t)pagebuf + HPAGE_SIZE);
>  	for (i = 0; i < 256; i++) {
>  		skey.val = get_storage_key(pagebuf + i * PAGE_SIZE);
>  		skey.val &= SKEY_ACC | SKEY_FP;
> @@ -99,6 +102,7 @@ static void test_1m_clear(void)
>  	int i;
>  	union pfmf_r1 r1;
>  	unsigned long sum = 0;
> +	void *addr = pagebuf;
>  
>  	r1.val = 0;
>  	r1.reg.cf = 1;
> @@ -106,7 +110,9 @@ static void test_1m_clear(void)
>  
>  	report_prefix_push("1M");
>  	memset(pagebuf, 42, PAGE_SIZE * 256);
> -	pfmf(r1.val, pagebuf);
> +	do {
> +		addr = pfmf(r1.val, addr);
> +	} while ((uintptr_t)addr != (uintptr_t)pagebuf + HPAGE_SIZE);
>  	for (i = 0; i < PAGE_SIZE * 256; i++)
>  		sum |= pagebuf[i];
>  	report(!sum, "clear memory");

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

[Claudio Imbrenda]

On Wed, 22 Sep 2021 07:18:05 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> 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>

> ---
>  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 66a11160..5e351120 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");

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

[Claudio Imbrenda]

On Wed, 22 Sep 2021 07:18:06 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> 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>

> ---
>  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);
>  }
>  
>  /*

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

[Claudio Imbrenda]

On Wed, 22 Sep 2021 07:18:07 +0000
Janosch Frank <frankja@linux.ibm.com> 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>

but see a nit below

> ---
>  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..0e958ad7 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -12,6 +12,9 @@
>  #ifndef _ASMS390X_UV_H_
>  #define _ASMS390X_UV_H_
>  
> +/* Enables printing of command code and return codes for failed UVCs
> */ +#define UVC_ERR_DEBUG	0
> +
>  #define UVC_RC_EXECUTED		0x0001
>  #define UVC_RC_INV_CMD		0x0002
>  #define UVC_RC_INV_STATE	0x0003
> @@ -194,6 +197,15 @@ 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
> +	if (cc)

it probably looks cleaner like this:

	if (UVC_ERR_DEBUG && cc)

and without the #if; the compiler should be smart enough to remove the
dead code. In practice it doesn't really matter in the end, so feel free
to ignore this comment :)

> +		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);
> +#endif
> +
>  	return cc;
>  }
>  

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

[Claudio Imbrenda]

On Wed, 22 Sep 2021 07:18:08 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> 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>

> ---
>  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);

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

[Claudio Imbrenda]

On Wed, 22 Sep 2021 07:18:10 +0000
Janosch Frank <frankja@linux.ibm.com> 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>

but see comment below

> ---
>  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;

I am not a big fan of the asm constraints, in the kernel we are
trying to move away from them, but I guess as long as it works it's ok

>  	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);

Re: [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly

[Claudio Imbrenda]

On Wed, 22 Sep 2021 07:18:11 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> It's a base + displacement address so we need to address it via 0(%[addr]).
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

but see comment below

> ---
>  s390x/skrf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 8ca7588c..84fb762c 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -103,7 +103,7 @@ static void test_tprot(void)
>  {
>  	report_prefix_push("tprot");
>  	expect_pgm_int();
> -	asm volatile("tprot	%[addr],0xf0(0)\n"
> +	asm volatile("tprot	0(%[addr]),0xf0(0)\n"

I think the displacement defaults to 0 if not specified?

did you get a warning, or why are you changing this now?

>  		     : : [addr] "a" (pagebuf) : );
>  	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>  	report_prefix_pop();

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

[Janosch Frank]

On 9/22/21 11:12 AM, Claudio Imbrenda wrote:
> On Wed, 22 Sep 2021 07:18:03 +0000
> Janosch Frank <frankja@linux.ibm.com> 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  | 2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
>> index f05ae4c3..e7446e03 100644
>> --- a/s390x/uv-guest.c
>> +++ b/s390x/uv-guest.c
>> @@ -70,8 +70,8 @@ static void test_query(void)
>>  	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>>  
>>  	uvcb.header.len = sizeof(uvcb);
>> -	cc = uv_call(0, (u64)&uvcb);
>> -	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
>> +	uv_call(0, (u64)&uvcb);
>> +	report(uvcb.header.rc == UVC_RC_EXECUTED || uvcb.header.rc
>> == 0x100, "successful query");
> 
> if you want to be even more pedantic:
> 	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED ||
> 		cc == 1 && uvcb.header.rc == 0x100, ... 

Yeah I pondered about that but at the end I chose to drop the cc check

> 
>>  
>>  	/*
>>  	 * These bits have been introduced with the very first
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index 28035707..66a11160 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -401,7 +401,7 @@ static void test_query(void)
>>  
>>  	uvcb_qui.header.len = sizeof(uvcb_qui);
>>  	uv_call(0, (uint64_t)&uvcb_qui);
>> -	report(uvcb_qui.header.rc == UVC_RC_EXECUTED, "successful query");
>> +	report(uvcb_qui.header.rc == UVC_RC_EXECUTED || uvcb_qui.header.rc == 0x100, "successful query");
> 
> same here
> 
>>  
>>  	for (i = 0; cmds[i].name; i++)
>>  		report(uv_query_test_call(cmds[i].call_bit), "%s", cmds[i].name);
> 

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

[Janosch Frank]

On 9/22/21 11:23 AM, Claudio Imbrenda wrote:
> On Wed, 22 Sep 2021 07:18:07 +0000
> Janosch Frank <frankja@linux.ibm.com> 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>
> 
> but see a nit below
> 
>> ---
>>  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..0e958ad7 100644
>> --- a/lib/s390x/asm/uv.h
>> +++ b/lib/s390x/asm/uv.h
>> @@ -12,6 +12,9 @@
>>  #ifndef _ASMS390X_UV_H_
>>  #define _ASMS390X_UV_H_
>>  
>> +/* Enables printing of command code and return codes for failed UVCs
>> */ +#define UVC_ERR_DEBUG	0
>> +
>>  #define UVC_RC_EXECUTED		0x0001
>>  #define UVC_RC_INV_CMD		0x0002
>>  #define UVC_RC_INV_STATE	0x0003
>> @@ -194,6 +197,15 @@ 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
>> +	if (cc)
> 
> it probably looks cleaner like this:
> 
> 	if (UVC_ERR_DEBUG && cc)
> 
> and without the #if; the compiler should be smart enough to remove the
> dead code. In practice it doesn't really matter in the end, so feel free
> to ignore this comment :)

Didn't consider that, I'll fix it up.
Thank you

> 
>> +		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);
>> +#endif
>> +
>>  	return cc;
>>  }
>>  
> 

Re: [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly

[Janosch Frank]

On 9/22/21 11:34 AM, Claudio Imbrenda wrote:
> On Wed, 22 Sep 2021 07:18:11 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> It's a base + displacement address so we need to address it via 0(%[addr]).
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> but see comment below
> 
>> ---
>>  s390x/skrf.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/s390x/skrf.c b/s390x/skrf.c
>> index 8ca7588c..84fb762c 100644
>> --- a/s390x/skrf.c
>> +++ b/s390x/skrf.c
>> @@ -103,7 +103,7 @@ static void test_tprot(void)
>>  {
>>  	report_prefix_push("tprot");
>>  	expect_pgm_int();
>> -	asm volatile("tprot	%[addr],0xf0(0)\n"
>> +	asm volatile("tprot	0(%[addr]),0xf0(0)\n"
> 
> I think the displacement defaults to 0 if not specified?
> 
> did you get a warning, or why are you changing this now?

It fixes one of the ~18 clang warnings and making it explicit directly
tells you it's a B+D instruction i.e. it looks cleaner to me.

> 
>>  		     : : [addr] "a" (pagebuf) : );
>>  	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>>  	report_prefix_pop();
> 

Re: [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly

[Janosch Frank]

On 9/22/21 3:41 PM, Janis Schoetterl-Glausch wrote:
> On Wed, Sep 22, 2021 at 07:18:11AM +0000, Janosch Frank wrote:
>> It's a base + displacement address so we need to address it via 0(%[addr]).
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/skrf.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/s390x/skrf.c b/s390x/skrf.c
>> index 8ca7588c..84fb762c 100644
>> --- a/s390x/skrf.c
>> +++ b/s390x/skrf.c
>> @@ -103,7 +103,7 @@ static void test_tprot(void)
>>  {
>>  	report_prefix_push("tprot");
>>  	expect_pgm_int();
>> -	asm volatile("tprot	%[addr],0xf0(0)\n"
>> +	asm volatile("tprot	0(%[addr]),0xf0(0)\n"
>>  		     : : [addr] "a" (pagebuf) : );
>>  	check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>>  	report_prefix_pop();
>> -- 
>> 2.30.2
>>
> 
> Let's add an argument to tprot instead.

Sure, LGTM

> -- >8 --
> Subject: [kvm-unit-tests PATCH] lib: s390x: Add access key argument to tprot
> 
> 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 302ef1f..55f3124 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -206,15 +206,15 @@ static inline unsigned short stap(void)
>  	return cpu_address;
>  }
>  
> -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 9502d16..0272249 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 9488c32..e0a1007 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -102,8 +102,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();
>  }
> 

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

[Thomas Huth]

On 22/09/2021 13.36, Janosch Frank wrote:
> On 9/22/21 11:12 AM, Claudio Imbrenda wrote:
>> On Wed, 22 Sep 2021 07:18:03 +0000
>> Janosch Frank <frankja@linux.ibm.com> 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  | 2 +-
>>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
>>> index f05ae4c3..e7446e03 100644
>>> --- a/s390x/uv-guest.c
>>> +++ b/s390x/uv-guest.c
>>> @@ -70,8 +70,8 @@ static void test_query(void)
>>>   	report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>>>   
>>>   	uvcb.header.len = sizeof(uvcb);
>>> -	cc = uv_call(0, (u64)&uvcb);
>>> -	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
>>> +	uv_call(0, (u64)&uvcb);
>>> +	report(uvcb.header.rc == UVC_RC_EXECUTED || uvcb.header.rc
>>> == 0x100, "successful query");
>>
>> if you want to be even more pedantic:
>> 	report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED ||
>> 		cc == 1 && uvcb.header.rc == 0x100, ...
> 
> Yeah I pondered about that but at the end I chose to drop the cc check

Well, but we're in the kvm-unit-tests here where we check for the exact 
behavior of the system ... so I'd vote for keeping/extening the cc checking, 
even if it's more code to read in the end.

  Thomas

Re: [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling

[Thomas Huth]

On 22/09/2021 09.18, Janosch Frank wrote:
> On everything larger than 4k pfmf will update the address in GR2 when
> it's interrupted so we should loop on pfmf and not trust that it
> doesn't get interrupted.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/pfmf.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)

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

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

[Thomas Huth]

On 22/09/2021 09.18, Janosch Frank wrote:
> 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>
> ---
>   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 66a11160..5e351120 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;
> +	}

So this is a bug in the firmware? Any chance that it will still get fixed 
for the z15? If so, would it make sense to turn this into a report_xfail() 
instead?

  Thomas

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

[Thomas Huth]

On 22/09/2021 09.18, Janosch Frank wrote:
> 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

Maybe: s/share/share()/
... so that it is clear that you're talking about the function here...

> avoid linking problems so lets remove the report_info().
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.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);
>   }

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

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

[Thomas Huth]

On 22/09/2021 09.18, 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>
> ---
>   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..0e958ad7 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -12,6 +12,9 @@
>   #ifndef _ASMS390X_UV_H_
>   #define _ASMS390X_UV_H_
>   
> +/* Enables printing of command code and return codes for failed UVCs */
> +#define UVC_ERR_DEBUG	0

Do we maybe want a "#ifndef UVC_ERR_DEBUG" in front of this, so that we 
could also set the macro to 1 from individual *.c files (or from the Makefile)?

  Thomas

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

[Thomas Huth]

On 22/09/2021 09.18, Janosch Frank wrote:
> 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>
> ---
>   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);
> 

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

Re: [kvm-unit-tests PATCH 7/9] s390x: Makefile: Remove snippet flatlib linking

[Thomas Huth]

On 22/09/2021 09.18, Janosch Frank wrote:
> We can't link the flatlib as we do not export everything that it needs
> and we don't (want to) call the init functions.
> 
> In the future we might implement a tiny lib that uses select lib

s/select/selected/ ?

> object files and re-implements functions like assert() and
> test_facility() to be able to use some parts of the lib.
> 
> 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 5d1a33a0..d09c0a17 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -92,7 +92,7 @@ $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
>   	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
>   
>   $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_asmlib) $(FLATLIBS)
> -	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib) $(FLATLIBS)
> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib)

Don't we need memcpy() and friends in some cases? ... well, likely not, 
otherwise linking would fail... So:

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

Re: [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling

[Janosch Frank]

On 9/27/21 17:23, Thomas Huth wrote:
> On 22/09/2021 09.18, Janosch Frank wrote:
>> On everything larger than 4k pfmf will update the address in GR2 when
>> it's interrupted so we should loop on pfmf and not trust that it
>> doesn't get interrupted.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    s390x/pfmf.c | 10 ++++++++--
>>    1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 

I had another discussion with the people that told me to change this and 
after some starring at the documentation we found out this won't fix 
anything because it's not a problem. FW will update GR2 and back off the 
PSW when it has been interrupted so it effectively loops itself.

So I'm dropping this patch

Re: [kvm-unit-tests PATCH 7/9] s390x: Makefile: Remove snippet flatlib linking

[Janosch Frank]

On 9/27/21 19:47, Thomas Huth wrote:
> On 22/09/2021 09.18, Janosch Frank wrote:
>> We can't link the flatlib as we do not export everything that it needs
>> and we don't (want to) call the init functions.
>>
>> In the future we might implement a tiny lib that uses select lib
> 
> s/select/selected/ ?

Sure

> 
>> object files and re-implements functions like assert() and
>> test_facility() to be able to use some parts of the lib.
>>
>> 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 5d1a33a0..d09c0a17 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -92,7 +92,7 @@ $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
>>    	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
>>    
>>    $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_asmlib) $(FLATLIBS)
>> -	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib) $(FLATLIBS)
>> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib)
> 
> Don't we need memcpy() and friends in some cases? ... well, likely not,
> otherwise linking would fail... So:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Currently not but we can still import header files and use static things 
which is very helpful already. Fixing this will require a lot of thought 
since I don't really want to write a new lib just for the snippets but 
making them compatible to the flatlib will also require a lot of work.

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

[Janosch Frank]

On 9/27/21 19:41, Thomas Huth wrote:
> On 22/09/2021 09.18, 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>
>> ---
>>    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..0e958ad7 100644
>> --- a/lib/s390x/asm/uv.h
>> +++ b/lib/s390x/asm/uv.h
>> @@ -12,6 +12,9 @@
>>    #ifndef _ASMS390X_UV_H_
>>    #define _ASMS390X_UV_H_
>>    
>> +/* Enables printing of command code and return codes for failed UVCs */
>> +#define UVC_ERR_DEBUG	0
> 
> Do we maybe want a "#ifndef UVC_ERR_DEBUG" in front of this, so that we
> could also set the macro to 1 from individual *.c files (or from the Makefile)?
> 
>    Thomas
> 

When testing it's the least amount of work to set this to 1 so I 
implemented it this way. If you think it's useful then I'll add the ifndef.

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

[Janosch Frank]

On 9/27/21 17:26, Thomas Huth wrote:
> On 22/09/2021 09.18, Janosch Frank wrote:
>> 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>
>> ---
>>    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 66a11160..5e351120 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;
>> +	}
> 
> So this is a bug in the firmware? Any chance that it will still get fixed
> for the z15? If so, would it make sense to turn this into a report_xfail()
> instead?
> 
>    Thomas
> 

No, a xfail will not help here.

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

[Thomas Huth]

On 28/09/2021 13.21, Janosch Frank wrote:
> On 9/27/21 17:26, Thomas Huth wrote:
>> On 22/09/2021 09.18, Janosch Frank wrote:
>>> 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>
>>> ---
>>>    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;

You could skip the masking line since the function returns an uint61_t anyway.

>>> +
>>> +    return cpuid;
>>> +}
>>> +
>>>    static inline int tprot(unsigned long addr)
>>>    {
>>>        int cc;
>>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>>> index 66a11160..5e351120 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;
>>> +    }
>>
>> So this is a bug in the firmware? Any chance that it will still get fixed
>> for the z15? If so, would it make sense to turn this into a report_xfail()
>> instead?
>>
>>    Thomas
>>
> 
> No, a xfail will not help here.

Ok, fair, then I think the patch is fine:

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