diff mbox series

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

Message ID 20230323103913.40720-8-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: uv-host: Fixups and extensions part 1 | expand

Commit Message

Janosch Frank March 23, 2023, 10:39 a.m. UTC 
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(-)

Comments

Nico Boehr March 23, 2023, 1:19 p.m. UTC | #1 
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.
Janosch Frank March 23, 2023, 2:02 p.m. UTC | #2 
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().
diff mbox series

Patch

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