[kvm-unit-tests PATCH 0/8] s390x: Cleanup and maintenance 4

[kvm-unit-tests PATCH 0/8] s390x: Cleanup and maintenance 4

[Janosch Frank]

A few small cleanups and two patches that I forgot to upstream which
have now been rebased onto the machine.h library functions.

Janosch Frank (8):
  s390x: css: Skip if we're not run by qemu
  s390x: diag308: Only test subcode 2 under QEMU
  s390x: pfmf: Initialize pfmf_r1 union on declaration
  s390x: snippets: asm: Add license and copyright headers
  s390x: pv-diags: Cleanup includes
  s390x: css: Cleanup includes
  s390x: iep: Cleanup includes
  s390x: mvpg: Cleanup includes

 s390x/css.c                                | 17 ++++++----
 s390x/diag308.c                            | 15 ++++++++-
 s390x/iep.c                                |  3 +-
 s390x/mvpg.c                               |  3 --
 s390x/pfmf.c                               | 39 +++++++++++-----------
 s390x/pv-diags.c                           | 17 ++--------
 s390x/snippets/asm/snippet-pv-diag-288.S   |  9 +++++
 s390x/snippets/asm/snippet-pv-diag-500.S   |  9 +++++
 s390x/snippets/asm/snippet-pv-diag-yield.S |  9 +++++
 9 files changed, 74 insertions(+), 47 deletions(-)

-- 
2.32.0

[kvm-unit-tests PATCH 1/8] 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 a333e55a..52d35f49 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 <hardware.h>
 
 #include <malloc_io.h>
 #include <css.h>
