[kvm-unit-tests PATCH 0/5] s390x: STFLE nested interpretation

[kvm-unit-tests PATCH 0/5] s390x: STFLE nested interpretation

[Nina Schoetterl-Glausch]

Add a test case that tests the interpretation of STFLE performed by a
nested guest using a snippet.
Also add some functionality to lib/, namely:
* simple pseudo random number generation (arch independent)
* exit (optionally with return code) from snippet (s390x)

Nina Schoetterl-Glausch (5):
  lib: Add pseudo random functions
  s390x: lib: Remove double include
  s390x: Add library functions for exiting from snippet
  s390x: Use library functions for snippet exit
  s390x: Add test for STFLE interpretive execution (format-0)

 Makefile                                |   1 +
 s390x/Makefile                          |   3 +
 lib/s390x/asm/arch_def.h                |  13 +++
 lib/s390x/asm/facility.h                |  10 +-
 lib/libcflat.h                          |   7 ++
 lib/s390x/sie.h                         |   1 +
 lib/s390x/snippet-guest.h               |  26 +++++
 lib/s390x/{snippet.h => snippet-host.h} |   9 +-
 lib/rand.c                              |  19 ++++
 lib/s390x/sie.c                         |  29 +++++-
 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                         |  12 +--
 s390x/snippets/c/sie-dat.c              |  19 +---
 s390x/snippets/c/stfle.c                |  26 +++++
 s390x/spec_ex-sie.c                     |   2 +-
 s390x/stfle-sie.c                       | 132 ++++++++++++++++++++++++
 s390x/uv-host.c                         |   2 +-
 22 files changed, 322 insertions(+), 39 deletions(-)
 create mode 100644 lib/s390x/snippet-guest.h
 rename lib/s390x/{snippet.h => snippet-host.h} (93%)
 create mode 100644 lib/rand.c
 create mode 100644 lib/s390x/snippet-host.c
 create mode 100644 s390x/snippets/c/stfle.c
 create mode 100644 s390x/stfle-sie.c


base-commit: 6b31aa76a038bb56b144825f55301b2ab64c02e9
-- 
2.41.0

[kvm-unit-tests PATCH 1/5] lib: Add pseudo random functions

[Nina Schoetterl-Glausch]

Add functions for generating pseudo random 32 and 64 bit values.
The implementation is very simple and the randomness likely not
of high quality.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 Makefile       |  1 +
 lib/libcflat.h |  7 +++++++
 lib/rand.c     | 19 +++++++++++++++++++
 3 files changed, 27 insertions(+)
 create mode 100644 lib/rand.c

diff --git a/Makefile b/Makefile
index 602910dd..7997e035 100644
--- a/Makefile
+++ b/Makefile
@@ -28,6 +28,7 @@ cflatobjs := \
 	lib/printf.o \
 	lib/string.o \
 	lib/abort.o \
+	lib/rand.o \
 	lib/report.o \
 	lib/stack.o
 
diff --git a/lib/libcflat.h b/lib/libcflat.h
index 700f4352..ed947f98 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -83,6 +83,13 @@ extern void abort(void) __attribute__((noreturn));
 extern long atol(const char *ptr);
 extern char *getenv(const char *name);
 
+typedef struct {
+	uint32_t val;
+} rand_state;
+#define RAND_STATE_INIT(x) ((rand_state){ .val = (x) })
+uint32_t rand32(rand_state *state);
+uint64_t rand64(rand_state *state);
+
 extern int printf(const char *fmt, ...)
 					__attribute__((format(printf, 1, 2)));
 extern int snprintf(char *buf, int size, const char *fmt, ...)
diff --git a/lib/rand.c b/lib/rand.c
new file mode 100644
index 00000000..658c4cbf
--- /dev/null
+++ b/lib/rand.c
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * pseudo random functions
+ *
+ * Copyright IBM Corp. 2023
+ */
+
+#include "libcflat.h"
+
+uint32_t rand32(rand_state *state)
+{
+	state->val = 0x915f77f5 * state->val + 1;
+	return state->val ^ (state->val >> 16);
+}
+
+uint64_t rand64(rand_state *state)
+{
+	return (uint64_t)rand32(state) << 32 | rand32(state);
+}
-- 
2.41.0

Re: [kvm-unit-tests PATCH 1/5] lib: Add pseudo random functions

[Andrew Jones]

On Wed, Dec 13, 2023 at 01:49:38PM +0100, Nina Schoetterl-Glausch wrote:
> Add functions for generating pseudo random 32 and 64 bit values.
> The implementation is very simple and the randomness likely not
> of high quality.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>  Makefile       |  1 +
>  lib/libcflat.h |  7 +++++++
>  lib/rand.c     | 19 +++++++++++++++++++
>  3 files changed, 27 insertions(+)
>  create mode 100644 lib/rand.c
> 
> diff --git a/Makefile b/Makefile
> index 602910dd..7997e035 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -28,6 +28,7 @@ cflatobjs := \
>  	lib/printf.o \
>  	lib/string.o \
>  	lib/abort.o \
> +	lib/rand.o \
>  	lib/report.o \
>  	lib/stack.o
>  
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 700f4352..ed947f98 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -83,6 +83,13 @@ extern void abort(void) __attribute__((noreturn));
>  extern long atol(const char *ptr);
>  extern char *getenv(const char *name);
>  
> +typedef struct {
> +	uint32_t val;
> +} rand_state;
> +#define RAND_STATE_INIT(x) ((rand_state){ .val = (x) })
> +uint32_t rand32(rand_state *state);
> +uint64_t rand64(rand_state *state);
> +
>  extern int printf(const char *fmt, ...)
>  					__attribute__((format(printf, 1, 2)));
>  extern int snprintf(char *buf, int size, const char *fmt, ...)
> diff --git a/lib/rand.c b/lib/rand.c
> new file mode 100644
> index 00000000..658c4cbf
> --- /dev/null
> +++ b/lib/rand.c
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * pseudo random functions
> + *
> + * Copyright IBM Corp. 2023
> + */
> +
> +#include "libcflat.h"
> +
> +uint32_t rand32(rand_state *state)
> +{
> +	state->val = 0x915f77f5 * state->val + 1;
> +	return state->val ^ (state->val >> 16);
> +}
> +
> +uint64_t rand64(rand_state *state)
> +{
> +	return (uint64_t)rand32(state) << 32 | rand32(state);
> +}
> -- 
> 2.41.0
>

