[kvm-unit-tests PATCH v3 0/9] s390x: uv-host: Fixups and extensions part 1

[kvm-unit-tests PATCH v3 0/9] s390x: uv-host: Fixups and extensions part 1

[Janosch Frank]

The uv-host test has a lot of historical growth problems which have
largely been overlooked since running it is harder than running a KVM
(guest 2) based test.

This series fixes up smaller problems but still leaves the test with
fails when running create config base and variable storage
tests. Those problems will either be fixed up with the second series
or with a firmware fix since I'm unsure on which side of the os/fw
fence the problem exists.

The series is based on my other series that introduces pv-ipl and
pv-icpt. The memory allocation fix will be added to the new version of
that series so all G1 tests are fixed.

v3:
	- Re-based on the ipl/icpt series
	- Added review-bys

v2:
	- Added patch that exchanges sigp_retry with the smp variant
	- Re-worked the create config test handling
	- Minor fixups

Janosch Frank (9):
  s390x: uv-host: Fix UV init test memory allocation
  s390x: uv-host: Check for sufficient amount of memory
  s390x: uv-host: Beautify code
  s390x: uv-host: Add cpu number check
  s390x: uv-host: Fix create guest variable storage prefix check
  s390x: uv-host: Switch to smp_sigp
  s390x: uv-host: Properly handle config creation errors
  s390x: uv-host: Fence access checks when UV debug is enabled
  s390x: uv-host: Add the test to unittests.conf

 lib/s390x/asm/uv.h  |   1 +
 s390x/unittests.cfg |   5 ++
 s390x/uv-host.c     | 137 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 114 insertions(+), 29 deletions(-)

-- 
2.34.1

[kvm-unit-tests PATCH v3 1/9] s390x: uv-host: Fix UV init test memory allocation

[Janosch Frank]

The init memory has to be above 2G and 1M aligned but we're currently
aligning on 2G which means the allocations need a lot of unused
memory.

Also the second block of memory was never actually used for the double
init test since its address is never put into the uvcb.

Let's fix that.

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

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 33e6eec6..9dfaebd7 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -500,14 +500,17 @@ static void test_config_create(void)
 static void test_init(void)
 {
 	int rc;
-	uint64_t mem;
+	uint64_t tmp;
 
-	/* Donated storage needs to be over 2GB */
-	mem = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);
+	/*
+	 * Donated storage needs to be over 2GB, AREA_NORMAL does that
+	 * on s390x.
+	 */
+	tmp = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);
 
 	uvcb_init.header.len = sizeof(uvcb_init);
 	uvcb_init.header.cmd = UVC_CMD_INIT_UV;
-	uvcb_init.stor_origin = mem;
+	uvcb_init.stor_origin = tmp;
 	uvcb_init.stor_len = uvcb_qui.uv_base_stor_len;
 
 	report_prefix_push("init");
@@ -528,14 +531,14 @@ static void test_init(void)
 	rc = uv_call(0, (uint64_t)&uvcb_init);
 	report(rc == 1 && (uvcb_init.header.rc == 0x104 || uvcb_init.header.rc == 0x105),
 	       "storage origin invalid");
-	uvcb_init.stor_origin = mem;
+	uvcb_init.stor_origin = tmp;
 
 	if (uvcb_init.stor_len >= HPAGE_SIZE) {
 		uvcb_init.stor_origin = get_max_ram_size() - HPAGE_SIZE;
 		rc = uv_call(0, (uint64_t)&uvcb_init);
 		report(rc == 1 && uvcb_init.header.rc == 0x105,
 		       "storage + length invalid");
-		uvcb_init.stor_origin = mem;
+		uvcb_init.stor_origin = tmp;
 	} else {
 		report_skip("storage + length invalid, stor_len < HPAGE_SIZE");
 	}
@@ -544,7 +547,7 @@ static void test_init(void)
 	rc = uv_call(0, (uint64_t)&uvcb_init);
 	report(rc == 1 && uvcb_init.header.rc == 0x108,
 	       "storage below 2GB");
-	uvcb_init.stor_origin = mem;
+	uvcb_init.stor_origin = tmp;
 
 	smp_cpu_setup(1, PSW_WITH_CUR_MASK(cpu_loop));
 	rc = uv_call(0, (uint64_t)&uvcb_init);
