[kvm-unit-tests PATCH v1 0/4] s390x: add migration test suport

[kvm-unit-tests PATCH v1 0/4] s390x: add migration test suport

[Nico Boehr]

Add migration test support for s390x.

arm and powerpc already support basic migration tests.

If a test is in the migration group, it can print "migrate" on its console. This
will cause it to be migrated to a new QEMU instance. When migration is finished,
the test will be able to read a newline from its standard input.

We need the following pieces for this to work under s390x:

* read support for the sclp console. This can be very basic, it doesn't even
  have to read anything useful, we just need to know something happened on
  the console.
* s390/run adjustments to call the migration helper script.

Nico Boehr (4):
  lib: s390x: add support for SCLP console read
  s390x: add support for migration tests
  s390x: don't run migration tests under PV
  s390x: add selftest for migration

 lib/s390x/sclp-console.c   | 81 +++++++++++++++++++++++++++++++++++---
 lib/s390x/sclp.h           |  7 ++++
 s390x/Makefile             |  2 +
 s390x/run                  |  7 +++-
 s390x/selftest-migration.c | 27 +++++++++++++
 s390x/unittests.cfg        |  4 ++
 scripts/s390x/func.bash    |  2 +-
 7 files changed, 122 insertions(+), 8 deletions(-)
 create mode 100644 s390x/selftest-migration.c

-- 
2.31.1

[kvm-unit-tests PATCH v1 1/4] lib: s390x: add support for SCLP console read

[Nico Boehr]

Add a basic implementation for reading from the SCLP ACII console. The goal of
this is to support migration tests on s390x. To know when the migration has been
finished, we need to listen for a newline on our console.

Hence, this implementation is focused on the SCLP ASCII console of QEMU and
currently won't work under e.g. LPAR.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/sclp-console.c | 81 +++++++++++++++++++++++++++++++++++++---
 lib/s390x/sclp.h         |  7 ++++
 s390x/Makefile           |  1 +
 3 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
index fa36a6a42381..8e22660bf25d 100644
--- a/lib/s390x/sclp-console.c
+++ b/lib/s390x/sclp-console.c
@@ -89,6 +89,10 @@ static char lm_buff[120];
 static unsigned char lm_buff_off;
 static struct spinlock lm_buff_lock;
 
+static char read_buf[4096];
+static int read_index = sizeof(read_buf) - 1;
+static int read_buf_end = 0;
+
 static void sclp_print_ascii(const char *str)
 {
 	int len = strlen(str);
@@ -185,7 +189,7 @@ static void sclp_print_lm(const char *str)
  * indicating which messages the control program (we) want(s) to
  * send/receive.
  */
-static void sclp_set_write_mask(void)
+static void sclp_write_event_mask(int receive_mask, int send_mask)
 {
 	WriteEventMask *sccb = (void *)_sccb;
 
@@ -195,18 +199,27 @@ static void sclp_set_write_mask(void)
 	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
 	sccb->mask_length = sizeof(sccb_mask_t);
 
-	/* For now we don't process sclp input. */
-	sccb->cp_receive_mask = 0;
-	/* We send ASCII and line mode. */
-	sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG;
+	sccb->cp_receive_mask = receive_mask;
+	sccb->cp_send_mask = send_mask;
 
 	sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb);
 	assert(sccb->h.response_code == SCLP_RC_NORMAL_COMPLETION);
 }
 
+static void sclp_console_enable_read(void)
+{
+	sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
+}
+
+static void sclp_console_disable_read(void)
+{
+	sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
+}
+
 void sclp_console_setup(void)
 {
-	sclp_set_write_mask();
+	/* We send ASCII and line mode. */
+	sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
 }
 
 void sclp_print(const char *str)
@@ -227,3 +240,59 @@ void sclp_print(const char *str)
 	sclp_print_ascii(str);
 	sclp_print_lm(str);
 }