Alex Bennée posted a prng patch a long time ago that never got merged.

https://www.spinics.net/lists/kvm-arm/msg50921.html

would it be better to merge that?

Thanks,
drew

Re: [kvm-unit-tests PATCH 1/5] lib: Add pseudo random functions

[Nina Schoetterl-Glausch]

On Wed, 2023-12-13 at 14:38 +0100, Andrew Jones wrote:
> On Wed, Dec 13, 2023 at 01:49:38PM +0100, Nina Schoetterl-Glausch wrote:
> > Add functions for generating pseudo random 32 and 64 bit values.
> > The implementation is very simple and the randomness likely not
> > of high quality.

[...]

> Alex Bennée posted a prng patch a long time ago that never got merged.
> 
> https://www.spinics.net/lists/kvm-arm/msg50921.html
> 
> would it be better to merge that?

Well, it's hard to say what metric to apply here.
How good does the randomness need to be?
I chose a minimal interface that should be amendable to evolution.
That's why the state is hidden behind a typedef.
I think this would be good for Alex' patch, too, and there is nothing gained
by exposing the implementation by prefixing everything with isaac.
My patch is also simpler to review.
But I'm certainly not opposed to better algorithms.
> 
> Thanks,
> drew

Re: [kvm-unit-tests PATCH 1/5] lib: Add pseudo random functions

[Andrew Jones]

On Wed, Dec 13, 2023 at 06:43:51PM +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-12-13 at 14:38 +0100, Andrew Jones wrote:
> > On Wed, Dec 13, 2023 at 01:49:38PM +0100, Nina Schoetterl-Glausch wrote:
> > > Add functions for generating pseudo random 32 and 64 bit values.
> > > The implementation is very simple and the randomness likely not
> > > of high quality.
> 
> [...]
> 
> > Alex Bennée posted a prng patch a long time ago that never got merged.
> > 
> > https://www.spinics.net/lists/kvm-arm/msg50921.html
> > 
> > would it be better to merge that?
> 
> Well, it's hard to say what metric to apply here.
> How good does the randomness need to be?

Better randomness would improve coverage for random sample set type tests
and possibly help stress more for stress tests.

> I chose a minimal interface that should be amendable to evolution.
> That's why the state is hidden behind a typedef.
> I think this would be good for Alex' patch, too, and there is nothing gained
> by exposing the implementation by prefixing everything with isaac.

I agree.

> My patch is also simpler to review.
> But I'm certainly not opposed to better algorithms.

Do you mind reposting Alex's patch, maintaining his authorship, but with
your interface changes?

Thanks,
drew

[kvm-unit-tests PATCH 2/5] s390x: lib: Remove double include

[Nina Schoetterl-Glausch]

libcflat.h was included twice.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 lib/s390x/sie.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
index 28fbf146..40936bd2 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -14,7 +14,6 @@
 #include <sie.h>
 #include <asm/page.h>
 #include <asm/interrupt.h>
-#include <libcflat.h>
 #include <alloc_page.h>
 #include <vmalloc.h>
 #include <sclp.h>
-- 
2.41.0

Re: [kvm-unit-tests PATCH 2/5] s390x: lib: Remove double include

[Claudio Imbrenda]

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

> libcflat.h was included twice.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/s390x/sie.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index 28fbf146..40936bd2 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -14,7 +14,6 @@
>  #include <sie.h>
>  #include <asm/page.h>
>  #include <asm/interrupt.h>
> -#include <libcflat.h>
>  #include <alloc_page.h>
>  #include <vmalloc.h>
>  #include <sclp.h>

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

[Nina Schoetterl-Glausch]

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.

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/s390x/Makefile b/s390x/Makefile
index f79fd009..a10695a2 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -109,6 +109,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)
+	);
+}
+
 #endif
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index c1724cf2..18fdd72e 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -281,6 +281,7 @@ void sie_expect_validity(struct vm *vm);
 uint16_t sie_get_validity(struct vm *vm);
 void sie_check_validity(struct vm *vm, uint16_t vir_exp);
 void sie_handle_validity(struct vm *vm);
+bool sie_is_diag_icpt(struct vm *vm, unsigned int diag);
 void sie_guest_sca_create(struct vm *vm);
 void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
 void sie_guest_destroy(struct vm *vm);
diff --git a/lib/s390x/snippet-guest.h b/lib/s390x/snippet-guest.h
new file mode 100644
index 00000000..e82e8e29
--- /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 */
+}
+
+#endif
diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet-host.h
similarity index 93%
rename from lib/s390x/snippet.h
rename to lib/s390x/snippet-host.h
index 910849aa..d21dd958 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,7 @@ static inline void snippet_setup_guest(struct vm *vm, bool is_pv)
 	}
 }
 
+bool snippet_check_force_exit(struct vm *vm);
+bool snippet_get_force_exit_value(struct vm *vm, uint64_t *value);
+void snippet_check_force_exit_value(struct vm *vm, uint64_t exit_exp);
 #endif
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;
+	uint16_t displace;
+	uint8_t base;
+	bool ret = true;
+
+	ret = ret && vm->sblk->icptcode == ICPT_INST;
+	ret = ret && (vm->sblk->ipa & 0xff00) == 0x8300;
+	switch (diag) {
+	case 0x44:
+	case 0x9c:
+		ret = ret && !(ipb & 0xffff);
+		ipb >>= 16;
+		displace = ipb & 0xfff;
+		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;
+}
+
 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);
+}
diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
index 23a86179..81e9a71e 100644
--- a/lib/s390x/uv.c
+++ b/lib/s390x/uv.c
@@ -18,7 +18,7 @@
 #include <asm/uv.h>
 #include <uv.h>
 #include <sie.h>
