[kvm-unit-tests PATCH 0/5] s390x: Allocation and hosting environment detection fixes

[kvm-unit-tests PATCH 0/5] s390x: Allocation and hosting environment detection fixes

[Janosch Frank]

I took some time before Christmas to write a test runner for lpar
which automatically runs all tests and sends me the logs. It's based
on the zhmc library to control starting and stopping of the lpar and
works by having a menu entry for each kvm unit test.

This revealed a number of test fails when the tests are run under lpar
as there are a few differences:
   * lpars most often have a very high memory amount (upwards of 8GB)
     compared to our qemu env (256MB)
   * lpar supports diag308 subcode 2
   * lpar does not provide virtio devices

The higher memory amount leads to allocations crossing the 2GB or 4GB
border which made sclp and sigp calls fail that expect 31/32 bit
addresses.

Janosch Frank (5):
  lib: s390x: vm: Add kvm and lpar vm queries
  s390x: css: Skip if we're not run by qemu
  s390x: diag308: Only test subcode 2 under QEMU
  s390x: smp: Allocate memory in DMA31 space
  s390x: firq: Fix sclp buffer allocation

 lib/s390x/vm.c  | 39 +++++++++++++++++++++++++++++++++++++++
 lib/s390x/vm.h  | 23 +++++++++++++++++++++++
 s390x/css.c     | 10 +++++++++-
 s390x/diag308.c | 15 ++++++++++++++-
 s390x/firq.c    |  2 +-
 s390x/smp.c     |  4 ++--
 s390x/stsi.c    | 21 +--------------------
 7 files changed, 89 insertions(+), 25 deletions(-)

-- 
2.32.0

[kvm-unit-tests PATCH 1/5] lib: s390x: vm: Add kvm and lpar vm queries

[Janosch Frank]

This patch will likely (in parts) be replaced by Pierre's patch from
his topology test series.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/vm.c | 39 +++++++++++++++++++++++++++++++++++++++
 lib/s390x/vm.h | 23 +++++++++++++++++++++++
 s390x/stsi.c   | 21 +--------------------
 3 files changed, 63 insertions(+), 20 deletions(-)

diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
index a5b92863..266a81c1 100644
--- a/lib/s390x/vm.c
+++ b/lib/s390x/vm.c
@@ -26,6 +26,11 @@ bool vm_is_tcg(void)
 	if (initialized)
 		return is_tcg;
 
+	if (stsi_get_fc() < 3) {
+		initialized = true;
+		return false;
+	}
+
 	buf = alloc_page();
 	if (!buf)
 		return false;
@@ -43,3 +48,37 @@ out:
 	free_page(buf);
 	return is_tcg;
 }
+
+bool vm_is_kvm(void)
+{
+	const uint8_t cpi_kvm[] = { 0xd2, 0xe5, 0xd4, 0x61 };
+	struct stsi_322 *buf;
+	static bool initialized = false;
+	static bool is_kvm = false;
+
+	if (initialized)
+		return is_kvm;
+
+	if (stsi_get_fc() < 3) {
+		initialized = true;
+		return false;
+	}
+
+	buf = alloc_page();
+	if (!buf)
+		return false;
+
+	if (stsi(buf, 3, 2, 2))
+		goto out;
+
+	is_kvm = !(memcmp(&buf->vm[0].cpi, cpi_kvm, sizeof(cpi_kvm))) && !vm_is_tcg();
+	initialized = true;
+out:
+	free_page(buf);
+	return is_kvm;
+}
+
+bool vm_is_lpar(void)
+{
+	return stsi_get_fc() == 1;
+}
diff --git a/lib/s390x/vm.h b/lib/s390x/vm.h
index 7abba0cc..d4a82fc0 100644
--- a/lib/s390x/vm.h
+++ b/lib/s390x/vm.h
@@ -8,6 +8,29 @@
 #ifndef _S390X_VM_H_
 #define _S390X_VM_H_
 
+struct stsi_322 {
+	uint8_t reserved[31];
+	uint8_t count;
+	struct {
+		uint8_t reserved2[4];
+		uint16_t total_cpus;
+		uint16_t conf_cpus;
+		uint16_t standby_cpus;
+		uint16_t reserved_cpus;
+		uint8_t name[8];
+		uint32_t caf;
+		uint8_t cpi[16];
+		uint8_t reserved5[3];
+		uint8_t ext_name_encoding;
+		uint32_t reserved3;
+		uint8_t uuid[16];
+	} vm[8];
+	uint8_t reserved4[1504];
+	uint8_t ext_names[8][256];
+};
+
 bool vm_is_tcg(void);
+bool vm_is_kvm(void);
+bool vm_is_lpar(void);
 
 #endif  /* _S390X_VM_H_ */
diff --git a/s390x/stsi.c b/s390x/stsi.c
index 391f8849..e66d07a1 100644
--- a/s390x/stsi.c
+++ b/s390x/stsi.c
@@ -13,27 +13,8 @@
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <smp.h>
+#include <vm.h>
 
-struct stsi_322 {
-	uint8_t reserved[31];
-	uint8_t count;
-	struct {
-		uint8_t reserved2[4];
-		uint16_t total_cpus;
-		uint16_t conf_cpus;
-		uint16_t standby_cpus;
-		uint16_t reserved_cpus;
-		uint8_t name[8];
-		uint32_t caf;
-		uint8_t cpi[16];
-		uint8_t reserved5[3];
-		uint8_t ext_name_encoding;
-		uint32_t reserved3;
-		uint8_t uuid[16];
-	} vm[8];
-	uint8_t reserved4[1504];
-	uint8_t ext_names[8][256];
-};
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 
 static void test_specs(void)
-- 
2.32.0

[kvm-unit-tests PATCH 2/5] s390x: css: Skip if we're not run by qemu

[Janosch Frank]

There's no guarantee that we even find a device at the address we're
testing for if we're not running under QEMU.

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