@@ -555,10 +558,12 @@ static void test_init(void)
 	rc = uv_call(0, (uint64_t)&uvcb_init);
 	report(rc == 0 && uvcb_init.header.rc == UVC_RC_EXECUTED, "successful");
 
-	mem = (uint64_t)memalign(1UL << 31, uvcb_qui.uv_base_stor_len);
+	tmp = uvcb_init.stor_origin;
+	uvcb_init.stor_origin =	(uint64_t)memalign_pages_flags(HPAGE_SIZE, uvcb_qui.uv_base_stor_len, AREA_NORMAL);
 	rc = uv_call(0, (uint64_t)&uvcb_init);
 	report(rc == 1 && uvcb_init.header.rc == 0x101, "double init");
-	free((void *)mem);
+	free((void *)uvcb_init.stor_origin);
+	uvcb_init.stor_origin = tmp;
 
 	report_prefix_pop();
 }
-- 
2.34.1

Re: [kvm-unit-tests PATCH v3 1/9] s390x: uv-host: Fix UV init test memory allocation

[Nico Boehr]

Quoting Janosch Frank (2023-05-02 15:07:24)
> The init memory has to be above 2G and 1M aligned but we're currently
> aligning on 2G which means the allocations need a lot of unused
> memory.

I know I already gave my R-b here, but...

> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 33e6eec6..9dfaebd7 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -500,14 +500,17 @@ static void test_config_create(void)
>  static void test_init(void)
>  {
>         int rc;
> -       uint64_t mem;
> +       uint64_t tmp;
>  
> -       /* Donated storage needs to be over 2GB */
> -       mem = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);

...maybe out of coffee, but can you point me to the place where we're aligning
to 2G here? I only see alignment to 1M and your change only seems to rename
mem to tmp:

> +       /*
> +        * Donated storage needs to be over 2GB, AREA_NORMAL does that
> +        * on s390x.
> +        */
> +       tmp = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);

Re: [kvm-unit-tests PATCH v3 1/9] s390x: uv-host: Fix UV init test memory allocation

[Janosch Frank]

On 5/10/23 10:51, Nico Boehr wrote:
> Quoting Janosch Frank (2023-05-02 15:07:24)
>> The init memory has to be above 2G and 1M aligned but we're currently
>> aligning on 2G which means the allocations need a lot of unused
>> memory.
> 
> I know I already gave my R-b here, but...
> 
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index 33e6eec6..9dfaebd7 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -500,14 +500,17 @@ static void test_config_create(void)
>>   static void test_init(void)
>>   {
>>          int rc;
>> -       uint64_t mem;
>> +       uint64_t tmp;
>>   
>> -       /* Donated storage needs to be over 2GB */
>> -       mem = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);
> 
> ...maybe out of coffee, but can you point me to the place where we're aligning
> to 2G here? I only see alignment to 1M and your change only seems to rename
> mem to tmp:
> 
>> +       /*
>> +        * Donated storage needs to be over 2GB, AREA_NORMAL does that
>> +        * on s390x.
>> +        */

This comment explains it :)
Its a re-name of mem to tmp and an extension of this comment so it makes 
more sense.

>> +       tmp = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);

Re: [kvm-unit-tests PATCH v3 1/9] s390x: uv-host: Fix UV init test memory allocation

[Nico Boehr]

Quoting Janosch Frank (2023-05-10 11:03:54)
> On 5/10/23 10:51, Nico Boehr wrote:
> > Quoting Janosch Frank (2023-05-02 15:07:24)
> >> The init memory has to be above 2G and 1M aligned but we're currently
> >> aligning on 2G which means the allocations need a lot of unused
> >> memory.
> > 
> > I know I already gave my R-b here, but...
> > 
> >> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> >> index 33e6eec6..9dfaebd7 100644
> >> --- a/s390x/uv-host.c
> >> +++ b/s390x/uv-host.c
> >> @@ -500,14 +500,17 @@ static void test_config_create(void)
> >>   static void test_init(void)
> >>   {
> >>          int rc;
> >> -       uint64_t mem;
> >> +       uint64_t tmp;
> >>   
> >> -       /* Donated storage needs to be over 2GB */
> >> -       mem = (uint64_t)memalign_pages_flags(SZ_1M, uvcb_qui.uv_base_stor_len, AREA_NORMAL);
> > 
> > ...maybe out of coffee, but can you point me to the place where we're aligning
> > to 2G here? I only see alignment to 1M and your change only seems to rename
> > mem to tmp:
> > 
> >> +       /*
> >> +        * Donated storage needs to be over 2GB, AREA_NORMAL does that
> >> +        * on s390x.
> >> +        */
> 
> This comment explains it :)
> Its a re-name of mem to tmp and an extension of this comment so it makes 
> more sense.

