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

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

[Nina Schoetterl-Glausch]

v2 -> v3:
 * pick up Ack (thanks Andrew)
 * minor cosmetic change to rand generator
 * add sie_is_pv function
 * extend sie_is_diag_icpt to support pv, replace pv_icptdata_check_diag

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:
* pseudo random number generation (arch independent)
* exit (optionally with return code) from snippet (s390x)
* add function for checking diag intercepts, replacing
      pv_icptdata_check_diag (s390x)

v1 -> v2:
 * implement SHA-256 based PRNG
 * pick up R-b (thanks Claudio)
 * change snippet exit API and implementation (thanks Claudio)
 * add stfle-sie to unittests.cfg

Nina Schoetterl-Glausch (7):
  lib: Add pseudo random functions
  s390x: lib: Remove double include
  s390x: Add sie_is_pv
  s390x: Add function for checking diagnose intercepts
  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/rand.h                              |  21 +++
 lib/s390x/pv_icptdata.h                 |  42 ------
 lib/s390x/sie.h                         |  18 +++
 lib/s390x/snippet-guest.h               |  26 ++++
 lib/s390x/{snippet.h => snippet-host.h} |  10 +-
 lib/rand.c                              | 177 ++++++++++++++++++++++++
 lib/s390x/sie.c                         |  58 +++++++-
 lib/s390x/snippet-host.c                |  42 ++++++
 lib/s390x/uv.c                          |   2 +-
 s390x/mvpg-sie.c                        |   2 +-
 s390x/pv-diags.c                        |  10 +-
 s390x/pv-icptcode.c                     |  13 +-
 s390x/pv-ipl.c                          |   9 +-
 s390x/sie-dat.c                         |  13 +-
 s390x/snippets/c/sie-dat.c              |  19 +--
 s390x/snippets/c/stfle.c                |  26 ++++
 s390x/spec_ex-sie.c                     |   2 +-
 s390x/stfle-sie.c                       | 134 ++++++++++++++++++
 s390x/uv-host.c                         |   2 +-
 s390x/unittests.cfg                     |   3 +
 24 files changed, 558 insertions(+), 98 deletions(-)
 create mode 100644 lib/rand.h
 delete mode 100644 lib/s390x/pv_icptdata.h
 create mode 100644 lib/s390x/snippet-guest.h
 rename lib/s390x/{snippet.h => snippet-host.h} (92%)
 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