diff --git a/s390x/css.c b/s390x/css.c
index 881206ba..c24119b4 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -15,6 +15,7 @@
 #include <interrupt.h>
 #include <asm/arch_def.h>
 #include <alloc_page.h>
+#include <vm.h>
 
 #include <malloc_io.h>
 #include <css.h>
@@ -350,6 +351,12 @@ int main(int argc, char *argv[])
 {
 	int i;
 
+	/* There's no guarantee where our devices are without qemu */
+	if (!vm_is_kvm() && !vm_is_tcg()) {
+		report_skip("Not running under QEMU");
+		goto done;
+	}
+
 	report_prefix_push("Channel Subsystem");
 	enable_io_isc(0x80 >> IO_SCH_ISC);
 	for (i = 0; tests[i].name; i++) {
@@ -357,7 +364,8 @@ int main(int argc, char *argv[])
 		tests[i].func();
 		report_prefix_pop();
 	}
-	report_prefix_pop();
 
+done:
+	report_prefix_pop();
 	return report_summary();
 }
-- 
2.32.0

[kvm-unit-tests PATCH 3/5] s390x: diag308: Only test subcode 2 under QEMU

[Janosch Frank]

Other hypervisors might implement it and therefore not send a
specification exception.

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

diff --git a/s390x/diag308.c b/s390x/diag308.c
index c9d6c499..414dbdf4 100644
--- a/s390x/diag308.c
+++ b/s390x/diag308.c
@@ -8,6 +8,7 @@
 #include <libcflat.h>
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
+#include <vm.h>
 
 /* The diagnose calls should be blocked in problem state */
 static void test_priv(void)