Alright, thanks. I guess coffee level too low.

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

[kvm-unit-tests PATCH v3 2/9] s390x: uv-host: Check for sufficient amount of memory

[Janosch Frank]

The UV init storage needs to be above 2G so we need a little over 2G
of memory when running the test.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/uv-host.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 9dfaebd7..34ca7148 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -15,6 +15,7 @@
 #include <sclp.h>
 #include <smp.h>
 #include <uv.h>
+#include <snippet.h>
 #include <mmu.h>
 #include <asm/page.h>
 #include <asm/sigp.h>
@@ -720,6 +721,13 @@ int main(void)
 	test_invalid();
 	test_uv_uninitialized();
 	test_query();
+
+	if (get_ram_size() < SNIPPET_PV_MIN_MEM_SIZE) {
+		report_skip("Not enough memory. This test needs about %ld MB of memory",
+			    SNIPPET_PV_MIN_MEM_SIZE / 1024 / 1024);
+		goto done;
+	}
+
 	test_init();
 
 	setup_vmem();
-- 
2.34.1

[kvm-unit-tests PATCH v3 3/9] s390x: uv-host: Beautify code

[Janosch Frank]

Fixup top comment and add missing space.

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

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 34ca7148..1c04d8ef 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
- * Guest Ultravisor Call tests
+ * Host Ultravisor Call tests
  *
  * Copyright (c) 2021 IBM Corp
  *
@@ -34,7 +34,7 @@ static struct uv_cb_csc uvcb_csc;
 
 extern int diag308_load_reset(u64 code);
 
-struct cmd_list{
+struct cmd_list {
 	const char *name;
 	uint16_t cmd;
 	uint16_t len;
-- 
2.34.1

[kvm-unit-tests PATCH v3 4/9] s390x: uv-host: Add cpu number check

[Janosch Frank]

We should only run a test that needs more than one cpu if a sufficient
number of cpus are available.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/uv-host.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 1c04d8ef..ed13bc26 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -550,11 +550,15 @@ static void test_init(void)
 	       "storage below 2GB");
 	uvcb_init.stor_origin = tmp;
 
-	smp_cpu_setup(1, PSW_WITH_CUR_MASK(cpu_loop));
-	rc = uv_call(0, (uint64_t)&uvcb_init);
-	report(rc == 1 && uvcb_init.header.rc == 0x102,
-	       "too many running cpus");
-	smp_cpu_stop(1);
+	if (smp_query_num_cpus() > 1) {
+		smp_cpu_setup(1, PSW_WITH_CUR_MASK(cpu_loop));
+		rc = uv_call(0, (uint64_t)&uvcb_init);
+		report(rc == 1 && uvcb_init.header.rc == 0x102,
+		       "too many running cpus");
+		smp_cpu_stop(1);
+	} else {
+		report_skip("Not enough cpus for 0x102 test");
+	}
 
 	rc = uv_call(0, (uint64_t)&uvcb_init);
 	report(rc == 0 && uvcb_init.header.rc == UVC_RC_EXECUTED, "successful");
-- 
2.34.1

[kvm-unit-tests PATCH v3 5/9] s390x: uv-host: Fix create guest variable storage prefix check

[Janosch Frank]

We want more than one cpu and the rc is 10B, not 109.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/uv-host.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index ed13bc26..0048a19f 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -435,11 +435,15 @@ static void test_config_create(void)
 	       "base storage origin contains lowcore");
 	uvcb_cgc.conf_base_stor_origin = tmp;
 