-#include <snippet.h>
+#include <snippet-host.h>
 
 static struct uv_cb_qui uvcb_qui = {
 	.header.cmd = UVC_CMD_QUI,
diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c
index d182b49a..3aec9d71 100644
--- a/s390x/mvpg-sie.c
+++ b/s390x/mvpg-sie.c
@@ -19,7 +19,7 @@
 #include <alloc_page.h>
 #include <sclp.h>
 #include <sie.h>
-#include <snippet.h>
+#include <snippet-host.h>
 
 static struct vm vm;
 
diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
index 3193ad99..7fb7f091 100644
--- a/s390x/pv-diags.c
+++ b/s390x/pv-diags.c
@@ -8,7 +8,7 @@
  *  Janosch Frank <frankja@linux.ibm.com>
  */
 #include <libcflat.h>
-#include <snippet.h>
+#include <snippet-host.h>
 #include <pv_icptdata.h>
 #include <sie.h>
 #include <sclp.h>
diff --git a/s390x/pv-icptcode.c b/s390x/pv-icptcode.c
index d7c47d6f..9a9c7357 100644
--- a/s390x/pv-icptcode.c
+++ b/s390x/pv-icptcode.c
@@ -12,7 +12,7 @@
 #include <sie.h>
 #include <smp.h>
 #include <sclp.h>
-#include <snippet.h>
+#include <snippet-host.h>
 #include <pv_icptdata.h>
 #include <asm/facility.h>
 #include <asm/barrier.h>
diff --git a/s390x/pv-ipl.c b/s390x/pv-ipl.c
index cc46e7f7..7d654b84 100644
--- a/s390x/pv-ipl.c
+++ b/s390x/pv-ipl.c
@@ -10,7 +10,7 @@
 #include <libcflat.h>
 #include <sie.h>
 #include <sclp.h>
-#include <snippet.h>
+#include <snippet-host.h>
 #include <pv_icptdata.h>
 #include <asm/facility.h>
 #include <asm/uv.h>
diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
index f0257770..9e60f26e 100644
--- a/s390x/sie-dat.c
+++ b/s390x/sie-dat.c
@@ -16,7 +16,7 @@
 #include <alloc_page.h>
 #include <sclp.h>
 #include <sie.h>
-#include <snippet.h>
+#include <snippet-host.h>
 #include "snippets/c/sie-dat.h"
 
 static struct vm vm;
diff --git a/s390x/spec_ex-sie.c b/s390x/spec_ex-sie.c
index fe2f23ee..0ad7ec08 100644
--- a/s390x/spec_ex-sie.c
+++ b/s390x/spec_ex-sie.c
@@ -13,7 +13,7 @@
 #include <asm/arch_def.h>
 #include <alloc_page.h>
 #include <sie.h>
-#include <snippet.h>
+#include <snippet-host.h>
 #include <hardware.h>
 
 static struct vm vm;
diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 55b46446..87d108b6 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -15,7 +15,7 @@
 #include <sclp.h>
 #include <smp.h>
 #include <uv.h>
-#include <snippet.h>
+#include <snippet-host.h>
 #include <mmu.h>
 #include <asm/page.h>
 #include <asm/pgtable.h>
-- 
2.41.0

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

[Claudio Imbrenda]

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


[...]

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

[Nina Schoetterl-Glausch]

On Wed, 2023-12-13 at 17:42 +0100, Claudio Imbrenda wrote:
> 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

Well, I'm not splitting anything. Is it not clear that "the host's"
refers to snippet.h?

How about:
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/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;

(*) see below
> 
> > +	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:

I don't know, now I gotta be able to do rudimentary arithmetic :D
I don't really have a preference.
I wonder if defining a bit field would be worth it.
> 
> 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

Not sure what you mean, do you want an early return at (*)?
> 
> > +}
> > +
> >  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

Hmm, I chose to do the report in order to be consistent with check_pgm_int_code.
> 
> 
> [...]

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

[Claudio Imbrenda]

On Thu, 14 Dec 2023 21:02:53 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> On Wed, 2023-12-13 at 17:42 +0100, Claudio Imbrenda wrote:
> > 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  
> 
> Well, I'm not splitting anything. Is it not clear that "the host's"
> refers to snippet.h?
> 
> How about:
> Add a guest specific snippet header file and rename snippet.h to reflect
> that it is host specific.

sounds good

[...]

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

something like this:

assert(diag == 0x44 || diag == 0x9c);

if (vm->sblk->icptcode != ICPT_INST)
	return false;
if ((vm->sblk->ipa & 0xff00) != 0x8300)
	return false;
if (vm->sblk->ipb & 0xffff)
	return false;

code = ....

return code == diag;

> > ret = vm->sblk->icptcode == ICPT_INST && (vm->sblk->ipa & 0xff00) ==
> > 0x8300;  
> 
> (*) see below
> >   
> > > +	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:  
> 
> I don't know, now I gotta be able to do rudimentary arithmetic :D
> I don't really have a preference.
> I wonder if defining a bit field would be worth it.

I think it would.

maybe something like:

union ip_text {
	struct {
		unsigned long ipa:16;
		unsigned long ipb:32;
	};
	struct {
		unsigned long opcode:8;
		...
	};
}

then you can do this at the beginning of the function:

union ip_text ip = { .ipa = vm->sblk->ipa, .ipb = ... };

and then use only the bitfields

[...]

[kvm-unit-tests PATCH 4/5] s390x: Use library functions for snippet exit

[Nina Schoetterl-Glausch]

Replace the existing code for exiting from snippets with the newly
introduced library functionality.
This causes additional report()s, otherwise no change in functionality
intended.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/sie-dat.c            | 10 ++--------
 s390x/snippets/c/sie-dat.c | 19 +------------------
 2 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
index 9e60f26e..6b6e6868 100644
--- a/s390x/sie-dat.c
+++ b/s390x/sie-dat.c
@@ -27,23 +27,17 @@ static void test_sie_dat(void)
 	uint64_t test_page_gpa, test_page_hpa;
 	uint8_t *test_page_hva, expected_val;
 	bool contents_match;
-	uint8_t r1;
 
 	/* guest will tell us the guest physical address of the test buffer */
 	sie(&vm);
-	assert(vm.sblk->icptcode == ICPT_INST &&
-	       (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000);
-
-	r1 = (vm.sblk->ipa & 0xf0) >> 4;
-	test_page_gpa = vm.save_area.guest.grs[r1];
+	assert(snippet_get_force_exit_value(&vm, &test_page_gpa));
 	test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa);
 	test_page_hva = __va(test_page_hpa);
 	report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva);
 
 	/* guest will now write to the test buffer and we verify the contents */
 	sie(&vm);