@@ -75,7 +76,7 @@ static void test_subcode6(void)
 /* Unsupported subcodes should generate a specification exception */
 static void test_unsupported_subcode(void)
 {
-	int subcodes[] = { 2, 0x101, 0xffff, 0x10001, -1 };
+	int subcodes[] = { 0x101, 0xffff, 0x10001, -1 };
 	int idx;
 
 	for (idx = 0; idx < ARRAY_SIZE(subcodes); idx++) {
@@ -85,6 +86,18 @@ static void test_unsupported_subcode(void)
 		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
 		report_prefix_pop();
 	}
+
+	/*
+	 * Subcode 2 is not available under QEMU but might be on other
+	 * hypervisors.
+	 */
+	if (vm_is_tcg() || vm_is_kvm()) {
+		report_prefix_pushf("0x%04x", 2);
+		expect_pgm_int();
+		asm volatile ("diag %0,%1,0x308" :: "d"(0), "d"(2));
+		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+		report_prefix_pop();
+	}
 }
 
 static struct {
-- 
2.32.0

[kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Janosch Frank]

The store status at address order works with 31 bit addresses so let's
use them.

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

diff --git a/s390x/smp.c b/s390x/smp.c
index 32f128b3..c91f170b 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -124,7 +124,7 @@ static void test_stop_store_status(void)
 
 static void test_store_status(void)
 {
-	struct cpu_status *status = alloc_pages(1);
+	struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
 	uint32_t r;
 
 	report_prefix_push("store status at address");
@@ -244,7 +244,7 @@ static void test_func_initial(void)
 
 static void test_reset_initial(void)
 {
-	struct cpu_status *status = alloc_pages(0);
+	struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
 	struct psw psw;
 	int i;
 
-- 
2.32.0

[kvm-unit-tests PATCH 5/5] s390x: firq: Fix sclp buffer allocation

[Janosch Frank]

We need a 32 bit address for the sclp buffer so let's use a page from
the first 31 bits.

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

diff --git a/s390x/firq.c b/s390x/firq.c
index 1f877183..a0ef1555 100644
--- a/s390x/firq.c
+++ b/s390x/firq.c
@@ -87,7 +87,7 @@ static void test_wait_state_delivery(void)
 	 */
 	while(smp_sense_running_status(1));
 
-	h = alloc_page();
+	h = alloc_pages_flags(1, AREA_DMA31);
 	h->length = 4096;
 	ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h));
 	if (ret) {
-- 
2.32.0

Re: [kvm-unit-tests PATCH 3/5] s390x: diag308: Only test subcode 2 under QEMU

[Claudio Imbrenda]

On Fri, 14 Jan 2022 10:02:43 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Other hypervisors might implement it and therefore not send a
> specification exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/diag308.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/diag308.c b/s390x/diag308.c
> index c9d6c499..414dbdf4 100644
> --- a/s390x/diag308.c
> +++ b/s390x/diag308.c
> @@ -8,6 +8,7 @@
>  #include <libcflat.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/interrupt.h>
> +#include <vm.h>
>  
>  /* The diagnose calls should be blocked in problem state */
>  static void test_priv(void)
> @@ -75,7 +76,7 @@ static void test_subcode6(void)
>  /* Unsupported subcodes should generate a specification exception */
>  static void test_unsupported_subcode(void)
>  {
> -	int subcodes[] = { 2, 0x101, 0xffff, 0x10001, -1 };
> +	int subcodes[] = { 0x101, 0xffff, 0x10001, -1 };
>  	int idx;
>  
>  	for (idx = 0; idx < ARRAY_SIZE(subcodes); idx++) {
> @@ -85,6 +86,18 @@ static void test_unsupported_subcode(void)
>  		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>  		report_prefix_pop();
>  	}
> +
> +	/*
> +	 * Subcode 2 is not available under QEMU but might be on other
> +	 * hypervisors.
> +	 */
> +	if (vm_is_tcg() || vm_is_kvm()) {
> +		report_prefix_pushf("0x%04x", 2);
> +		expect_pgm_int();
> +		asm volatile ("diag %0,%1,0x308" :: "d"(0), "d"(2));
> +		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +		report_prefix_pop();
> +	}

maybe add

	else { report_skip("0x0002 Only tested in Qemu"); }

>  }
>  
>  static struct {

Re: [kvm-unit-tests PATCH 2/5] s390x: css: Skip if we're not run by qemu

[Claudio Imbrenda]

On Fri, 14 Jan 2022 10:02:42 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> There's no guarantee that we even find a device at the address we're
> testing for if we're not running under QEMU.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/css.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index 881206ba..c24119b4 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -15,6 +15,7 @@
>  #include <interrupt.h>
>  #include <asm/arch_def.h>
>  #include <alloc_page.h>
> +#include <vm.h>
>  
>  #include <malloc_io.h>
>  #include <css.h>
> @@ -350,6 +351,12 @@ int main(int argc, char *argv[])
>  {
>  	int i;
>  
> +	/* There's no guarantee where our devices are without qemu */
> +	if (!vm_is_kvm() && !vm_is_tcg()) {
> +		report_skip("Not running under QEMU");
> +		goto done;
> +	}
> +
>  	report_prefix_push("Channel Subsystem");
>  	enable_io_isc(0x80 >> IO_SCH_ISC);
>  	for (i = 0; tests[i].name; i++) {
> @@ -357,7 +364,8 @@ int main(int argc, char *argv[])
>  		tests[i].func();
>  		report_prefix_pop();
>  	}
> -	report_prefix_pop();
>  
> +done:
> +	report_prefix_pop();
>  	return report_summary();
>  }

Re: [kvm-unit-tests PATCH 1/5] lib: s390x: vm: Add kvm and lpar vm queries

[Claudio Imbrenda]

On Fri, 14 Jan 2022 10:02:41 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> This patch will likely (in parts) be replaced by Pierre's patch from
> his topology test series.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

although I think there is some room for improvement, but nothing too
serious, I'll probably fix it myself later

> ---
>  lib/s390x/vm.c | 39 +++++++++++++++++++++++++++++++++++++++
>  lib/s390x/vm.h | 23 +++++++++++++++++++++++
>  s390x/stsi.c   | 21 +--------------------
>  3 files changed, 63 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
> index a5b92863..266a81c1 100644
> --- a/lib/s390x/vm.c
> +++ b/lib/s390x/vm.c
> @@ -26,6 +26,11 @@ bool vm_is_tcg(void)
>  	if (initialized)
>  		return is_tcg;
>  
> +	if (stsi_get_fc() < 3) {
> +		initialized = true;
> +		return false;
> +	}
> +
>  	buf = alloc_page();
>  	if (!buf)
>  		return false;
> @@ -43,3 +48,37 @@ out:
>  	free_page(buf);
>  	return is_tcg;
>  }
> +
> +bool vm_is_kvm(void)
> +{
> +	const uint8_t cpi_kvm[] = { 0xd2, 0xe5, 0xd4, 0x61 };
> +	struct stsi_322 *buf;
> +	static bool initialized = false;
> +	static bool is_kvm = false;
> +
> +	if (initialized)
> +		return is_kvm;
> +
> +	if (stsi_get_fc() < 3) {
> +		initialized = true;
> +		return false;
> +	}
> +
> +	buf = alloc_page();
> +	if (!buf)
> +		return false;
> +
> +	if (stsi(buf, 3, 2, 2))
> +		goto out;
> +
> +	is_kvm = !(memcmp(&buf->vm[0].cpi, cpi_kvm, sizeof(cpi_kvm))) && !vm_is_tcg();
> +	initialized = true;
> +out:
> +	free_page(buf);
> +	return is_kvm;
> +}
> +
> +bool vm_is_lpar(void)
> +{
> +	return stsi_get_fc() == 1;
> +}
> diff --git a/lib/s390x/vm.h b/lib/s390x/vm.h
> index 7abba0cc..d4a82fc0 100644
> --- a/lib/s390x/vm.h
> +++ b/lib/s390x/vm.h
> @@ -8,6 +8,29 @@
>  #ifndef _S390X_VM_H_
>  #define _S390X_VM_H_
>  
> +struct stsi_322 {
> +	uint8_t reserved[31];
> +	uint8_t count;
> +	struct {
> +		uint8_t reserved2[4];
> +		uint16_t total_cpus;
> +		uint16_t conf_cpus;
> +		uint16_t standby_cpus;
> +		uint16_t reserved_cpus;
> +		uint8_t name[8];
> +		uint32_t caf;
> +		uint8_t cpi[16];
> +		uint8_t reserved5[3];
> +		uint8_t ext_name_encoding;
> +		uint32_t reserved3;
> +		uint8_t uuid[16];
> +	} vm[8];
> +	uint8_t reserved4[1504];
> +	uint8_t ext_names[8][256];
> +};
> +
>  bool vm_is_tcg(void);
> +bool vm_is_kvm(void);
> +bool vm_is_lpar(void);
>  
>  #endif  /* _S390X_VM_H_ */
> diff --git a/s390x/stsi.c b/s390x/stsi.c
> index 391f8849..e66d07a1 100644
> --- a/s390x/stsi.c
> +++ b/s390x/stsi.c
> @@ -13,27 +13,8 @@
>  #include <asm/asm-offsets.h>
>  #include <asm/interrupt.h>
>  #include <smp.h>
> +#include <vm.h>
>  
> -struct stsi_322 {
> -	uint8_t reserved[31];
> -	uint8_t count;
> -	struct {
> -		uint8_t reserved2[4];
> -		uint16_t total_cpus;
> -		uint16_t conf_cpus;
> -		uint16_t standby_cpus;
> -		uint16_t reserved_cpus;
> -		uint8_t name[8];
> -		uint32_t caf;
> -		uint8_t cpi[16];
> -		uint8_t reserved5[3];
> -		uint8_t ext_name_encoding;
> -		uint32_t reserved3;
> -		uint8_t uuid[16];
> -	} vm[8];
> -	uint8_t reserved4[1504];
> -	uint8_t ext_names[8][256];
> -};
>  static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
>  
>  static void test_specs(void)

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Claudio Imbrenda]

On Fri, 14 Jan 2022 10:02:44 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> The store status at address order works with 31 bit addresses so let's
> use them.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/smp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 32f128b3..c91f170b 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -124,7 +124,7 @@ static void test_stop_store_status(void)
>  
>  static void test_store_status(void)
>  {
> -	struct cpu_status *status = alloc_pages(1);
> +	struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
>  	uint32_t r;
>  
>  	report_prefix_push("store status at address");
> @@ -244,7 +244,7 @@ static void test_func_initial(void)
>  
>  static void test_reset_initial(void)
>  {
> -	struct cpu_status *status = alloc_pages(0);
> +	struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
>  	struct psw psw;
>  	int i;
>  

Re: [kvm-unit-tests PATCH 5/5] s390x: firq: Fix sclp buffer allocation

[Claudio Imbrenda]

On Fri, 14 Jan 2022 10:02:45 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> We need a 32 bit address for the sclp buffer so let's use a page from
> the first 31 bits.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/firq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/firq.c b/s390x/firq.c
> index 1f877183..a0ef1555 100644
> --- a/s390x/firq.c
> +++ b/s390x/firq.c
> @@ -87,7 +87,7 @@ static void test_wait_state_delivery(void)
>  	 */
>  	while(smp_sense_running_status(1));
>  
> -	h = alloc_page();
> +	h = alloc_pages_flags(1, AREA_DMA31);
>  	h->length = 4096;
>  	ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h));
>  	if (ret) {

Re: [kvm-unit-tests PATCH 0/5] s390x: Allocation and hosting environment detection fixes

[Claudio Imbrenda]

On Fri, 14 Jan 2022 10:02:40 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> I took some time before Christmas to write a test runner for lpar
> which automatically runs all tests and sends me the logs. It's based
> on the zhmc library to control starting and stopping of the lpar and
> works by having a menu entry for each kvm unit test.
> 
> This revealed a number of test fails when the tests are run under lpar
> as there are a few differences:
>    * lpars most often have a very high memory amount (upwards of 8GB)
>      compared to our qemu env (256MB)
>    * lpar supports diag308 subcode 2
>    * lpar does not provide virtio devices
> 
> The higher memory amount leads to allocations crossing the 2GB or 4GB
> border which made sclp and sigp calls fail that expect 31/32 bit
> addresses.
> 

the series looks good to me; if you send me a fixed patch 3, I'll queue
this together with the other ones

> Janosch Frank (5):
>   lib: s390x: vm: Add kvm and lpar vm queries
>   s390x: css: Skip if we're not run by qemu
>   s390x: diag308: Only test subcode 2 under QEMU
>   s390x: smp: Allocate memory in DMA31 space
>   s390x: firq: Fix sclp buffer allocation
> 
>  lib/s390x/vm.c  | 39 +++++++++++++++++++++++++++++++++++++++
>  lib/s390x/vm.h  | 23 +++++++++++++++++++++++
>  s390x/css.c     | 10 +++++++++-
>  s390x/diag308.c | 15 ++++++++++++++-
>  s390x/firq.c    |  2 +-
>  s390x/smp.c     |  4 ++--
>  s390x/stsi.c    | 21 +--------------------
>  7 files changed, 89 insertions(+), 25 deletions(-)
> 

Re: [kvm-unit-tests PATCH 0/5] s390x: Allocation and hosting environment detection fixes

[Janosch Frank]

On 1/14/22 12:19, Claudio Imbrenda wrote:
> On Fri, 14 Jan 2022 10:02:40 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> I took some time before Christmas to write a test runner for lpar
>> which automatically runs all tests and sends me the logs. It's based
>> on the zhmc library to control starting and stopping of the lpar and
>> works by having a menu entry for each kvm unit test.
>>
>> This revealed a number of test fails when the tests are run under lpar
>> as there are a few differences:
>>     * lpars most often have a very high memory amount (upwards of 8GB)
>>       compared to our qemu env (256MB)
>>     * lpar supports diag308 subcode 2
>>     * lpar does not provide virtio devices
>>
>> The higher memory amount leads to allocations crossing the 2GB or 4GB
>> border which made sclp and sigp calls fail that expect 31/32 bit
>> addresses.
>>
> 
> the series looks good to me; if you send me a fixed patch 3, I'll queue
> this together with the other ones

Well, since Pierre originally came up with a large part of the code for 
patch 1 I'll wait with a new version until we picked his fixed patch so 
I can rebase on it.

But you can already pick the allocation patches if you want.

> 
>> Janosch Frank (5):
>>    lib: s390x: vm: Add kvm and lpar vm queries
>>    s390x: css: Skip if we're not run by qemu
>>    s390x: diag308: Only test subcode 2 under QEMU
>>    s390x: smp: Allocate memory in DMA31 space
>>    s390x: firq: Fix sclp buffer allocation
>>
>>   lib/s390x/vm.c  | 39 +++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/vm.h  | 23 +++++++++++++++++++++++
>>   s390x/css.c     | 10 +++++++++-
>>   s390x/diag308.c | 15 ++++++++++++++-
>>   s390x/firq.c    |  2 +-
>>   s390x/smp.c     |  4 ++--
>>   s390x/stsi.c    | 21 +--------------------
>>   7 files changed, 89 insertions(+), 25 deletions(-)
>>
> 

Re: [kvm-unit-tests PATCH 1/5] lib: s390x: vm: Add kvm and lpar vm queries

[Janosch Frank]

On 1/14/22 12:18, Claudio Imbrenda wrote:
> On Fri, 14 Jan 2022 10:02:41 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> This patch will likely (in parts) be replaced by Pierre's patch from
>> his topology test series.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> although I think there is some room for improvement, but nothing too
> serious, I'll probably fix it myself later

I think I should add this to clean up the checks in the following patches:

bool vm_is_qemu(void)
{
	return vm_is_kvm() || vm_is_tcg();
}

That of course also isn't a 100% correct, we could have KVM + non-QEMU 
but I'll cross that bridge when I get there.

And as I already mentioned in Pierre's patch, the STSI values are a bit 
of a mystery to me since AFAIK we store KVM/Linux even if we run under TCG.

> 
>> ---
>>   lib/s390x/vm.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/vm.h | 23 +++++++++++++++++++++++
>>   s390x/stsi.c   | 21 +--------------------
>>   3 files changed, 63 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
>> index a5b92863..266a81c1 100644
>> --- a/lib/s390x/vm.c
>> +++ b/lib/s390x/vm.c
>> @@ -26,6 +26,11 @@ bool vm_is_tcg(void)
>>   	if (initialized)
>>   		return is_tcg;
>>   
>> +	if (stsi_get_fc() < 3) {
>> +		initialized = true;
>> +		return false;
>> +	}
>> +
>>   	buf = alloc_page();
>>   	if (!buf)
>>   		return false;
>> @@ -43,3 +48,37 @@ out:
>>   	free_page(buf);
>>   	return is_tcg;
>>   }
>> +
>> +bool vm_is_kvm(void)
>> +{
>> +	const uint8_t cpi_kvm[] = { 0xd2, 0xe5, 0xd4, 0x61 };
>> +	struct stsi_322 *buf;
>> +	static bool initialized = false;
>> +	static bool is_kvm = false;
>> +
>> +	if (initialized)
>> +		return is_kvm;
>> +
>> +	if (stsi_get_fc() < 3) {
>> +		initialized = true;
>> +		return false;
>> +	}
>> +
>> +	buf = alloc_page();
>> +	if (!buf)
>> +		return false;
>> +
>> +	if (stsi(buf, 3, 2, 2))
>> +		goto out;
>> +
>> +	is_kvm = !(memcmp(&buf->vm[0].cpi, cpi_kvm, sizeof(cpi_kvm))) && !vm_is_tcg();
>> +	initialized = true;
>> +out:
>> +	free_page(buf);
>> +	return is_kvm;
>> +}
>> +
>> +bool vm_is_lpar(void)
>> +{
>> +	return stsi_get_fc() == 1;
>> +}
>> diff --git a/lib/s390x/vm.h b/lib/s390x/vm.h
>> index 7abba0cc..d4a82fc0 100644
>> --- a/lib/s390x/vm.h
>> +++ b/lib/s390x/vm.h
>> @@ -8,6 +8,29 @@
>>   #ifndef _S390X_VM_H_
>>   #define _S390X_VM_H_
>>   
>> +struct stsi_322 {
>> +	uint8_t reserved[31];
>> +	uint8_t count;
>> +	struct {
>> +		uint8_t reserved2[4];
>> +		uint16_t total_cpus;
>> +		uint16_t conf_cpus;
>> +		uint16_t standby_cpus;
>> +		uint16_t reserved_cpus;
>> +		uint8_t name[8];
>> +		uint32_t caf;
>> +		uint8_t cpi[16];
>> +		uint8_t reserved5[3];
>> +		uint8_t ext_name_encoding;
>> +		uint32_t reserved3;
>> +		uint8_t uuid[16];
>> +	} vm[8];
>> +	uint8_t reserved4[1504];
>> +	uint8_t ext_names[8][256];
>> +};
>> +
>>   bool vm_is_tcg(void);
>> +bool vm_is_kvm(void);
>> +bool vm_is_lpar(void);
>>   
>>   #endif  /* _S390X_VM_H_ */
>> diff --git a/s390x/stsi.c b/s390x/stsi.c
>> index 391f8849..e66d07a1 100644
>> --- a/s390x/stsi.c
>> +++ b/s390x/stsi.c
>> @@ -13,27 +13,8 @@
>>   #include <asm/asm-offsets.h>
>>   #include <asm/interrupt.h>
>>   #include <smp.h>
>> +#include <vm.h>
>>   
>> -struct stsi_322 {
>> -	uint8_t reserved[31];
>> -	uint8_t count;
>> -	struct {
>> -		uint8_t reserved2[4];
>> -		uint16_t total_cpus;
>> -		uint16_t conf_cpus;
>> -		uint16_t standby_cpus;
>> -		uint16_t reserved_cpus;
>> -		uint8_t name[8];
>> -		uint32_t caf;
>> -		uint8_t cpi[16];
>> -		uint8_t reserved5[3];
>> -		uint8_t ext_name_encoding;
>> -		uint32_t reserved3;
>> -		uint8_t uuid[16];
>> -	} vm[8];
>> -	uint8_t reserved4[1504];
>> -	uint8_t ext_names[8][256];
>> -};
>>   static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
>>   
>>   static void test_specs(void)
> 

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Nico Boehr]