@@ -641,6 +642,12 @@ int main(int argc, char *argv[])
 {
 	int i;
 
+	/* There's no guarantee where our devices are without qemu */
+	if (detect_host() != HOST_IS_KVM && detect_host() != HOST_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++) {
@@ -648,7 +655,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 2/8] 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..9614f9a9 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 <hardware.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 (detect_host() != HOST_IS_TCG && detect_host() != HOST_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 3/8] s390x: pfmf: Initialize pfmf_r1 union on declaration

[Janosch Frank]

Let's make this test look a bit nicer.

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

diff --git a/s390x/pfmf.c b/s390x/pfmf.c
index aa130529..178abb5a 100644
--- a/s390x/pfmf.c
+++ b/s390x/pfmf.c
@@ -28,7 +28,11 @@ static void test_priv(void)
 
 static void test_4k_key(void)
 {
-	union pfmf_r1 r1;
+	union pfmf_r1 r1 = {
+		.reg.sk = 1,
+		.reg.fsc = PFMF_FSC_4K,
+		.reg.key = 0x30,
+	};
 	union skey skey;
 
 	report_prefix_push("4K");
@@ -36,10 +40,6 @@ static void test_4k_key(void)
 		report_skip("storage key removal facility is active");
 		goto out;
 	}
-	r1.val = 0;
-	r1.reg.sk = 1;
-	r1.reg.fsc = PFMF_FSC_4K;
-	r1.reg.key = 0x30;
 	pfmf(r1.val, pagebuf);
 	skey.val = get_storage_key(pagebuf);
 	skey.val &= SKEY_ACC | SKEY_FP;
@@ -52,18 +52,19 @@ static void test_1m_key(void)
 {
 	int i;
 	bool rp = true;
-	union pfmf_r1 r1;
 	union skey skey;
+	union pfmf_r1 r1 = {
+		.reg.fsc = PFMF_FSC_1M,
+		.reg.key = 0x30,
+		.reg.sk = 1,
+	};
 
 	report_prefix_push("1M");
 	if (test_facility(169)) {
 		report_skip("storage key removal facility is active");
 		goto out;
 	}
-	r1.val = 0;
-	r1.reg.sk = 1;
-	r1.reg.fsc = PFMF_FSC_1M;
-	r1.reg.key = 0x30;
+
 	pfmf(r1.val, pagebuf);
 	for (i = 0; i < 256; i++) {
 		skey.val = get_storage_key(pagebuf + i * PAGE_SIZE);
@@ -80,11 +81,10 @@ out:
 
 static void test_4k_clear(void)
 {
-	union pfmf_r1 r1;
-
-	r1.val = 0;
-	r1.reg.cf = 1;
-	r1.reg.fsc = PFMF_FSC_4K;
+	union pfmf_r1 r1 = {
+		.reg.cf = 1,
+		.reg.fsc = PFMF_FSC_4K,
+	};
 
 	report_prefix_push("4K");
 	memset(pagebuf, 42, PAGE_SIZE);
@@ -97,13 +97,12 @@ static void test_4k_clear(void)
 static void test_1m_clear(void)
 {
 	int i;
-	union pfmf_r1 r1;
+	union pfmf_r1 r1 = {
+		.reg.cf = 1,
+		.reg.fsc = PFMF_FSC_1M,
+	};
 	unsigned long sum = 0;
 
-	r1.val = 0;
-	r1.reg.cf = 1;
-	r1.reg.fsc = PFMF_FSC_1M;
-
 	report_prefix_push("1M");
 	memset(pagebuf, 42, PAGE_SIZE * 256);
 	pfmf(r1.val, pagebuf);
-- 
2.32.0

[kvm-unit-tests PATCH 4/8] s390x: snippets: asm: Add license and copyright headers

[Janosch Frank]

Time for some cleanup of the snippets to make them look like any other
test file.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/snippets/asm/snippet-pv-diag-288.S   | 9 +++++++++
 s390x/snippets/asm/snippet-pv-diag-500.S   | 9 +++++++++
 s390x/snippets/asm/snippet-pv-diag-yield.S | 9 +++++++++
 3 files changed, 27 insertions(+)

diff --git a/s390x/snippets/asm/snippet-pv-diag-288.S b/s390x/snippets/asm/snippet-pv-diag-288.S
index e3e63121..aaee3cd1 100644
--- a/s390x/snippets/asm/snippet-pv-diag-288.S
+++ b/s390x/snippets/asm/snippet-pv-diag-288.S
@@ -1,3 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Diagnose 0x288 snippet used for PV interception testing.
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Janosch Frank <frankja@linux.ibm.com>
+ */
 #include <asm/asm-offsets.h>
 .section .text
 
diff --git a/s390x/snippets/asm/snippet-pv-diag-500.S b/s390x/snippets/asm/snippet-pv-diag-500.S
index 50c06779..8dd66bd9 100644
--- a/s390x/snippets/asm/snippet-pv-diag-500.S
+++ b/s390x/snippets/asm/snippet-pv-diag-500.S
@@ -1,3 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Diagnose 0x500 snippet used for PV interception tests
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Janosch Frank <frankja@linux.ibm.com>
+ */
 #include <asm/asm-offsets.h>
 .section .text
 
diff --git a/s390x/snippets/asm/snippet-pv-diag-yield.S b/s390x/snippets/asm/snippet-pv-diag-yield.S
index 5795cf0f..78a5b07a 100644
--- a/s390x/snippets/asm/snippet-pv-diag-yield.S
+++ b/s390x/snippets/asm/snippet-pv-diag-yield.S
@@ -1,3 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Diagnose 0x44 and 0x9c snippet used for PV interception tests
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Janosch Frank <frankja@linux.ibm.com>
+ */
 .section .text
 
 xgr	%r0, %r0
-- 
2.32.0

[kvm-unit-tests PATCH 5/8] s390x: pv-diags: Cleanup includes

[Janosch Frank]

This file has way too much includes. Time to remove some.

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

diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
index 6899b859..9ced68c7 100644
--- a/s390x/pv-diags.c
+++ b/s390x/pv-diags.c
@@ -8,23 +8,10 @@
  *  Janosch Frank <frankja@linux.ibm.com>
  */
 #include <libcflat.h>
-#include <asm/asm-offsets.h>
-#include <asm-generic/barrier.h>
-#include <asm/interrupt.h>
-#include <asm/pgtable.h>
-#include <mmu.h>
-#include <asm/page.h>
-#include <asm/facility.h>
-#include <asm/mem.h>
-#include <asm/sigp.h>
-#include <smp.h>
-#include <alloc_page.h>
-#include <vmalloc.h>
-#include <sclp.h>
 #include <snippet.h>
 #include <sie.h>
-#include <uv.h>
-#include <asm/uv.h>
+#include <sclp.h>
+#include <asm/facility.h>
 
 static struct vm vm;
 
-- 
2.32.0

[kvm-unit-tests PATCH 6/8] s390x: css: Cleanup includes

[Janosch Frank]

Most includes were related to allocation but that's done in the io
allocation library so having them in the test doesn't make sense.

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

diff --git a/s390x/css.c b/s390x/css.c
index 52d35f49..9cfcfad4 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -9,17 +9,14 @@
  */
 
 #include <libcflat.h>
-#include <alloc_phys.h>
-#include <asm/page.h>
-#include <string.h>
 #include <interrupt.h>
-#include <asm/arch_def.h>
-#include <alloc_page.h>
 #include <hardware.h>
 
+#include <asm/arch_def.h>
+#include <asm/page.h>
+
 #include <malloc_io.h>
 #include <css.h>
-#include <asm/barrier.h>
 
 #define DEFAULT_CU_TYPE		0x3832 /* virtio-ccw */
 static unsigned long cu_type = DEFAULT_CU_TYPE;
-- 
2.32.0

[kvm-unit-tests PATCH 7/8] s390x: iep: Cleanup includes

[Janosch Frank]

We don't use barriers so let's remove the include.

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

diff --git a/s390x/iep.c b/s390x/iep.c
index 8d5e044b..4b3e09a7 100644
--- a/s390x/iep.c
+++ b/s390x/iep.c
@@ -9,11 +9,10 @@
  */
 #include <libcflat.h>
 #include <vmalloc.h>
+#include <mmu.h>
 #include <asm/facility.h>
 #include <asm/interrupt.h>
-#include <mmu.h>
 #include <asm/pgtable.h>
-#include <asm-generic/barrier.h>
 
 static void test_iep(void)
 {
-- 
2.32.0

[kvm-unit-tests PATCH 8/8] s390x: mvpg: Cleanup includes

[Janosch Frank]

Time to remove unneeded includes.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/mvpg.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/s390x/mvpg.c b/s390x/mvpg.c
index 62f0fc5a..04e5218f 100644
--- a/s390x/mvpg.c
+++ b/s390x/mvpg.c
@@ -9,15 +9,12 @@
  */
 #include <libcflat.h>
 #include <asm/asm-offsets.h>
-#include <asm-generic/barrier.h>
 #include <asm/interrupt.h>
 #include <asm/pgtable.h>
 #include <mmu.h>
 #include <asm/page.h>
 #include <asm/facility.h>
 #include <asm/mem.h>
-#include <asm/sigp.h>
-#include <smp.h>
 #include <alloc_page.h>
 #include <bitops.h>
 #include <hardware.h>
-- 
2.32.0

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

[Thomas Huth]

On 05/04/2022 09.52, 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 a333e55a..52d35f49 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 <hardware.h>
>   
>   #include <malloc_io.h>
>   #include <css.h>
> @@ -641,6 +642,12 @@ int main(int argc, char *argv[])
>   {
>   	int i;
>   
> +	/* There's no guarantee where our devices are without qemu */
> +	if (detect_host() != HOST_IS_KVM && detect_host() != HOST_IS_TCG) {
> +		report_skip("Not running under QEMU");
> +		goto done;
> +	}
> +
>   	report_prefix_push("Channel Subsystem");

Prefix gets pushed after the if-statement (where you jump to "done" via 
goto) ...

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

... but here it gets popped in the case of "goto done", too.
==> Please either move the if-statement or the "done" label.

  Thomas


>   	return report_summary();
>   }

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

[Thomas Huth]

On 05/04/2022 09.52, 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..9614f9a9 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 <hardware.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 (detect_host() != HOST_IS_TCG && detect_host() != HOST_IS_KVM) {

Shouldn't this be rather the other way round instead?

	if (detect_host() == HOST_IS_TCG || detect_host() == HOST_IS_KVM)

?

... anyway, since you already used a similar if-clause in your first patch, 
it might make sense to add a helper function a la host_is_qemu() to check 
whether we're running on QEMU or not.

> +		report_prefix_pushf("0x%04x", 2);

	report_prefix_pushf("0x02") ?

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

  Thomas

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

[Janosch Frank]

On 4/5/22 11:18, Thomas Huth wrote:
> On 05/04/2022 09.52, 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..9614f9a9 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 <hardware.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 (detect_host() != HOST_IS_TCG && detect_host() != HOST_IS_KVM) {
> 
> Shouldn't this be rather the other way round instead?
> 
> 	if (detect_host() == HOST_IS_TCG || detect_host() == HOST_IS_KVM)
> 
> ?

The css if checks if we are under QEMU, this one checks if we're not 
under QEMU.

> 
> ... anyway, since you already used a similar if-clause in your first patch,
> it might make sense to add a helper function a la host_is_qemu() to check
> whether we're running on QEMU or not.

Will do

> 
>> +		report_prefix_pushf("0x%04x", 2);
> 
> 	report_prefix_pushf("0x02") ?
> 
>> +		expect_pgm_int();
>> +		asm volatile ("diag %0,%1,0x308" :: "d"(0), "d"(2));
>> +		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +		report_prefix_pop();
>> +	}
>>    }
> 
>    Thomas
> 

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

[Thomas Huth]

On 05/04/2022 11.33, Janosch Frank wrote:
> On 4/5/22 11:18, Thomas Huth wrote:
>> On 05/04/2022 09.52, 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..9614f9a9 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 <hardware.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 (detect_host() != HOST_IS_TCG && detect_host() != HOST_IS_KVM) {
>>
>> Shouldn't this be rather the other way round instead?
>>
>>     if (detect_host() == HOST_IS_TCG || detect_host() == HOST_IS_KVM)
>>
>> ?
> 
> The css if checks if we are under QEMU, this one checks if we're not under 
> QEMU.

but ...

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

... don't we want to check here whether the diag causes a spec exception if 
we are *under* QEMU here?
/me feels confused now.

  Thomas

Re: [kvm-unit-tests PATCH 3/8] s390x: pfmf: Initialize pfmf_r1 union on declaration

[Thomas Huth]

On 05/04/2022 09.52, Janosch Frank wrote:
> Let's make this test look a bit nicer.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/pfmf.c | 39 +++++++++++++++++++--------------------
>   1 file changed, 19 insertions(+), 20 deletions(-)

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

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

[Janosch Frank]

On 4/5/22 11:50, Thomas Huth wrote:
> On 05/04/2022 11.33, Janosch Frank wrote:
>> On 4/5/22 11:18, Thomas Huth wrote:
>>> On 05/04/2022 09.52, 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..9614f9a9 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 <hardware.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 (detect_host() != HOST_IS_TCG && detect_host() != HOST_IS_KVM) {
>>>
>>> Shouldn't this be rather the other way round instead?
>>>
>>>      if (detect_host() == HOST_IS_TCG || detect_host() == HOST_IS_KVM)
>>>
>>> ?
>>
>> The css if checks if we are under QEMU, this one checks if we're not under
>> QEMU.
> 
> but ...
> 
>>>
>>>> +        expect_pgm_int();
>>>> +        asm volatile ("diag %0,%1,0x308" :: "d"(0), "d"(2));
>>>> +        check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> 
> ... don't we want to check here whether the diag causes a spec exception if
> we are *under* QEMU here?
> /me feels confused now.

Seems like it's me who's confused today
Anyway, I'll clean that up.

> 
>    Thomas
> 

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

[Claudio Imbrenda]

On Tue,  5 Apr 2022 07:52:18 +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>
> ---
>  s390x/css.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index a333e55a..52d35f49 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 <hardware.h>
>  
>  #include <malloc_io.h>
>  #include <css.h>
> @@ -641,6 +642,12 @@ int main(int argc, char *argv[])
>  {
>  	int i;
>  
> +	/* There's no guarantee where our devices are without qemu */
> +	if (detect_host() != HOST_IS_KVM && detect_host() != HOST_IS_TCG) {

you could also do !host_is_kvm() && !host_is_tcg() , I think it's more
readable, but I do not have strong opinions regarding that

> +		report_skip("Not running under QEMU");
> +		goto done;
> +	}
> +
>  	report_prefix_push("Channel Subsystem");

the prefix push should probably go before the if

>  	enable_io_isc(0x80 >> IO_SCH_ISC);
>  	for (i = 0; tests[i].name; i++) {
> @@ -648,7 +655,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 3/8] s390x: pfmf: Initialize pfmf_r1 union on declaration

[Claudio Imbrenda]

On Tue,  5 Apr 2022 07:52:20 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's make this test look a bit nicer.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/pfmf.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/s390x/pfmf.c b/s390x/pfmf.c
> index aa130529..178abb5a 100644
> --- a/s390x/pfmf.c
> +++ b/s390x/pfmf.c
> @@ -28,7 +28,11 @@ static void test_priv(void)
>  
>  static void test_4k_key(void)
>  {
> -	union pfmf_r1 r1;
> +	union pfmf_r1 r1 = {
> +		.reg.sk = 1,
> +		.reg.fsc = PFMF_FSC_4K,
> +		.reg.key = 0x30,
> +	};
>  	union skey skey;
>  
>  	report_prefix_push("4K");
> @@ -36,10 +40,6 @@ static void test_4k_key(void)
>  		report_skip("storage key removal facility is active");
>  		goto out;
>  	}
> -	r1.val = 0;
> -	r1.reg.sk = 1;
> -	r1.reg.fsc = PFMF_FSC_4K;
> -	r1.reg.key = 0x30;
>  	pfmf(r1.val, pagebuf);
>  	skey.val = get_storage_key(pagebuf);
>  	skey.val &= SKEY_ACC | SKEY_FP;
> @@ -52,18 +52,19 @@ static void test_1m_key(void)
>  {
>  	int i;
>  	bool rp = true;
> -	union pfmf_r1 r1;
>  	union skey skey;
> +	union pfmf_r1 r1 = {
> +		.reg.fsc = PFMF_FSC_1M,
> +		.reg.key = 0x30,
> +		.reg.sk = 1,
> +	};
>  
>  	report_prefix_push("1M");
>  	if (test_facility(169)) {
>  		report_skip("storage key removal facility is active");
>  		goto out;
>  	}
> -	r1.val = 0;
> -	r1.reg.sk = 1;
> -	r1.reg.fsc = PFMF_FSC_1M;
> -	r1.reg.key = 0x30;
> +
>  	pfmf(r1.val, pagebuf);
>  	for (i = 0; i < 256; i++) {
>  		skey.val = get_storage_key(pagebuf + i * PAGE_SIZE);
> @@ -80,11 +81,10 @@ out:
>  
>  static void test_4k_clear(void)
>  {
> -	union pfmf_r1 r1;
> -
> -	r1.val = 0;
> -	r1.reg.cf = 1;
> -	r1.reg.fsc = PFMF_FSC_4K;
> +	union pfmf_r1 r1 = {
> +		.reg.cf = 1,
> +		.reg.fsc = PFMF_FSC_4K,
> +	};
>  
>  	report_prefix_push("4K");
>  	memset(pagebuf, 42, PAGE_SIZE);
> @@ -97,13 +97,12 @@ static void test_4k_clear(void)
>  static void test_1m_clear(void)
>  {
>  	int i;
> -	union pfmf_r1 r1;
> +	union pfmf_r1 r1 = {
> +		.reg.cf = 1,
> +		.reg.fsc = PFMF_FSC_1M,
> +	};
>  	unsigned long sum = 0;
>  
> -	r1.val = 0;
> -	r1.reg.cf = 1;
> -	r1.reg.fsc = PFMF_FSC_1M;
> -
>  	report_prefix_push("1M");
>  	memset(pagebuf, 42, PAGE_SIZE * 256);
>  	pfmf(r1.val, pagebuf);

Re: [kvm-unit-tests PATCH 4/8] s390x: snippets: asm: Add license and copyright headers

[Claudio Imbrenda]

On Tue,  5 Apr 2022 07:52:21 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Time for some cleanup of the snippets to make them look like any other
> test file.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/snippets/asm/snippet-pv-diag-288.S   | 9 +++++++++
>  s390x/snippets/asm/snippet-pv-diag-500.S   | 9 +++++++++
>  s390x/snippets/asm/snippet-pv-diag-yield.S | 9 +++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/s390x/snippets/asm/snippet-pv-diag-288.S b/s390x/snippets/asm/snippet-pv-diag-288.S
> index e3e63121..aaee3cd1 100644
> --- a/s390x/snippets/asm/snippet-pv-diag-288.S
> +++ b/s390x/snippets/asm/snippet-pv-diag-288.S
> @@ -1,3 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Diagnose 0x288 snippet used for PV interception testing.
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Janosch Frank <frankja@linux.ibm.com>
> + */
>  #include <asm/asm-offsets.h>
>  .section .text
>  
> diff --git a/s390x/snippets/asm/snippet-pv-diag-500.S b/s390x/snippets/asm/snippet-pv-diag-500.S
> index 50c06779..8dd66bd9 100644
> --- a/s390x/snippets/asm/snippet-pv-diag-500.S
> +++ b/s390x/snippets/asm/snippet-pv-diag-500.S
> @@ -1,3 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Diagnose 0x500 snippet used for PV interception tests
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Janosch Frank <frankja@linux.ibm.com>
> + */
>  #include <asm/asm-offsets.h>
>  .section .text
>  
> diff --git a/s390x/snippets/asm/snippet-pv-diag-yield.S b/s390x/snippets/asm/snippet-pv-diag-yield.S
> index 5795cf0f..78a5b07a 100644
> --- a/s390x/snippets/asm/snippet-pv-diag-yield.S
> +++ b/s390x/snippets/asm/snippet-pv-diag-yield.S
> @@ -1,3 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Diagnose 0x44 and 0x9c snippet used for PV interception tests
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Janosch Frank <frankja@linux.ibm.com>
> + */
>  .section .text
>  
>  xgr	%r0, %r0

Re: [kvm-unit-tests PATCH 5/8] s390x: pv-diags: Cleanup includes

[Claudio Imbrenda]

On Tue,  5 Apr 2022 07:52:22 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> This file has way too much includes. Time to remove some.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/pv-diags.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
> index 6899b859..9ced68c7 100644
> --- a/s390x/pv-diags.c
> +++ b/s390x/pv-diags.c
> @@ -8,23 +8,10 @@
>   *  Janosch Frank <frankja@linux.ibm.com>
>   */
>  #include <libcflat.h>
> -#include <asm/asm-offsets.h>
> -#include <asm-generic/barrier.h>
> -#include <asm/interrupt.h>
> -#include <asm/pgtable.h>
> -#include <mmu.h>
> -#include <asm/page.h>
> -#include <asm/facility.h>
> -#include <asm/mem.h>
> -#include <asm/sigp.h>
> -#include <smp.h>
> -#include <alloc_page.h>
> -#include <vmalloc.h>
> -#include <sclp.h>
>  #include <snippet.h>
>  #include <sie.h>
> -#include <uv.h>
> -#include <asm/uv.h>
> +#include <sclp.h>
> +#include <asm/facility.h>
>  
>  static struct vm vm;
>  

Re: [kvm-unit-tests PATCH 6/8] s390x: css: Cleanup includes

[Claudio Imbrenda]

On Tue,  5 Apr 2022 07:52:23 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Most includes were related to allocation but that's done in the io
> allocation library so having them in the test doesn't make sense.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/css.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/s390x/css.c b/s390x/css.c
> index 52d35f49..9cfcfad4 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -9,17 +9,14 @@
>   */
>  
>  #include <libcflat.h>
> -#include <alloc_phys.h>
> -#include <asm/page.h>
> -#include <string.h>
>  #include <interrupt.h>
> -#include <asm/arch_def.h>
> -#include <alloc_page.h>
>  #include <hardware.h>
>  
> +#include <asm/arch_def.h>
> +#include <asm/page.h>
> +
>  #include <malloc_io.h>
>  #include <css.h>
> -#include <asm/barrier.h>
>  
>  #define DEFAULT_CU_TYPE		0x3832 /* virtio-ccw */
>  static unsigned long cu_type = DEFAULT_CU_TYPE;

Re: [kvm-unit-tests PATCH 7/8] s390x: iep: Cleanup includes

[Claudio Imbrenda]

On Tue,  5 Apr 2022 07:52:24 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> We don't use barriers so let's remove the include.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/iep.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/s390x/iep.c b/s390x/iep.c
> index 8d5e044b..4b3e09a7 100644
> --- a/s390x/iep.c
> +++ b/s390x/iep.c
> @@ -9,11 +9,10 @@
>   */
>  #include <libcflat.h>
>  #include <vmalloc.h>
> +#include <mmu.h>
>  #include <asm/facility.h>
>  #include <asm/interrupt.h>
> -#include <mmu.h>
>  #include <asm/pgtable.h>
> -#include <asm-generic/barrier.h>
>  
>  static void test_iep(void)
>  {

Re: [kvm-unit-tests PATCH 8/8] s390x: mvpg: Cleanup includes

[Claudio Imbrenda]

On Tue,  5 Apr 2022 07:52:25 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Time to remove unneeded includes.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/mvpg.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/s390x/mvpg.c b/s390x/mvpg.c
> index 62f0fc5a..04e5218f 100644
> --- a/s390x/mvpg.c
> +++ b/s390x/mvpg.c
> @@ -9,15 +9,12 @@
>   */
>  #include <libcflat.h>
>  #include <asm/asm-offsets.h>
> -#include <asm-generic/barrier.h>
>  #include <asm/interrupt.h>
>  #include <asm/pgtable.h>
>  #include <mmu.h>
>  #include <asm/page.h>
>  #include <asm/facility.h>
>  #include <asm/mem.h>
> -#include <asm/sigp.h>
> -#include <smp.h>
>  #include <alloc_page.h>
>  #include <bitops.h>
>  #include <hardware.h>

Re: [kvm-unit-tests PATCH 5/8] s390x: pv-diags: Cleanup includes

[Thomas Huth]

On 05/04/2022 09.52, Janosch Frank wrote:
> This file has way too much includes. Time to remove some.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/pv-diags.c | 17 ++---------------
>   1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
> index 6899b859..9ced68c7 100644
> --- a/s390x/pv-diags.c
> +++ b/s390x/pv-diags.c
> @@ -8,23 +8,10 @@
>    *  Janosch Frank <frankja@linux.ibm.com>
>    */
>   #include <libcflat.h>
> -#include <asm/asm-offsets.h>
> -#include <asm-generic/barrier.h>
> -#include <asm/interrupt.h>
> -#include <asm/pgtable.h>
> -#include <mmu.h>
> -#include <asm/page.h>
> -#include <asm/facility.h>
> -#include <asm/mem.h>
> -#include <asm/sigp.h>
> -#include <smp.h>
> -#include <alloc_page.h>
> -#include <vmalloc.h>
> -#include <sclp.h>
>   #include <snippet.h>
>   #include <sie.h>
> -#include <uv.h>
> -#include <asm/uv.h>
> +#include <sclp.h>
> +#include <asm/facility.h>

Wow, how did we end up with that huge list? Copy-n-paste from other files?

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

Re: [kvm-unit-tests PATCH 5/8] s390x: pv-diags: Cleanup includes

[Janosch Frank]

On 4/6/22 08:50, Thomas Huth wrote:
> On 05/04/2022 09.52, Janosch Frank wrote:
>> This file has way too much includes. Time to remove some.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    s390x/pv-diags.c | 17 ++---------------
>>    1 file changed, 2 insertions(+), 15 deletions(-)
>>
>> diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
>> index 6899b859..9ced68c7 100644
>> --- a/s390x/pv-diags.c
>> +++ b/s390x/pv-diags.c
>> @@ -8,23 +8,10 @@
>>     *  Janosch Frank <frankja@linux.ibm.com>
>>     */
>>    #include <libcflat.h>
>> -#include <asm/asm-offsets.h>
>> -#include <asm-generic/barrier.h>
>> -#include <asm/interrupt.h>
>> -#include <asm/pgtable.h>
>> -#include <mmu.h>
>> -#include <asm/page.h>
>> -#include <asm/facility.h>
>> -#include <asm/mem.h>
>> -#include <asm/sigp.h>
>> -#include <smp.h>
>> -#include <alloc_page.h>
>> -#include <vmalloc.h>
>> -#include <sclp.h>
>>    #include <snippet.h>
>>    #include <sie.h>
>> -#include <uv.h>
>> -#include <asm/uv.h>
>> +#include <sclp.h>
>> +#include <asm/facility.h>
> 
> Wow, how did we end up with that huge list? Copy-n-paste from other files?
> 

Yes, and lots of work to get it working in the first place which pulled 
in headers which weren't removed later once they were unused.

That's one of the reasons why I suggested the templates to you :-)

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

Thanks