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

[kvm-unit-tests PATCH 0/8] 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 smaler 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.

Janosch Frank (8):
  s390x: uv-host: Fix UV init test memory allocation
  s390x: uv-host: Check for sufficient amount of memory
  s390x: Add PV tests to unittests.cfg
  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: Properly handle config creation errors
  s390x: uv-host: Fence access checks when UV debug is enabled

 lib/s390x/asm/uv.h  |   1 +
 s390x/unittests.cfg |  16 +++++++
 s390x/uv-host.c     | 101 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 99 insertions(+), 19 deletions(-)

-- 
2.34.1

[kvm-unit-tests PATCH 1/8] 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.

Let's fix that.

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

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 33e6eec6..68de47e7 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -555,7 +555,7 @@ 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);
+	mem = (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);
-- 
2.34.1

[kvm-unit-tests PATCH 2/8] 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>
---
 s390x/uv-host.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 68de47e7..e38a9913 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -715,6 +715,17 @@ int main(void)
 	test_invalid();
 	test_uv_uninitialized();
 	test_query();
+
+	/*
+	 * Some of the UV memory needs to be allocated with >31 bit
+	 * addresses which means we need a lot more memory than other
+	 * tests.
+	 */
+	if (get_ram_size() < (SZ_1M * 2200UL)) {
+		report_skip("Not enough memory. This test needs about 2200MB of memory");
+		goto done;
+	}
+
 	test_init();
 
 	setup_vmem();
-- 
2.34.1

[kvm-unit-tests PATCH 3/8] s390x: Add PV tests to unittests.cfg

[Janosch Frank]

Even if the first thing those tests do is skipping they should still
be run to make sure the tests boots and the skipping works.
---
 s390x/unittests.cfg | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 96977f2b..c9d341ea 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -222,3 +222,19 @@ file = migration-cmm.elf
 groups = migration
 smp = 2
 extra_params = -append '--parallel'
+
+[pv-diags]
+file = pv-diags.elf
+extra_params = -m 2200
+
+[pv-icptcode]
+file = pv-icptcode.elf
+extra_params = -m 2200 -smp 3
+
+[pv-ipl]
+file = pv-ipl.elf
+extra_params = -m 2200
+
+[uv-host]
+file = uv-host.elf
+extra_params = -m 2200 -smp 2
-- 
2.34.1

[kvm-unit-tests PATCH 4/8] s390x: uv-host: Beautify code

[Janosch Frank]

Fixup top comment and add missing space.

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 e38a9913..0f550415 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
  *
@@ -33,7 +33,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 5/8] 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>
---
 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 0f550415..42ea2a53 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -546,11 +546,15 @@ static void test_init(void)
 	       "storage below 2GB");
 	uvcb_init.stor_origin = mem;
 
-	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 6/8] 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>
---
 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 42ea2a53..d92571b5 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -434,11 +434,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 7/8] 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 usign stale data.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/uv.h |  1 +
 s390x/uv-host.c    | 52 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 44 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 d92571b5..b23d51c9 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -370,6 +370,38 @@ 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_DSTR_NEEDED_FLG))
+		return;
+
+	assert(uvcb->guest_handle);
+
+	assert(!uv_cmd_nodata(uvcb->guest_handle, UVC_CMD_DESTROY_SEC_CONF,
+			      &rc, &rrc));
+}
+
+/* This function expects errors, not successes */
+static bool cgc_check_data(struct uv_cb_cgc *uvcb, uint16_t rc_expected)
+{
+	cgc_destroy_if_needed(uvcb);
+	/*
+	 * 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)
+		return false;
+	/* Now that we checked the handle, we need to zero it for the next test */
+	uvcb->guest_handle = 0;
+	return (uvcb->header.rc & ~UVC_RC_DSTR_NEEDED_FLG) == rc_expected;
+}
+
 static void test_config_create(void)
 {
 	int rc;
@@ -398,40 +430,40 @@ static void test_config_create(void)
 
 	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");
 	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");
 	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");
 	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");
 	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");
 	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);
 	uvcb_cgc.conf_base_stor_origin = tmp;
 
 	/*
@@ -450,14 +482,14 @@ static void test_config_create(void)
 	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");
 	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");
 	uvcb_cgc.guest_sca = tmp;
 
@@ -477,6 +509,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. */
@@ -494,6 +527,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

[kvm-unit-tests PATCH 8/8] 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>
---
 s390x/uv-host.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index b23d51c9..1f73034f 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -164,6 +164,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++) {
@@ -196,6 +205,7 @@ static void test_access(void)
 	uvcb += 1;
 	unprotect_page(uvcb, PAGE_ENTRY_I);
 
+out:
 	free_pages(pages);
 	report_prefix_pop();
 }