On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:
> The store status at address order works with 31 bit addresses so
> let's
> use them.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 32f128b3..c91f170b 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c

[...]

> @@ -244,7 +244,7 @@ static void test_func_initial(void)
>  
>  static void test_reset_initial(void)
>  {
> -       struct cpu_status *status = alloc_pages(0);
> +       struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);

Why do we need two pages now?

Re: [kvm-unit-tests PATCH 1/5] lib: s390x: vm: Add kvm and lpar vm queries

[Claudio Imbrenda]

On Fri, 14 Jan 2022 13:28:19 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 1/14/22 12:18, Claudio Imbrenda wrote:
> > On Fri, 14 Jan 2022 10:02:41 +0000
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> This patch will likely (in parts) be replaced by Pierre's patch from
> >> his topology test series.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>  
> > 
> > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > 
> > although I think there is some room for improvement, but nothing too
> > serious, I'll probably fix it myself later  
> 
> I think I should add this to clean up the checks in the following patches:
> 
> bool vm_is_qemu(void)
> {
> 	return vm_is_kvm() || vm_is_tcg();
> }
> 
> That of course also isn't a 100% correct, we could have KVM + non-QEMU 
> but I'll cross that bridge when I get there.
> 
> And as I already mentioned in Pierre's patch, the STSI values are a bit 
> of a mystery to me since AFAIK we store KVM/Linux even if we run under TCG.