+
+#define SCLP_EVENT_ASCII_DATA_STREAM_FOLLOWS 0
+
+static int console_refill_read_buffer(void)
+{
+	const int MAX_EVENT_BUFFER_LEN = SCCB_SIZE - offsetof(ReadEventDataAsciiConsole, ebh);
+	ReadEventDataAsciiConsole *sccb = (void *)_sccb;
+	const int EVENT_BUFFER_ASCII_RECV_HEADER_LEN = sizeof(sccb->ebh) + sizeof(sccb->type);
+	int ret = -1;
+
+	sclp_console_enable_read();
+
+	sclp_mark_busy();
+	memset(sccb, 0, 4096);
+	sccb->h.length = PAGE_SIZE;
+	sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
+	sccb->h.control_mask[2] = 0x80;
+
+	sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
+
+	if ((sccb->h.response_code == SCLP_RC_NO_EVENT_BUFFERS_STORED) ||
+	    (sccb->ebh.type != SCLP_EVENT_ASCII_CONSOLE_DATA) ||
+	    (sccb->type != SCLP_EVENT_ASCII_DATA_STREAM_FOLLOWS)) {
+		ret = -1;
+		goto out;
+	}
+
+	assert(sccb->ebh.length <= MAX_EVENT_BUFFER_LEN);
+	assert(sccb->ebh.length > EVENT_BUFFER_ASCII_RECV_HEADER_LEN);
+
+	read_buf_end = sccb->ebh.length - EVENT_BUFFER_ASCII_RECV_HEADER_LEN;
+
+	assert(read_buf_end <= sizeof(read_buf));
+	memcpy(read_buf, sccb->data, read_buf_end);
+
+	read_index = 0;
+
+out:
+	sclp_console_disable_read();
+
+	return ret;
+}
+
+int __getchar(void)
+{
+	int ret;
+
+	if (read_index >= read_buf_end) {
+		ret = console_refill_read_buffer();
+		if (ret < 0) {
+			return ret;
+		}
+	}
+
+	return read_buf[read_index++];
+}
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index fead007a6037..5bd1741d721d 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -313,6 +313,13 @@ typedef struct ReadEventData {
 	uint32_t mask;
 } __attribute__((packed)) ReadEventData;
 
+typedef struct ReadEventDataAsciiConsole {
+	SCCBHeader h;
+	EventBufferHeader ebh;
+	uint8_t type;
+	char data[];
+} __attribute__((packed)) ReadEventDataAsciiConsole;
+
 extern char _sccb[];
 void sclp_setup_int(void);
 void sclp_handle_ext(void);
diff --git a/s390x/Makefile b/s390x/Makefile
index 53b0fe044fe7..62e197cb93d7 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -71,6 +71,7 @@ cflatobjs += lib/alloc_phys.o
 cflatobjs += lib/alloc_page.o
 cflatobjs += lib/vmalloc.o
 cflatobjs += lib/alloc_phys.o
+cflatobjs += lib/getchar.o
 cflatobjs += lib/s390x/io.o
 cflatobjs += lib/s390x/stack.o
 cflatobjs += lib/s390x/sclp.o
-- 
2.31.1

[kvm-unit-tests PATCH v1 2/4] s390x: add support for migration tests

[Nico Boehr]

Now that we have SCLP console read support, run our tests with migration_cmd, so
we can get migration support on s390x.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/run | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/run b/s390x/run
index 064ecd1b337a..2bcdabbaa14f 100755
--- a/s390x/run
+++ b/s390x/run
@@ -25,7 +25,7 @@ M+=",accel=$ACCEL"
 command="$qemu -nodefaults -nographic $M"
 command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
 command+=" -kernel"
-command="$(timeout_cmd) $command"
+command="$(migration_cmd) $(timeout_cmd) $command"
 
 # We return the exit code via stdout, not via the QEMU return code
 run_qemu_status $command "$@"
-- 
2.31.1

[kvm-unit-tests PATCH v1 3/4] s390x: don't run migration tests under PV

[Nico Boehr]

PV doesn't support migration, so don't run the migration tests there.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/run               | 5 +++++
 scripts/s390x/func.bash | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/s390x/run b/s390x/run