-- 
2.34.1

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

[Nico Boehr]

Quoting Janosch Frank (2023-03-23 11:39:06)
> 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.
> 
> Let's fix that.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/uv-host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 33e6eec6..68de47e7 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -555,7 +555,7 @@ 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);
> +       mem = (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);

Your fix looks reasonable, but I think this is still broken, no?

We allocate a new mem, initalize the ultravisor again and then free the memory. We do absolutely nothing with the allocated memory, do we?

Are we maybe missing a
  uvcb_init.stor_origin = mem;
here?

Also, a comment stating that AREA_NORMAL starts at PFN 524288 (2G) might be nice.

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

[Nico Boehr]

Quoting Janosch Frank (2023-03-23 11:39:07)
> 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>

Re: [kvm-unit-tests PATCH 4/8] s390x: uv-host: Beautify code

[Nico Boehr]

Quoting Janosch Frank (2023-03-23 11:39:09)
> Fixup top comment and add missing space.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

Re: [kvm-unit-tests PATCH 5/8] s390x: uv-host: Add cpu number check

[Nico Boehr]

Quoting Janosch Frank (2023-03-23 11:39:10)
> 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>

Re: [kvm-unit-tests PATCH 6/8] s390x: uv-host: Fix create guest variable storage prefix check

[Nico Boehr]

Quoting Janosch Frank (2023-03-23 11:39:11)
> We want more than one cpu and the rc is 10B, not 109.
                                                     ^
Code says 10e        --------------------------------|

> 
> Signed-off-by: Janosch Frank <frankja@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 42ea2a53..d92571b5 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -434,11 +434,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);

While at it, this should be smp_sigp, no?

With the commit message fixup:

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

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

[Nico Boehr]

Quoting Janosch Frank (2023-03-23 11:39:12)
[...]
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index d92571b5..b23d51c9 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -370,6 +370,38 @@ 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)

Is there a reason why we can't make this a cgc_uv_call() function which performs the uv_call and the cleanups if needed?

Mixing reports and cleanup activity feels a bit odd to me.

[...]
> +/* This function expects errors, not successes */

I am confused by this comment. What does it mean?

> +static bool cgc_check_data(struct uv_cb_cgc *uvcb, uint16_t rc_expected)

Rename to cgc_check_rc_and_handle?

> +{
> +       cgc_destroy_if_needed(uvcb);
> +       /*
> +        * We should only receive a handle when the rc is 1 or the
> +        * first bit is set.

Where is the code that checks for rc == 1?

Ah OK, so that's what you mean with the comment above, this function only works if the UVC fails, right?

> +        */
> +       if (!(uvcb->header.rc & UVC_RC_DSTR_NEEDED_FLG) && uvcb->guest_handle)
> +               return false;

It would be nicer if I got a proper report message that tells me that we got a handle even though we shouldn't destroy.

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

[Nico Boehr]

Quoting Janosch Frank (2023-03-23 11:39:13)
> 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>

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

[Janosch Frank]

On 3/23/23 14:19, Nico Boehr wrote:
> Quoting Janosch Frank (2023-03-23 11:39:12)
> [...]
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index d92571b5..b23d51c9 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -370,6 +370,38 @@ 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)
> 
> Is there a reason why we can't make this a cgc_uv_call() function which performs the uv_call and the cleanups if needed?

I'd much rather put the destroy into the cleanup area after the report.

> 
> Mixing reports and cleanup activity feels a bit odd to me.
> 
> [...]
>> +/* This function expects errors, not successes */
> 
> I am confused by this comment. What does it mean?
> 
>> +static bool cgc_check_data(struct uv_cb_cgc *uvcb, uint16_t rc_expected)
> 
> Rename to cgc_check_rc_and_handle?
> 
>> +{
>> +       cgc_destroy_if_needed(uvcb);
>> +       /*
>> +        * We should only receive a handle when the rc is 1 or the
>> +        * first bit is set.
> 
> Where is the code that checks for rc == 1?
> 
> Ah OK, so that's what you mean with the comment above, this function only works if the UVC fails, right?
> 
>> +        */
>> +       if (!(uvcb->header.rc & UVC_RC_DSTR_NEEDED_FLG) && uvcb->guest_handle)
>> +               return false;
> 
> It would be nicer if I got a proper report message that tells me that we got a handle even though we shouldn't destroy.

We can report_info() or report_abort().