I was actually thinking of a generic detection function that will call
STSI only once and set all the necessary flags. 

then the various vm_is_* functions can become something like this:

bool vm_is_${TYPE}()
{
	if (vm_type == VM_TYPE_NOT_INITIALIZED)
		do_vm_detection();
	return vm_type == VM_${TYPE};
}

and then have all the magic hidden in the one do_vm_detection, which
will call STSI once and try to find out what we are running on.

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Claudio Imbrenda]

On Fri, 14 Jan 2022 13:50:52 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:
> > The store status at address order works with 31 bit addresses so
> > let's
> > use them.
> > 
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >  s390x/smp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/s390x/smp.c b/s390x/smp.c
> > index 32f128b3..c91f170b 100644
> > --- a/s390x/smp.c
> > +++ b/s390x/smp.c  
> 
> [...]
> 
> > @@ -244,7 +244,7 @@ static void test_func_initial(void)
> >  
> >  static void test_reset_initial(void)
> >  {
> > -       struct cpu_status *status = alloc_pages(0);
> > +       struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);  
> 
> Why do we need two pages now?

oh, good catch

the next patch has the same issue

I can fix them up when I queue them

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Claudio Imbrenda]

On Fri, 14 Jan 2022 13:50:52 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:
> > The store status at address order works with 31 bit addresses so
> > let's
> > use them.
> > 
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >  s390x/smp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/s390x/smp.c b/s390x/smp.c
> > index 32f128b3..c91f170b 100644
> > --- a/s390x/smp.c
> > +++ b/s390x/smp.c  
> 
> [...]
> 
> > @@ -244,7 +244,7 @@ static void test_func_initial(void)
> >  
> >  static void test_reset_initial(void)
> >  {
> > -       struct cpu_status *status = alloc_pages(0);
> > +       struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);  
> 
> Why do we need two pages now?