-	if (smp_query_num_cpus() == 1) {
+	/*
+	 * Let's not make it too easy and use a second cpu to set a
+	 * non-zero prefix.
+	 */
+	if (smp_query_num_cpus() > 1) {
 		sigp_retry(1, SIGP_SET_PREFIX,
 			   uvcb_cgc.conf_var_stor_origin + PAGE_SIZE, NULL);
 		rc = uv_call(0, (uint64_t)&uvcb_cgc);
-		report(uvcb_cgc.header.rc == 0x10e && rc == 1 &&
+		report(uvcb_cgc.header.rc == 0x10b && rc == 1 &&
 		       !uvcb_cgc.guest_handle, "variable storage area contains lowcore");
 		sigp_retry(1, SIGP_SET_PREFIX, 0x0, NULL);
 	}
-- 
2.34.1

[kvm-unit-tests PATCH v3 6/9] s390x: uv-host: Switch to smp_sigp

[Janosch Frank]

Let's move to the new smp_sigp() interface which abstracts cpu
numbers.

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

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 0048a19f..a9543593 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -440,12 +440,12 @@ static void test_config_create(void)
 	 * non-zero prefix.
 	 */
 	if (smp_query_num_cpus() > 1) {
-		sigp_retry(1, SIGP_SET_PREFIX,
+		smp_sigp(1, SIGP_SET_PREFIX,
 			   uvcb_cgc.conf_var_stor_origin + PAGE_SIZE, NULL);
 		rc = uv_call(0, (uint64_t)&uvcb_cgc);
 		report(uvcb_cgc.header.rc == 0x10b && rc == 1 &&
 		       !uvcb_cgc.guest_handle, "variable storage area contains lowcore");
-		sigp_retry(1, SIGP_SET_PREFIX, 0x0, NULL);
+		smp_sigp(1, SIGP_SET_PREFIX, 0x0, NULL);
 	}
 
 	tmp = uvcb_cgc.guest_sca;
-- 
2.34.1

Re: [kvm-unit-tests PATCH v3 6/9] s390x: uv-host: Switch to smp_sigp

[Nico Boehr]

Quoting Janosch Frank (2023-05-02 15:07:29)
> Let's move to the new smp_sigp() interface which abstracts cpu
> numbers.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Did you intentionally drop my R-b?

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

[kvm-unit-tests PATCH v3 7/9] s390x: uv-host: Properly handle config creation errors

[Janosch Frank]

If the first bit is set on a error rc, the hypervisor will need to
destroy the config before trying again. Let's properly handle those
cases so we're not using stale data.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/uv.h |  1 +
 s390x/uv-host.c    | 66 +++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index 38920461..e9fb19af 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -24,6 +24,7 @@
 #define UVC_RC_NO_RESUME	0x0007
 #define UVC_RC_INV_GHANDLE	0x0020
 #define UVC_RC_INV_CHANDLE	0x0021
+#define UVC_RC_DSTR_NEEDED_FLG	0x8000
 
 #define UVC_CMD_QUI			0x0001
 #define UVC_CMD_INIT_UV			0x000f
diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index a9543593..4418e9b9 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -371,6 +371,42 @@ static void test_cpu_create(void)
 	report_prefix_pop();
 }
 
+/*
+ * If the first bit of the rc is set we need to destroy the
+ * configuration before testing other create config errors.
+ */
+static void cgc_destroy_if_needed(struct uv_cb_cgc *uvcb)
+{
+	uint16_t rc, rrc;
+
+	if (uvcb->header.rc != UVC_RC_EXECUTED &&
+	    !(uvcb->header.rc & UVC_RC_DSTR_NEEDED_FLG))
+		return;
+
+	assert(uvcb->guest_handle);
+	assert(!uv_cmd_nodata(uvcb->guest_handle, UVC_CMD_DESTROY_SEC_CONF,
+			      &rc, &rrc));
+
+	/* We need to zero it for the next test */
+	uvcb->guest_handle = 0;
+}
+
+static bool cgc_check_data(struct uv_cb_cgc *uvcb, uint16_t rc_expected)
+{
+	/* This function purely checks for error rcs */
+	if (uvcb->header.rc == UVC_RC_EXECUTED)
+		return false;
+
+	/*
+	 * We should only receive a handle when the rc is 1 or the
+	 * first bit is set.
+	 */
+	if (!(uvcb->header.rc & UVC_RC_DSTR_NEEDED_FLG) && uvcb->guest_handle)
+		report_abort("Received a handle when we didn't expect one");
+
+	return (uvcb->header.rc & ~UVC_RC_DSTR_NEEDED_FLG) == rc_expected;
+}
+
 static void test_config_create(void)
 {
 	int rc;
@@ -395,44 +431,51 @@ static void test_config_create(void)
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
 	report(uvcb_cgc.header.rc == UVC_RC_INV_LEN && rc == 1 &&
 	       !uvcb_cgc.guest_handle, "hdr invalid length");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.header.len += 8;
 
 	uvcb_cgc.guest_stor_origin = uvcb_qui.max_guest_stor_addr + (1UL << 20) * 2 + 1;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
-	report(uvcb_cgc.header.rc == 0x101 && rc == 1,
+	report(cgc_check_data(&uvcb_cgc, 0x101) && rc == 1,
 	       "MSO > max guest addr");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.guest_stor_origin = 0;
 
 	uvcb_cgc.guest_stor_origin = uvcb_qui.max_guest_stor_addr - (1UL << 20);
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
-	report(uvcb_cgc.header.rc == 0x102 && rc == 1,
+	report(cgc_check_data(&uvcb_cgc, 0x102) && rc == 1,
 	       "MSO + MSL > max guest addr");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.guest_stor_origin = 0;
 
 	uvcb_cgc.guest_asce &= ~ASCE_P;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
-	report(uvcb_cgc.header.rc == 0x105 && rc == 1,
+	report(cgc_check_data(&uvcb_cgc, 0x105) && rc == 1,
 	       "ASCE private bit missing");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.guest_asce |= ASCE_P;
 
 	uvcb_cgc.guest_asce |= 0x20;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
-	report(uvcb_cgc.header.rc == 0x105 && rc == 1,
+	report(cgc_check_data(&uvcb_cgc, 0x105) && rc == 1,
 	       "ASCE bit 58 set");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.guest_asce &= ~0x20;
 
 	tmp = uvcb_cgc.conf_base_stor_origin;
 	uvcb_cgc.conf_base_stor_origin = get_max_ram_size() + 8;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
-	report(uvcb_cgc.header.rc == 0x108 && rc == 1,
+	report(cgc_check_data(&uvcb_cgc, 0x108) && rc == 1,
 	       "base storage origin > available memory");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.conf_base_stor_origin = tmp;
 
 	tmp = uvcb_cgc.conf_base_stor_origin;
 	uvcb_cgc.conf_base_stor_origin = 0x1000;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
-	report(uvcb_cgc.header.rc == 0x109 && rc == 1,
-	       "base storage origin contains lowcore");
+	report(cgc_check_data(&uvcb_cgc, 0x109) && rc == 1,
+	       "base storage origin contains lowcore %x",  uvcb_cgc.header.rc);
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.conf_base_stor_origin = tmp;
 
 	/*
@@ -445,21 +488,24 @@ static void test_config_create(void)
 		rc = uv_call(0, (uint64_t)&uvcb_cgc);
 		report(uvcb_cgc.header.rc == 0x10b && rc == 1 &&
 		       !uvcb_cgc.guest_handle, "variable storage area contains lowcore");
+		cgc_destroy_if_needed(&uvcb_cgc);
 		smp_sigp(1, SIGP_SET_PREFIX, 0x0, NULL);
 	}
 
 	tmp = uvcb_cgc.guest_sca;
 	uvcb_cgc.guest_sca = 0;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
-	report(uvcb_cgc.header.rc == 0x10c && rc == 1,
+	report(cgc_check_data(&uvcb_cgc, 0x10c) && rc == 1,
 	       "sca == 0");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.guest_sca = tmp;
 
 	tmp = uvcb_cgc.guest_sca;
 	uvcb_cgc.guest_sca = get_max_ram_size() + PAGE_SIZE * 4;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
-	report(uvcb_cgc.header.rc == 0x10d && rc == 1,
+	report(cgc_check_data(&uvcb_cgc, 0x10d) && rc == 1,
 	       "sca inaccessible");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.guest_sca = tmp;
 
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
@@ -478,6 +524,7 @@ static void test_config_create(void)
 	uvcb_cgc.guest_handle = 0;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
 	report(uvcb_cgc.header.rc >= 0x100 && rc == 1, "reuse uvcb");
+	cgc_destroy_if_needed(&uvcb_cgc);
 	uvcb_cgc.guest_handle = tmp;
 
 	/* Copy over most data from uvcb_cgc, so we have the ASCE that was used. */
@@ -495,6 +542,7 @@ static void test_config_create(void)
 	rc = uv_call(0, (uint64_t)&uvcb);
 	report(uvcb.header.rc >= 0x104 && rc == 1 && !uvcb.guest_handle,
 	       "reuse ASCE");
+	cgc_destroy_if_needed(&uvcb);
 	free((void *)uvcb.conf_base_stor_origin);
 	free((void *)uvcb.conf_var_stor_origin);
 
-- 
2.34.1

Re: [kvm-unit-tests PATCH v3 7/9] s390x: uv-host: Properly handle config creation errors

[Nico Boehr]

Quoting Janosch Frank (2023-05-02 15:07:30)
> If the first bit is set on a error rc, the hypervisor will need to
> destroy the config before trying again. Let's properly handle those
> cases so we're not using stale data.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

I am not a huge fan of the naming of cgc_check_data(), but since I can't
really come up with a better suggestion let's avoid the bike shedding and
be pragmatic:

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

[kvm-unit-tests PATCH v3 8/9] s390x: uv-host: Fence access checks when UV debug is enabled

[Janosch Frank]

The debug print directly accesses the UV header which will result in a
second accesses exception which will abort the test. Let's fence the
access tests instead.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/uv-host.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 4418e9b9..19bdd465 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -165,6 +165,15 @@ static void test_access(void)
 
 	report_prefix_push("access");
 
+	/*
+	 * If debug is enabled info from the uv header is printed
+	 * which would lead to a second exception and a test abort.
+	 */
+	if (UVC_ERR_DEBUG) {
+		report_skip("Debug doesn't work with access tests");
+		goto out;
+	}
+
 	report_prefix_push("non-crossing");
 	protect_page(uvcb, PAGE_ENTRY_I);
 	for (i = 0; cmds[i].name; i++) {
@@ -197,6 +206,7 @@ static void test_access(void)
 	uvcb += 1;
 	unprotect_page(uvcb, PAGE_ENTRY_I);
 
+out:
 	free_pages(pages);
 	report_prefix_pop();
 }
-- 
2.34.1

[kvm-unit-tests PATCH v3 9/9] s390x: uv-host: Add the test to unittests.conf

[Janosch Frank]

Better to skip than to not run it at all.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/unittests.cfg | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index d6e7b170..3bba03ce 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -231,3 +231,8 @@ extra_params = -m 2200
 [pv-diags]
 file = pv-diags.elf
 extra_params = -m 2200
+
+[uv-host]
+file = uv-host.elf
+smp = 2
+extra_params = -m 2200
-- 
2.34.1

Re: [kvm-unit-tests PATCH v3 9/9] s390x: uv-host: Add the test to unittests.conf

[Nico Boehr]

Quoting Janosch Frank (2023-05-02 15:07:32)
> Better to skip than to not run it at all.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Yes, that's a good idea.

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

Re: [kvm-unit-tests PATCH v3 0/9] s390x: uv-host: Fixups and extensions part 1

[Nico Boehr]

Quoting Janosch Frank (2023-05-02 15:07:23)
> The uv-host test has a lot of historical growth problems which have
> largely been overlooked since running it is harder than running a KVM
> (guest 2) based test.
> 
> This series fixes up smaller problems but still leaves the test with
> fails when running create config base and variable storage
> tests. Those problems will either be fixed up with the second series
> or with a firmware fix since I'm unsure on which side of the os/fw
> fence the problem exists.
> 
> The series is based on my other series that introduces pv-ipl and
> pv-icpt. The memory allocation fix will be added to the new version of
> that series so all G1 tests are fixed.

I have also pushed this to our CI, thanks.

Also here, I took the liberty of adding
groups = pv-host
in the last patch. If you are OK with it, I can carry that when picking for the
PR.

Re: [kvm-unit-tests PATCH v3 0/9] s390x: uv-host: Fixups and extensions part 1

[Janosch Frank]

On 5/31/23 16:47, Nico Boehr wrote:
> Quoting Janosch Frank (2023-05-02 15:07:23)
>> The uv-host test has a lot of historical growth problems which have
>> largely been overlooked since running it is harder than running a KVM
>> (guest 2) based test.
>>
>> This series fixes up smaller problems but still leaves the test with
>> fails when running create config base and variable storage
>> tests. Those problems will either be fixed up with the second series
>> or with a firmware fix since I'm unsure on which side of the os/fw
>> fence the problem exists.
>>
>> The series is based on my other series that introduces pv-ipl and
>> pv-icpt. The memory allocation fix will be added to the new version of
>> that series so all G1 tests are fixed.
> 
> I have also pushed this to our CI, thanks.
> 
> Also here, I took the liberty of adding
> groups = pv-host
> in the last patch. If you are OK with it, I can carry that when picking for the
> PR.

Sure, since Thomas reviewed the patch introducing the group I'm fine 
with that.