Range-diff against v2:
1:  6c869961 ! 1:  baecabf2 lib: Add pseudo random functions
    @@ Commit message
                 for i in range(8):
                     yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")
     
    +    Acked-by: Andrew Jones <andrew.jones@linux.dev>
         Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
     
     
    @@ lib/rand.c (new)
     +	return rot(x, 17) ^ rot(x, 19) ^ (x >> 10);
     +}
     +
    -+enum alphabet { a, b, c, d, e, f, g, h, };
    ++enum alphabet { A, B, C, D, E, F, G, H, };
     +
     +static void sha256_chunk(const uint32_t (*chunk)[16], uint32_t (*hash)[8])
     +{
    @@ lib/rand.c (new)
     +	for (int i = 0; i < 64; i++) {
     +		uint32_t t1, t2;
     +
    -+		t1 = w_hash[h] +
    -+		     upper_sig1(w_hash[e]) +
    -+		     ch(w_hash[e], w_hash[f], w_hash[g]) +
    ++		t1 = w_hash[H] +
    ++		     upper_sig1(w_hash[E]) +
    ++		     ch(w_hash[E], w_hash[F], w_hash[G]) +
     +		     K[i] +
     +		     w[i];
     +
    -+		t2 = upper_sig0(w_hash[a]) + maj(w_hash[a], w_hash[b], w_hash[c]);
    ++		t2 = upper_sig0(w_hash[A]) + maj(w_hash[A], w_hash[B], w_hash[C]);
     +
    -+		w_hash[h] = w_hash[g];
    -+		w_hash[g] = w_hash[f];
    -+		w_hash[f] = w_hash[e];
    -+		w_hash[e] = w_hash[d] + t1;
    -+		w_hash[d] = w_hash[c];
    -+		w_hash[c] = w_hash[b];
    -+		w_hash[b] = w_hash[a];
    -+		w_hash[a] = t1 + t2;
    ++		w_hash[H] = w_hash[G];
    ++		w_hash[G] = w_hash[F];
    ++		w_hash[F] = w_hash[E];
    ++		w_hash[E] = w_hash[D] + t1;
    ++		w_hash[D] = w_hash[C];
    ++		w_hash[C] = w_hash[B];
    ++		w_hash[B] = w_hash[A];
    ++		w_hash[A] = t1 + t2;
     +	}
     +
     +	for (int i = 0; i < 8; i++)
2:  77319d3e = 2:  b30314eb s390x: lib: Remove double include
-:  -------- > 3:  f2af539b s390x: Add sie_is_pv
-:  -------- > 4:  c4331d19 s390x: Add function for checking diagnose intercepts
3:  e2c2cad8 ! 5:  a3f92777 s390x: Add library functions for exiting from snippet
    @@ lib/s390x/asm/arch_def.h: static inline uint32_t get_prefix(void)
     +
      #endif
     
    - ## lib/s390x/sie.h ##
    -@@ lib/s390x/sie.h: 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);
    -
      ## lib/s390x/snippet-guest.h (new) ##
     @@
     +/* SPDX-License-Identifier: GPL-2.0-only */
    @@ lib/s390x/snippet-guest.h (new)
     +	mb(); /* allow host to modify guest memory */
     +}
     +
    -+#endif
    ++#endif /* _S390X_SNIPPET_GUEST_H_ */
     
      ## lib/s390x/snippet.h => lib/s390x/snippet-host.h ##
     @@
    @@ lib/s390x/snippet-host.h: static inline void snippet_setup_guest(struct vm *vm,
     +void snippet_check_force_exit_value(struct vm *vm, uint64_t exit_exp);
      #endif
     
    - ## lib/s390x/sie.c ##
    -@@ lib/s390x/sie.c: 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)
    -+{
    -+	union {
    -+		struct {
    -+			uint64_t     : 16;
    -+			uint64_t ipa : 16;
    -+			uint64_t ipb : 32;
    -+		};
    -+		struct {
    -+			uint64_t          : 16;
    -+			uint64_t opcode   :  8;
    -+			uint64_t r_1      :  4;
    -+			uint64_t r_2      :  4;
    -+			uint64_t r_base   :  4;
    -+			uint64_t displace : 12;
    -+			uint64_t zero     : 16;
    -+		};
    -+	} instr = { .ipa = vm->sblk->ipa, .ipb = vm->sblk->ipb };
    -+	uint64_t code;
    -+
    -+	assert(diag == 0x44 || diag == 0x9c);
    -+
    -+	if (vm->sblk->icptcode != ICPT_INST)
    -+		return false;
    -+	if (instr.opcode != 0x83 || instr.zero)
    -+		return false;
    -+	code = instr.r_base ? vm->save_area.guest.grs[instr.r_base] : 0;
    -+	code = (code + instr.displace) & 0xffff;
    -+	return code == diag;
    -+}
    -+
    - void sie_handle_validity(struct vm *vm)
    - {
    - 	if (vm->sblk->icptcode != ICPT_VALIDITY)
    -
      ## lib/s390x/snippet-host.c (new) ##
     @@
     +/* SPDX-License-Identifier: GPL-2.0-only */
    @@ s390x/pv-diags.c
      #include <libcflat.h>
     -#include <snippet.h>
     +#include <snippet-host.h>
    - #include <pv_icptdata.h>
      #include <sie.h>
      #include <sclp.h>
    + #include <asm/facility.h>
     
      ## s390x/pv-icptcode.c ##
     @@
    @@ s390x/pv-icptcode.c
      #include <sclp.h>
     -#include <snippet.h>
     +#include <snippet-host.h>
    - #include <pv_icptdata.h>
      #include <asm/facility.h>
      #include <asm/barrier.h>
    + #include <asm/sigp.h>
     
      ## s390x/pv-ipl.c ##
     @@
    @@ s390x/pv-ipl.c
      #include <sclp.h>
     -#include <snippet.h>
     +#include <snippet-host.h>
    - #include <pv_icptdata.h>
      #include <asm/facility.h>
      #include <asm/uv.h>
    + 
     
      ## s390x/sie-dat.c ##
     @@
4:  67fbf0bb ! 6:  a1db588b s390x: Use library functions for snippet exit
    @@ s390x/sie-dat.c: static void test_sie_dat(void)
     
      ## s390x/snippets/c/sie-dat.c ##
     @@
    -  */
      #include <libcflat.h>
      #include <asm-generic/page.h>
    + #include <asm/mem.h>
     +#include <snippet-guest.h>
      #include "sie-dat.h"
      
5:  157079f2 ! 7:  0b89b3c6 s390x: Add test for STFLE interpretive execution (format-0)
    @@ s390x/stfle-sie.c (new)
     +}
     
      ## s390x/unittests.cfg ##
    -@@ s390x/unittests.cfg: extra_params = """-cpu max,ctop=on -smp cpus=1,drawers=2,books=2,sockets=2,cores
    +@@ s390x/unittests.cfg: file = sie-dat.elf
      
    - [sie-dat]
    - file = sie-dat.elf
    + [pv-attest]
    + file = pv-attest.elf
     +
     +[stfle-sie]
     +file = stfle-sie.elf

base-commit: 28ac3b10d6f982b1d9c2fe629f23d23ec5024b4f
-- 
2.44.0

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

[Nina Schoetterl-Glausch]

Add functions for generating pseudo random 32 and 64 bit values.
The implementation uses SHA-256 and so the randomness should have good
quality.
Implement the necessary subset of SHA-256.
The PRNG algorithm is equivalent to the following python snippet:

def prng32(seed):
    from hashlib import sha256
    state = seed.to_bytes(8, byteorder="big")
    while True:
        state = sha256(state).digest()
        for i in range(8):
            yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")

Acked-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---

Notes:
    Since a PRNG with better quality was asked for I decided to use SHA-256
    because:
     * it is a standard, commonly used algorithm
     * high quality randomness is assured
     * the implementation can be checked against the spec
     * the implementation can be easily checked via comparison
    
    I tested the implementation in the following way:
    
    cat <<'EOF' > rand.py
    #!/usr/bin/python3
    
    def prng32(seed):
        from hashlib import sha256
        state = seed.to_bytes(8, byteorder="big")
        while True:
            state = sha256(state).digest()
            for i in range(8):
                yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")
    
    r = prng32(0)
    for i in range(100):
        print(f"{next(r):08x}")
    
    EOF
    
    cat <<'EOF' > rand.c
    #include <stdio.h>
    #include "rand.h"
    
    void main(void)
    {
    	prng_state state = prng_init(0);
    	for (int i = 0; i < 100; i++) {
    		printf("%08x\n", prng32(&state));
    	}
    }
    EOF
    cat <<'EOF' > libcflat.h
    #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
    EOF
    chmod +x rand.py
    ln -s lib/rand.c librand.c
    gcc -Ilib librand.c rand.c
    diff <(./a.out) <(./rand.py)

 Makefile   |   1 +
 lib/rand.h |  21 +++++++
 lib/rand.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 199 insertions(+)
 create mode 100644 lib/rand.h
 create mode 100644 lib/rand.c

diff --git a/Makefile b/Makefile
index 5b7998b7..3d51cb72 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/rand.h b/lib/rand.h
new file mode 100644
index 00000000..cdce8bd7
--- /dev/null
+++ b/lib/rand.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * (pseudo) random functions
+ *
+ * Copyright IBM Corp. 2024
+ */
+#ifndef _RAND_H_
+#define _RAND_H_
+
+#include <stdint.h>
+
+/* Non cryptographically secure PRNG */
+typedef struct {
+	uint32_t hash[8];
+	uint8_t next_word;
+} prng_state;
+prng_state prng_init(uint64_t seed);
+uint32_t prng32(prng_state *state);
+uint64_t prng64(prng_state *state);
+
+#endif /* _RAND_H_ */
diff --git a/lib/rand.c b/lib/rand.c
new file mode 100644
index 00000000..583d2d9f
--- /dev/null
+++ b/lib/rand.c
@@ -0,0 +1,177 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * (pseudo) random functions
+ * Currently uses SHA-256 to scramble the PRNG state.
+ *
+ * Copyright IBM Corp. 2024
+ */
+
+#include "libcflat.h"
+#include "rand.h"
+#include <string.h>
+
+/* Begin SHA-256 related definitions */
+
+#define INITAL_HASH { \
+	0x6a09e667, \
+	0xbb67ae85, \
+	0x3c6ef372, \
+	0xa54ff53a, \
+	0x510e527f, \
+	0x9b05688c, \
+	0x1f83d9ab, \
+	0x5be0cd19, \
+}
+
+static const uint32_t K[] = {
+	0x428a2f98, 0x71374491, 0xb5c0fbcf, 0xe9b5dba5, 0x3956c25b, 0x59f111f1, 0x923f82a4, 0xab1c5ed5,
+	0xd807aa98, 0x12835b01, 0x243185be, 0x550c7dc3, 0x72be5d74, 0x80deb1fe, 0x9bdc06a7, 0xc19bf174,
+	0xe49b69c1, 0xefbe4786, 0x0fc19dc6, 0x240ca1cc, 0x2de92c6f, 0x4a7484aa, 0x5cb0a9dc, 0x76f988da,
+	0x983e5152, 0xa831c66d, 0xb00327c8, 0xbf597fc7, 0xc6e00bf3, 0xd5a79147, 0x06ca6351, 0x14292967,
+	0x27b70a85, 0x2e1b2138, 0x4d2c6dfc, 0x53380d13, 0x650a7354, 0x766a0abb, 0x81c2c92e, 0x92722c85,
+	0xa2bfe8a1, 0xa81a664b, 0xc24b8b70, 0xc76c51a3, 0xd192e819, 0xd6990624, 0xf40e3585, 0x106aa070,
+	0x19a4c116, 0x1e376c08, 0x2748774c, 0x34b0bcb5, 0x391c0cb3, 0x4ed8aa4a, 0x5b9cca4f, 0x682e6ff3,
+	0x748f82ee, 0x78a5636f, 0x84c87814, 0x8cc70208, 0x90befffa, 0xa4506ceb, 0xbef9a3f7, 0xc67178f2,
+};
+
+static inline uint32_t ch(uint32_t x, uint32_t y, uint32_t z)
+{
+	return (x & y) ^ ((~x) & z);
+}
+
+static inline uint32_t maj(uint32_t x, uint32_t y, uint32_t z)
+{
+	return (x & y) ^ (x & z) ^ (y & z);
+}
+
+static inline uint32_t rot(uint32_t value, unsigned int count)
+{
+	return value >> count | value << (32 - count);
+}
+
+static inline uint32_t upper_sig0(uint32_t x)
+{
+	return rot(x, 2) ^ rot(x, 13) ^ rot(x, 22);
+}
+
+static inline uint32_t upper_sig1(uint32_t x)
+{
+	return rot(x, 6) ^ rot(x, 11) ^ rot(x, 25);
+}
+
+static inline uint32_t lower_sig0(uint32_t x)
+{
+	return rot(x, 7) ^ rot(x, 18) ^ (x >> 3);
+}
+
+static inline uint32_t lower_sig1(uint32_t x)
+{
+	return rot(x, 17) ^ rot(x, 19) ^ (x >> 10);
+}
+
+enum alphabet { A, B, C, D, E, F, G, H, };
+
+static void sha256_chunk(const uint32_t (*chunk)[16], uint32_t (*hash)[8])
+{
+	uint32_t w[64];
+	uint32_t w_hash[8];
+
+	memcpy(w, chunk, sizeof(*chunk));
+
+	for (int i = 16; i < 64; i++)
+		w[i] = lower_sig1(w[i - 2]) + w[i - 7] + lower_sig0(w[i - 15]) + w[i - 16];
+
+	memcpy(w_hash, hash, sizeof(*hash));
+
+	for (int i = 0; i < 64; i++) {
+		uint32_t t1, t2;
+
+		t1 = w_hash[H] +
+		     upper_sig1(w_hash[E]) +
+		     ch(w_hash[E], w_hash[F], w_hash[G]) +
+		     K[i] +
+		     w[i];
+
+		t2 = upper_sig0(w_hash[A]) + maj(w_hash[A], w_hash[B], w_hash[C]);
+
+		w_hash[H] = w_hash[G];
+		w_hash[G] = w_hash[F];
+		w_hash[F] = w_hash[E];
+		w_hash[E] = w_hash[D] + t1;
+		w_hash[D] = w_hash[C];
+		w_hash[C] = w_hash[B];
+		w_hash[B] = w_hash[A];
+		w_hash[A] = t1 + t2;
+	}
+
+	for (int i = 0; i < 8; i++)
+		(*hash)[i] += w_hash[i];
+}
+
+/**
+ * sha256_hash - Calculate SHA-256 of input. Only a limited subset of inputs supported.
+ * @n: Number of words to hash, must be <= 13
+ * @input: Input data to hash
+ * @hash: Output hash as a word array, ordered such that the first word contains
+ *        the first/leftmost bits of the 256 bit hash
+ *
+ * Calculate the SHA-256 hash of the input where the input must be a multiple of
+ * 4 bytes and at most 52 long. The input is used without any adjustment, so,
+ * should the caller want to hash bytes it needs to interpret the bytes in the
+ * ordering as defined by the specification, that is big endian.
+ * The same applies to interpreting the output array as bytes.
+ * The function computes the same as: printf "%08x" ${input[@]} | xxd -r -p | sha256sum .
+ */
+static void sha256_hash(unsigned int n, const uint32_t (*input)[n], uint32_t (*hash)[8])
+{
+	/*
+	 * Pad according to SHA-2 specification.
+	 * First set up length in bits.
+	 */
+	uint32_t chunk[16] = {
+		[15] = sizeof(*input) * 8,
+	};
+
+	memcpy(chunk, input, sizeof(*input));
+	/* Then add separator */
+	chunk[n] = 1 << 31;
+	memcpy(hash, (uint32_t[])INITAL_HASH, sizeof(*hash));
+	sha256_chunk(&chunk, hash);
+}
+
+/* End SHA-256 related definitions */
+
+prng_state prng_init(uint64_t seed)
+{
+	prng_state state = { .next_word = 0 };
+	uint32_t seed_arr[2] = { seed >> 32, seed };
+
+	sha256_hash(ARRAY_SIZE(seed_arr), &seed_arr, &state.hash);
+	return state;
+}
+
+static void prng_scramble(prng_state *state)
+{
+	uint32_t input[8];
+
+	memcpy(input, state->hash, sizeof(state->hash));
+	sha256_hash(ARRAY_SIZE(input), &input, &state->hash);
+	state->next_word = 0;
+}
+
+uint32_t prng32(prng_state *state)
+{
+	if (state->next_word < ARRAY_SIZE(state->hash))
+		return state->hash[state->next_word++];
+
+	prng_scramble(state);
+	return prng32(state);
+}
+
+uint64_t prng64(prng_state *state)
+{
+	/* explicitly evaluate the high word first */
+	uint64_t high = prng32(state);
+
+	return high << 32 | prng32(state);
+}
-- 
2.44.0

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

[Nina Schoetterl-Glausch]

libcflat.h was included twice.

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
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.44.0

[kvm-unit-tests PATCH v3 3/7] s390x: Add sie_is_pv

[Nina Schoetterl-Glausch]

Add a function to check if a guest VM is currently running protected.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 lib/s390x/sie.h | 6 ++++++
 lib/s390x/sie.c | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index c1724cf2..53cd767f 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -281,6 +281,12 @@ 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);
+
+static inline bool sie_is_pv(struct vm *vm)
+{
+	return vm->sblk->sdf == 2;
+}
+
 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/sie.c b/lib/s390x/sie.c
index 40936bd2..0fa915cf 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -59,7 +59,7 @@ void sie(struct vm *vm)
 	/* When a pgm int code is set, we'll never enter SIE below. */
 	assert(!read_pgm_int_code());
 
-	if (vm->sblk->sdf == 2)
+	if (sie_is_pv(vm))
 		memcpy(vm->sblk->pv_grregs, vm->save_area.guest.grs,
 		       sizeof(vm->save_area.guest.grs));
 
@@ -98,7 +98,7 @@ void sie(struct vm *vm)
 	/* restore the old CR 13 */
 	lctlg(13, old_cr13);
 
-	if (vm->sblk->sdf == 2)
+	if (sie_is_pv(vm))
 		memcpy(vm->save_area.guest.grs, vm->sblk->pv_grregs,
 		       sizeof(vm->save_area.guest.grs));
 }
-- 
2.44.0

[kvm-unit-tests PATCH v3 4/7] s390x: Add function for checking diagnose intercepts

[Nina Schoetterl-Glausch]

sie_is_diag_icpt() checks if the intercept is due to an expected
diagnose call and is valid.
It subsumes pv_icptdata_check_diag.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 lib/s390x/pv_icptdata.h | 42 --------------------------------
 lib/s390x/sie.h         | 12 ++++++++++
 lib/s390x/sie.c         | 53 +++++++++++++++++++++++++++++++++++++++++
 s390x/pv-diags.c        |  8 +++----
 s390x/pv-icptcode.c     | 11 ++++-----
 s390x/pv-ipl.c          |  7 +++---
 6 files changed, 76 insertions(+), 57 deletions(-)
 delete mode 100644 lib/s390x/pv_icptdata.h

diff --git a/lib/s390x/pv_icptdata.h b/lib/s390x/pv_icptdata.h
deleted file mode 100644
index 4746117e..00000000
--- a/lib/s390x/pv_icptdata.h
+++ /dev/null
@@ -1,42 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Commonly used checks for PV SIE intercept data
- *
- * Copyright IBM Corp. 2023
- * Author: Janosch Frank <frankja@linux.ibm.com>
- */
-
-#ifndef _S390X_PV_ICPTDATA_H_
-#define _S390X_PV_ICPTDATA_H_
-
-#include <sie.h>
-
-/*
- * Checks the diagnose instruction intercept data for consistency with
- * the constants defined by the PV SIE architecture
- *
- * Supports: 0x44, 0x9c, 0x288, 0x308, 0x500
- */
-static bool pv_icptdata_check_diag(struct vm *vm, int diag)
-{
-	int icptcode;
-
-	switch (diag) {
-	case 0x44:
-	case 0x9c:
-	case 0x288:
-	case 0x308:
-		icptcode = ICPT_PV_NOTIFY;
-		break;
-	case 0x500:
-		icptcode = ICPT_PV_INSTR;
-		break;
-	default:
-		/* If a new diag is introduced add it to the cases above! */
-		assert(0);
-	}
-
-	return vm->sblk->icptcode == icptcode && vm->sblk->ipa == 0x8302 &&
-	       vm->sblk->ipb == 0x50000000 && vm->save_area.guest.grs[5] == diag;
-}
-#endif
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index 53cd767f..6d1a0d6e 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -287,6 +287,18 @@ static inline bool sie_is_pv(struct vm *vm)
 	return vm->sblk->sdf == 2;
 }
 
+/**
+ * sie_is_diag_icpt() - Check if intercept is due to diagnose instruction
+ * @vm: the guest
+ * @diag: the expected diagnose code
+ *
+ * Check that the intercept is due to diagnose @diag and valid.
+ * For protected virtualisation, check that the intercept data meets additional
+ * constraints.
+ *
+ * Returns: true if intercept is due to a valid and has matching diagnose code
+ */
+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/sie.c b/lib/s390x/sie.c
index 0fa915cf..d4ba2a40 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -42,6 +42,59 @@ 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)
+{
+	union {
+		struct {
+			uint64_t     : 16;
+			uint64_t ipa : 16;
+			uint64_t ipb : 32;
+		};
+		struct {
+			uint64_t          : 16;
+			uint64_t opcode   :  8;
+			uint64_t r_1      :  4;
+			uint64_t r_2      :  4;
+			uint64_t r_base   :  4;
+			uint64_t displace : 12;
+			uint64_t zero     : 16;
+		};
+	} instr = { .ipa = vm->sblk->ipa, .ipb = vm->sblk->ipb };
+	uint8_t icptcode;
+	uint64_t code;
+
+	switch (diag) {
+	case 0x44:
+	case 0x9c:
+	case 0x288:
+	case 0x308:
+		icptcode = ICPT_PV_NOTIFY;
+		break;
+	case 0x500:
+		icptcode = ICPT_PV_INSTR;
+		break;
+	default:
+		/* If a new diag is introduced add it to the cases above! */
+		assert_msg(false, "unknown diag");
+	}
+
+	if (sie_is_pv(vm)) {
+		if (instr.r_1 != 0 || instr.r_2 != 2 || instr.r_base != 5)
+			return false;
+		if (instr.displace)
+			return false;
+	} else {
+		icptcode = ICPT_INST;
+	}
+	if (vm->sblk->icptcode != icptcode)
+		return false;
+	if (instr.opcode != 0x83 || instr.zero)
+		return false;
+	code = instr.r_base ? vm->save_area.guest.grs[instr.r_base] : 0;
+	code = (code + instr.displace) & 0xffff;
+	return code == diag;
+}
+
 void sie_handle_validity(struct vm *vm)
 {
 	if (vm->sblk->icptcode != ICPT_VALIDITY)
diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
index 3193ad99..6ebe469a 100644
--- a/s390x/pv-diags.c
+++ b/s390x/pv-diags.c
@@ -9,7 +9,6 @@
  */
 #include <libcflat.h>
 #include <snippet.h>
-#include <pv_icptdata.h>
 #include <sie.h>
 #include <sclp.h>
 #include <asm/facility.h>
@@ -32,8 +31,7 @@ static void test_diag_500(void)
 			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
 
 	sie(&vm);
-	report(pv_icptdata_check_diag(&vm, 0x500),
-	       "intercept values");
+	report(sie_is_diag_icpt(&vm, 0x500), "intercept values");
 	report(vm.save_area.guest.grs[1] == 1 &&
 	       vm.save_area.guest.grs[2] == 2 &&
 	       vm.save_area.guest.grs[3] == 3 &&
@@ -45,7 +43,7 @@ static void test_diag_500(void)
 	 */
 	vm.sblk->iictl = IICTL_CODE_OPERAND;
 	sie(&vm);
-	report(pv_icptdata_check_diag(&vm, 0x9c) &&
+	report(sie_is_diag_icpt(&vm, 0x9c) &&
 	       vm.save_area.guest.grs[0] == PGM_INT_CODE_OPERAND,
 	       "operand exception");
 
@@ -57,7 +55,7 @@ static void test_diag_500(void)
 	vm.sblk->iictl = IICTL_CODE_SPECIFICATION;
 	/* Inject PGM, next exit should be 9c */
 	sie(&vm);
-	report(pv_icptdata_check_diag(&vm, 0x9c) &&
+	report(sie_is_diag_icpt(&vm, 0x9c) &&
 	       vm.save_area.guest.grs[0] == PGM_INT_CODE_SPECIFICATION,
 	       "specification exception");
 
diff --git a/s390x/pv-icptcode.c b/s390x/pv-icptcode.c
index d7c47d6f..bc90df1e 100644
--- a/s390x/pv-icptcode.c
+++ b/s390x/pv-icptcode.c
@@ -13,7 +13,6 @@
 #include <smp.h>
 #include <sclp.h>
 #include <snippet.h>
-#include <pv_icptdata.h>
 #include <asm/facility.h>
 #include <asm/barrier.h>
 #include <asm/sigp.h>
@@ -47,7 +46,7 @@ static void test_validity_timing(void)
 			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
 
 	sie(&vm);
-	report(pv_icptdata_check_diag(&vm, 0x44), "spt done");
+	report(sie_is_diag_icpt(&vm, 0x44), "spt done");
 	stck(&time_exit);
 	tmp = vm.sblk->cputm;
 	mb();
@@ -258,7 +257,7 @@ static void test_validity_asce(void)
 
 	/* Try if we can still do an entry with the correct asce */
 	sie(&vm);
-	report(pv_icptdata_check_diag(&vm, 0x44), "re-entry with valid CR1");
+	report(sie_is_diag_icpt(&vm, 0x44), "re-entry with valid CR1");
 	uv_destroy_guest(&vm);
 	free_pages(pgd_new);
 	report_prefix_pop();
@@ -294,7 +293,7 @@ static void run_icpt_122_tests_prefix(unsigned long prefix)
 
 	sie(&vm);
 	/* Guest indicates that it has been setup via the diag 0x44 */
-	assert(pv_icptdata_check_diag(&vm, 0x44));
+	assert(sie_is_diag_icpt(&vm, 0x44));
 	/* If the pages have not been shared these writes will cause exceptions */
 	ptr = (uint32_t *)prefix;
 	WRITE_ONCE(ptr, 0);
@@ -328,7 +327,7 @@ static void test_icpt_112(void)
 
 	/* Setup of the guest's state for 0x0 prefix */
 	sie(&vm);
-	assert(pv_icptdata_check_diag(&vm, 0x44));
+	assert(sie_is_diag_icpt(&vm, 0x44));
 
 	/* Test on standard 0x0 prefix */
 	run_icpt_122_tests_prefix(0);
@@ -348,7 +347,7 @@ static void test_icpt_112(void)
 
 	/* Try a re-entry after everything has been imported again */
 	sie(&vm);
-	report(pv_icptdata_check_diag(&vm, 0x9c) &&
+	report(sie_is_diag_icpt(&vm, 0x9c) &&
 	       vm.save_area.guest.grs[0] == 42,
 	       "re-entry successful");
 	report_prefix_pop();
diff --git a/s390x/pv-ipl.c b/s390x/pv-ipl.c
index cc46e7f7..cd49bd95 100644
--- a/s390x/pv-ipl.c
+++ b/s390x/pv-ipl.c
@@ -11,7 +11,6 @@
 #include <sie.h>
 #include <sclp.h>
 #include <snippet.h>
-#include <pv_icptdata.h>
 #include <asm/facility.h>
 #include <asm/uv.h>
 
@@ -35,7 +34,7 @@ static void test_diag_308(int subcode)
 
 	/* First exit is a diag 0x500 */
 	sie(&vm);
-	assert(pv_icptdata_check_diag(&vm, 0x500));
+	assert(sie_is_diag_icpt(&vm, 0x500));
 
 	/*
 	 * The snippet asked us for the subcode and we answer by
@@ -46,7 +45,7 @@ static void test_diag_308(int subcode)
 
 	/* Continue after diag 0x500, next icpt should be the 0x308 */
 	sie(&vm);
-	assert(pv_icptdata_check_diag(&vm, 0x308));
+	assert(sie_is_diag_icpt(&vm, 0x308));
 	assert(vm.save_area.guest.grs[2] == subcode);
 
 	/*
@@ -118,7 +117,7 @@ static void test_diag_308(int subcode)
 	 * see a diagnose 0x9c PV instruction notification.
 	 */
 	sie(&vm);
-	report(pv_icptdata_check_diag(&vm, 0x9c) &&
+	report(sie_is_diag_icpt(&vm, 0x9c) &&
 	       vm.save_area.guest.grs[0] == 42,
 	       "continue after load");
 
-- 
2.44.0

[kvm-unit-tests PATCH v3 5/7] 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 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)
+	);
+}
+
 #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 */
+}
+
+#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);
+	} else {
+		report_fail("guest forced exit with value");
+	}
+}
diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
index 723bb4f2..c9fffbfc 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 6ebe469a..a686c688 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 <sie.h>
 #include <sclp.h>
 #include <asm/facility.h>
diff --git a/s390x/pv-icptcode.c b/s390x/pv-icptcode.c
index bc90df1e..82fd4246 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 <asm/facility.h>
 #include <asm/barrier.h>
 #include <asm/sigp.h>
diff --git a/s390x/pv-ipl.c b/s390x/pv-ipl.c
index cd49bd95..2acf7d0a 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 <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.44.0

[kvm-unit-tests PATCH v3 6/7] s390x: Use library functions for snippet exit

[Nina Schoetterl-Glausch]

Replace the existing code for exiting from snippets with the newly
introduced library functionality.

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

diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
index 9e60f26e..c8f38220 100644
--- a/s390x/sie-dat.c
+++ b/s390x/sie-dat.c
@@ -27,23 +27,18 @@ 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_is_force_exit_value(&vm));
+	test_page_gpa = snippet_get_force_exit_value(&vm);
 	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_is_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 9d89801d..26f045b1 100644
--- a/s390x/snippets/c/sie-dat.c
+++ b/s390x/snippets/c/sie-dat.c
@@ -10,28 +10,11 @@
 #include <libcflat.h>
 #include <asm-generic/page.h>
 #include <asm/mem.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.44.0

[kvm-unit-tests PATCH v3 7/7] 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        | 134 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg      |   3 +
 5 files changed, 174 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 12445fb5..7c38d66a 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -44,6 +44,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
@@ -129,6 +130,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..a3e7f1c9
--- /dev/null
+++ b/s390x/stfle-sie.c
@@ -0,0 +1,134 @@
+/* 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>
+#include <rand.h>
+
+static struct vm vm;
+static uint64_t (*fac)[PAGE_SIZE / sizeof(uint64_t)];
+static prng_state prng_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_is_force_exit_value(&vm));
+	guest_stfle_addr = snippet_get_force_exit_value(&vm);
+	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], prng64(&prng_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("PRNG seed: 0x%lx", args.seed);
+	prng_s = prng_init(args.seed);
+	setup_guest();
+	if (test_facility(7))
+		test_stfle_format_0();
+out:
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 3a9decc9..f2203069 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -392,3 +392,6 @@ file = sie-dat.elf
 
 [pv-attest]
 file = pv-attest.elf
+
+[stfle-sie]
+file = stfle-sie.elf
-- 
2.44.0

Re: [kvm-unit-tests PATCH v3 3/7] s390x: Add sie_is_pv

[Claudio Imbrenda]

On Thu, 20 Jun 2024 16:16:56 +0200
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> Add a function to check if a guest VM is currently running protected.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

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

> ---
>  lib/s390x/sie.h | 6 ++++++
>  lib/s390x/sie.c | 4 ++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index c1724cf2..53cd767f 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -281,6 +281,12 @@ 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);
> +
> +static inline bool sie_is_pv(struct vm *vm)
> +{
> +	return vm->sblk->sdf == 2;
> +}
> +
>  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/sie.c b/lib/s390x/sie.c
> index 40936bd2..0fa915cf 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -59,7 +59,7 @@ void sie(struct vm *vm)
>  	/* When a pgm int code is set, we'll never enter SIE below. */
>  	assert(!read_pgm_int_code());
>  
> -	if (vm->sblk->sdf == 2)
> +	if (sie_is_pv(vm))
>  		memcpy(vm->sblk->pv_grregs, vm->save_area.guest.grs,
>  		       sizeof(vm->save_area.guest.grs));
>  
> @@ -98,7 +98,7 @@ void sie(struct vm *vm)
>  	/* restore the old CR 13 */
>  	lctlg(13, old_cr13);
>  
> -	if (vm->sblk->sdf == 2)
> +	if (sie_is_pv(vm))
>  		memcpy(vm->save_area.guest.grs, vm->sblk->pv_grregs,
>  		       sizeof(vm->save_area.guest.grs));
>  }

Re: [kvm-unit-tests PATCH v3 4/7] s390x: Add function for checking diagnose intercepts

[Claudio Imbrenda]

On Thu, 20 Jun 2024 16:16:57 +0200
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> sie_is_diag_icpt() checks if the intercept is due to an expected
> diagnose call and is valid.
> It subsumes pv_icptdata_check_diag.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>


[...]


> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index 0fa915cf..d4ba2a40 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -42,6 +42,59 @@ 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)
> +{
> +	union {
> +		struct {
> +			uint64_t     : 16;
> +			uint64_t ipa : 16;
> +			uint64_t ipb : 32;
> +		};
> +		struct {
> +			uint64_t          : 16;
> +			uint64_t opcode   :  8;
> +			uint64_t r_1      :  4;
> +			uint64_t r_2      :  4;
> +			uint64_t r_base   :  4;
> +			uint64_t displace : 12;
> +			uint64_t zero     : 16;
> +		};
> +	} instr = { .ipa = vm->sblk->ipa, .ipb = vm->sblk->ipb };
> +	uint8_t icptcode;
> +	uint64_t code;
> +
> +	switch (diag) {
> +	case 0x44:
> +	case 0x9c:
> +	case 0x288:
> +	case 0x308:
> +		icptcode = ICPT_PV_NOTIFY;
> +		break;
> +	case 0x500:
> +		icptcode = ICPT_PV_INSTR;
> +		break;
> +	default:
> +		/* If a new diag is introduced add it to the cases above! */
> +		assert_msg(false, "unknown diag");

just a nit, but would it be possible to also print the diag number that
causes the error?


otherwise looks good

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

[Claudio Imbrenda]

On Thu, 20 Jun 2024 16:16:58 +0200
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 snippet.h to reflect
> that it is host specific.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>


[...]


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

[...]

> +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 */
> +}

why not adding "memory" to the clobbers of the inline asm? (not a big
deal, I'm just curious if there is a specific reason for an explicit
mb())


[...]

Re: [kvm-unit-tests PATCH v3 6/7] s390x: Use library functions for snippet exit

[Claudio Imbrenda]

On Thu, 20 Jun 2024 16:16:59 +0200
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> Replace the existing code for exiting from snippets with the newly
> introduced library functionality.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

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

> ---
>  s390x/sie-dat.c            | 11 +++--------
>  s390x/snippets/c/sie-dat.c | 19 +------------------
>  2 files changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> index 9e60f26e..c8f38220 100644
> --- a/s390x/sie-dat.c
> +++ b/s390x/sie-dat.c
> @@ -27,23 +27,18 @@ 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_is_force_exit_value(&vm));
> +	test_page_gpa = snippet_get_force_exit_value(&vm);
>  	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_is_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 9d89801d..26f045b1 100644
> --- a/s390x/snippets/c/sie-dat.c
> +++ b/s390x/snippets/c/sie-dat.c
> @@ -10,28 +10,11 @@
>  #include <libcflat.h>
>  #include <asm-generic/page.h>
>  #include <asm/mem.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 v3 5/7] s390x: Add library functions for exiting from snippet

[Nina Schoetterl-Glausch]

On Thu, 2024-06-20 at 18:55 +0200, Claudio Imbrenda wrote:
> On Thu, 20 Jun 2024 16:16:58 +0200
> 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 snippet.h to reflect
> > that it is host specific.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> 
> [...]
> 
> 
> > +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
> 
> [...]
> 
> > +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 */
> > +}
> 
> why not adding "memory" to the clobbers of the inline asm? (not a big
> deal, I'm just curious if there is a specific reason for an explicit
> mb())

Mostly a matter of taste I guess.
The diag functions are just convenience wrappers, doing nothing but
executing the diag.
force_exit is a protocol between the host and guest that uses the diags
and adds additional semantics on top.
In theory you could have other use cases where the diags are just a timeslice yield.
> 
> 
> [...]

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

[Claudio Imbrenda]

On Thu, 20 Jun 2024 16:17:00 +0200
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>


[...]


> +struct guest_stfle_res {
> +	uint16_t len;
> +	uint64_t reg;

you don't really use reg, do you?

> +	unsigned char *mem;
> +};
> +
> +static struct guest_stfle_res run_guest(void)
> +{
> +	struct guest_stfle_res res;
> +	uint64_t guest_stfle_addr;

uint64_t tmp;

> +
> +	sie(&vm);
> +	assert(snippet_is_force_exit_value(&vm));
> +	guest_stfle_addr = snippet_get_force_exit_value(&vm);
> +	res.mem = &vm.guest_mem[guest_stfle_addr];
> +	memcpy(&res.reg, res.mem, sizeof(res.reg));

memcpy(&tmp, res.mem, etc);

> +	res.len = (res.reg & 0xff) + 1;

(tmp & 0xff) + 1

etc 

> +	res.mem += sizeof(res.reg);

(you could just do res.mem++ if you had declared it as uint64_t *
instead of unsigned char *)

> +	return res;
> +}
> +


[...]


> +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("PRNG seed: 0x%lx", args.seed);
> +	prng_s = prng_init(args.seed);
> +	setup_guest();
> +	if (test_facility(7))
> +		test_stfle_format_0();

since STFLE is literally the feature you are testing, maybe you can
just skip, like you did for SIEF2?

> +out:
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3a9decc9..f2203069 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -392,3 +392,6 @@ file = sie-dat.elf
>  
>  [pv-attest]
>  file = pv-attest.elf
> +
> +[stfle-sie]
> +file = stfle-sie.elf

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

[Claudio Imbrenda]

On Thu, 20 Jun 2024 19:16:05 +0200
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> On Thu, 2024-06-20 at 18:55 +0200, Claudio Imbrenda wrote:
> > On Thu, 20 Jun 2024 16:16:58 +0200
> > 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 snippet.h to reflect
> > > that it is host specific.
> > > 
> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>  
> > 
> > 
> > [...]
> > 
> >   
> > > +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  
> > 
> > [...]
> >   
> > > +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 */
> > > +}  
> > 
> > why not adding "memory" to the clobbers of the inline asm? (not a big
> > deal, I'm just curious if there is a specific reason for an explicit
> > mb())  
> 
> Mostly a matter of taste I guess.
> The diag functions are just convenience wrappers, doing nothing but
> executing the diag.
> force_exit is a protocol between the host and guest that uses the diags
> and adds additional semantics on top.
> In theory you could have other use cases where the diags are just a timeslice yield.

fair enough

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

> > 
> > 
> > [...]  
> 

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

[Nina Schoetterl-Glausch]

On Thu, 2024-06-20 at 19:25 +0200, Claudio Imbrenda wrote:
> On Thu, 20 Jun 2024 16:17:00 +0200
> 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>
> 
> 
> [...]
> 
> 
> > +struct guest_stfle_res {
> > +	uint16_t len;
> > +	uint64_t reg;
> 
> you don't really use reg, do you?

No, and I don't think I will either, must be some vestige.

[...]
> 
> > +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("PRNG seed: 0x%lx", args.seed);
> > +	prng_s = prng_init(args.seed);
> > +	setup_guest();
> > +	if (test_facility(7))
> > +		test_stfle_format_0();
> 
> since STFLE is literally the feature you are testing, maybe you can
> just skip, like you did for SIEF2?

Yeah, that's better.
> 
> > +out:
> > +	return report_summary();
> > +}
> > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > index 3a9decc9..f2203069 100644
> > --- a/s390x/unittests.cfg
> > +++ b/s390x/unittests.cfg
> > @@ -392,3 +392,6 @@ file = sie-dat.elf
> >  
> >  [pv-attest]
> >  file = pv-attest.elf
> > +
> > +[stfle-sie]
> > +file = stfle-sie.elf
> 

Re: [kvm-unit-tests PATCH v3 4/7] s390x: Add function for checking diagnose intercepts

[Nina Schoetterl-Glausch]

On Thu, 2024-06-20 at 18:47 +0200, Claudio Imbrenda wrote:
> On Thu, 20 Jun 2024 16:16:57 +0200
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> > sie_is_diag_icpt() checks if the intercept is due to an expected
> > diagnose call and is valid.
> > It subsumes pv_icptdata_check_diag.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> 
> [...]
> 
> 
> > diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> > index 0fa915cf..d4ba2a40 100644
> > --- a/lib/s390x/sie.c
> > +++ b/lib/s390x/sie.c
> > @@ -42,6 +42,59 @@ 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)
> > +{
> > +	union {
> > +		struct {
> > +			uint64_t     : 16;
> > +			uint64_t ipa : 16;
> > +			uint64_t ipb : 32;
> > +		};
> > +		struct {
> > +			uint64_t          : 16;
> > +			uint64_t opcode   :  8;
> > +			uint64_t r_1      :  4;
> > +			uint64_t r_2      :  4;
> > +			uint64_t r_base   :  4;
> > +			uint64_t displace : 12;
> > +			uint64_t zero     : 16;
> > +		};
> > +	} instr = { .ipa = vm->sblk->ipa, .ipb = vm->sblk->ipb };
> > +	uint8_t icptcode;
> > +	uint64_t code;
> > +
> > +	switch (diag) {
> > +	case 0x44:
> > +	case 0x9c:
> > +	case 0x288:
> > +	case 0x308:
> > +		icptcode = ICPT_PV_NOTIFY;
> > +		break;
> > +	case 0x500:
> > +		icptcode = ICPT_PV_INSTR;
> > +		break;
> > +	default:
> > +		/* If a new diag is introduced add it to the cases above! */
> > +		assert_msg(false, "unknown diag");
> 
> just a nit, but would it be possible to also print the diag number that
> causes the error?

Yes and easy
		assert_msg(false, "unknown diag 0x%x", diag);
> 
> 
> otherwise looks good
> 
> 

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

[Janosch Frank]

On 6/20/24 16:16, Nina Schoetterl-Glausch wrote:
> libcflat.h was included twice.
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Janosch Frank <frankja@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>

Re: [kvm-unit-tests PATCH v3 3/7] s390x: Add sie_is_pv

[Janosch Frank]

On 6/20/24 16:16, Nina Schoetterl-Glausch wrote:
> Add a function to check if a guest VM is currently running protected.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Re: [kvm-unit-tests PATCH v3 4/7] s390x: Add function for checking diagnose intercepts

[Janosch Frank]

On 6/20/24 16:16, Nina Schoetterl-Glausch wrote:
> sie_is_diag_icpt() checks if the intercept is due to an expected
> diagnose call and is valid.
> It subsumes pv_icptdata_check_diag.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>   lib/s390x/pv_icptdata.h | 42 --------------------------------
>   lib/s390x/sie.h         | 12 ++++++++++
>   lib/s390x/sie.c         | 53 +++++++++++++++++++++++++++++++++++++++++
>   s390x/pv-diags.c        |  8 +++----
>   s390x/pv-icptcode.c     | 11 ++++-----
>   s390x/pv-ipl.c          |  7 +++---
>   6 files changed, 76 insertions(+), 57 deletions(-)
>   delete mode 100644 lib/s390x/pv_icptdata.h
> 
> diff --git a/lib/s390x/pv_icptdata.h b/lib/s390x/pv_icptdata.h
> deleted file mode 100644
> index 4746117e..00000000
> --- a/lib/s390x/pv_icptdata.h
> +++ /dev/null
> @@ -1,42 +0,0 @@

There's a reason why I didn't put this in sie.c and I'm still torn on 
whether this should be in the lib or in s390x. It's not related to 
actually running snippets and managing them.

> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Commonly used checks for PV SIE intercept data
> - *
> - * Copyright IBM Corp. 2023
> - * Author: Janosch Frank <frankja@linux.ibm.com>
> - */
[...]
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index 53cd767f..6d1a0d6e 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -287,6 +287,18 @@ static inline bool sie_is_pv(struct vm *vm)
>   	return vm->sblk->sdf == 2;
>   }
>   
> +/**
> + * sie_is_diag_icpt() - Check if intercept is due to diagnose instruction
> + * @vm: the guest
> + * @diag: the expected diagnose code
> + *
> + * Check that the intercept is due to diagnose @diag and valid.
> + * For protected virtualisation, check that the intercept data meets additional

virtualization

> + * constraints.
> + *
> + * Returns: true if intercept is due to a valid and has matching diagnose code
> + */
> +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/sie.c b/lib/s390x/sie.c

Re: [kvm-unit-tests PATCH v3 3/7] s390x: Add sie_is_pv

[Nicholas Piggin]

On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:
> Add a function to check if a guest VM is currently running protected.
>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  lib/s390x/sie.h | 6 ++++++
>  lib/s390x/sie.c | 4 ++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index c1724cf2..53cd767f 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -281,6 +281,12 @@ 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);
> +
> +static inline bool sie_is_pv(struct vm *vm)
> +{
> +	return vm->sblk->sdf == 2;
> +}
> +
>  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/sie.c b/lib/s390x/sie.c
> index 40936bd2..0fa915cf 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -59,7 +59,7 @@ void sie(struct vm *vm)
>  	/* When a pgm int code is set, we'll never enter SIE below. */
>  	assert(!read_pgm_int_code());
>  
> -	if (vm->sblk->sdf == 2)
> +	if (sie_is_pv(vm))
>  		memcpy(vm->sblk->pv_grregs, vm->save_area.guest.grs,
>  		       sizeof(vm->save_area.guest.grs));
>  
> @@ -98,7 +98,7 @@ void sie(struct vm *vm)
>  	/* restore the old CR 13 */
>  	lctlg(13, old_cr13);
>  
> -	if (vm->sblk->sdf == 2)
> +	if (sie_is_pv(vm))
>  		memcpy(vm->save_area.guest.grs, vm->sblk->pv_grregs,
>  		       sizeof(vm->save_area.guest.grs));
>  }

Re: [kvm-unit-tests PATCH v3 4/7] s390x: Add function for checking diagnose intercepts

[Nicholas Piggin]

On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:
> sie_is_diag_icpt() checks if the intercept is due to an expected
> diagnose call and is valid.
> It subsumes pv_icptdata_check_diag.
>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>  lib/s390x/pv_icptdata.h | 42 --------------------------------
>  lib/s390x/sie.h         | 12 ++++++++++
>  lib/s390x/sie.c         | 53 +++++++++++++++++++++++++++++++++++++++++
>  s390x/pv-diags.c        |  8 +++----
>  s390x/pv-icptcode.c     | 11 ++++-----
>  s390x/pv-ipl.c          |  7 +++---
>  6 files changed, 76 insertions(+), 57 deletions(-)
>  delete mode 100644 lib/s390x/pv_icptdata.h
>
> diff --git a/lib/s390x/pv_icptdata.h b/lib/s390x/pv_icptdata.h
> deleted file mode 100644
> index 4746117e..00000000
> --- a/lib/s390x/pv_icptdata.h
> +++ /dev/null
> @@ -1,42 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Commonly used checks for PV SIE intercept data
> - *
> - * Copyright IBM Corp. 2023
> - * Author: Janosch Frank <frankja@linux.ibm.com>
> - */
> -
> -#ifndef _S390X_PV_ICPTDATA_H_
> -#define _S390X_PV_ICPTDATA_H_
> -
> -#include <sie.h>
> -
> -/*
> - * Checks the diagnose instruction intercept data for consistency with
> - * the constants defined by the PV SIE architecture
> - *
> - * Supports: 0x44, 0x9c, 0x288, 0x308, 0x500
> - */
> -static bool pv_icptdata_check_diag(struct vm *vm, int diag)
> -{
> -	int icptcode;
> -
> -	switch (diag) {
> -	case 0x44:
> -	case 0x9c:
> -	case 0x288:
> -	case 0x308:
> -		icptcode = ICPT_PV_NOTIFY;
> -		break;
> -	case 0x500:
> -		icptcode = ICPT_PV_INSTR;
> -		break;
> -	default:
> -		/* If a new diag is introduced add it to the cases above! */
> -		assert(0);
> -	}
> -
> -	return vm->sblk->icptcode == icptcode && vm->sblk->ipa == 0x8302 &&
> -	       vm->sblk->ipb == 0x50000000 && vm->save_area.guest.grs[5] == diag;
> -}
> -#endif
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index 53cd767f..6d1a0d6e 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -287,6 +287,18 @@ static inline bool sie_is_pv(struct vm *vm)
>  	return vm->sblk->sdf == 2;
>  }
>  
> +/**
> + * sie_is_diag_icpt() - Check if intercept is due to diagnose instruction
> + * @vm: the guest
> + * @diag: the expected diagnose code
> + *
> + * Check that the intercept is due to diagnose @diag and valid.
> + * For protected virtualisation, check that the intercept data meets additional
> + * constraints.
> + *
> + * Returns: true if intercept is due to a valid and has matching diagnose code
> + */
> +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/sie.c b/lib/s390x/sie.c
> index 0fa915cf..d4ba2a40 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -42,6 +42,59 @@ 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)
> +{
> +	union {
> +		struct {
> +			uint64_t     : 16;
> +			uint64_t ipa : 16;
> +			uint64_t ipb : 32;
> +		};
> +		struct {
> +			uint64_t          : 16;
> +			uint64_t opcode   :  8;
> +			uint64_t r_1      :  4;
> +			uint64_t r_2      :  4;
> +			uint64_t r_base   :  4;
> +			uint64_t displace : 12;
> +			uint64_t zero     : 16;
> +		};
> +	} instr = { .ipa = vm->sblk->ipa, .ipb = vm->sblk->ipb };
> +	uint8_t icptcode;
> +	uint64_t code;
> +
> +	switch (diag) {
> +	case 0x44:
> +	case 0x9c:
> +	case 0x288:
> +	case 0x308:
> +		icptcode = ICPT_PV_NOTIFY;
> +		break;
> +	case 0x500:
> +		icptcode = ICPT_PV_INSTR;
> +		break;
> +	default:
> +		/* If a new diag is introduced add it to the cases above! */
> +		assert_msg(false, "unknown diag");
> +	}
> +
> +	if (sie_is_pv(vm)) {
> +		if (instr.r_1 != 0 || instr.r_2 != 2 || instr.r_base != 5)
> +			return false;
> +		if (instr.displace)
> +			return false;
> +	} else {
> +		icptcode = ICPT_INST;
> +	}
> +	if (vm->sblk->icptcode != icptcode)
> +		return false;
> +	if (instr.opcode != 0x83 || instr.zero)
> +		return false;
> +	code = instr.r_base ? vm->save_area.guest.grs[instr.r_base] : 0;
> +	code = (code + instr.displace) & 0xffff;
> +	return code == diag;
> +}

It looks like this transformation is equivalent for the PV case. You
could put the switch into the sie_is_pv() branch? Otherwise looks okay.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> +
>  void sie_handle_validity(struct vm *vm)
>  {
>  	if (vm->sblk->icptcode != ICPT_VALIDITY)
> diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
> index 3193ad99..6ebe469a 100644
> --- a/s390x/pv-diags.c
> +++ b/s390x/pv-diags.c
> @@ -9,7 +9,6 @@
>   */
>  #include <libcflat.h>
>  #include <snippet.h>
> -#include <pv_icptdata.h>
>  #include <sie.h>
>  #include <sclp.h>
>  #include <asm/facility.h>
> @@ -32,8 +31,7 @@ static void test_diag_500(void)
>  			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
>  
>  	sie(&vm);
> -	report(pv_icptdata_check_diag(&vm, 0x500),
> -	       "intercept values");
> +	report(sie_is_diag_icpt(&vm, 0x500), "intercept values");
>  	report(vm.save_area.guest.grs[1] == 1 &&
>  	       vm.save_area.guest.grs[2] == 2 &&
>  	       vm.save_area.guest.grs[3] == 3 &&
> @@ -45,7 +43,7 @@ static void test_diag_500(void)
>  	 */
>  	vm.sblk->iictl = IICTL_CODE_OPERAND;
>  	sie(&vm);
> -	report(pv_icptdata_check_diag(&vm, 0x9c) &&
> +	report(sie_is_diag_icpt(&vm, 0x9c) &&
>  	       vm.save_area.guest.grs[0] == PGM_INT_CODE_OPERAND,
>  	       "operand exception");
>  
> @@ -57,7 +55,7 @@ static void test_diag_500(void)
>  	vm.sblk->iictl = IICTL_CODE_SPECIFICATION;
>  	/* Inject PGM, next exit should be 9c */
>  	sie(&vm);
> -	report(pv_icptdata_check_diag(&vm, 0x9c) &&
> +	report(sie_is_diag_icpt(&vm, 0x9c) &&
>  	       vm.save_area.guest.grs[0] == PGM_INT_CODE_SPECIFICATION,
>  	       "specification exception");
>  
> diff --git a/s390x/pv-icptcode.c b/s390x/pv-icptcode.c
> index d7c47d6f..bc90df1e 100644
> --- a/s390x/pv-icptcode.c
> +++ b/s390x/pv-icptcode.c
> @@ -13,7 +13,6 @@
>  #include <smp.h>
>  #include <sclp.h>
>  #include <snippet.h>
> -#include <pv_icptdata.h>
>  #include <asm/facility.h>
>  #include <asm/barrier.h>
>  #include <asm/sigp.h>
> @@ -47,7 +46,7 @@ static void test_validity_timing(void)
>  			size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
>  
>  	sie(&vm);
> -	report(pv_icptdata_check_diag(&vm, 0x44), "spt done");
> +	report(sie_is_diag_icpt(&vm, 0x44), "spt done");
>  	stck(&time_exit);
>  	tmp = vm.sblk->cputm;
>  	mb();
> @@ -258,7 +257,7 @@ static void test_validity_asce(void)
>  
>  	/* Try if we can still do an entry with the correct asce */
>  	sie(&vm);
> -	report(pv_icptdata_check_diag(&vm, 0x44), "re-entry with valid CR1");
> +	report(sie_is_diag_icpt(&vm, 0x44), "re-entry with valid CR1");
>  	uv_destroy_guest(&vm);
>  	free_pages(pgd_new);
>  	report_prefix_pop();
> @@ -294,7 +293,7 @@ static void run_icpt_122_tests_prefix(unsigned long prefix)
>  
>  	sie(&vm);
>  	/* Guest indicates that it has been setup via the diag 0x44 */
> -	assert(pv_icptdata_check_diag(&vm, 0x44));
> +	assert(sie_is_diag_icpt(&vm, 0x44));
>  	/* If the pages have not been shared these writes will cause exceptions */
>  	ptr = (uint32_t *)prefix;
>  	WRITE_ONCE(ptr, 0);
> @@ -328,7 +327,7 @@ static void test_icpt_112(void)
>  
>  	/* Setup of the guest's state for 0x0 prefix */
>  	sie(&vm);
> -	assert(pv_icptdata_check_diag(&vm, 0x44));
> +	assert(sie_is_diag_icpt(&vm, 0x44));
>  
>  	/* Test on standard 0x0 prefix */
>  	run_icpt_122_tests_prefix(0);
> @@ -348,7 +347,7 @@ static void test_icpt_112(void)
>  
>  	/* Try a re-entry after everything has been imported again */
>  	sie(&vm);
> -	report(pv_icptdata_check_diag(&vm, 0x9c) &&
> +	report(sie_is_diag_icpt(&vm, 0x9c) &&
>  	       vm.save_area.guest.grs[0] == 42,
>  	       "re-entry successful");
>  	report_prefix_pop();
> diff --git a/s390x/pv-ipl.c b/s390x/pv-ipl.c
> index cc46e7f7..cd49bd95 100644
> --- a/s390x/pv-ipl.c
> +++ b/s390x/pv-ipl.c
> @@ -11,7 +11,6 @@
>  #include <sie.h>
>  #include <sclp.h>
>  #include <snippet.h>
> -#include <pv_icptdata.h>
>  #include <asm/facility.h>
>  #include <asm/uv.h>
>  
> @@ -35,7 +34,7 @@ static void test_diag_308(int subcode)
>  
>  	/* First exit is a diag 0x500 */
>  	sie(&vm);
> -	assert(pv_icptdata_check_diag(&vm, 0x500));
> +	assert(sie_is_diag_icpt(&vm, 0x500));
>  
>  	/*
>  	 * The snippet asked us for the subcode and we answer by
> @@ -46,7 +45,7 @@ static void test_diag_308(int subcode)
>  
>  	/* Continue after diag 0x500, next icpt should be the 0x308 */
>  	sie(&vm);
> -	assert(pv_icptdata_check_diag(&vm, 0x308));
> +	assert(sie_is_diag_icpt(&vm, 0x308));
>  	assert(vm.save_area.guest.grs[2] == subcode);
>  
>  	/*
> @@ -118,7 +117,7 @@ static void test_diag_308(int subcode)
>  	 * see a diagnose 0x9c PV instruction notification.
>  	 */
>  	sie(&vm);
> -	report(pv_icptdata_check_diag(&vm, 0x9c) &&
> +	report(sie_is_diag_icpt(&vm, 0x9c) &&
>  	       vm.save_area.guest.grs[0] == 42,
>  	       "continue after load");
>  

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

[Nicholas Piggin]

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

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

[Nicholas Piggin]

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)
> +	);
> +}
> +
>  #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 */
> +}
> +
> +#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];

Hmm, you have a nice instr struct that you made earlier and now you're
back to mask and shift... What about exposing that struct and add a
function to create it so you could do grs[sblk_to_instr(sblk).r1]
here... Just a thought.

Thanks,
Nick

Re: [kvm-unit-tests PATCH v3 6/7] s390x: Use library functions for snippet exit

[Nicholas Piggin]

On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:
> Replace the existing code for exiting from snippets with the newly
> introduced library functionality.
>

Is like for like AFAIKS

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>  s390x/sie-dat.c            | 11 +++--------
>  s390x/snippets/c/sie-dat.c | 19 +------------------
>  2 files changed, 4 insertions(+), 26 deletions(-)
>
> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> index 9e60f26e..c8f38220 100644
> --- a/s390x/sie-dat.c
> +++ b/s390x/sie-dat.c
> @@ -27,23 +27,18 @@ 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_is_force_exit_value(&vm));
> +	test_page_gpa = snippet_get_force_exit_value(&vm);
>  	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_is_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 9d89801d..26f045b1 100644
> --- a/s390x/snippets/c/sie-dat.c
> +++ b/s390x/snippets/c/sie-dat.c
> @@ -10,28 +10,11 @@
>  #include <libcflat.h>
>  #include <asm-generic/page.h>
>  #include <asm/mem.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 v3 1/7] lib: Add pseudo random functions

[Nicholas Piggin]

On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:
> Add functions for generating pseudo random 32 and 64 bit values.
> The implementation uses SHA-256 and so the randomness should have good
> quality.
> Implement the necessary subset of SHA-256.
> The PRNG algorithm is equivalent to the following python snippet:
>
> def prng32(seed):
>     from hashlib import sha256
>     state = seed.to_bytes(8, byteorder="big")
>     while True:
>         state = sha256(state).digest()
>         for i in range(8):
>             yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")
>

Nice, could use this for powerpc radom SPR value tests (and
probably other things too).

> Acked-by: Andrew Jones <andrew.jones@linux.dev>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>
> Notes:
>     Since a PRNG with better quality was asked for I decided to use SHA-256
>     because:
>      * it is a standard, commonly used algorithm
>      * high quality randomness is assured
>      * the implementation can be checked against the spec
>      * the implementation can be easily checked via comparison
>     
>     I tested the implementation in the following way:
>     
>     cat <<'EOF' > rand.py
>     #!/usr/bin/python3
>     
>     def prng32(seed):
>         from hashlib import sha256
>         state = seed.to_bytes(8, byteorder="big")
>         while True:
>             state = sha256(state).digest()
>             for i in range(8):
>                 yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")
>     
>     r = prng32(0)
>     for i in range(100):
>         print(f"{next(r):08x}")
>     
>     EOF
>     
>     cat <<'EOF' > rand.c
>     #include <stdio.h>
>     #include "rand.h"
>     
>     void main(void)
>     {
>     	prng_state state = prng_init(0);
>     	for (int i = 0; i < 100; i++) {
>     		printf("%08x\n", prng32(&state));
>     	}
>     }
>     EOF
>     cat <<'EOF' > libcflat.h
>     #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
>     EOF
>     chmod +x rand.py
>     ln -s lib/rand.c librand.c
>     gcc -Ilib librand.c rand.c
>     diff <(./a.out) <(./rand.py)

Cool... you made a unit test for the unit tests. We could start a
make check? :)

Acked-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Nick

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

[Nicholas Piggin]

On Fri Jun 21, 2024 at 12:17 AM AEST, Nina Schoetterl-Glausch 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>

This one is beyond me... one minor thing, you could move the
prng patch to just before this one.

> +++ b/s390x/snippets/c/stfle.c
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright IBM Corp. 2023

> +++ b/s390x/stfle-sie.c
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright IBM Corp. 2023

Time to flip your calendar page? :)

Thanks,
Nick

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

[Nicholas Piggin]

On Fri Jun 21, 2024 at 3:26 AM AEST, Claudio Imbrenda wrote:
> On Thu, 20 Jun 2024 19:16:05 +0200
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
>
> > On Thu, 2024-06-20 at 18:55 +0200, Claudio Imbrenda wrote:
> > > On Thu, 20 Jun 2024 16:16:58 +0200
> > > 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 snippet.h to reflect
> > > > that it is host specific.
> > > > 
> > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>  
> > > 
> > > 
> > > [...]
> > > 
> > >   
> > > > +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  
> > > 
> > > [...]
> > >   
> > > > +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 */
> > > > +}  
> > > 
> > > why not adding "memory" to the clobbers of the inline asm? (not a big
> > > deal, I'm just curious if there is a specific reason for an explicit
> > > mb())  
> > 
> > Mostly a matter of taste I guess.
> > The diag functions are just convenience wrappers, doing nothing but
> > executing the diag.
> > force_exit is a protocol between the host and guest that uses the diags
> > and adds additional semantics on top.
> > In theory you could have other use cases where the diags are just a timeslice yield.
>
> fair enough

Sorry I missed these comments. I still had a question about the
barriers (e.g., one needed before the diag()?)

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

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

[Nina Schoetterl-Glausch]

On Tue, 2024-06-25 at 13:08 +1000, Nicholas Piggin wrote:
> On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:

[...]

> >     I tested the implementation in the following way:
> >     
> >     cat <<'EOF' > rand.py
> >     #!/usr/bin/python3
> >     
> >     def prng32(seed):
> >         from hashlib import sha256
> >         state = seed.to_bytes(8, byteorder="big")
> >         while True:
> >             state = sha256(state).digest()
> >             for i in range(8):
> >                 yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")
> >     
> >     r = prng32(0)
> >     for i in range(100):
> >         print(f"{next(r):08x}")
> >     
> >     EOF
> >     
> >     cat <<'EOF' > rand.c
> >     #include <stdio.h>
> >     #include "rand.h"
> >     
> >     void main(void)
> >     {
> >     	prng_state state = prng_init(0);
> >     	for (int i = 0; i < 100; i++) {
> >     		printf("%08x\n", prng32(&state));
> >     	}
> >     }
> >     EOF
> >     cat <<'EOF' > libcflat.h
> >     #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
> >     EOF
> >     chmod +x rand.py
> >     ln -s lib/rand.c librand.c
> >     gcc -Ilib librand.c rand.c
> >     diff <(./a.out) <(./rand.py)
> 
> Cool... you made a unit test for the unit tests. We could start a
> make check? :)

I wouldn't complain about it, but my test is a bit hacky and I don't
expect the code to get touched much.
> 
> Acked-by: Nicholas Piggin <npiggin@gmail.com>
> 
> Thanks,
> Nick

Re: [kvm-unit-tests PATCH v3 4/7] s390x: Add function for checking diagnose intercepts

[Nina Schoetterl-Glausch]

On Tue, 2024-06-25 at 12:14 +1000, Nicholas Piggin wrote:
> On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:
> > sie_is_diag_icpt() checks if the intercept is due to an expected
> > diagnose call and is valid.
> > It subsumes pv_icptdata_check_diag.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> >  lib/s390x/pv_icptdata.h | 42 --------------------------------
> >  lib/s390x/sie.h         | 12 ++++++++++
> >  lib/s390x/sie.c         | 53 +++++++++++++++++++++++++++++++++++++++++
> >  s390x/pv-diags.c        |  8 +++----
> >  s390x/pv-icptcode.c     | 11 ++++-----
> >  s390x/pv-ipl.c          |  7 +++---
> >  6 files changed, 76 insertions(+), 57 deletions(-)
> >  delete mode 100644 lib/s390x/pv_icptdata.h

[...]

> > +bool sie_is_diag_icpt(struct vm *vm, unsigned int diag)
> > +{
> > +	union {
> > +		struct {
> > +			uint64_t     : 16;
> > +			uint64_t ipa : 16;
> > +			uint64_t ipb : 32;
> > +		};
> > +		struct {
> > +			uint64_t          : 16;
> > +			uint64_t opcode   :  8;
> > +			uint64_t r_1      :  4;
> > +			uint64_t r_2      :  4;
> > +			uint64_t r_base   :  4;
> > +			uint64_t displace : 12;
> > +			uint64_t zero     : 16;
> > +		};
> > +	} instr = { .ipa = vm->sblk->ipa, .ipb = vm->sblk->ipb };
> > +	uint8_t icptcode;
> > +	uint64_t code;
> > +
> > +	switch (diag) {
> > +	case 0x44:
> > +	case 0x9c:
> > +	case 0x288:
> > +	case 0x308:
> > +		icptcode = ICPT_PV_NOTIFY;
> > +		break;
> > +	case 0x500:
> > +		icptcode = ICPT_PV_INSTR;
> > +		break;
> > +	default:
> > +		/* If a new diag is introduced add it to the cases above! */
> > +		assert_msg(false, "unknown diag");
> > +	}
> > +
> > +	if (sie_is_pv(vm)) {
> > +		if (instr.r_1 != 0 || instr.r_2 != 2 || instr.r_base != 5)
> > +			return false;
> > +		if (instr.displace)
> > +			return false;
> > +	} else {
> > +		icptcode = ICPT_INST;
> > +	}
> > +	if (vm->sblk->icptcode != icptcode)
> > +		return false;
> > +	if (instr.opcode != 0x83 || instr.zero)
> > +		return false;
> > +	code = instr.r_base ? vm->save_area.guest.grs[instr.r_base] : 0;
> > +	code = (code + instr.displace) & 0xffff;
> > +	return code == diag;
> > +}
> 
> It looks like this transformation is equivalent for the PV case.

Yes, the PV case just has hardcoded values that we want to check.

> You
> could put the switch into the sie_is_pv() branch? Otherwise looks okay.

I want to validate diag for both PV and non PV.
> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks!

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

[Claudio Imbrenda]

On Tue, 25 Jun 2024 12:57:11 +1000
"Nicholas Piggin" <npiggin@gmail.com> wrote:

[...]

> Hmm, you have a nice instr struct that you made earlier and now you're
> back to mask and shift... What about exposing that struct and add a

this is actually a nice idea, we could make a union of the structs for
the instruction formats we need, and then we can do what you propose
here

> function to create it so you could do grs[sblk_to_instr(sblk).r1]

probably something like _INSTR(sblk).${FORMAT}.r1 

> here... Just a thought.

and maybe we could do this for the kernel as well

> 
> Thanks,
> Nick

Re: [kvm-unit-tests PATCH v3 4/7] s390x: Add function for checking diagnose intercepts

[Nicholas Piggin]

On Tue Jun 25, 2024 at 6:11 PM AEST, Nina Schoetterl-Glausch wrote:
> On Tue, 2024-06-25 at 12:14 +1000, Nicholas Piggin wrote:
> > On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:
> > > sie_is_diag_icpt() checks if the intercept is due to an expected
> > > diagnose call and is valid.
> > > It subsumes pv_icptdata_check_diag.
> > > 
> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > > ---
> > >  lib/s390x/pv_icptdata.h | 42 --------------------------------
> > >  lib/s390x/sie.h         | 12 ++++++++++
> > >  lib/s390x/sie.c         | 53 +++++++++++++++++++++++++++++++++++++++++
> > >  s390x/pv-diags.c        |  8 +++----
> > >  s390x/pv-icptcode.c     | 11 ++++-----
> > >  s390x/pv-ipl.c          |  7 +++---
> > >  6 files changed, 76 insertions(+), 57 deletions(-)
> > >  delete mode 100644 lib/s390x/pv_icptdata.h
>
> [...]
>
> > > +bool sie_is_diag_icpt(struct vm *vm, unsigned int diag)
> > > +{
> > > +	union {
> > > +		struct {
> > > +			uint64_t     : 16;
> > > +			uint64_t ipa : 16;
> > > +			uint64_t ipb : 32;
> > > +		};
> > > +		struct {
> > > +			uint64_t          : 16;
> > > +			uint64_t opcode   :  8;
> > > +			uint64_t r_1      :  4;
> > > +			uint64_t r_2      :  4;
> > > +			uint64_t r_base   :  4;
> > > +			uint64_t displace : 12;
> > > +			uint64_t zero     : 16;
> > > +		};
> > > +	} instr = { .ipa = vm->sblk->ipa, .ipb = vm->sblk->ipb };
> > > +	uint8_t icptcode;
> > > +	uint64_t code;
> > > +
> > > +	switch (diag) {
> > > +	case 0x44:
> > > +	case 0x9c:
> > > +	case 0x288:
> > > +	case 0x308:
> > > +		icptcode = ICPT_PV_NOTIFY;
> > > +		break;
> > > +	case 0x500:
> > > +		icptcode = ICPT_PV_INSTR;
> > > +		break;
> > > +	default:
> > > +		/* If a new diag is introduced add it to the cases above! */
> > > +		assert_msg(false, "unknown diag");
> > > +	}
> > > +
> > > +	if (sie_is_pv(vm)) {
> > > +		if (instr.r_1 != 0 || instr.r_2 != 2 || instr.r_base != 5)
> > > +			return false;
> > > +		if (instr.displace)
> > > +			return false;
> > > +	} else {
> > > +		icptcode = ICPT_INST;
> > > +	}
> > > +	if (vm->sblk->icptcode != icptcode)
> > > +		return false;
> > > +	if (instr.opcode != 0x83 || instr.zero)
> > > +		return false;
> > > +	code = instr.r_base ? vm->save_area.guest.grs[instr.r_base] : 0;
> > > +	code = (code + instr.displace) & 0xffff;
> > > +	return code == diag;
> > > +}
> > 
> > It looks like this transformation is equivalent for the PV case.
>
> Yes, the PV case just has hardcoded values that we want to check.
>
> > You
> > could put the switch into the sie_is_pv() branch? Otherwise looks okay.
>
> I want to validate diag for both PV and non PV.

To make sure it is one of listed cases, okay I missed that
point. All those same diag numbers are valid for !PV?
In that case it's fine.

Thanks,
Nick

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

[Andrew Jones]

On Thu, Jun 20, 2024 at 04:16:54PM GMT, Nina Schoetterl-Glausch wrote:
> Add functions for generating pseudo random 32 and 64 bit values.
> The implementation uses SHA-256 and so the randomness should have good
> quality.
> Implement the necessary subset of SHA-256.
> The PRNG algorithm is equivalent to the following python snippet:
> 
> def prng32(seed):
>     from hashlib import sha256
>     state = seed.to_bytes(8, byteorder="big")
>     while True:
>         state = sha256(state).digest()
>         for i in range(8):
>             yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")
> 
> Acked-by: Andrew Jones <andrew.jones@linux.dev>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>

There was a discussion about potentially needing random numbers for
RISC-V SBI tests, so I've queued this on riscv/sbi,
https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv%2Fsbi

Thanks,
drew

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

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00)
[...]
> 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)

Why unsigned int?

[...]
> 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
[...]
> +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]),

Out of curiosity:

Q = Memory reference without index register and with short displacement
S = Memory reference without index register but with long displacement

Which one is it?

And: is long displacement even appropriate here?

The cast also is hard to understand. Since this is not super high
performance code, do we just want to clobber memory so this gets a bit
easier to understand?

> +                 [len] "+RT"(res[0])

Same question about RT as above.

[...]
> diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c
> new file mode 100644
> index 00000000..a3e7f1c9
> --- /dev/null
> +++ b/s390x/stfle-sie.c
[...]
> +static struct guest_stfle_res run_guest(void)
> +{
> +       struct guest_stfle_res res;
> +       uint64_t guest_stfle_addr;
> +
> +       sie(&vm);
> +       assert(snippet_is_force_exit_value(&vm));
> +       guest_stfle_addr = snippet_get_force_exit_value(&vm);
> +       res.mem = &vm.guest_mem[guest_stfle_addr];
> +       memcpy(&res.reg, res.mem, sizeof(res.reg));
> +       res.len = (res.reg & 0xff) + 1;

If I'm not mistaken, you subtracted 1 in the guest. Here you add it again.
Is there a particular reason why?

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

[Nina Schoetterl-Glausch]

On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00)
> [...]
> > 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)
> 
> Why unsigned int?

The return value is 1-256, the size of the type is a bit arbitrary I suppose.

> 
> [...]
> > 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
> [...]
> > +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]),
> 
> Out of curiosity:
> 
> Q = Memory reference without index register and with short displacement
> S = Memory reference without index register but with long displacement
> 
> Which one is it?

Ups, just short displacement actually.

> 
> And: is long displacement even appropriate here?
> 
> The cast also is hard to understand. Since this is not super high
> performance code, do we just want to clobber memory so this gets a bit
> easier to understand?
> 
> > +                 [len] "+RT"(res[0])
> 
> Same question about RT as above.

Long, but providing a short displacement should be fine too.
Not sure if there is any benefit to letting the compiler choose.

> 
> [...]
> > diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c
> > new file mode 100644
> > index 00000000..a3e7f1c9
> > --- /dev/null
> > +++ b/s390x/stfle-sie.c
> [...]
> > +static struct guest_stfle_res run_guest(void)
> > +{
> > +       struct guest_stfle_res res;
> > +       uint64_t guest_stfle_addr;
> > +
> > +       sie(&vm);
> > +       assert(snippet_is_force_exit_value(&vm));
> > +       guest_stfle_addr = snippet_get_force_exit_value(&vm);
> > +       res.mem = &vm.guest_mem[guest_stfle_addr];
> > +       memcpy(&res.reg, res.mem, sizeof(res.reg));
> > +       res.len = (res.reg & 0xff) + 1;
> 
> If I'm not mistaken, you subtracted 1 in the guest. Here you add it again.
> Is there a particular reason why?

No, it's the direct result of STFLE on register 0.

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

[Heiko Carstens]

On Mon, Sep 02, 2024 at 04:24:53PM +0200, Nina Schoetterl-Glausch wrote:
> On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote:
> > Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00)
> > 
> > And: is long displacement even appropriate here?
> > 
> > The cast also is hard to understand. Since this is not super high
> > performance code, do we just want to clobber memory so this gets a bit
> > easier to understand?
> > 
> > > +                 [len] "+RT"(res[0])
> > 
> > Same question about RT as above.
> 
> Long, but providing a short displacement should be fine too.
> Not sure if there is any benefit to letting the compiler choose.

There are some older gcc compilers around which fail to compile if you
specify T for long displacement, but the compiler sees that a short
displacement would work. So please specify RT to avoid such compile
bugs.

See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=3e4be43f69da

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

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2024-06-20 16:16:54)
> Add functions for generating pseudo random 32 and 64 bit values.
> The implementation uses SHA-256 and so the randomness should have good
> quality.
> Implement the necessary subset of SHA-256.
> The PRNG algorithm is equivalent to the following python snippet:
> 
> def prng32(seed):
>     from hashlib import sha256
>     state = seed.to_bytes(8, byteorder="big")
>     while True:
>         state = sha256(state).digest()
>         for i in range(8):
>             yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")

Thomas, Andrew,
do you want to take this patch or is it OK it if comes with the whole
series via s390x tree?

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

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2024-09-02 16:24:53)
> On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote:
> > Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00)
> > [...]
> > > 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)
> > 
> > Why unsigned int?
> 
> The return value is 1-256, the size of the type is a bit arbitrary I suppose.
> 
> > 
> > [...]
> > > 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
> > [...]
> > > +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]),

Nina, do you mind sending a new version where we have the constraints
simplified, e.g. with just a memory clobber?

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

[Andrew Jones]

On Thu, Oct 10, 2024 at 10:25:47AM GMT, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2024-06-20 16:16:54)
> > Add functions for generating pseudo random 32 and 64 bit values.
> > The implementation uses SHA-256 and so the randomness should have good
> > quality.
> > Implement the necessary subset of SHA-256.
> > The PRNG algorithm is equivalent to the following python snippet:
> > 
> > def prng32(seed):
> >     from hashlib import sha256
> >     state = seed.to_bytes(8, byteorder="big")
> >     while True:
> >         state = sha256(state).digest()
> >         for i in range(8):
> >             yield int.from_bytes(state[i*4:(i+1)*4], byteorder="big")
> 
> Thomas, Andrew,
> do you want to take this patch or is it OK it if comes with the whole
> series via s390x tree?

It's already merged, commit e83373301c43 ("lib: Add pseudo random functions")

Thanks,
drew

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

[Nina Schoetterl-Glausch]

On Thu, 2024-10-10 at 10:27 +0200, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2024-09-02 16:24:53)
> > On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote:
> > > Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00)
> > > [...]
> > > > 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)
> > > 
> > > Why unsigned int?
> > 
> > The return value is 1-256, the size of the type is a bit arbitrary I suppose.
> > 
> > > 
> > > [...]
> > > > 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
> > > [...]
> > > > +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]),
> 
> Nina, do you mind sending a new version where we have the constraints
> simplified, e.g. with just a memory clobber?

Will do

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

[Nina Schoetterl-Glausch]

On Tue, 2024-06-25 at 12:43 +1000, Nicholas Piggin wrote:
> 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/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?

Yeah, makes sense to add another before.
> 
> 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.

[...]

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

[...]

> > +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?

It's this way round so the output reads:

FAIL: guest forced exit with value

What's after the colon is what failed and the message
is the same for PASS/FAIL. Indeed a bit confusing.

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

No, the function does just this check and if you need to handle
more complicated situations you need to do that in the caller.

> 
> Thanks,
> Nick