actually, wait.....

        struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
        uint32_t r;

        report_prefix_push("store status at address");
        memset(status, 0, PAGE_SIZE * 2);

we were allocating one page, and using 2!

@Janosch do we need 1 or 2 pages?

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Janosch Frank]

On 1/14/22 13:57, Claudio Imbrenda wrote:
> On Fri, 14 Jan 2022 13:50:52 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
>> On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:
>>> The store status at address order works with 31 bit addresses so
>>> let's
>>> use them.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>   s390x/smp.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/s390x/smp.c b/s390x/smp.c
>>> index 32f128b3..c91f170b 100644
>>> --- a/s390x/smp.c
>>> +++ b/s390x/smp.c
>>
>> [...]
>>
>>> @@ -244,7 +244,7 @@ static void test_func_initial(void)
>>>   
>>>   static void test_reset_initial(void)
>>>   {
>>> -       struct cpu_status *status = alloc_pages(0);
>>> +       struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
>>
>> Why do we need two pages now?
> 
> oh, good catch

Indeed

> 
> the next patch has the same issue
> 
> I can fix them up when I queue them
> 

Probably because I copied that line from somewhere :-)
Yeah, you can fix that up if you want.

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Janosch Frank]

On 1/14/22 14:01, Claudio Imbrenda wrote:
> On Fri, 14 Jan 2022 13:50:52 +0100
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
>> On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:
>>> The store status at address order works with 31 bit addresses so
>>> let's
>>> use them.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>   s390x/smp.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/s390x/smp.c b/s390x/smp.c
>>> index 32f128b3..c91f170b 100644
>>> --- a/s390x/smp.c
>>> +++ b/s390x/smp.c
>>
>> [...]
>>
>>> @@ -244,7 +244,7 @@ static void test_func_initial(void)
>>>   
>>>   static void test_reset_initial(void)
>>>   {
>>> -       struct cpu_status *status = alloc_pages(0);
>>> +       struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
>>
>> Why do we need two pages now?
> 
> actually, wait.....
> 
>          struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
>          uint32_t r;
> 
>          report_prefix_push("store status at address");
>          memset(status, 0, PAGE_SIZE * 2);
> 
> we were allocating one page, and using 2!
> 
> @Janosch do we need 1 or 2 pages?
> 

Have a look at the memcmp() below those lines.

I test if the status page has changed by doing a memcmp against the 
second page.

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Claudio Imbrenda]

On Fri, 14 Jan 2022 14:13:01 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 1/14/22 14:01, Claudio Imbrenda wrote:
> > On Fri, 14 Jan 2022 13:50:52 +0100
> > Nico Boehr <nrb@linux.ibm.com> wrote:
> >   
> >> On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:  
> >>> The store status at address order works with 31 bit addresses so
> >>> let's
> >>> use them.
> >>>
> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>> ---
> >>>   s390x/smp.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/s390x/smp.c b/s390x/smp.c
> >>> index 32f128b3..c91f170b 100644
> >>> --- a/s390x/smp.c
> >>> +++ b/s390x/smp.c  
> >>
> >> [...]
> >>  
> >>> @@ -244,7 +244,7 @@ static void test_func_initial(void)
> >>>   
> >>>   static void test_reset_initial(void)
> >>>   {
> >>> -       struct cpu_status *status = alloc_pages(0);
> >>> +       struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);  
> >>
> >> Why do we need two pages now?  
> > 
> > actually, wait.....
> > 
> >          struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
> >          uint32_t r;
> > 
> >          report_prefix_push("store status at address");
> >          memset(status, 0, PAGE_SIZE * 2);
> > 
> > we were allocating one page, and using 2!
> > 
> > @Janosch do we need 1 or 2 pages?
> >   
> 
> Have a look at the memcmp() below those lines.
> 
> I test if the status page has changed by doing a memcmp against the 
> second page.

