Re: [kvm-unit-tests PATCH 3/5] s390x: Add library functions for exiting from snippet

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Wed, 13 Dec 2023 13:49:40 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> It is useful to be able to force an exit to the host from the snippet,
> as well as do so while returning a value.
> Add this functionality, also add helper functions for the host to check
> for an exit and get or check the value.
> Use diag 0x44 and 0x9c for this.
> Add a guest specific snippet header file and rename the host's.

you should also mention here that you are splitting snippet.h into a
host-only part and a guest-only part

> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>  s390x/Makefile                          |  1 +
>  lib/s390x/asm/arch_def.h                | 13 ++++++++
>  lib/s390x/sie.h                         |  1 +
>  lib/s390x/snippet-guest.h               | 26 ++++++++++++++++
>  lib/s390x/{snippet.h => snippet-host.h} |  9 ++++--
>  lib/s390x/sie.c                         | 28 +++++++++++++++++
>  lib/s390x/snippet-host.c                | 40 +++++++++++++++++++++++++
>  lib/s390x/uv.c                          |  2 +-
>  s390x/mvpg-sie.c                        |  2 +-
>  s390x/pv-diags.c                        |  2 +-
>  s390x/pv-icptcode.c                     |  2 +-
>  s390x/pv-ipl.c                          |  2 +-
>  s390x/sie-dat.c                         |  2 +-
>  s390x/spec_ex-sie.c                     |  2 +-
>  s390x/uv-host.c                         |  2 +-
>  15 files changed, 123 insertions(+), 11 deletions(-)
>  create mode 100644 lib/s390x/snippet-guest.h
>  rename lib/s390x/{snippet.h => snippet-host.h} (93%)
>  create mode 100644 lib/s390x/snippet-host.c

[...]

> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index 40936bd2..908b0130 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -42,6 +42,34 @@ void sie_check_validity(struct vm *vm, uint16_t vir_exp)
>  	report(vir_exp == vir, "VALIDITY: %x", vir);
>  }
>  
> +bool sie_is_diag_icpt(struct vm *vm, unsigned int diag)
> +{
> +	uint32_t ipb = vm->sblk->ipb;
> +	uint64_t code;

uint64_t code = 0;

> +	uint16_t displace;
> +	uint8_t base;
> +	bool ret = true;

bool ret;

> +
> +	ret = ret && vm->sblk->icptcode == ICPT_INST;
> +	ret = ret && (vm->sblk->ipa & 0xff00) == 0x8300;

ret = vm->sblk->icptcode == ICPT_INST && (vm->sblk->ipa & 0xff00) ==
0x8300;

> +	switch (diag) {
> +	case 0x44:
> +	case 0x9c:
> +		ret = ret && !(ipb & 0xffff);
> +		ipb >>= 16;
> +		displace = ipb & 0xfff;

maybe it's more readable to avoid shifting thigs around all the time:

displace = (ipb >> 16) & 0xfff;
base = (ipb >> 28) & 0xf;
if (base)
	code = vm->....[base];
code = (code + displace) & 0xffff;
if (ipb & 0xffff || code != diag)
	return false;

> +		ipb >>= 12;
> +		base = ipb & 0xf;
> +		code = base ? vm->save_area.guest.grs[base] + displace : displace;
> +		code &= 0xffff;
> +		ret = ret && (code == diag);
> +		break;
> +	default:
> +		abort(); /* not implemented */
> +	}
> +	return ret;

although I have the feeling that this would be more readable if you
would check diag immediately, and avoid using ret

> +}
> +
>  void sie_handle_validity(struct vm *vm)
>  {
>  	if (vm->sblk->icptcode != ICPT_VALIDITY)
> diff --git a/lib/s390x/snippet-host.c b/lib/s390x/snippet-host.c
> new file mode 100644
> index 00000000..a829c1d5
> --- /dev/null
> +++ b/lib/s390x/snippet-host.c
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Snippet functionality for the host.
> + *
> + * Copyright IBM Corp. 2023
> + */
> +
> +#include <libcflat.h>
> +#include <snippet-host.h>
> +#include <sie.h>
> +
> +bool snippet_check_force_exit(struct vm *vm)
> +{
> +	bool r;
> +
> +	r = sie_is_diag_icpt(vm, 0x44);
> +	report(r, "guest forced exit");
> +	return r;
> +}
> +
> +bool snippet_get_force_exit_value(struct vm *vm, uint64_t *value)
> +{
> +	struct kvm_s390_sie_block *sblk = vm->sblk;
> +
> +	if (sie_is_diag_icpt(vm, 0x9c)) {
> +		*value = vm->save_area.guest.grs[(sblk->ipa & 0xf0) >> 4];
> +		report_pass("guest forced exit with value: 0x%lx", *value);
> +		return true;
> +	}
> +	report_fail("guest forced exit with value");
> +	return false;
> +}
> +
> +void snippet_check_force_exit_value(struct vm *vm, uint64_t value_exp)
> +{
> +	uint64_t value;
> +
> +	if (snippet_get_force_exit_value(vm, &value))
> +		report(value == value_exp, "guest exit value matches 0x%lx", value_exp);
> +}

from a readability and a consistency perspective, it would be better if
the functions would only check stuff and return a bool or a value, and
do the report() in the body of the testcase


[...]