index 2bcdabbaa14f..24138f6803be 100755
--- a/s390x/run
+++ b/s390x/run
@@ -20,6 +20,11 @@ if [ "${1: -7}" = ".pv.bin" ] || [ "${TESTNAME: -3}" = "_PV" ] && [ "$ACCEL" = "
 	exit 2
 fi
 
+if [ "${1: -7}" = ".pv.bin" ] || [ "${TESTNAME: -3}" = "_PV" ] && [ "$MIGRATION" = "yes" ]; then
+	echo "Migration isn't supported under Protected Virtualization"
+	exit 2
+fi
+
 M='-machine s390-ccw-virtio'
 M+=",accel=$ACCEL"
 command="$qemu -nodefaults -nographic $M"
diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
index bf799a567c3b..2a941bbb0794 100644
--- a/scripts/s390x/func.bash
+++ b/scripts/s390x/func.bash
@@ -21,7 +21,7 @@ function arch_cmd_s390x()
 	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 
 	# run PV test case
-	if [ "$ACCEL" = 'tcg' ]; then
+	if [ "$ACCEL" = 'tcg' ] || find_word "migration" "$groups"; then
 		return
 	fi
 	kernel=${kernel%.elf}.pv.bin
-- 
2.31.1

[kvm-unit-tests PATCH v1 4/4] s390x: add selftest for migration

[Nico Boehr]

Add a selftest to check we can do migration tests.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile             |  1 +
 s390x/selftest-migration.c | 27 +++++++++++++++++++++++++++
 s390x/unittests.cfg        |  4 ++++
 3 files changed, 32 insertions(+)
 create mode 100644 s390x/selftest-migration.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 62e197cb93d7..2e43e323bcb5 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -26,6 +26,7 @@ tests += $(TEST_DIR)/edat.elf
 tests += $(TEST_DIR)/mvpg-sie.elf
 tests += $(TEST_DIR)/spec_ex-sie.elf
 tests += $(TEST_DIR)/firq.elf
+tests += $(TEST_DIR)/selftest-migration.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/selftest-migration.c b/s390x/selftest-migration.c
new file mode 100644
index 000000000000..8884322a84ca
--- /dev/null
+++ b/s390x/selftest-migration.c
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Migration Selftest
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+#include <libcflat.h>
+
+int main(void)
+{
+	/* don't say migrate here otherwise we will migrate right away */
+	report_prefix_push("selftest migration");
+
+	/* ask migrate_cmd to migrate (it listens for 'migrate') */
+	puts("Please migrate me\n");
+
+	/* wait for migration to finish, we will read a newline */
+	(void)getchar();
+
+	report_pass("Migrated");
+
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 1600e714c8b9..b0417a69705d 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -24,6 +24,10 @@ groups = selftest
 # please keep the kernel cmdline in sync with $(TEST_DIR)/selftest.parmfile
 extra_params = -append 'test 123'
 
+[selftest-migration]
+file = selftest-migration.elf
+groups = selftest migration
+
 [intercept]
 file = intercept.elf
 
-- 
2.31.1

Re: [kvm-unit-tests PATCH v1 4/4] s390x: add selftest for migration

[Claudio Imbrenda]

On Mon, 11 Apr 2022 12:07:50 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Add a selftest to check we can do migration tests.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/Makefile             |  1 +
>  s390x/selftest-migration.c | 27 +++++++++++++++++++++++++++
>  s390x/unittests.cfg        |  4 ++++
>  3 files changed, 32 insertions(+)
>  create mode 100644 s390x/selftest-migration.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 62e197cb93d7..2e43e323bcb5 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -26,6 +26,7 @@ tests += $(TEST_DIR)/edat.elf
>  tests += $(TEST_DIR)/mvpg-sie.elf
>  tests += $(TEST_DIR)/spec_ex-sie.elf
>  tests += $(TEST_DIR)/firq.elf
> +tests += $(TEST_DIR)/selftest-migration.elf
>  
>  pv-tests += $(TEST_DIR)/pv-diags.elf
>  
> diff --git a/s390x/selftest-migration.c b/s390x/selftest-migration.c
> new file mode 100644
> index 000000000000..8884322a84ca
> --- /dev/null
> +++ b/s390x/selftest-migration.c
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Migration Selftest
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +#include <libcflat.h>
> +
> +int main(void)
> +{
> +	/* don't say migrate here otherwise we will migrate right away */
> +	report_prefix_push("selftest migration");
> +
> +	/* ask migrate_cmd to migrate (it listens for 'migrate') */
> +	puts("Please migrate me\n");
> +
> +	/* wait for migration to finish, we will read a newline */
> +	(void)getchar();

how hard would it be to actually check that you got the newline?

> +
> +	report_pass("Migrated");
> +
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 1600e714c8b9..b0417a69705d 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -24,6 +24,10 @@ groups = selftest
>  # please keep the kernel cmdline in sync with $(TEST_DIR)/selftest.parmfile
>  extra_params = -append 'test 123'
>  
> +[selftest-migration]
> +file = selftest-migration.elf
> +groups = selftest migration
> +
>  [intercept]
>  file = intercept.elf
>  

Re: [kvm-unit-tests PATCH v1 3/4] s390x: don't run migration tests under PV

[Claudio Imbrenda]

On Mon, 11 Apr 2022 12:07:49 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> PV doesn't support migration, so don't run the migration tests there.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

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

> ---
>  s390x/run               | 5 +++++
>  scripts/s390x/func.bash | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/run b/s390x/run
> index 2bcdabbaa14f..24138f6803be 100755
> --- a/s390x/run
> +++ b/s390x/run
> @@ -20,6 +20,11 @@ if [ "${1: -7}" = ".pv.bin" ] || [ "${TESTNAME: -3}" = "_PV" ] && [ "$ACCEL" = "
>  	exit 2
>  fi
>  
> +if [ "${1: -7}" = ".pv.bin" ] || [ "${TESTNAME: -3}" = "_PV" ] && [ "$MIGRATION" = "yes" ]; then
> +	echo "Migration isn't supported under Protected Virtualization"
> +	exit 2
> +fi
> +
>  M='-machine s390-ccw-virtio'
>  M+=",accel=$ACCEL"
>  command="$qemu -nodefaults -nographic $M"
> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
> index bf799a567c3b..2a941bbb0794 100644
> --- a/scripts/s390x/func.bash
> +++ b/scripts/s390x/func.bash
> @@ -21,7 +21,7 @@ function arch_cmd_s390x()
>  	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>  
>  	# run PV test case
> -	if [ "$ACCEL" = 'tcg' ]; then
> +	if [ "$ACCEL" = 'tcg' ] || find_word "migration" "$groups"; then
>  		return
>  	fi
>  	kernel=${kernel%.elf}.pv.bin

Re: [kvm-unit-tests PATCH v1 2/4] s390x: add support for migration tests

[Claudio Imbrenda]

On Mon, 11 Apr 2022 12:07:48 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Now that we have SCLP console read support, run our tests with migration_cmd, so
> we can get migration support on s390x.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

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

> ---
>  s390x/run | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/run b/s390x/run
> index 064ecd1b337a..2bcdabbaa14f 100755
> --- a/s390x/run
> +++ b/s390x/run
> @@ -25,7 +25,7 @@ M+=",accel=$ACCEL"
>  command="$qemu -nodefaults -nographic $M"
>  command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
>  command+=" -kernel"
> -command="$(timeout_cmd) $command"
> +command="$(migration_cmd) $(timeout_cmd) $command"
>  
>  # We return the exit code via stdout, not via the QEMU return code
>  run_qemu_status $command "$@"

Re: [kvm-unit-tests PATCH v1 4/4] s390x: add selftest for migration

[Thomas Huth]

On 11/04/2022 12.07, Nico Boehr wrote:
> Add a selftest to check we can do migration tests.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   s390x/Makefile             |  1 +
>   s390x/selftest-migration.c | 27 +++++++++++++++++++++++++++
>   s390x/unittests.cfg        |  4 ++++
>   3 files changed, 32 insertions(+)
>   create mode 100644 s390x/selftest-migration.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 62e197cb93d7..2e43e323bcb5 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -26,6 +26,7 @@ tests += $(TEST_DIR)/edat.elf
>   tests += $(TEST_DIR)/mvpg-sie.elf
>   tests += $(TEST_DIR)/spec_ex-sie.elf
>   tests += $(TEST_DIR)/firq.elf
> +tests += $(TEST_DIR)/selftest-migration.elf
>   
>   pv-tests += $(TEST_DIR)/pv-diags.elf
>   
> diff --git a/s390x/selftest-migration.c b/s390x/selftest-migration.c
> new file mode 100644
> index 000000000000..8884322a84ca
> --- /dev/null
> +++ b/s390x/selftest-migration.c
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Migration Selftest
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +#include <libcflat.h>
> +
> +int main(void)
> +{
> +	/* don't say migrate here otherwise we will migrate right away */
> +	report_prefix_push("selftest migration");
> +
> +	/* ask migrate_cmd to migrate (it listens for 'migrate') */
> +	puts("Please migrate me\n");
> +
> +	/* wait for migration to finish, we will read a newline */
> +	(void)getchar();
> +
> +	report_pass("Migrated");
> +
> +	report_prefix_pop();
> +	return report_summary();
> +}

Thanks for tackling this!

Having written powerpc/sprs.c in the past, I've got one question / request:

Could we turn this into a "real" test immediately? E.g. write a sane value 
to all control registers that are currently not in use by the k-u-t before 
migration, and then check whether the values are still in there after 
migration? Maybe also some vector registers and the "guarded storage control"?
... or is this rather something for a later update?

  Thomas

Re: [kvm-unit-tests PATCH v1 4/4] s390x: add selftest for migration

[Nico Boehr]

On Mon, 2022-04-11 at 17:30 +0200, Thomas Huth wrote:
> Thanks for tackling this!
> 
> Having written powerpc/sprs.c in the past, I've got one question /
> request:
> 
> Could we turn this into a "real" test immediately? E.g. write a sane
> value 
> to all control registers that are currently not in use by the k-u-t
> before 
> migration, and then check whether the values are still in there after
> migration? Maybe also some vector registers and the "guarded storage
> control"?
> ... or is this rather something for a later update?

My plan was to first add the infrastructure for migration tests
including the selftest and then later one by one add "real" tests. 

But if you think it is preferable, I can extend the scope and add some
inital "real" tests in this series.

Re: [kvm-unit-tests PATCH v1 4/4] s390x: add selftest for migration

[Thomas Huth]

On 12/04/2022 09.41, Nico Boehr wrote:
> On Mon, 2022-04-11 at 17:30 +0200, Thomas Huth wrote:
>> Thanks for tackling this!
>>
>> Having written powerpc/sprs.c in the past, I've got one question /
>> request:
>>
>> Could we turn this into a "real" test immediately? E.g. write a sane
>> value
>> to all control registers that are currently not in use by the k-u-t
>> before
>> migration, and then check whether the values are still in there after
>> migration? Maybe also some vector registers and the "guarded storage
>> control"?
>> ... or is this rather something for a later update?
> 
> My plan was to first add the infrastructure for migration tests
> including the selftest and then later one by one add "real" tests.
> 
> But if you think it is preferable, I can extend the scope and add some
> inital "real" tests in this series.

I think a simple test that checks some register values should not be too 
hard to implement, so I'd prefer that instead of this simple selftest ... 
but if you're too short in time right now, I also won't insist.

  Thomas

Re: [kvm-unit-tests PATCH v1 1/4] lib: s390x: add support for SCLP console read

[Thomas Huth]

On 11/04/2022 12.07, Nico Boehr wrote:
> Add a basic implementation for reading from the SCLP ACII console. The goal of

s/ACII/ASCII/

> this is to support migration tests on s390x. To know when the migration has been
> finished, we need to listen for a newline on our console.
> 
> Hence, this implementation is focused on the SCLP ASCII console of QEMU and
> currently won't work under e.g. LPAR.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   lib/s390x/sclp-console.c | 81 +++++++++++++++++++++++++++++++++++++---
>   lib/s390x/sclp.h         |  7 ++++
>   s390x/Makefile           |  1 +
>   3 files changed, 83 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
> index fa36a6a42381..8e22660bf25d 100644
> --- a/lib/s390x/sclp-console.c
> +++ b/lib/s390x/sclp-console.c
> @@ -89,6 +89,10 @@ static char lm_buff[120];
>   static unsigned char lm_buff_off;
>   static struct spinlock lm_buff_lock;
>   
> +static char read_buf[4096];
> +static int read_index = sizeof(read_buf) - 1;
> +static int read_buf_end = 0;
> +
>   static void sclp_print_ascii(const char *str)
>   {
>   	int len = strlen(str);
> @@ -185,7 +189,7 @@ static void sclp_print_lm(const char *str)
>    * indicating which messages the control program (we) want(s) to
>    * send/receive.
>    */
> -static void sclp_set_write_mask(void)
> +static void sclp_write_event_mask(int receive_mask, int send_mask)
>   {
>   	WriteEventMask *sccb = (void *)_sccb;
>   
> @@ -195,18 +199,27 @@ static void sclp_set_write_mask(void)
>   	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
>   	sccb->mask_length = sizeof(sccb_mask_t);
>   
> -	/* For now we don't process sclp input. */
> -	sccb->cp_receive_mask = 0;
> -	/* We send ASCII and line mode. */
> -	sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG;
> +	sccb->cp_receive_mask = receive_mask;
> +	sccb->cp_send_mask = send_mask;
>   
>   	sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb);
>   	assert(sccb->h.response_code == SCLP_RC_NORMAL_COMPLETION);
>   }
>   
> +static void sclp_console_enable_read(void)
> +{
> +	sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
> +}
> +
> +static void sclp_console_disable_read(void)
> +{
> +	sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
> +}
> +
>   void sclp_console_setup(void)
>   {
> -	sclp_set_write_mask();
> +	/* We send ASCII and line mode. */
> +	sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
>   }
>   
>   void sclp_print(const char *str)
> @@ -227,3 +240,59 @@ void sclp_print(const char *str)
>   	sclp_print_ascii(str);
>   	sclp_print_lm(str);
>   }
> +
> +#define SCLP_EVENT_ASCII_DATA_STREAM_FOLLOWS 0
> +
> +static int console_refill_read_buffer(void)
> +{
> +	const int MAX_EVENT_BUFFER_LEN = SCCB_SIZE - offsetof(ReadEventDataAsciiConsole, ebh);
> +	ReadEventDataAsciiConsole *sccb = (void *)_sccb;
> +	const int EVENT_BUFFER_ASCII_RECV_HEADER_LEN = sizeof(sccb->ebh) + sizeof(sccb->type);
> +	int ret = -1;
> +
> +	sclp_console_enable_read();
> +
> +	sclp_mark_busy();
> +	memset(sccb, 0, 4096);
> +	sccb->h.length = PAGE_SIZE;
> +	sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
> +	sccb->h.control_mask[2] = 0x80;

Add at least a comment about what the 0x80 means, please?

> +
> +	sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
> +
> +	if ((sccb->h.response_code == SCLP_RC_NO_EVENT_BUFFERS_STORED) ||
> +	    (sccb->ebh.type != SCLP_EVENT_ASCII_CONSOLE_DATA) ||
> +	    (sccb->type != SCLP_EVENT_ASCII_DATA_STREAM_FOLLOWS)) {
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	assert(sccb->ebh.length <= MAX_EVENT_BUFFER_LEN);
> +	assert(sccb->ebh.length > EVENT_BUFFER_ASCII_RECV_HEADER_LEN);
> +
> +	read_buf_end = sccb->ebh.length - EVENT_BUFFER_ASCII_RECV_HEADER_LEN;
> +
> +	assert(read_buf_end <= sizeof(read_buf));
> +	memcpy(read_buf, sccb->data, read_buf_end);
> +
> +	read_index = 0;

Set "ret = 0" here?

> +out:
> +	sclp_console_disable_read();
> +
> +	return ret;
> +}
> +
> +int __getchar(void)
> +{
> +	int ret;
> +
> +	if (read_index >= read_buf_end) {
> +		ret = console_refill_read_buffer();
> +		if (ret < 0) {
> +			return ret;
> +		}
> +	}
> +
> +	return read_buf[read_index++];
> +}

  Thomas

Re: [kvm-unit-tests PATCH v1 2/4] s390x: add support for migration tests

[Thomas Huth]

On 11/04/2022 12.07, Nico Boehr wrote:
> Now that we have SCLP console read support, run our tests with migration_cmd, so
> we can get migration support on s390x.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   s390x/run | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/run b/s390x/run
> index 064ecd1b337a..2bcdabbaa14f 100755
> --- a/s390x/run
> +++ b/s390x/run
> @@ -25,7 +25,7 @@ M+=",accel=$ACCEL"
>   command="$qemu -nodefaults -nographic $M"
>   command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
>   command+=" -kernel"
> -command="$(timeout_cmd) $command"
> +command="$(migration_cmd) $(timeout_cmd) $command"
>   
>   # We return the exit code via stdout, not via the QEMU return code
>   run_qemu_status $command "$@"

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v1 3/4] s390x: don't run migration tests under PV

[Thomas Huth]

On 11/04/2022 12.07, Nico Boehr wrote:
> PV doesn't support migration, so don't run the migration tests there.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   s390x/run               | 5 +++++
>   scripts/s390x/func.bash | 2 +-
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/run b/s390x/run
> index 2bcdabbaa14f..24138f6803be 100755
> --- a/s390x/run
> +++ b/s390x/run
> @@ -20,6 +20,11 @@ if [ "${1: -7}" = ".pv.bin" ] || [ "${TESTNAME: -3}" = "_PV" ] && [ "$ACCEL" = "
>   	exit 2
>   fi
>   
> +if [ "${1: -7}" = ".pv.bin" ] || [ "${TESTNAME: -3}" = "_PV" ] && [ "$MIGRATION" = "yes" ]; then
> +	echo "Migration isn't supported under Protected Virtualization"
> +	exit 2
> +fi
> +
>   M='-machine s390-ccw-virtio'
>   M+=",accel=$ACCEL"
>   command="$qemu -nodefaults -nographic $M"
> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
> index bf799a567c3b..2a941bbb0794 100644
> --- a/scripts/s390x/func.bash
> +++ b/scripts/s390x/func.bash
> @@ -21,7 +21,7 @@ function arch_cmd_s390x()
>   	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>   
>   	# run PV test case
> -	if [ "$ACCEL" = 'tcg' ]; then
> +	if [ "$ACCEL" = 'tcg' ] || find_word "migration" "$groups"; then
>   		return
>   	fi
>   	kernel=${kernel%.elf}.pv.bin

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v1 4/4] s390x: add selftest for migration

[Nico Boehr]

On Mon, 2022-04-11 at 14:49 +0200, Claudio Imbrenda wrote:
[...]
> > diff --git a/s390x/selftest-migration.c b/s390x/selftest-
> > migration.c
[...]
> > +int main(void)
> > +{
> > +       /* don't say migrate here otherwise we will migrate right
> > away */
> > +       report_prefix_push("selftest migration");
> > +
> > +       /* ask migrate_cmd to migrate (it listens for 'migrate') */
> > +       puts("Please migrate me\n");
> > +
> > +       /* wait for migration to finish, we will read a newline */
> > +       (void)getchar();
> 
> how hard would it be to actually check that you got the newline?

It would be simple. I decided for ignoring what we actually read
because that's what ARM and PPC do.

But I am also OK checking we really read a newline. What would you
suggest to do if we read something that's not a newline? Read again
until we actually do get a newline?

Re: [kvm-unit-tests PATCH v1 1/4] lib: s390x: add support for SCLP console read

[Janosch Frank]

On 4/11/22 12:07, Nico Boehr wrote:
> Add a basic implementation for reading from the SCLP ACII console. The goal of
> this is to support migration tests on s390x. To know when the migration has been
> finished, we need to listen for a newline on our console.
> 
> Hence, this implementation is focused on the SCLP ASCII console of QEMU and
> currently won't work under e.g. LPAR.

How much pain would it be to add the line mode read?

> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
[..]
>   
> +static void sclp_console_enable_read(void)
> +{
> +	sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
> +}
> +
> +static void sclp_console_disable_read(void)
> +{
> +	sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
> +}
> +
>   void sclp_console_setup(void)
>   {
> -	sclp_set_write_mask();
> +	/* We send ASCII and line mode. */
> +	sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
>   }
>   
>   void sclp_print(const char *str)
> @@ -227,3 +240,59 @@ void sclp_print(const char *str)
>   	sclp_print_ascii(str);
>   	sclp_print_lm(str);
>   }
> +
> +#define SCLP_EVENT_ASCII_DATA_STREAM_FOLLOWS 0

-> sclp.h

> +
> +static int console_refill_read_buffer(void)
> +{
> +	const int MAX_EVENT_BUFFER_LEN = SCCB_SIZE - offsetof(ReadEventDataAsciiConsole, ebh);
> +	ReadEventDataAsciiConsole *sccb = (void *)_sccb;
> +	const int EVENT_BUFFER_ASCII_RECV_HEADER_LEN = sizeof(sccb->ebh) + sizeof(sccb->type);
> +	int ret = -1;

Reverse Christmas tree
The const int variables are all caps because they are essentially constants?

> +
> +	sclp_console_enable_read();
> +
> +	sclp_mark_busy();
> +	memset(sccb, 0, 4096);

sizeof(*sccb)

> +	sccb->h.length = PAGE_SIZE;
> +	sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
> +	sccb->h.control_mask[2] = 0x80;
> +
> +	sclp_service_call(SCLP_CMD_READ_EVENT_DATA, sccb);
> +
> +	if ((sccb->h.response_code == SCLP_RC_NO_EVENT_BUFFERS_STORED) ||
> +	    (sccb->ebh.type != SCLP_EVENT_ASCII_CONSOLE_DATA) ||
> +	    (sccb->type != SCLP_EVENT_ASCII_DATA_STREAM_FOLLOWS)) {
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	assert(sccb->ebh.length <= MAX_EVENT_BUFFER_LEN);
> +	assert(sccb->ebh.length > EVENT_BUFFER_ASCII_RECV_HEADER_LEN);
> +
> +	read_buf_end = sccb->ebh.length - EVENT_BUFFER_ASCII_RECV_HEADER_LEN;
> +
> +	assert(read_buf_end <= sizeof(read_buf));
> +	memcpy(read_buf, sccb->data, read_buf_end);
> +
> +	read_index = 0;
> +
> +out:
> +	sclp_console_disable_read();
> +
> +	return ret;
> +}
> +
> +int __getchar(void)
> +{
> +	int ret;
> +
> +	if (read_index >= read_buf_end) {
> +		ret = console_refill_read_buffer();
> +		if (ret < 0) {
> +			return ret;
> +		}
> +	}
> +
> +	return read_buf[read_index++];
> +}
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index fead007a6037..5bd1741d721d 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -313,6 +313,13 @@ typedef struct ReadEventData {
>   	uint32_t mask;
>   } __attribute__((packed)) ReadEventData;
>   
> +typedef struct ReadEventDataAsciiConsole {
> +	SCCBHeader h;
> +	EventBufferHeader ebh;
> +	uint8_t type;
> +	char data[];
> +} __attribute__((packed)) ReadEventDataAsciiConsole;
> +
>   extern char _sccb[];
>   void sclp_setup_int(void);
>   void sclp_handle_ext(void);
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 53b0fe044fe7..62e197cb93d7 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -71,6 +71,7 @@ cflatobjs += lib/alloc_phys.o
>   cflatobjs += lib/alloc_page.o
>   cflatobjs += lib/vmalloc.o
>   cflatobjs += lib/alloc_phys.o
> +cflatobjs += lib/getchar.o
>   cflatobjs += lib/s390x/io.o
>   cflatobjs += lib/s390x/stack.o
>   cflatobjs += lib/s390x/sclp.o

Re: [kvm-unit-tests PATCH v1 4/4] s390x: add selftest for migration

[Claudio Imbrenda]

On Tue, 12 Apr 2022 13:49:21 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> On Mon, 2022-04-11 at 14:49 +0200, Claudio Imbrenda wrote:
> [...]
> > > diff --git a/s390x/selftest-migration.c b/s390x/selftest-
> > > migration.c  
> [...]
> > > +int main(void)
> > > +{
> > > +       /* don't say migrate here otherwise we will migrate right
> > > away */
> > > +       report_prefix_push("selftest migration");
> > > +
> > > +       /* ask migrate_cmd to migrate (it listens for 'migrate') */
> > > +       puts("Please migrate me\n");
> > > +
> > > +       /* wait for migration to finish, we will read a newline */
> > > +       (void)getchar();  
> > 
> > how hard would it be to actually check that you got the newline?  
> 
> It would be simple. I decided for ignoring what we actually read
> because that's what ARM and PPC do.

oh, then it's fine as it is

> 
> But I am also OK checking we really read a newline. What would you
> suggest to do if we read something that's not a newline? Read again
> until we actually do get a newline?

I was more thinking that it's a failure, but see the comment above

Re: [kvm-unit-tests PATCH v1 4/4] s390x: add selftest for migration

[Nico Boehr]

On Tue, 2022-04-12 at 09:49 +0200, Thomas Huth wrote:
> I think a simple test that checks some register values should not be
> too 
> hard to implement, so I'd prefer that instead of this simple selftest
> ... 
> but if you're too short in time right now, I also won't insist.

It makes sense. I will add it.

Re: [kvm-unit-tests PATCH v1 1/4] lib: s390x: add support for SCLP console read

[Nico Boehr]

On Tue, 2022-04-12 at 10:02 +0200, Thomas Huth wrote:
> On 11/04/2022 12.07, Nico Boehr wrote:
> > Add a basic implementation for reading from the SCLP ACII console.
> > The goal of
> 
> s/ACII/ASCII/

Thanks, fixed.

[...]
> > diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
> > index fa36a6a42381..8e22660bf25d 100644
[...]
> > +static int console_refill_read_buffer(void)
> > +{
> > +       const int MAX_EVENT_BUFFER_LEN = SCCB_SIZE -
> > offsetof(ReadEventDataAsciiConsole, ebh);
> > +       ReadEventDataAsciiConsole *sccb = (void *)_sccb;
> > +       const int EVENT_BUFFER_ASCII_RECV_HEADER_LEN = sizeof(sccb-
> > >ebh) + sizeof(sccb->type);
> > +       int ret = -1;
> > +
> > +       sclp_console_enable_read();
> > +
> > +       sclp_mark_busy();
> > +       memset(sccb, 0, 4096);
> > +       sccb->h.length = PAGE_SIZE;
> > +       sccb->h.function_code = SCLP_UNCONDITIONAL_READ;
> > +       sccb->h.control_mask[2] = 0x80;
> 
> Add at least a comment about what the 0x80 means, please?

Oh yes, thanks. We already have a define for it which I will use
instead of the comment: SCLP_CM2_VARIABLE_LENGTH_RESPONSE


[...]
> > +
> > +       read_buf_end = sccb->ebh.length -
> > EVENT_BUFFER_ASCII_RECV_HEADER_LEN;
> > +
> > +       assert(read_buf_end <= sizeof(read_buf));
> > +       memcpy(read_buf, sccb->data, read_buf_end);
> > +
> > +       read_index = 0;
> 
> Set "ret = 0" here?

Oooops, excellent catch. Thanks, fixed.

Re: [kvm-unit-tests PATCH v1 1/4] lib: s390x: add support for SCLP console read

[Nico Boehr]

On Tue, 2022-04-12 at 17:32 +0200, Janosch Frank wrote:
> On 4/11/22 12:07, Nico Boehr wrote:
> > Add a basic implementation for reading from the SCLP ACII console.
> > The goal of
> > this is to support migration tests on s390x. To know when the
> > migration has been
> > finished, we need to listen for a newline on our console.
> > 
> > Hence, this implementation is focused on the SCLP ASCII console of
> > QEMU and
> > currently won't work under e.g. LPAR.
> 
> How much pain would it be to add the line mode read?

I am not terribly familiar with the line mode, but I can say it would
make the implementation of the ASCII console more complex. Right now we
can just assume there will just be events from the ASCII console when
we read event data.

Not impossible to do, but I thought we don't need it so I kept things
simple. Is there some benefit we would have from the line mode console?

[...]
> >   
> > +static void sclp_console_enable_read(void)
> > +{
> > +       sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII,
> > SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
> > +}
> > +
> > +static void sclp_console_disable_read(void)
> > +{
> > +       sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII |
> > SCLP_EVENT_MASK_MSG);
> > +}
> > +
> >   void sclp_console_setup(void)
> >   {
> > -       sclp_set_write_mask();
> > +       /* We send ASCII and line mode. */
> > +       sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII |
> > SCLP_EVENT_MASK_MSG);
> >   }
> >   
> >   void sclp_print(const char *str)
> > @@ -227,3 +240,59 @@ void sclp_print(const char *str)
> >         sclp_print_ascii(str);
> >         sclp_print_lm(str);
> >   }
> > +
> > +#define SCLP_EVENT_ASCII_DATA_STREAM_FOLLOWS 0
> 
> -> sclp.h

Yes, thanks.

> 
> > +
> > +static int console_refill_read_buffer(void)
> > +{
> > +       const int MAX_EVENT_BUFFER_LEN = SCCB_SIZE -
> > offsetof(ReadEventDataAsciiConsole, ebh);
> > +       ReadEventDataAsciiConsole *sccb = (void *)_sccb;
> > +       const int EVENT_BUFFER_ASCII_RECV_HEADER_LEN = sizeof(sccb-
> > >ebh) + sizeof(sccb->type);
> > +       int ret = -1;
> 
> Reverse Christmas tree

Hm, I think it's not possible for EVENT_BUFFER_ASCII_RECV_HEADER_LEN
because it needs sccb first. I would want to leave as-is except if you
have a better idea on how to do this?

> The const int variables are all caps because they are essentially
> constants?

Yes, that was my reasoning. But it is uncommon in kvm-unit-test to have
it uppercase, all const ints in the codebase are lowercase, so I will
lowercase it.

> > +
> > +       sclp_console_enable_read();
> > +
> > +       sclp_mark_busy();
> > +       memset(sccb, 0, 4096);
> 
> sizeof(*sccb)

If you are OK with it, I would prefer to use SCCB_SIZE, s.t. the entire
buffer is cleared.