-	assert(vm.sblk->icptcode == ICPT_INST &&
-	       vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000);
+	assert(snippet_check_force_exit(&vm));
 
 	contents_match = true;
 	for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {
diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c
index ecfcb60e..414afd42 100644
--- a/s390x/snippets/c/sie-dat.c
+++ b/s390x/snippets/c/sie-dat.c
@@ -9,28 +9,11 @@
  */
 #include <libcflat.h>
 #include <asm-generic/page.h>
+#include <snippet-guest.h>
 #include "sie-dat.h"
 
 static uint8_t test_pages[GUEST_TEST_PAGE_COUNT * PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
 
-static inline void force_exit(void)
-{
-	asm volatile("diag	0,0,0x44\n"
-		     :
-		     :
-		     : "memory"
-	);
-}
-
-static inline void force_exit_value(uint64_t val)
-{
-	asm volatile("diag	%[val],0,0x9c\n"
-		     :
-		     : [val] "d"(val)
-		     : "memory"
-	);
-}
-
 int main(void)
 {
 	uint8_t *invalid_ptr;
-- 
2.41.0

Re: [kvm-unit-tests PATCH 4/5] s390x: Use library functions for snippet exit

[Claudio Imbrenda]

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

> Replace the existing code for exiting from snippets with the newly
> introduced library functionality.
> This causes additional report()s, otherwise no change in functionality
> intended.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>  s390x/sie-dat.c            | 10 ++--------
>  s390x/snippets/c/sie-dat.c | 19 +------------------
>  2 files changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> index 9e60f26e..6b6e6868 100644
> --- a/s390x/sie-dat.c
> +++ b/s390x/sie-dat.c
> @@ -27,23 +27,17 @@ static void test_sie_dat(void)
>  	uint64_t test_page_gpa, test_page_hpa;
>  	uint8_t *test_page_hva, expected_val;
>  	bool contents_match;
> -	uint8_t r1;
>  
>  	/* guest will tell us the guest physical address of the test buffer */
>  	sie(&vm);
> -	assert(vm.sblk->icptcode == ICPT_INST &&
> -	       (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000);
> -
> -	r1 = (vm.sblk->ipa & 0xf0) >> 4;
> -	test_page_gpa = vm.save_area.guest.grs[r1];
> +	assert(snippet_get_force_exit_value(&vm, &test_page_gpa));

but the function inside the assert will already report a failure, right?
then why the assert? (the point I'm trying to make is that the function
should not report stuff, see my comments in the previous patch)

>  	test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa);
>  	test_page_hva = __va(test_page_hpa);
>  	report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva);
>  
>  	/* guest will now write to the test buffer and we verify the contents */
>  	sie(&vm);
> -	assert(vm.sblk->icptcode == ICPT_INST &&
> -	       vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000);
> +	assert(snippet_check_force_exit(&vm));
>  
>  	contents_match = true;
>  	for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {
> diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c
> index ecfcb60e..414afd42 100644
> --- a/s390x/snippets/c/sie-dat.c
> +++ b/s390x/snippets/c/sie-dat.c
> @@ -9,28 +9,11 @@
>   */
>  #include <libcflat.h>
>  #include <asm-generic/page.h>
> +#include <snippet-guest.h>
>  #include "sie-dat.h"
>  
>  static uint8_t test_pages[GUEST_TEST_PAGE_COUNT * PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
>  
> -static inline void force_exit(void)
> -{
> -	asm volatile("diag	0,0,0x44\n"
> -		     :
> -		     :
> -		     : "memory"
> -	);
> -}
> -
> -static inline void force_exit_value(uint64_t val)
> -{
> -	asm volatile("diag	%[val],0,0x9c\n"
> -		     :
> -		     : [val] "d"(val)
> -		     : "memory"
> -	);
> -}
> -
>  int main(void)
>  {
>  	uint8_t *invalid_ptr;

Re: [kvm-unit-tests PATCH 4/5] s390x: Use library functions for snippet exit

[Nina Schoetterl-Glausch]

On Wed, 2023-12-13 at 17:45 +0100, Claudio Imbrenda wrote:
> On Wed, 13 Dec 2023 13:49:41 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> > Replace the existing code for exiting from snippets with the newly
> > introduced library functionality.
> > This causes additional report()s, otherwise no change in functionality
> > intended.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> >  s390x/sie-dat.c            | 10 ++--------
> >  s390x/snippets/c/sie-dat.c | 19 +------------------
> >  2 files changed, 3 insertions(+), 26 deletions(-)
> > 
> > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> > index 9e60f26e..6b6e6868 100644
> > --- a/s390x/sie-dat.c
> > +++ b/s390x/sie-dat.c
> > @@ -27,23 +27,17 @@ static void test_sie_dat(void)
> >  	uint64_t test_page_gpa, test_page_hpa;
> >  	uint8_t *test_page_hva, expected_val;
> >  	bool contents_match;
> > -	uint8_t r1;
> >  
> >  	/* guest will tell us the guest physical address of the test buffer */
> >  	sie(&vm);
> > -	assert(vm.sblk->icptcode == ICPT_INST &&
> > -	       (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000);
> > -
> > -	r1 = (vm.sblk->ipa & 0xf0) >> 4;
> > -	test_page_gpa = vm.save_area.guest.grs[r1];
> > +	assert(snippet_get_force_exit_value(&vm, &test_page_gpa));
> 
> but the function inside the assert will already report a failure, right?
> then why the assert? (the point I'm trying to make is that the function

The assert makes sense, if it fails something has gone
completely off the rails. The question is indeed rather if to report.
There is no harm in it, but I'm thinking now that there should be
_get_ functions that don't report and optionally also _check_ functions that do.

So:
snippet_get_force_exit
snippet_check_force_exit (exists, but only the _get_ variant is currently required)
snippet_get_force_exit_value (exists, required)
snippet_check_force_exit_value (exists, but unused)

So minimally, we could do:
snippet_get_force_exit
snippet_get_force_exit_value

I'm thinking we should go for the following:
bool snippet_has_force_exit(...)
bool snippet_has_force_exit_value(...)
uint64_t snippet_get_force_exit_value(...) (internally does assert(snippet_has_forced_exit_value))
void snippet_check_force_exit_value(...) (or whoever needs this in the future adds it)
(Naming open to suggestions)

Then this becomes:
 	/* guest will tell us the guest physical address of the test buffer */
 	sie(&vm);
-	assert(vm.sblk->icptcode == ICPT_INST &&
-	       (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000);
-
+	assert(snippet_has_force_exit_value(&vm);
-	r1 = (vm.sblk->ipa & 0xf0) >> 4;
-	test_page_gpa = vm.save_area.guest.grs[r1];
+	test_page_gpa = snippet_get_force_exit_value(&vm);

> should not report stuff, see my comments in the previous patch)
> 
> >  	test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa);
> >  	test_page_hva = __va(test_page_hpa);
> >  	report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva);
> >  
> >  	/* guest will now write to the test buffer and we verify the contents */
> >  	sie(&vm);
> > -	assert(vm.sblk->icptcode == ICPT_INST &&
> > -	       vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000);
> > +	assert(snippet_check_force_exit(&vm));
> >  
> >  	contents_match = true;
> >  	for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {

[...]

Re: [kvm-unit-tests PATCH 4/5] s390x: Use library functions for snippet exit

[Claudio Imbrenda]

On Fri, 15 Dec 2023 12:50:15 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> On Wed, 2023-12-13 at 17:45 +0100, Claudio Imbrenda wrote:
> > On Wed, 13 Dec 2023 13:49:41 +0100
> > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> >   
> > > Replace the existing code for exiting from snippets with the newly
> > > introduced library functionality.
> > > This causes additional report()s, otherwise no change in functionality
> > > intended.
> > > 
> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > > ---
> > >  s390x/sie-dat.c            | 10 ++--------
> > >  s390x/snippets/c/sie-dat.c | 19 +------------------
> > >  2 files changed, 3 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> > > index 9e60f26e..6b6e6868 100644
> > > --- a/s390x/sie-dat.c
> > > +++ b/s390x/sie-dat.c
> > > @@ -27,23 +27,17 @@ static void test_sie_dat(void)
> > >  	uint64_t test_page_gpa, test_page_hpa;
> > >  	uint8_t *test_page_hva, expected_val;
> > >  	bool contents_match;
> > > -	uint8_t r1;
> > >  
> > >  	/* guest will tell us the guest physical address of the test buffer */
> > >  	sie(&vm);
> > > -	assert(vm.sblk->icptcode == ICPT_INST &&
> > > -	       (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000);
> > > -
> > > -	r1 = (vm.sblk->ipa & 0xf0) >> 4;
> > > -	test_page_gpa = vm.save_area.guest.grs[r1];
> > > +	assert(snippet_get_force_exit_value(&vm, &test_page_gpa));  
> > 
> > but the function inside the assert will already report a failure, right?
> > then why the assert? (the point I'm trying to make is that the function  
> 
> The assert makes sense, if it fails something has gone
> completely off the rails. The question is indeed rather if to report.
> There is no harm in it, but I'm thinking now that there should be
> _get_ functions that don't report and optionally also _check_ functions that do.
> 
> So:
> snippet_get_force_exit
> snippet_check_force_exit (exists, but only the _get_ variant is currently required)
> snippet_get_force_exit_value (exists, required)
> snippet_check_force_exit_value (exists, but unused)
> 
> So minimally, we could do:
> snippet_get_force_exit
> snippet_get_force_exit_value
> 
> I'm thinking we should go for the following:
> bool snippet_has_force_exit(...)
> bool snippet_has_force_exit_value(...)
> uint64_t snippet_get_force_exit_value(...) (internally does assert(snippet_has_forced_exit_value))
> void snippet_check_force_exit_value(...) (or whoever needs this in the future adds it)
> (Naming open to suggestions)

I like this.

I would call the bool ones "snippet_is_force_exit", in line with
similar functions. usually "has" is used for capabilities or features.

but I'm open to other suggestions/ideas 

> 
> Then this becomes:
>  	/* guest will tell us the guest physical address of the test buffer */
>  	sie(&vm);
> -	assert(vm.sblk->icptcode == ICPT_INST &&
> -	       (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000);
> -
> +	assert(snippet_has_force_exit_value(&vm);
> -	r1 = (vm.sblk->ipa & 0xf0) >> 4;
> -	test_page_gpa = vm.save_area.guest.grs[r1];
> +	test_page_gpa = snippet_get_force_exit_value(&vm);
> 
> > should not report stuff, see my comments in the previous patch)
> >   
> > >  	test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa);
> > >  	test_page_hva = __va(test_page_hpa);
> > >  	report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva);
> > >  
> > >  	/* guest will now write to the test buffer and we verify the contents */
> > >  	sie(&vm);
> > > -	assert(vm.sblk->icptcode == ICPT_INST &&
> > > -	       vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000);
> > > +	assert(snippet_check_force_exit(&vm));
> > >  
> > >  	contents_match = true;
> > >  	for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {  
> 
> [...]

[kvm-unit-tests PATCH 5/5] s390x: Add test for STFLE interpretive execution (format-0)

[Nina Schoetterl-Glausch]

The STFLE instruction indicates installed facilities.
SIE can interpretively execute STFLE.
Use a snippet guest executing STFLE to get the result of
interpretive execution and check the result.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 s390x/Makefile           |   2 +
 lib/s390x/asm/facility.h |  10 ++-
 s390x/snippets/c/stfle.c |  26 ++++++++
 s390x/stfle-sie.c        | 132 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 169 insertions(+), 1 deletion(-)
 create mode 100644 s390x/snippets/c/stfle.c
 create mode 100644 s390x/stfle-sie.c

diff --git a/s390x/Makefile b/s390x/Makefile
index a10695a2..12eb3053 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -42,6 +42,7 @@ tests += $(TEST_DIR)/exittime.elf
 tests += $(TEST_DIR)/ex.elf
 tests += $(TEST_DIR)/topology.elf
 tests += $(TEST_DIR)/sie-dat.elf
+tests += $(TEST_DIR)/stfle-sie.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 pv-tests += $(TEST_DIR)/pv-icptcode.elf
@@ -127,6 +128,7 @@ snippet_lib = $(snippet_asmlib) lib/auxinfo.o
 $(TEST_DIR)/mvpg-sie.elf: snippets = $(SNIPPET_DIR)/c/mvpg-snippet.gbin
 $(TEST_DIR)/sie-dat.elf: snippets = $(SNIPPET_DIR)/c/sie-dat.gbin
 $(TEST_DIR)/spec_ex-sie.elf: snippets = $(SNIPPET_DIR)/c/spec_ex.gbin
+$(TEST_DIR)/stfle-sie.elf: snippets = $(SNIPPET_DIR)/c/stfle.gbin
 
 $(TEST_DIR)/pv-diags.elf: pv-snippets += $(SNIPPET_DIR)/asm/pv-diag-yield.gbin
 $(TEST_DIR)/pv-diags.elf: pv-snippets += $(SNIPPET_DIR)/asm/pv-diag-288.gbin
diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
index a66fe56a..2bad05c5 100644
--- a/lib/s390x/asm/facility.h
+++ b/lib/s390x/asm/facility.h
@@ -27,12 +27,20 @@ static inline void stfl(void)
 	asm volatile("	stfl	0(0)\n" : : : "memory");
 }
 
-static inline void stfle(uint64_t *fac, unsigned int nb_doublewords)
+static inline unsigned int stfle(uint64_t *fac, unsigned int nb_doublewords)
 {
 	register unsigned long r0 asm("0") = nb_doublewords - 1;
 
 	asm volatile("	.insn	s,0xb2b00000,0(%1)\n"
 		     : "+d" (r0) : "a" (fac) : "memory", "cc");
+	return r0 + 1;
+}
+
+static inline unsigned long stfle_size(void)
+{
+	uint64_t dummy;
+
+	return stfle(&dummy, 1);
 }
 
 static inline void setup_facilities(void)
diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c
new file mode 100644
index 00000000..eb024a6a
--- /dev/null
+++ b/s390x/snippets/c/stfle.c
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright IBM Corp. 2023
+ *
+ * Snippet used by the STLFE interpretive execution facilities test.
+ */
+#include <libcflat.h>
+#include <snippet-guest.h>
+
+int main(void)
+{
+	const unsigned int max_fac_len = 8;
+	uint64_t res[max_fac_len + 1];
+
+	res[0] = max_fac_len - 1;
+	asm volatile ( "lg	0,%[len]\n"
+		"	stfle	%[fac]\n"
+		"	stg	0,%[len]\n"
+		: [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]),
+		  [len] "+RT"(res[0])
+		:
+		: "%r0", "cc"
+	);
+	force_exit_value((uint64_t)&res);
+	return 0;
+}
diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c
new file mode 100644
index 00000000..574319ed
--- /dev/null
+++ b/s390x/stfle-sie.c
@@ -0,0 +1,132 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright IBM Corp. 2023
+ *
+ * SIE with STLFE interpretive execution facilities test.
+ */
+#include <libcflat.h>
+#include <stdlib.h>
+#include <asm/facility.h>
+#include <asm/time.h>
+#include <snippet-host.h>
+#include <alloc_page.h>
+#include <sclp.h>
+
+static struct vm vm;
+static uint64_t (*fac)[PAGE_SIZE / sizeof(uint64_t)];
+static rand_state rand_s;
+
+static void setup_guest(void)
+{
+	extern const char SNIPPET_NAME_START(c, stfle)[];
+	extern const char SNIPPET_NAME_END(c, stfle)[];
+
+	setup_vm();
+	fac = alloc_pages_flags(0, AREA_DMA31);
+
+	snippet_setup_guest(&vm, false);
+	snippet_init(&vm, SNIPPET_NAME_START(c, stfle),
+		     SNIPPET_LEN(c, stfle), SNIPPET_UNPACK_OFF);
+}
+
+struct guest_stfle_res {
+	uint16_t len;
+	uint64_t reg;
+	unsigned char *mem;
+};
+
+static struct guest_stfle_res run_guest(void)
+{
+	struct guest_stfle_res res;
+	uint64_t guest_stfle_addr;
+
+	sie(&vm);
+	assert(snippet_get_force_exit_value(&vm, &guest_stfle_addr));
+	res.mem = &vm.guest_mem[guest_stfle_addr];
+	memcpy(&res.reg, res.mem, sizeof(res.reg));
+	res.len = (res.reg & 0xff) + 1;
+	res.mem += sizeof(res.reg);
+	return res;
+}
+
+static void test_stfle_format_0(void)
+{
+	struct guest_stfle_res res;
+
+	report_prefix_push("format-0");
+	for (int j = 0; j < stfle_size(); j++)
+		WRITE_ONCE((*fac)[j], rand64(&rand_s));
+	vm.sblk->fac = (uint32_t)(uint64_t)fac;
+	res = run_guest();
+	report(res.len == stfle_size(), "stfle len correct");
+	report(!memcmp(*fac, res.mem, res.len * sizeof(uint64_t)),
+	       "Guest facility list as specified");
+	report_prefix_pop();
+}
+
+struct args {
+	uint64_t seed;
+};
+
+static bool parse_uint64_t(const char *arg, uint64_t *out)
+{
+	char *end;
+	uint64_t num;
+
+	if (arg[0] == '\0')
+		return false;
+	num = strtoul(arg, &end, 0);
+	if (end[0] != '\0')
+		return false;
+	*out = num;
+	return true;
+}
+
+static struct args parse_args(int argc, char **argv)
+{
+	struct args args;
+	const char *flag;
+	unsigned int i;
+	uint64_t arg;
+	bool has_arg;
+
+	stck(&args.seed);
+
+	for (i = 1; i < argc; i++) {
+		if (i + 1 < argc)
+			has_arg = parse_uint64_t(argv[i + 1], &arg);
+		else
+			has_arg = false;
+
+		flag = "--seed";
+		if (!strcmp(flag, argv[i])) {
+			if (!has_arg)
+				report_abort("%s needs an uint64_t parameter", flag);
+			args.seed = arg;
+			++i;
+			continue;
+		}
+		report_abort("Unsupported parameter '%s'",
+			     argv[i]);
+	}
+
+	return args;
+}
+
+int main(int argc, char **argv)
+{
+	struct args args = parse_args(argc, argv);
+
+	if (!sclp_facilities.has_sief2) {
+		report_skip("SIEF2 facility unavailable");
+		goto out;
+	}
+
+	report_info("pseudo rand seed: 0x%lx", args.seed);
+	rand_s = RAND_STATE_INIT(args.seed);
+	setup_guest();
+	if (test_facility(7))
+		test_stfle_format_0();
+out:
+	return report_summary();
+}
-- 
2.41.0

Re: [kvm-unit-tests PATCH 5/5] s390x: Add test for STFLE interpretive execution (format-0)

[Claudio Imbrenda]

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

> The STFLE instruction indicates installed facilities.
> SIE can interpretively execute STFLE.
> Use a snippet guest executing STFLE to get the result of
> interpretive execution and check the result.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

[...]

>  static inline void setup_facilities(void)
> diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c
> new file mode 100644
> index 00000000..eb024a6a
> --- /dev/null
> +++ b/s390x/snippets/c/stfle.c
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright IBM Corp. 2023
> + *
> + * Snippet used by the STLFE interpretive execution facilities test.
> + */
> +#include <libcflat.h>
> +#include <snippet-guest.h>
> +
> +int main(void)
> +{
> +	const unsigned int max_fac_len = 8;

why 8?

> +	uint64_t res[max_fac_len + 1];
> +
> +	res[0] = max_fac_len - 1;
> +	asm volatile ( "lg	0,%[len]\n"
> +		"	stfle	%[fac]\n"
> +		"	stg	0,%[len]\n"
> +		: [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]),
> +		  [len] "+RT"(res[0])
> +		:
> +		: "%r0", "cc"
> +	);
> +	force_exit_value((uint64_t)&res);
> +	return 0;
> +}
> diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c
> new file mode 100644
> index 00000000..574319ed
> --- /dev/null
> +++ b/s390x/stfle-sie.c
> @@ -0,0 +1,132 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright IBM Corp. 2023
> + *
> + * SIE with STLFE interpretive execution facilities test.
> + */
> +#include <libcflat.h>
> +#include <stdlib.h>
> +#include <asm/facility.h>
> +#include <asm/time.h>
> +#include <snippet-host.h>
> +#include <alloc_page.h>
> +#include <sclp.h>
> +
> +static struct vm vm;
> +static uint64_t (*fac)[PAGE_SIZE / sizeof(uint64_t)];
> +static rand_state rand_s;
> +
> +static void setup_guest(void)
> +{
> +	extern const char SNIPPET_NAME_START(c, stfle)[];
> +	extern const char SNIPPET_NAME_END(c, stfle)[];
> +
> +	setup_vm();
> +	fac = alloc_pages_flags(0, AREA_DMA31);
> +
> +	snippet_setup_guest(&vm, false);
> +	snippet_init(&vm, SNIPPET_NAME_START(c, stfle),
> +		     SNIPPET_LEN(c, stfle), SNIPPET_UNPACK_OFF);
> +}
> +
> +struct guest_stfle_res {
> +	uint16_t len;
> +	uint64_t reg;
> +	unsigned char *mem;
> +};
> +
> +static struct guest_stfle_res run_guest(void)
> +{
> +	struct guest_stfle_res res;
> +	uint64_t guest_stfle_addr;
> +
> +	sie(&vm);
> +	assert(snippet_get_force_exit_value(&vm, &guest_stfle_addr));
> +	res.mem = &vm.guest_mem[guest_stfle_addr];
> +	memcpy(&res.reg, res.mem, sizeof(res.reg));
> +	res.len = (res.reg & 0xff) + 1;
> +	res.mem += sizeof(res.reg);
> +	return res;
> +}
> +
> +static void test_stfle_format_0(void)
> +{
> +	struct guest_stfle_res res;
> +
> +	report_prefix_push("format-0");
> +	for (int j = 0; j < stfle_size(); j++)
> +		WRITE_ONCE((*fac)[j], rand64(&rand_s));

do you really need random numbers? can't you use a static pattern?
maybe something like 0x0001020304050607 etc...

> +	vm.sblk->fac = (uint32_t)(uint64_t)fac;
> +	res = run_guest();
> +	report(res.len == stfle_size(), "stfle len correct");
> +	report(!memcmp(*fac, res.mem, res.len * sizeof(uint64_t)),
> +	       "Guest facility list as specified");

it seems like you are comparing the full facility list (stfle_size
doublewords long) with the result of STFLE in the guest, but the guest
is limited to 8 double words?

> +	report_prefix_pop();
> +}
> +
> +struct args {
> +	uint64_t seed;
> +};
> +
> +static bool parse_uint64_t(const char *arg, uint64_t *out)
> +{
> +	char *end;
> +	uint64_t num;
> +
> +	if (arg[0] == '\0')
> +		return false;
> +	num = strtoul(arg, &end, 0);
> +	if (end[0] != '\0')
> +		return false;
> +	*out = num;
> +	return true;
> +}
> +
> +static struct args parse_args(int argc, char **argv)
> +{
> +	struct args args;
> +	const char *flag;
> +	unsigned int i;
> +	uint64_t arg;
> +	bool has_arg;
> +
> +	stck(&args.seed);
> +
> +	for (i = 1; i < argc; i++) {
> +		if (i + 1 < argc)
> +			has_arg = parse_uint64_t(argv[i + 1], &arg);
> +		else
> +			has_arg = false;
> +
> +		flag = "--seed";
> +		if (!strcmp(flag, argv[i])) {
> +			if (!has_arg)
> +				report_abort("%s needs an uint64_t parameter", flag);
> +			args.seed = arg;
> +			++i;
> +			continue;
> +		}
> +		report_abort("Unsupported parameter '%s'",
> +			     argv[i]);
> +	}
> +
> +	return args;
> +}

this is lots of repeated code in all tests, I should really resurrect
and polish my patchseries for argument parsing

> +
> +int main(int argc, char **argv)
> +{
> +	struct args args = parse_args(argc, argv);
> +
> +	if (!sclp_facilities.has_sief2) {
> +		report_skip("SIEF2 facility unavailable");
> +		goto out;
> +	}
> +
> +	report_info("pseudo rand seed: 0x%lx", args.seed);
> +	rand_s = RAND_STATE_INIT(args.seed);
> +	setup_guest();
> +	if (test_facility(7))
> +		test_stfle_format_0();
> +out:
> +	return report_summary();
> +}

Re: [kvm-unit-tests PATCH 5/5] s390x: Add test for STFLE interpretive execution (format-0)

[Nina Schoetterl-Glausch]

On Wed, 2023-12-13 at 18:00 +0100, Claudio Imbrenda wrote:
> On Wed, 13 Dec 2023 13:49:42 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> > The STFLE instruction indicates installed facilities.
> > SIE can interpretively execute STFLE.
> > Use a snippet guest executing STFLE to get the result of
> > interpretive execution and check the result.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> [...]
> 
> >  static inline void setup_facilities(void)
> > diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c
> > new file mode 100644
> > index 00000000..eb024a6a
> > --- /dev/null
> > +++ b/s390x/snippets/c/stfle.c
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright IBM Corp. 2023
> > + *
> > + * Snippet used by the STLFE interpretive execution facilities test.
> > + */
> > +#include <libcflat.h>
> > +#include <snippet-guest.h>
> > +
> > +int main(void)
> > +{
> > +	const unsigned int max_fac_len = 8;
> 
> why 8?

8 is a somewhat arbitrary, large number :)
I suppose I could choose an even larger one, maybe even PAGE_SIZE/8.
That would guarantee that max_fac_len >= stfle_size() (8 is enough for that today)
It's not necessary for max_fac_len >= stfle_size(), but probably good for
test coverage.
> 
> > +	uint64_t res[max_fac_len + 1];
> > +
> > +	res[0] = max_fac_len - 1;
> > +	asm volatile ( "lg	0,%[len]\n"
> > +		"	stfle	%[fac]\n"
> > +		"	stg	0,%[len]\n"
> > +		: [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]),
> > +		  [len] "+RT"(res[0])
> > +		:
> > +		: "%r0", "cc"
> > +	);
> > +	force_exit_value((uint64_t)&res);
> > +	return 0;
> > +}
> > diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c
> > new file mode 100644
> > index 00000000..574319ed
> > --- /dev/null
> > +++ b/s390x/stfle-sie.c
> > @@ -0,0 +1,132 @@

[...]
> > +
> > +static void test_stfle_format_0(void)
> > +{
> > +	struct guest_stfle_res res;
> > +
> > +	report_prefix_push("format-0");
> > +	for (int j = 0; j < stfle_size(); j++)
> > +		WRITE_ONCE((*fac)[j], rand64(&rand_s));
> 
> do you really need random numbers? can't you use a static pattern?
> maybe something like 0x0001020304050607 etc...

Doesn't really need to be random, I need some arbitrary test pattern,
but I don't think some cumbersome constant literal improves anything.
The RNG is just initialized with the time, because why not.

> 
> > +	vm.sblk->fac = (uint32_t)(uint64_t)fac;
> > +	res = run_guest();
> > +	report(res.len == stfle_size(), "stfle len correct");

^ should be

+	report(res.len == min(stfle_size(), 8), "stfle len correct");

For the case that the guest buffer was shorter.

> > +	report(!memcmp(*fac, res.mem, res.len * sizeof(uint64_t)),
> > +	       "Guest facility list as specified");
> 
> it seems like you are comparing the full facility list (stfle_size
> doublewords long) with the result of STFLE in the guest, but the guest
> is limited to 8 double words?

Their prefixes must be the same. res.len is the guest length, so max 8 right now.
> 
> > +	report_prefix_pop();
> > +}

Re: [kvm-unit-tests PATCH 5/5] s390x: Add test for STFLE interpretive execution (format-0)

[Nina Schoetterl-Glausch]

On Wed, 2023-12-13 at 18:31 +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-12-13 at 18:00 +0100, Claudio Imbrenda wrote:
> > On Wed, 13 Dec 2023 13:49:42 +0100
> > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> > 
> > > The STFLE instruction indicates installed facilities.
> > > SIE can interpretively execute STFLE.
> > > Use a snippet guest executing STFLE to get the result of
> > > interpretive execution and check the result.
> > > 
> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > 
> > [...]
> > 
> > >  static inline void setup_facilities(void)
> > > diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c
> > > new file mode 100644
> > > index 00000000..eb024a6a
> > > --- /dev/null
> > > +++ b/s390x/snippets/c/stfle.c
> > > @@ -0,0 +1,26 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright IBM Corp. 2023
> > > + *
> > > + * Snippet used by the STLFE interpretive execution facilities test.
> > > + */
> > > +#include <libcflat.h>
> > > +#include <snippet-guest.h>
> > > +
> > > +int main(void)
> > > +{
> > > +	const unsigned int max_fac_len = 8;
> > 
> > why 8?
> 
> 8 is a somewhat arbitrary, large number :)
> I suppose I could choose an even larger one, maybe even PAGE_SIZE/8.
> That would guarantee that max_fac_len >= stfle_size() (8 is enough for that today)
> It's not necessary for max_fac_len >= stfle_size(), but probably good for
> test coverage.
> > 
> > > +	uint64_t res[max_fac_len + 1];
> > > +
> > > +	res[0] = max_fac_len - 1;
> > > +	asm volatile ( "lg	0,%[len]\n"
> > > +		"	stfle	%[fac]\n"
> > > +		"	stg	0,%[len]\n"
> > > +		: [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]),
> > > +		  [len] "+RT"(res[0])
> > > +		:
> > > +		: "%r0", "cc"
> > > +	);
> > > +	force_exit_value((uint64_t)&res);
> > > +	return 0;
> > > +}
> > > diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c
> > > new file mode 100644
> > > index 00000000..574319ed
> > > --- /dev/null
> > > +++ b/s390x/stfle-sie.c
> > > @@ -0,0 +1,132 @@

[...]

> > > +	vm.sblk->fac = (uint32_t)(uint64_t)fac;
> > > +	res = run_guest();
> > > +	report(res.len == stfle_size(), "stfle len correct");

You're right, disregard everything below.
> 
> ^ should be
> 
> +	report(res.len == min(stfle_size(), 8), "stfle len correct");
> 
> For the case that the guest buffer was shorter.
> 
> > > +	report(!memcmp(*fac, res.mem, res.len * sizeof(uint64_t)),
> > > +	       "Guest facility list as specified");
> > 
> > it seems like you are comparing the full facility list (stfle_size
> > doublewords long) with the result of STFLE in the guest, but the guest
> > is limited to 8 double words?
> 
> Their prefixes must be the same. res.len is the guest length, so max 8 right now.
> > 
> > > +	report_prefix_pop();
> > > +}