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

["Nicholas Piggin" <npiggin@gmail.com>]

On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch 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 snippet.h to reflect
> that it is host specific.
>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>  s390x/Makefile                          |  1 +
>  lib/s390x/asm/arch_def.h                | 13 ++++++++
>  lib/s390x/snippet-guest.h               | 26 +++++++++++++++
>  lib/s390x/{snippet.h => snippet-host.h} | 10 ++++--
>  lib/s390x/snippet-host.c                | 42 +++++++++++++++++++++++++
>  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 +-
>  13 files changed, 97 insertions(+), 11 deletions(-)
>  create mode 100644 lib/s390x/snippet-guest.h
>  rename lib/s390x/{snippet.h => snippet-host.h} (92%)
>  create mode 100644 lib/s390x/snippet-host.c
>
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 23342bd6..12445fb5 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -111,6 +111,7 @@ cflatobjs += lib/s390x/css_lib.o
>  cflatobjs += lib/s390x/malloc_io.o
>  cflatobjs += lib/s390x/uv.o
>  cflatobjs += lib/s390x/sie.o
> +cflatobjs += lib/s390x/snippet-host.o
>  cflatobjs += lib/s390x/fault.o
>  
>  OBJDIRS += lib/s390x
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 745a3387..db04deca 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -504,4 +504,17 @@ static inline uint32_t get_prefix(void)
>  	return current_prefix;
>  }
>  
> +static inline void diag44(void)
> +{
> +	asm volatile("diag	0,0,0x44\n");
> +}
> +
> +static inline void diag9c(uint64_t val)
> +{
> +	asm volatile("diag	%[val],0,0x9c\n"
> +		:
> +		: [val] "d"(val)
> +	);
> +}

Would you add a "memory" clobber to these maybe? In theory I think
gcc can move even volatile asm around unless there are depdendencies.
Maybe I am overly paranoid.

> +
>  #endif
> diff --git a/lib/s390x/snippet-guest.h b/lib/s390x/snippet-guest.h
> new file mode 100644
> index 00000000..3cc098e1
> --- /dev/null
> +++ b/lib/s390x/snippet-guest.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Snippet functionality for the guest.
> + *
> + * Copyright IBM Corp. 2023
> + */
> +
> +#ifndef _S390X_SNIPPET_GUEST_H_
> +#define _S390X_SNIPPET_GUEST_H_
> +
> +#include <asm/arch_def.h>
> +#include <asm/barrier.h>
> +
> +static inline void force_exit(void)
> +{
> +	diag44();
> +	mb(); /* allow host to modify guest memory */
> +}
> +
> +static inline void force_exit_value(uint64_t val)
> +{
> +	diag9c(val);
> +	mb(); /* allow host to modify guest memory */
> +}

You have barriers here, but couldn't the diag get moved before a prior
store by the guest?

Silly question since I don't understand the s390x arch or snippet design
too well... the diag here causes a guest exit to the host. After the
host handles that, it may resume guest at the next instruction? If that
is correct, then the barrier here (I think) is for when the guest
resumes it would not reorder subsequent loads from guest memory before
the diag, because the host might have modified it.

> +
> +#endif /* _S390X_SNIPPET_GUEST_H_ */
> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet-host.h
> similarity index 92%
> rename from lib/s390x/snippet.h
> rename to lib/s390x/snippet-host.h
> index 910849aa..230b25b0 100644
> --- a/lib/s390x/snippet.h
> +++ b/lib/s390x/snippet-host.h
> @@ -1,13 +1,13 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  /*
> - * Snippet definitions
> + * Snippet functionality for the host.
>   *
>   * Copyright IBM Corp. 2021
>   * Author: Janosch Frank <frankja@linux.ibm.com>
>   */
>  
> -#ifndef _S390X_SNIPPET_H_
> -#define _S390X_SNIPPET_H_
> +#ifndef _S390X_SNIPPET_HOST_H_
> +#define _S390X_SNIPPET_HOST_H_
>  
>  #include <sie.h>
>  #include <uv.h>
> @@ -144,4 +144,8 @@ static inline void snippet_setup_guest(struct vm *vm, bool is_pv)
>  	}
>  }
>  
> +bool snippet_is_force_exit(struct vm *vm);
> +bool snippet_is_force_exit_value(struct vm *vm);
> +uint64_t snippet_get_force_exit_value(struct vm *vm);
> +void snippet_check_force_exit_value(struct vm *vm, uint64_t exit_exp);
>  #endif
> diff --git a/lib/s390x/snippet-host.c b/lib/s390x/snippet-host.c
> new file mode 100644
> index 00000000..44a60bb9
> --- /dev/null
> +++ b/lib/s390x/snippet-host.c
> @@ -0,0 +1,42 @@
> +/* 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_is_force_exit(struct vm *vm)
> +{
> +	return sie_is_diag_icpt(vm, 0x44);
> +}
> +
> +bool snippet_is_force_exit_value(struct vm *vm)
> +{
> +	return sie_is_diag_icpt(vm, 0x9c);
> +}
> +
> +uint64_t snippet_get_force_exit_value(struct vm *vm)
> +{
> +	struct kvm_s390_sie_block *sblk = vm->sblk;
> +
> +	assert(snippet_is_force_exit_value(vm));
> +
> +	return vm->save_area.guest.grs[(sblk->ipa & 0xf0) >> 4];
> +}
> +
> +void snippet_check_force_exit_value(struct vm *vm, uint64_t value_exp)
> +{
> +	uint64_t value;
> +
> +	if (snippet_is_force_exit_value(vm)) {
> +		value = snippet_get_force_exit_value(vm);
> +		report(value == value_exp, "guest forced exit with value (0x%lx == 0x%lx)",
> +		       value, value_exp);

This is like kvm selftests guest/host synch design, which is quite
nice and useful.

> +	} else {
> +		report_fail("guest forced exit with value");
> +	}

Guest forced exit without value? And do you also need to check for non-value force
exit to distinguish from a normal snippet exit?

Thanks,
Nick