so we do need 2 pages, and using 1 was a bug

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Janosch Frank]

On 1/14/22 14:16, Claudio Imbrenda wrote:
> On Fri, 14 Jan 2022 14:13:01 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 1/14/22 14:01, Claudio Imbrenda wrote:
>>> On Fri, 14 Jan 2022 13:50:52 +0100
>>> Nico Boehr <nrb@linux.ibm.com> wrote:
>>>    
>>>> On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:
>>>>> The store status at address order works with 31 bit addresses so
>>>>> let's
>>>>> use them.
>>>>>
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>>    s390x/smp.c | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/s390x/smp.c b/s390x/smp.c
>>>>> index 32f128b3..c91f170b 100644
>>>>> --- a/s390x/smp.c
>>>>> +++ b/s390x/smp.c
>>>>
>>>> [...]
>>>>   
>>>>> @@ -244,7 +244,7 @@ static void test_func_initial(void)
>>>>>    
>>>>>    static void test_reset_initial(void)
>>>>>    {
>>>>> -       struct cpu_status *status = alloc_pages(0);
>>>>> +       struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
>>>>
>>>> Why do we need two pages now?
>>>
>>> actually, wait.....
>>>
>>>           struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
>>>           uint32_t r;
>>>
>>>           report_prefix_push("store status at address");
>>>           memset(status, 0, PAGE_SIZE * 2);
>>>
>>> we were allocating one page, and using 2!
>>>
>>> @Janosch do we need 1 or 2 pages?
>>>    
>>
>> Have a look at the memcmp() below those lines.
>>
>> I test if the status page has changed by doing a memcmp against the
>> second page.
> 
> so we do need 2 pages, and using 1 was a bug
> 

We need two for the store status at address tests but only one for the 
initial reset test Nico is pointing out here.

Re: [kvm-unit-tests PATCH 4/5] s390x: smp: Allocate memory in DMA31 space

[Claudio Imbrenda]

On Fri, 14 Jan 2022 14:13:01 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 1/14/22 14:01, Claudio Imbrenda wrote:
> > On Fri, 14 Jan 2022 13:50:52 +0100
> > Nico Boehr <nrb@linux.ibm.com> wrote:
> >   
> >> On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:  
> >>> The store status at address order works with 31 bit addresses so
> >>> let's
> >>> use them.
> >>>
> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>> ---
> >>>   s390x/smp.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/s390x/smp.c b/s390x/smp.c
> >>> index 32f128b3..c91f170b 100644
> >>> --- a/s390x/smp.c
> >>> +++ b/s390x/smp.c  
> >>
> >> [...]
> >>  
> >>> @@ -244,7 +244,7 @@ static void test_func_initial(void)
> >>>   
> >>>   static void test_reset_initial(void)
> >>>   {
> >>> -       struct cpu_status *status = alloc_pages(0);
> >>> +       struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);  
> >>
> >> Why do we need two pages now?  
> > 
> > actually, wait.....
> > 
> >          struct cpu_status *status = alloc_pages_flags(1, AREA_DMA31);
> >          uint32_t r;
> > 
> >          report_prefix_push("store status at address");
> >          memset(status, 0, PAGE_SIZE * 2);
> > 
> > we were allocating one page, and using 2!
> > 
> > @Janosch do we need 1 or 2 pages?
> >   
> 
> Have a look at the memcmp() below those lines.
> 
> I test if the status page has changed by doing a memcmp against the 
> second page.

ENOCOFEE

sorry I mixed things up

we were using 2, it's the second part of the patch that only needs one

Re: [kvm-unit-tests PATCH 1/5] lib: s390x: vm: Add kvm and lpar vm queries

[Nico Boehr]

On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:
> This patch will likely (in parts) be replaced by Pierre's patch from
> his topology test series.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/vm.c | 39 +++++++++++++++++++++++++++++++++++++++
>  lib/s390x/vm.h | 23 +++++++++++++++++++++++
>  s390x/stsi.c   | 21 +--------------------
>  3 files changed, 63 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
> index a5b92863..266a81c1 100644
> --- a/lib/s390x/vm.c
> +++ b/lib/s390x/vm.c
> @@ -26,6 +26,11 @@ bool vm_is_tcg(void)
>         if (initialized)
>                 return is_tcg;
>  
> +       if (stsi_get_fc() < 3) {
> +               initialized = true;
> +               return false;

Minor nit: By setting initialized to true, you rely on the previous
initialization of is_tcg to false for subsequent calls.

You could make this more obvious by saying:

return is_tcg;

Re: [kvm-unit-tests PATCH 1/5] lib: s390x: vm: Add kvm and lpar vm queries

[Janosch Frank]

On 1/14/22 14:27, Nico Boehr wrote:
> On Fri, 2022-01-14 at 10:02 +0000, Janosch Frank wrote:
>> This patch will likely (in parts) be replaced by Pierre's patch from
>> his topology test series.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/vm.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/vm.h | 23 +++++++++++++++++++++++
>>   s390x/stsi.c   | 21 +--------------------
>>   3 files changed, 63 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/s390x/vm.c b/lib/s390x/vm.c
>> index a5b92863..266a81c1 100644
>> --- a/lib/s390x/vm.c
>> +++ b/lib/s390x/vm.c
>> @@ -26,6 +26,11 @@ bool vm_is_tcg(void)
>>          if (initialized)
>>                  return is_tcg;
>>   
>> +       if (stsi_get_fc() < 3) {
>> +               initialized = true;
>> +               return false;
> 
> Minor nit: By setting initialized to true, you rely on the previous
> initialization of is_tcg to false for subsequent calls.
> 
> You could make this more obvious by saying:
> 
> return is_tcg;
> 

Have a look at Pierre's patch which I will be relying on when it's done. 
As I said in the commit message, this is only a placeholder for his patch.

Re: [kvm-unit-tests PATCH 1/5] lib: s390x: vm: Add kvm and lpar vm queries

[Nico Boehr]

On Fri, 2022-01-14 at 14:35 +0100, Janosch Frank wrote:
> Have a look at Pierre's patch which I will be relying on when it's
> done. 
> As I said in the commit message, this is only a placeholder for his
> patch.

There it is just as I suggested. Thanks.

Re: [kvm-unit-tests PATCH 2/5] s390x: css: Skip if we're not run by qemu

[Thomas Huth]

On 14/01/2022 11.02, Janosch Frank wrote:
> There's no guarantee that we even find a device at the address we're
> testing for if we're not running under QEMU.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/css.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index 881206ba..c24119b4 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -15,6 +15,7 @@
>   #include <interrupt.h>
>   #include <asm/arch_def.h>
>   #include <alloc_page.h>
> +#include <vm.h>
>   
>   #include <malloc_io.h>
>   #include <css.h>
> @@ -350,6 +351,12 @@ int main(int argc, char *argv[])
>   {
>   	int i;
>   
> +	/* There's no guarantee where our devices are without qemu */
> +	if (!vm_is_kvm() && !vm_is_tcg()) {
> +		report_skip("Not running under QEMU");
> +		goto done;
> +	}

You've added the check before the report_prefix_push() ...

>   	report_prefix_push("Channel Subsystem");
>   	enable_io_isc(0x80 >> IO_SCH_ISC);
>   	for (i = 0; tests[i].name; i++) {
> @@ -357,7 +364,8 @@ int main(int argc, char *argv[])
>   		tests[i].func();
>   		report_prefix_pop();
>   	}
> -	report_prefix_pop();
>   
> +done:
> +	report_prefix_pop();

... but in case of the goto you now do the pop without the push. I think you 
have to drop the second hunk.

  Thomas


>   	return report_summary();
>   }

Re: [kvm-unit-tests PATCH 3/5] s390x: diag308: Only test subcode 2 under QEMU

[Thomas Huth]

On 14/01/2022 11.02, Janosch Frank wrote:
> Other hypervisors might implement it and therefore not send a
> specification exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/diag308.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/diag308.c b/s390x/diag308.c
> index c9d6c499..414dbdf4 100644
> --- a/s390x/diag308.c
> +++ b/s390x/diag308.c
> @@ -8,6 +8,7 @@
>   #include <libcflat.h>
>   #include <asm/asm-offsets.h>
>   #include <asm/interrupt.h>
> +#include <vm.h>
>   
>   /* The diagnose calls should be blocked in problem state */
>   static void test_priv(void)
> @@ -75,7 +76,7 @@ static void test_subcode6(void)
>   /* Unsupported subcodes should generate a specification exception */
>   static void test_unsupported_subcode(void)
>   {
> -	int subcodes[] = { 2, 0x101, 0xffff, 0x10001, -1 };
> +	int subcodes[] = { 0x101, 0xffff, 0x10001, -1 };
>   	int idx;
>   
>   	for (idx = 0; idx < ARRAY_SIZE(subcodes); idx++) {
> @@ -85,6 +86,18 @@ static void test_unsupported_subcode(void)
>   		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>   		report_prefix_pop();
>   	}
> +
> +	/*
> +	 * Subcode 2 is not available under QEMU but might be on other
> +	 * hypervisors.
> +	 */
> +	if (vm_is_tcg() || vm_is_kvm()) {
> +		report_prefix_pushf("0x%04x", 2);

Maybe replace the format string with "0002" and drop the "2" parameter?

> +		expect_pgm_int();
> +		asm volatile ("diag %0,%1,0x308" :: "d"(0), "d"(2));
> +		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +		report_prefix_pop();
> +	}
>   }
>   
>   static struct {

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

Re: [kvm-unit-tests PATCH 3/5] s390x: diag308: Only test subcode 2 under QEMU

[Janosch Frank]

On 1/17/22 08:04, Thomas Huth wrote:
> On 14/01/2022 11.02, Janosch Frank wrote:
>> Other hypervisors might implement it and therefore not send a
>> specification exception.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    s390x/diag308.c | 15 ++++++++++++++-
>>    1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/s390x/diag308.c b/s390x/diag308.c
>> index c9d6c499..414dbdf4 100644
>> --- a/s390x/diag308.c
>> +++ b/s390x/diag308.c
>> @@ -8,6 +8,7 @@
>>    #include <libcflat.h>
>>    #include <asm/asm-offsets.h>
>>    #include <asm/interrupt.h>
>> +#include <vm.h>
>>    
>>    /* The diagnose calls should be blocked in problem state */
>>    static void test_priv(void)
>> @@ -75,7 +76,7 @@ static void test_subcode6(void)
>>    /* Unsupported subcodes should generate a specification exception */
>>    static void test_unsupported_subcode(void)
>>    {
>> -	int subcodes[] = { 2, 0x101, 0xffff, 0x10001, -1 };
>> +	int subcodes[] = { 0x101, 0xffff, 0x10001, -1 };
>>    	int idx;
>>    
>>    	for (idx = 0; idx < ARRAY_SIZE(subcodes); idx++) {
>> @@ -85,6 +86,18 @@ static void test_unsupported_subcode(void)
>>    		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>    		report_prefix_pop();
>>    	}
>> +
>> +	/*
>> +	 * Subcode 2 is not available under QEMU but might be on other
>> +	 * hypervisors.
>> +	 */
>> +	if (vm_is_tcg() || vm_is_kvm()) {
>> +		report_prefix_pushf("0x%04x", 2);
> 
> Maybe replace the format string with "0002" and drop the "2" parameter?

Sure

> 
>> +		expect_pgm_int();
>> +		asm volatile ("diag %0,%1,0x308" :: "d"(0), "d"(2));
>> +		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +		report_prefix_pop();
>> +	}
>>    }
>>    
>>    static struct {
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks