kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [kvm-unit-tests RFC v2 0/4] s390x: Add Protected VM support
@ 2020-08-12  9:27 Marc Hartmayer
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 1/4] common.bash: run `cmd` only if a test case was found Marc Hartmayer
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-12  9:27 UTC (permalink / raw)
  To: kvm
  Cc: Thomas Huth, David Hildenbrand, Janosch Frank, Cornelia Huck,
	Andrew Jones, Paolo Bonzini, Christian Borntraeger, linux-s390

Add support for Protected Virtual Machine (PVM) tests. For starting a
PVM guest we must be able to generate a PVM image by using the
`genprotimg` tool from the s390-tools collection. This requires the
ability to pass a machine-specific host-key document, so the option
`--host-key-document` is added to the configure script.

Sorry it took so long to send the second version :/

For everybody's convenience there is a branch:
https://gitlab.com/mhartmay/kvm-unit-tests/-/tree/test_alternative

Changelog:
 RFC v1 -> RFC v2:
  + Remove `pv_support` option (Janosch, David)
  + Add some preliminary patches:
    - move "testname guard"
    - add support for architecture dependent functions
  + Add support for specifying a parmline file for the PV image
    generation. This is necessary for the `selftest` because the
    kernel cmdline set on the QEMU command line is ignored for PV
    guests

Marc Hartmayer (4):
  common.bash: run `cmd` only if a test case was found
  scripts: add support for architecture dependent functions
  run_tests/mkstandalone: add arch dependent function to
    `for_each_unittest`
  s390x: add Protected VM support

 README.md               |  3 ++-
 configure               |  8 ++++++++
 run_tests.sh            |  5 +----
 s390x/Makefile          | 17 +++++++++++++++--
 s390x/selftest.parmfile |  1 +
 s390x/unittests.cfg     |  1 +
 scripts/common.bash     | 26 ++++++++++++++++++++++++--
 scripts/mkstandalone.sh |  6 +-----
 scripts/s390x/func.bash | 18 ++++++++++++++++++
 9 files changed, 71 insertions(+), 14 deletions(-)
 create mode 100644 s390x/selftest.parmfile
 create mode 100644 scripts/s390x/func.bash

-- 
2.25.4


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [kvm-unit-tests RFC v2 1/4] common.bash: run `cmd` only if a test case was found
  2020-08-12  9:27 [kvm-unit-tests RFC v2 0/4] s390x: Add Protected VM support Marc Hartmayer
@ 2020-08-12  9:27 ` Marc Hartmayer
  2020-08-13  7:40   ` Andrew Jones
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions Marc Hartmayer
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-12  9:27 UTC (permalink / raw)
  To: kvm
  Cc: Thomas Huth, David Hildenbrand, Janosch Frank, Cornelia Huck,
	Andrew Jones, Paolo Bonzini, Christian Borntraeger, linux-s390

It's only useful to run `cmd` in `for_each_unittest` if a test case
was found. This change allows us to remove the guards from the
functions `run_task` and `mkstandalone`.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 run_tests.sh            | 3 ---
 scripts/common.bash     | 8 ++++++--
 scripts/mkstandalone.sh | 4 ----
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index 01e36dcfa06e..24aba9cc3a98 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -126,9 +126,6 @@ RUNTIME_log_stdout () {
 function run_task()
 {
 	local testname="$1"
-	if [ -z "$testname" ]; then
-		return
-	fi
 
 	while (( $(jobs | wc -l) == $unittest_run_queues )); do
 		# wait for any background test to finish
diff --git a/scripts/common.bash b/scripts/common.bash
index 9a6ebbd7f287..96655c9ffd1f 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -17,7 +17,9 @@ function for_each_unittest()
 
 	while read -r -u $fd line; do
 		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
-			"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+			if [ -n "${testname}" ]; then
+				"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+			fi
 			testname=${BASH_REMATCH[1]}
 			smp=1
 			kernel=""
@@ -45,6 +47,8 @@ function for_each_unittest()
 			timeout=${BASH_REMATCH[1]}
 		fi
 	done
-	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+	if [ -n "${testname}" ]; then
+		"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+	fi
 	exec {fd}<&-
 }
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 9d506cc95072..cefdec30cb33 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -83,10 +83,6 @@ function mkstandalone()
 {
 	local testname="$1"
 
-	if [ -z "$testname" ]; then
-		return
-	fi
-
 	if [ -n "$one_testname" ] && [ "$testname" != "$one_testname" ]; then
 		return
 	fi
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions
  2020-08-12  9:27 [kvm-unit-tests RFC v2 0/4] s390x: Add Protected VM support Marc Hartmayer
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 1/4] common.bash: run `cmd` only if a test case was found Marc Hartmayer
@ 2020-08-12  9:27 ` Marc Hartmayer
  2020-08-13  7:49   ` Andrew Jones
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest` Marc Hartmayer
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support Marc Hartmayer
  3 siblings, 1 reply; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-12  9:27 UTC (permalink / raw)
  To: kvm
  Cc: Thomas Huth, David Hildenbrand, Janosch Frank, Cornelia Huck,
	Andrew Jones, Paolo Bonzini, Christian Borntraeger, linux-s390

This is necessary to keep architecture dependent code separate from
common code.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 README.md           | 3 ++-
 scripts/common.bash | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 48be206c6db1..24d4bdaaee0d 100644
--- a/README.md
+++ b/README.md
@@ -134,7 +134,8 @@ all unit tests.
 ## Directory structure
 
     .:                  configure script, top-level Makefile, and run_tests.sh
-    ./scripts:          helper scripts for building and running tests
+    ./scripts:          general architecture neutral helper scripts for building and running tests
+    ./scripts/<ARCH>:   architecture dependent helper scripts for building and running tests
     ./lib:              general architecture neutral services for the tests
     ./lib/<ARCH>:       architecture dependent services for the tests
     ./<ARCH>:           the sources of the tests and the created objects/images
diff --git a/scripts/common.bash b/scripts/common.bash
index 96655c9ffd1f..f9c15fd304bd 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -52,3 +52,8 @@ function for_each_unittest()
 	fi
 	exec {fd}<&-
 }
+
+ARCH_FUNC=scripts/${ARCH}/func.bash
+if [ -f "${ARCH_FUNC}" ]; then
+	source "${ARCH_FUNC}"
+fi
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest`
  2020-08-12  9:27 [kvm-unit-tests RFC v2 0/4] s390x: Add Protected VM support Marc Hartmayer
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 1/4] common.bash: run `cmd` only if a test case was found Marc Hartmayer
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions Marc Hartmayer
@ 2020-08-12  9:27 ` Marc Hartmayer
  2020-08-13  8:30   ` Andrew Jones
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support Marc Hartmayer
  3 siblings, 1 reply; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-12  9:27 UTC (permalink / raw)
  To: kvm
  Cc: Thomas Huth, David Hildenbrand, Janosch Frank, Cornelia Huck,
	Andrew Jones, Paolo Bonzini, Christian Borntraeger, linux-s390

This allows us, for example, to auto generate a new test case based on
an existing test case.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 run_tests.sh            |  2 +-
 scripts/common.bash     | 13 +++++++++++++
 scripts/mkstandalone.sh |  2 +-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index 24aba9cc3a98..23658392c488 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT
    # preserve stdout so that process_test_output output can write TAP to it
    exec 3>&1
    test "$tap_output" == "yes" && exec > /dev/null
-   for_each_unittest $config run_task
+   for_each_unittest $config run_task arch_cmd
 ) | postprocess_suite_output
 
 # wait until all tasks finish
diff --git a/scripts/common.bash b/scripts/common.bash
index f9c15fd304bd..62931a40b79a 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -1,8 +1,15 @@
+function arch_cmd()
+{
+	# Dummy function, can be overwritten by architecture dependent
+	# code
+	return
+}
 
 function for_each_unittest()
 {
 	local unittests="$1"
 	local cmd="$2"
+	local arch_cmd="${3-}"
 	local testname
 	local smp
 	local kernel
@@ -19,6 +26,9 @@ function for_each_unittest()
 		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
 			if [ -n "${testname}" ]; then
 				"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+				if [ "${arch_cmd}" ]; then
+					"${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+				fi
 			fi
 			testname=${BASH_REMATCH[1]}
 			smp=1
@@ -49,6 +59,9 @@ function for_each_unittest()
 	done
 	if [ -n "${testname}" ]; then
 		"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+		if [ "${arch_cmd}" ]; then
+			"${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+		fi
 	fi
 	exec {fd}<&-
 }
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index cefdec30cb33..3b18c0cf090b 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -128,4 +128,4 @@ fi
 
 mkdir -p tests
 
-for_each_unittest $cfg mkstandalone
+for_each_unittest $cfg mkstandalone arch_cmd
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support
  2020-08-12  9:27 [kvm-unit-tests RFC v2 0/4] s390x: Add Protected VM support Marc Hartmayer
                   ` (2 preceding siblings ...)
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest` Marc Hartmayer
@ 2020-08-12  9:27 ` Marc Hartmayer
  2020-08-13 11:56   ` Cornelia Huck
  3 siblings, 1 reply; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-12  9:27 UTC (permalink / raw)
  To: kvm
  Cc: Thomas Huth, David Hildenbrand, Janosch Frank, Cornelia Huck,
	Andrew Jones, Paolo Bonzini, Christian Borntraeger, linux-s390

Add support for Protected Virtual Machine (PVM) tests. For starting a
PVM guest we must be able to generate a PVM image by using the
`genprotimg` tool from the s390-tools collection. This requires the
ability to pass a machine-specific host-key document, so the option
`--host-key-document` is added to the configure script.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 configure               |  8 ++++++++
 s390x/Makefile          | 17 +++++++++++++++--
 s390x/selftest.parmfile |  1 +
 s390x/unittests.cfg     |  1 +
 scripts/s390x/func.bash | 18 ++++++++++++++++++
 5 files changed, 43 insertions(+), 2 deletions(-)
 create mode 100644 s390x/selftest.parmfile
 create mode 100644 scripts/s390x/func.bash

diff --git a/configure b/configure
index f9d030fd2f03..aa528af72534 100755
--- a/configure
+++ b/configure
@@ -18,6 +18,7 @@ u32_long=
 vmm="qemu"
 errata_force=0
 erratatxt="$srcdir/errata.txt"
+host_key_document=
 
 usage() {
     cat <<-EOF
@@ -40,6 +41,8 @@ usage() {
 	                           no environ is provided by the user (enabled by default)
 	    --erratatxt=FILE       specify a file to use instead of errata.txt. Use
 	                           '--erratatxt=' to ensure no file is used.
+	    --host-key-document=HOST_KEY_DOCUMENT
+	                           host-key-document to use (s390x only)
 EOF
     exit 1
 }
@@ -92,6 +95,9 @@ while [[ "$1" = -* ]]; do
 	    erratatxt=
 	    [ "$arg" ] && erratatxt=$(eval realpath "$arg")
 	    ;;
+	--host-key-document)
+	    host_key_document="$arg"
+	    ;;
 	--help)
 	    usage
 	    ;;
@@ -205,6 +211,8 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
 ENVIRON_DEFAULT=$environ_default
 ERRATATXT=$erratatxt
 U32_LONG_FMT=$u32_long
+GENPROTIMG=${GENPROTIMG-genprotimg}
+HOST_KEY_DOCUMENT=$host_key_document
 EOF
 
 cat <<EOF > lib/config.h
diff --git a/s390x/Makefile b/s390x/Makefile
index 0f54bf43bfb7..cd4e270952ec 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -18,12 +18,19 @@ tests += $(TEST_DIR)/skrf.elf
 tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/sclp.elf
 tests += $(TEST_DIR)/css.elf
-tests_binary = $(patsubst %.elf,%.bin,$(tests))
 
-all: directories test_cases test_cases_binary
+tests_binary = $(patsubst %.elf,%.bin,$(tests))
+ifneq ($(HOST_KEY_DOCUMENT),)
+tests_pv_binary = $(patsubst %.bin,%.pv.bin,$(tests_binary))
+else
+tests_pv_binary =
+endif
+
+all: directories test_cases test_cases_binary test_cases_pv
 
 test_cases: $(tests)
 test_cases_binary: $(tests_binary)
+test_cases_pv: $(tests_pv_binary)
 
 CFLAGS += -std=gnu99
 CFLAGS += -ffreestanding
@@ -72,6 +79,12 @@ FLATLIBS = $(libcflat)
 %.bin: %.elf
 	$(OBJCOPY) -O binary  $< $@
 
+%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@)
+	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --no-verify --image $< -o $@
+
+%.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
+	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
+
 arch_clean: asm_offsets_clean
 	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d
 
diff --git a/s390x/selftest.parmfile b/s390x/selftest.parmfile
new file mode 100644
index 000000000000..5613931aa5c6
--- /dev/null
+++ b/s390x/selftest.parmfile
@@ -0,0 +1 @@
+test 123
\ No newline at end of file
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 0f156afbe741..12f6fb613995 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -21,6 +21,7 @@
 [selftest-setup]
 file = selftest.elf
 groups = selftest
+# please keep the kernel cmdline in sync with $(TEST_DIR)/selftest.parmfile
 extra_params = -append 'test 123'
 
 [intercept]
diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
new file mode 100644
index 000000000000..5c682cb47f73
--- /dev/null
+++ b/scripts/s390x/func.bash
@@ -0,0 +1,18 @@
+# Run Protected VM test
+function arch_cmd()
+{
+	local cmd=$1
+	local testname=$2
+	local groups=$3
+	local smp=$4
+	local kernel=$5
+	local opts=$6
+	local arch=$7
+	local check=$8
+	local accel=$9
+	local timeout=${10}
+
+	kernel=${kernel%.elf}.pv.bin
+	# do not run PV test cases by default
+	"$cmd" "${testname}_PV" "$groups pv nodefault" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+}
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 1/4] common.bash: run `cmd` only if a test case was found
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 1/4] common.bash: run `cmd` only if a test case was found Marc Hartmayer
@ 2020-08-13  7:40   ` Andrew Jones
  2020-08-13 11:37     ` Marc Hartmayer
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-08-13  7:40 UTC (permalink / raw)
  To: Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Wed, Aug 12, 2020 at 11:27:02AM +0200, Marc Hartmayer wrote:
> It's only useful to run `cmd` in `for_each_unittest` if a test case
> was found. This change allows us to remove the guards from the
> functions `run_task` and `mkstandalone`.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  run_tests.sh            | 3 ---
>  scripts/common.bash     | 8 ++++++--
>  scripts/mkstandalone.sh | 4 ----
>  3 files changed, 6 insertions(+), 9 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions Marc Hartmayer
@ 2020-08-13  7:49   ` Andrew Jones
  2020-08-13 11:45     ` Marc Hartmayer
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-08-13  7:49 UTC (permalink / raw)
  To: Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Wed, Aug 12, 2020 at 11:27:03AM +0200, Marc Hartmayer wrote:
> This is necessary to keep architecture dependent code separate from
> common code.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  README.md           | 3 ++-
>  scripts/common.bash | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/README.md b/README.md
> index 48be206c6db1..24d4bdaaee0d 100644
> --- a/README.md
> +++ b/README.md
> @@ -134,7 +134,8 @@ all unit tests.
>  ## Directory structure
>  
>      .:                  configure script, top-level Makefile, and run_tests.sh
> -    ./scripts:          helper scripts for building and running tests
> +    ./scripts:          general architecture neutral helper scripts for building and running tests
> +    ./scripts/<ARCH>:   architecture dependent helper scripts for building and running tests
>      ./lib:              general architecture neutral services for the tests
>      ./lib/<ARCH>:       architecture dependent services for the tests
>      ./<ARCH>:           the sources of the tests and the created objects/images
> diff --git a/scripts/common.bash b/scripts/common.bash
> index 96655c9ffd1f..f9c15fd304bd 100644
> --- a/scripts/common.bash
> +++ b/scripts/common.bash
> @@ -52,3 +52,8 @@ function for_each_unittest()
>  	fi
>  	exec {fd}<&-
>  }
> +
> +ARCH_FUNC=scripts/${ARCH}/func.bash

The use of ${ARCH} adds a dependency on config.mak. It works now because
in the two places we source common.bash we source config.mak first, but
I'd prefer we make that dependency explicit. We could probably just
source it again from this file.

Thanks,
drew

> +if [ -f "${ARCH_FUNC}" ]; then
> +	source "${ARCH_FUNC}"
> +fi
> -- 
> 2.25.4
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest`
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest` Marc Hartmayer
@ 2020-08-13  8:30   ` Andrew Jones
  2020-08-14 13:06     ` Marc Hartmayer
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-08-13  8:30 UTC (permalink / raw)
  To: Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote:
> This allows us, for example, to auto generate a new test case based on
> an existing test case.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  run_tests.sh            |  2 +-
>  scripts/common.bash     | 13 +++++++++++++
>  scripts/mkstandalone.sh |  2 +-
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/run_tests.sh b/run_tests.sh
> index 24aba9cc3a98..23658392c488 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT
>     # preserve stdout so that process_test_output output can write TAP to it
>     exec 3>&1
>     test "$tap_output" == "yes" && exec > /dev/null
> -   for_each_unittest $config run_task
> +   for_each_unittest $config run_task arch_cmd

Let's just require that arch cmd hook be specified by the "$arch_cmd"
variable. Then we don't need to pass it to for_each_unittest.

>  ) | postprocess_suite_output
>  
>  # wait until all tasks finish
> diff --git a/scripts/common.bash b/scripts/common.bash
> index f9c15fd304bd..62931a40b79a 100644
> --- a/scripts/common.bash
> +++ b/scripts/common.bash
> @@ -1,8 +1,15 @@
> +function arch_cmd()
> +{
> +	# Dummy function, can be overwritten by architecture dependent
> +	# code
> +	return
> +}

This dummy function appears unused and can be dropped.

>  
>  function for_each_unittest()
>  {
>  	local unittests="$1"
>  	local cmd="$2"
> +	local arch_cmd="${3-}"
>  	local testname
>  	local smp
>  	local kernel
> @@ -19,6 +26,9 @@ function for_each_unittest()
>  		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
>  			if [ -n "${testname}" ]; then
>  				"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +				if [ "${arch_cmd}" ]; then
> +					"${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +				fi

Rather than assuming we should run both $cmd ... and $arch_cmd $cmd ...,
let's just run $arch_cmd $cmd ..., when it exists. If $arch_cmd wants to
run $cmd ... first, then it can do so itself.

>  			fi
>  			testname=${BASH_REMATCH[1]}
>  			smp=1
> @@ -49,6 +59,9 @@ function for_each_unittest()
>  	done
>  	if [ -n "${testname}" ]; then
>  		"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +		if [ "${arch_cmd}" ]; then
> +			"${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +		fi
>  	fi
>  	exec {fd}<&-
>  }
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index cefdec30cb33..3b18c0cf090b 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -128,4 +128,4 @@ fi
>  
>  mkdir -p tests
>  
> -for_each_unittest $cfg mkstandalone
> +for_each_unittest $cfg mkstandalone arch_cmd
> -- 
> 2.25.4
>

In summary, I think this patch should just be

diff --git a/scripts/common.bash b/scripts/common.bash
index 9a6ebbd7f287..b409b0529ea6 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -17,7 +17,7 @@ function for_each_unittest()
 
        while read -r -u $fd line; do
                if [[ "$line" =~ ^\[(.*)\]$ ]]; then
-                       "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+                       "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
                        testname=${BASH_REMATCH[1]}
                        smp=1
                        kernel=""
@@ -45,6 +45,6 @@ function for_each_unittest()
                        timeout=${BASH_REMATCH[1]}
                fi
        done
-       "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+       "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
        exec {fd}<&-
 }
 

Thanks,
drew


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 1/4] common.bash: run `cmd` only if a test case was found
  2020-08-13  7:40   ` Andrew Jones
@ 2020-08-13 11:37     ` Marc Hartmayer
  0 siblings, 0 replies; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-13 11:37 UTC (permalink / raw)
  To: Andrew Jones; +Cc: kvm, linux-s390

On Thu, Aug 13, 2020 at 09:40 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Aug 12, 2020 at 11:27:02AM +0200, Marc Hartmayer wrote:
>> It's only useful to run `cmd` in `for_each_unittest` if a test case
>> was found. This change allows us to remove the guards from the
>> functions `run_task` and `mkstandalone`.
>> 
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  run_tests.sh            | 3 ---
>>  scripts/common.bash     | 8 ++++++--
>>  scripts/mkstandalone.sh | 4 ----
>>  3 files changed, 6 insertions(+), 9 deletions(-)
>>
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>

Thanks.

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions
  2020-08-13  7:49   ` Andrew Jones
@ 2020-08-13 11:45     ` Marc Hartmayer
  2020-08-13 12:07       ` Andrew Jones
  0 siblings, 1 reply; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-13 11:45 UTC (permalink / raw)
  To: Andrew Jones, Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Thu, Aug 13, 2020 at 09:49 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Aug 12, 2020 at 11:27:03AM +0200, Marc Hartmayer wrote:
>> This is necessary to keep architecture dependent code separate from
>> common code.
>> 
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  README.md           | 3 ++-
>>  scripts/common.bash | 5 +++++
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/README.md b/README.md
>> index 48be206c6db1..24d4bdaaee0d 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -134,7 +134,8 @@ all unit tests.
>>  ## Directory structure
>>  
>>      .:                  configure script, top-level Makefile, and run_tests.sh
>> -    ./scripts:          helper scripts for building and running tests
>> +    ./scripts:          general architecture neutral helper scripts for building and running tests
>> +    ./scripts/<ARCH>:   architecture dependent helper scripts for building and running tests
>>      ./lib:              general architecture neutral services for the tests
>>      ./lib/<ARCH>:       architecture dependent services for the tests
>>      ./<ARCH>:           the sources of the tests and the created objects/images
>> diff --git a/scripts/common.bash b/scripts/common.bash
>> index 96655c9ffd1f..f9c15fd304bd 100644
>> --- a/scripts/common.bash
>> +++ b/scripts/common.bash
>> @@ -52,3 +52,8 @@ function for_each_unittest()
>>  	fi
>>  	exec {fd}<&-
>>  }
>> +
>> +ARCH_FUNC=scripts/${ARCH}/func.bash
>
> The use of ${ARCH} adds a dependency on config.mak. It works now because
> in the two places we source common.bash we source config.mak first

Yep, I know.

> , but
> I'd prefer we make that dependency explicit.

Okay.

> We could probably just
> source it again from this file.

Another option is to pass ${ARCH} as an argument when we `source
scripts/runtime.bash`

=> `source scripts/runtime.bash "${ARCH}"`

Which one do you prefer?

>
> Thanks,
> drew
>
>> +if [ -f "${ARCH_FUNC}" ]; then
>> +	source "${ARCH_FUNC}"
>> +fi
>> -- 
>> 2.25.4
>> 
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support
  2020-08-12  9:27 ` [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support Marc Hartmayer
@ 2020-08-13 11:56   ` Cornelia Huck
  2020-08-13 13:08     ` Marc Hartmayer
  0 siblings, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2020-08-13 11:56 UTC (permalink / raw)
  To: Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank, Andrew Jones,
	Paolo Bonzini, Christian Borntraeger, linux-s390

On Wed, 12 Aug 2020 11:27:05 +0200
Marc Hartmayer <mhartmay@linux.ibm.com> wrote:

> Add support for Protected Virtual Machine (PVM) tests. For starting a
> PVM guest we must be able to generate a PVM image by using the
> `genprotimg` tool from the s390-tools collection. This requires the
> ability to pass a machine-specific host-key document, so the option
> `--host-key-document` is added to the configure script.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  configure               |  8 ++++++++
>  s390x/Makefile          | 17 +++++++++++++++--
>  s390x/selftest.parmfile |  1 +
>  s390x/unittests.cfg     |  1 +
>  scripts/s390x/func.bash | 18 ++++++++++++++++++
>  5 files changed, 43 insertions(+), 2 deletions(-)
>  create mode 100644 s390x/selftest.parmfile
>  create mode 100644 scripts/s390x/func.bash
> 
> diff --git a/configure b/configure
> index f9d030fd2f03..aa528af72534 100755
> --- a/configure
> +++ b/configure
> @@ -18,6 +18,7 @@ u32_long=
>  vmm="qemu"
>  errata_force=0
>  erratatxt="$srcdir/errata.txt"
> +host_key_document=
>  
>  usage() {
>      cat <<-EOF
> @@ -40,6 +41,8 @@ usage() {
>  	                           no environ is provided by the user (enabled by default)
>  	    --erratatxt=FILE       specify a file to use instead of errata.txt. Use
>  	                           '--erratatxt=' to ensure no file is used.
> +	    --host-key-document=HOST_KEY_DOCUMENT
> +	                           host-key-document to use (s390x only)

Maybe a bit more verbose? If I see only this option, I have no idea
what it is used for and where to get it.

>  EOF
>      exit 1
>  }
> @@ -92,6 +95,9 @@ while [[ "$1" = -* ]]; do
>  	    erratatxt=
>  	    [ "$arg" ] && erratatxt=$(eval realpath "$arg")
>  	    ;;
> +	--host-key-document)
> +	    host_key_document="$arg"
> +	    ;;
>  	--help)
>  	    usage
>  	    ;;
> @@ -205,6 +211,8 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
>  ENVIRON_DEFAULT=$environ_default
>  ERRATATXT=$erratatxt
>  U32_LONG_FMT=$u32_long
> +GENPROTIMG=${GENPROTIMG-genprotimg}
> +HOST_KEY_DOCUMENT=$host_key_document
>  EOF
>  
>  cat <<EOF > lib/config.h
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 0f54bf43bfb7..cd4e270952ec 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -18,12 +18,19 @@ tests += $(TEST_DIR)/skrf.elf
>  tests += $(TEST_DIR)/smp.elf
>  tests += $(TEST_DIR)/sclp.elf
>  tests += $(TEST_DIR)/css.elf
> -tests_binary = $(patsubst %.elf,%.bin,$(tests))
>  
> -all: directories test_cases test_cases_binary
> +tests_binary = $(patsubst %.elf,%.bin,$(tests))
> +ifneq ($(HOST_KEY_DOCUMENT),)
> +tests_pv_binary = $(patsubst %.bin,%.pv.bin,$(tests_binary))
> +else
> +tests_pv_binary =
> +endif
> +
> +all: directories test_cases test_cases_binary test_cases_pv
>  
>  test_cases: $(tests)
>  test_cases_binary: $(tests_binary)
> +test_cases_pv: $(tests_pv_binary)
>  
>  CFLAGS += -std=gnu99
>  CFLAGS += -ffreestanding
> @@ -72,6 +79,12 @@ FLATLIBS = $(libcflat)
>  %.bin: %.elf
>  	$(OBJCOPY) -O binary  $< $@
>  
> +%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@)
> +	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --no-verify --image $< -o $@
> +
> +%.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
> +	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
> +
>  arch_clean: asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d
>  
> diff --git a/s390x/selftest.parmfile b/s390x/selftest.parmfile
> new file mode 100644
> index 000000000000..5613931aa5c6
> --- /dev/null
> +++ b/s390x/selftest.parmfile
> @@ -0,0 +1 @@
> +test 123
> \ No newline at end of file

Maybe add one? :)

> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 0f156afbe741..12f6fb613995 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -21,6 +21,7 @@
>  [selftest-setup]
>  file = selftest.elf
>  groups = selftest
> +# please keep the kernel cmdline in sync with $(TEST_DIR)/selftest.parmfile
>  extra_params = -append 'test 123'
>  
>  [intercept]
> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
> new file mode 100644
> index 000000000000..5c682cb47f73
> --- /dev/null
> +++ b/scripts/s390x/func.bash
> @@ -0,0 +1,18 @@
> +# Run Protected VM test
> +function arch_cmd()
> +{
> +	local cmd=$1
> +	local testname=$2
> +	local groups=$3
> +	local smp=$4
> +	local kernel=$5
> +	local opts=$6
> +	local arch=$7
> +	local check=$8
> +	local accel=$9
> +	local timeout=${10}
> +
> +	kernel=${kernel%.elf}.pv.bin
> +	# do not run PV test cases by default
> +	"$cmd" "${testname}_PV" "$groups pv nodefault" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"

If we don't run this test, can we maybe print some informative message
like "PV tests not run; specify --host-key-document to enable" or so?
(At whichever point that makes the most sense.)

> +}


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions
  2020-08-13 11:45     ` Marc Hartmayer
@ 2020-08-13 12:07       ` Andrew Jones
  2020-08-13 12:36         ` Marc Hartmayer
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-08-13 12:07 UTC (permalink / raw)
  To: Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Thu, Aug 13, 2020 at 01:45:46PM +0200, Marc Hartmayer wrote:
> On Thu, Aug 13, 2020 at 09:49 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
> > On Wed, Aug 12, 2020 at 11:27:03AM +0200, Marc Hartmayer wrote:
> >> This is necessary to keep architecture dependent code separate from
> >> common code.
> >> 
> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> >> ---
> >>  README.md           | 3 ++-
> >>  scripts/common.bash | 5 +++++
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/README.md b/README.md
> >> index 48be206c6db1..24d4bdaaee0d 100644
> >> --- a/README.md
> >> +++ b/README.md
> >> @@ -134,7 +134,8 @@ all unit tests.
> >>  ## Directory structure
> >>  
> >>      .:                  configure script, top-level Makefile, and run_tests.sh
> >> -    ./scripts:          helper scripts for building and running tests
> >> +    ./scripts:          general architecture neutral helper scripts for building and running tests
> >> +    ./scripts/<ARCH>:   architecture dependent helper scripts for building and running tests
> >>      ./lib:              general architecture neutral services for the tests
> >>      ./lib/<ARCH>:       architecture dependent services for the tests
> >>      ./<ARCH>:           the sources of the tests and the created objects/images
> >> diff --git a/scripts/common.bash b/scripts/common.bash
> >> index 96655c9ffd1f..f9c15fd304bd 100644
> >> --- a/scripts/common.bash
> >> +++ b/scripts/common.bash
> >> @@ -52,3 +52,8 @@ function for_each_unittest()
> >>  	fi
> >>  	exec {fd}<&-
> >>  }
> >> +
> >> +ARCH_FUNC=scripts/${ARCH}/func.bash
> >
> > The use of ${ARCH} adds a dependency on config.mak. It works now because
> > in the two places we source common.bash we source config.mak first
> 
> Yep, I know.
> 
> > , but
> > I'd prefer we make that dependency explicit.
> 
> Okay.
> 
> > We could probably just
> > source it again from this file.
> 
> Another option is to pass ${ARCH} as an argument when we `source
> scripts/runtime.bash`
> 
> => `source scripts/runtime.bash "${ARCH}"`
> 
> Which one do you prefer?

The first one. There's a chance that the arch helper functions will
need more than $ARCH from config.mak. Of course that means we have
a dependency on config.mak from the arch helper file too. We can
just add a comment in common.bash about the order of sourcing
though, as common.bash should be the only file sourcing the
arch helper file.

Thanks,
drew

> 
> >
> > Thanks,
> > drew
> >
> >> +if [ -f "${ARCH_FUNC}" ]; then
> >> +	source "${ARCH_FUNC}"
> >> +fi
> >> -- 
> >> 2.25.4
> >> 
> >
> -- 
> Kind regards / Beste Grüße
>    Marc Hartmayer
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzender des Aufsichtsrats: Gregor Pillen 
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions
  2020-08-13 12:07       ` Andrew Jones
@ 2020-08-13 12:36         ` Marc Hartmayer
  0 siblings, 0 replies; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-13 12:36 UTC (permalink / raw)
  To: Andrew Jones, Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Thu, Aug 13, 2020 at 02:07 PM +0200, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Aug 13, 2020 at 01:45:46PM +0200, Marc Hartmayer wrote:
>> On Thu, Aug 13, 2020 at 09:49 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
>> > On Wed, Aug 12, 2020 at 11:27:03AM +0200, Marc Hartmayer wrote:
>> >> This is necessary to keep architecture dependent code separate from
>> >> common code.
>> >> 
>> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> >> ---
>> >>  README.md           | 3 ++-
>> >>  scripts/common.bash | 5 +++++
>> >>  2 files changed, 7 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/README.md b/README.md
>> >> index 48be206c6db1..24d4bdaaee0d 100644
>> >> --- a/README.md
>> >> +++ b/README.md
>> >> @@ -134,7 +134,8 @@ all unit tests.
>> >>  ## Directory structure
>> >>  
>> >>      .:                  configure script, top-level Makefile, and run_tests.sh
>> >> -    ./scripts:          helper scripts for building and running tests
>> >> +    ./scripts:          general architecture neutral helper scripts for building and running tests
>> >> +    ./scripts/<ARCH>:   architecture dependent helper scripts for building and running tests
>> >>      ./lib:              general architecture neutral services for the tests
>> >>      ./lib/<ARCH>:       architecture dependent services for the tests
>> >>      ./<ARCH>:           the sources of the tests and the created objects/images
>> >> diff --git a/scripts/common.bash b/scripts/common.bash
>> >> index 96655c9ffd1f..f9c15fd304bd 100644
>> >> --- a/scripts/common.bash
>> >> +++ b/scripts/common.bash
>> >> @@ -52,3 +52,8 @@ function for_each_unittest()
>> >>  	fi
>> >>  	exec {fd}<&-
>> >>  }
>> >> +
>> >> +ARCH_FUNC=scripts/${ARCH}/func.bash
>> >
>> > The use of ${ARCH} adds a dependency on config.mak. It works now because
>> > in the two places we source common.bash we source config.mak first
>> 
>> Yep, I know.
>> 
>> > , but
>> > I'd prefer we make that dependency explicit.
>> 
>> Okay.
>> 
>> > We could probably just
>> > source it again from this file.
>> 
>> Another option is to pass ${ARCH} as an argument when we `source
>> scripts/runtime.bash`
>> 
>> => `source scripts/runtime.bash "${ARCH}"`
>> 
>> Which one do you prefer?
>
> The first one. There's a chance that the arch helper functions will
> need more than $ARCH from config.mak. Of course that means we have
> a dependency on config.mak from the arch helper file too. We can
> just add a comment in common.bash about the order of sourcing
> though, as common.bash should be the only file sourcing the
> arch helper file.

Will add it. Thanks!

>
> Thanks,
> drew
>
>> 
>> >
>> > Thanks,
>> > drew
>> >
>> >> +if [ -f "${ARCH_FUNC}" ]; then
>> >> +	source "${ARCH_FUNC}"
>> >> +fi
>> >> -- 
>> >> 2.25.4
>> >> 
>> >
>> -- 
>> Kind regards / Beste Grüße
>>    Marc Hartmayer
>> 
>> IBM Deutschland Research & Development GmbH
>> Vorsitzender des Aufsichtsrats: Gregor Pillen 
>> Geschäftsführung: Dirk Wittkopp
>> Sitz der Gesellschaft: Böblingen
>> Registergericht: Amtsgericht Stuttgart, HRB 243294
>> 
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support
  2020-08-13 11:56   ` Cornelia Huck
@ 2020-08-13 13:08     ` Marc Hartmayer
  2020-08-13 14:22       ` Cornelia Huck
  0 siblings, 1 reply; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-13 13:08 UTC (permalink / raw)
  To: Cornelia Huck, Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank, Andrew Jones,
	Paolo Bonzini, Christian Borntraeger, linux-s390

On Thu, Aug 13, 2020 at 01:56 PM +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> On Wed, 12 Aug 2020 11:27:05 +0200
> Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
>
>> Add support for Protected Virtual Machine (PVM) tests. For starting a
>> PVM guest we must be able to generate a PVM image by using the
>> `genprotimg` tool from the s390-tools collection. This requires the
>> ability to pass a machine-specific host-key document, so the option
>> `--host-key-document` is added to the configure script.
>> 
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  configure               |  8 ++++++++
>>  s390x/Makefile          | 17 +++++++++++++++--
>>  s390x/selftest.parmfile |  1 +
>>  s390x/unittests.cfg     |  1 +
>>  scripts/s390x/func.bash | 18 ++++++++++++++++++
>>  5 files changed, 43 insertions(+), 2 deletions(-)
>>  create mode 100644 s390x/selftest.parmfile
>>  create mode 100644 scripts/s390x/func.bash
>> 
>> diff --git a/configure b/configure
>> index f9d030fd2f03..aa528af72534 100755
>> --- a/configure
>> +++ b/configure
>> @@ -18,6 +18,7 @@ u32_long=
>>  vmm="qemu"
>>  errata_force=0
>>  erratatxt="$srcdir/errata.txt"
>> +host_key_document=
>>  
>>  usage() {
>>      cat <<-EOF
>> @@ -40,6 +41,8 @@ usage() {
>>  	                           no environ is provided by the user (enabled by default)
>>  	    --erratatxt=FILE       specify a file to use instead of errata.txt. Use
>>  	                           '--erratatxt=' to ensure no file is used.
>> +	    --host-key-document=HOST_KEY_DOCUMENT
>> +	                           host-key-document to use (s390x only)
>
> Maybe a bit more verbose? If I see only this option, I have no idea
> what it is used for and where to get it.

“Specifies the machine-specific host-key document required to create a
PVM image using the `genprotimg` tool from the s390-tools collection
(s390x only)”

Better?

>
>>  EOF
>>      exit 1
>>  }
>> @@ -92,6 +95,9 @@ while [[ "$1" = -* ]]; do
>>  	    erratatxt=
>>  	    [ "$arg" ] && erratatxt=$(eval realpath "$arg")
>>  	    ;;
>> +	--host-key-document)
>> +	    host_key_document="$arg"
>> +	    ;;
>>  	--help)
>>  	    usage
>>  	    ;;
>> @@ -205,6 +211,8 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
>>  ENVIRON_DEFAULT=$environ_default
>>  ERRATATXT=$erratatxt
>>  U32_LONG_FMT=$u32_long
>> +GENPROTIMG=${GENPROTIMG-genprotimg}
>> +HOST_KEY_DOCUMENT=$host_key_document
>>  EOF
>>  
>>  cat <<EOF > lib/config.h
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 0f54bf43bfb7..cd4e270952ec 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -18,12 +18,19 @@ tests += $(TEST_DIR)/skrf.elf
>>  tests += $(TEST_DIR)/smp.elf
>>  tests += $(TEST_DIR)/sclp.elf
>>  tests += $(TEST_DIR)/css.elf
>> -tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>  
>> -all: directories test_cases test_cases_binary
>> +tests_binary = $(patsubst %.elf,%.bin,$(tests))
>> +ifneq ($(HOST_KEY_DOCUMENT),)
>> +tests_pv_binary = $(patsubst %.bin,%.pv.bin,$(tests_binary))
>> +else
>> +tests_pv_binary =
>> +endif
>> +
>> +all: directories test_cases test_cases_binary test_cases_pv
>>  
>>  test_cases: $(tests)
>>  test_cases_binary: $(tests_binary)
>> +test_cases_pv: $(tests_pv_binary)
>>  
>>  CFLAGS += -std=gnu99
>>  CFLAGS += -ffreestanding
>> @@ -72,6 +79,12 @@ FLATLIBS = $(libcflat)
>>  %.bin: %.elf
>>  	$(OBJCOPY) -O binary  $< $@
>>  
>> +%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@)
>> +	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --no-verify --image $< -o $@
>> +
>> +%.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
>> +	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
>> +
>>  arch_clean: asm_offsets_clean
>>  	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d
>>  
>> diff --git a/s390x/selftest.parmfile b/s390x/selftest.parmfile
>> new file mode 100644
>> index 000000000000..5613931aa5c6
>> --- /dev/null
>> +++ b/s390x/selftest.parmfile
>> @@ -0,0 +1 @@
>> +test 123
>> \ No newline at end of file
>
> Maybe add one? :)

No, this’s intended, otherwise the test case fails.

>
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index 0f156afbe741..12f6fb613995 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -21,6 +21,7 @@
>>  [selftest-setup]
>>  file = selftest.elf
>>  groups = selftest
>> +# please keep the kernel cmdline in sync with $(TEST_DIR)/selftest.parmfile
>>  extra_params = -append 'test 123'
>>  
>>  [intercept]
>> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
>> new file mode 100644
>> index 000000000000..5c682cb47f73
>> --- /dev/null
>> +++ b/scripts/s390x/func.bash
>> @@ -0,0 +1,18 @@
>> +# Run Protected VM test
>> +function arch_cmd()
>> +{
>> +	local cmd=$1
>> +	local testname=$2
>> +	local groups=$3
>> +	local smp=$4
>> +	local kernel=$5
>> +	local opts=$6
>> +	local arch=$7
>> +	local check=$8
>> +	local accel=$9
>> +	local timeout=${10}
>> +
>> +	kernel=${kernel%.elf}.pv.bin
>> +	# do not run PV test cases by default
>> +	"$cmd" "${testname}_PV" "$groups pv nodefault" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>
> If we don't run this test, can we maybe print some informative message
> like "PV tests not run; specify --host-key-document to enable" or so?
> (At whichever point that makes the most sense.)

Currently, the output looks like this:

$ ./run_tests.sh    
PASS selftest-setup (14 tests)
SKIP selftest-setup_PV (test marked as manual run only)
PASS intercept (20 tests)
SKIP intercept_PV (test marked as manual run only)
…

And if you’re trying to run the PV tests without specifying the host-key
document it results in:

$ ./run_tests.sh -a
PASS selftest-setup (14 tests)
FAIL selftest-setup_PV 
PASS intercept (20 tests)
FAIL intercept_PV 
…

But if you like I can return a hint that the PVM image was not
generated. Should the PV test case then be skipped?

>
>> +}
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support
  2020-08-13 13:08     ` Marc Hartmayer
@ 2020-08-13 14:22       ` Cornelia Huck
  2020-08-13 15:13         ` Marc Hartmayer
  0 siblings, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2020-08-13 14:22 UTC (permalink / raw)
  To: Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank, Andrew Jones,
	Paolo Bonzini, Christian Borntraeger, linux-s390

On Thu, 13 Aug 2020 15:08:51 +0200
Marc Hartmayer <mhartmay@linux.ibm.com> wrote:

> On Thu, Aug 13, 2020 at 01:56 PM +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> > On Wed, 12 Aug 2020 11:27:05 +0200
> > Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
> >  
> >> Add support for Protected Virtual Machine (PVM) tests. For starting a
> >> PVM guest we must be able to generate a PVM image by using the
> >> `genprotimg` tool from the s390-tools collection. This requires the
> >> ability to pass a machine-specific host-key document, so the option
> >> `--host-key-document` is added to the configure script.
> >> 
> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> >> ---
> >>  configure               |  8 ++++++++
> >>  s390x/Makefile          | 17 +++++++++++++++--
> >>  s390x/selftest.parmfile |  1 +
> >>  s390x/unittests.cfg     |  1 +
> >>  scripts/s390x/func.bash | 18 ++++++++++++++++++
> >>  5 files changed, 43 insertions(+), 2 deletions(-)
> >>  create mode 100644 s390x/selftest.parmfile
> >>  create mode 100644 scripts/s390x/func.bash
> >> 
> >> diff --git a/configure b/configure
> >> index f9d030fd2f03..aa528af72534 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -18,6 +18,7 @@ u32_long=
> >>  vmm="qemu"
> >>  errata_force=0
> >>  erratatxt="$srcdir/errata.txt"
> >> +host_key_document=
> >>  
> >>  usage() {
> >>      cat <<-EOF
> >> @@ -40,6 +41,8 @@ usage() {
> >>  	                           no environ is provided by the user (enabled by default)
> >>  	    --erratatxt=FILE       specify a file to use instead of errata.txt. Use
> >>  	                           '--erratatxt=' to ensure no file is used.
> >> +	    --host-key-document=HOST_KEY_DOCUMENT
> >> +	                           host-key-document to use (s390x only)  
> >
> > Maybe a bit more verbose? If I see only this option, I have no idea
> > what it is used for and where to get it.  
> 
> “Specifies the machine-specific host-key document required to create a
> PVM image using the `genprotimg` tool from the s390-tools collection
> (s390x only)”
> 
> Better?

"specify the machine-specific host-key document for creating a PVM
image with 'genprotimg' (s390x only)"

I think you can figure out where to get genprotimg if you actually know
that you want it ;)

(...)

> >> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
> >> new file mode 100644
> >> index 000000000000..5c682cb47f73
> >> --- /dev/null
> >> +++ b/scripts/s390x/func.bash
> >> @@ -0,0 +1,18 @@
> >> +# Run Protected VM test
> >> +function arch_cmd()
> >> +{
> >> +	local cmd=$1
> >> +	local testname=$2
> >> +	local groups=$3
> >> +	local smp=$4
> >> +	local kernel=$5
> >> +	local opts=$6
> >> +	local arch=$7
> >> +	local check=$8
> >> +	local accel=$9
> >> +	local timeout=${10}
> >> +
> >> +	kernel=${kernel%.elf}.pv.bin
> >> +	# do not run PV test cases by default
> >> +	"$cmd" "${testname}_PV" "$groups pv nodefault" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"  
> >
> > If we don't run this test, can we maybe print some informative message
> > like "PV tests not run; specify --host-key-document to enable" or so?
> > (At whichever point that makes the most sense.)  
> 
> Currently, the output looks like this:
> 
> $ ./run_tests.sh    
> PASS selftest-setup (14 tests)
> SKIP selftest-setup_PV (test marked as manual run only)
> PASS intercept (20 tests)
> SKIP intercept_PV (test marked as manual run only)
> …
> 
> And if you’re trying to run the PV tests without specifying the host-key
> document it results in:
> 
> $ ./run_tests.sh -a
> PASS selftest-setup (14 tests)
> FAIL selftest-setup_PV 
> PASS intercept (20 tests)
> FAIL intercept_PV 
> …
> 
> But if you like I can return a hint that the PVM image was not
> generated. Should the PV test case then be skipped?

Yes, I was expecting something like

SKIP selftest-setup_PV (no host-key document specified)
SKIP intercept_PV (no host-key document specified)

so that you get a hint what you may want to set up.


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support
  2020-08-13 14:22       ` Cornelia Huck
@ 2020-08-13 15:13         ` Marc Hartmayer
  0 siblings, 0 replies; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-13 15:13 UTC (permalink / raw)
  To: Cornelia Huck, Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank, Andrew Jones,
	Paolo Bonzini, Christian Borntraeger, linux-s390

On Thu, Aug 13, 2020 at 04:22 PM +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> On Thu, 13 Aug 2020 15:08:51 +0200
> Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
>
>> On Thu, Aug 13, 2020 at 01:56 PM +0200, Cornelia Huck <cohuck@redhat.com> wrote:
>> > On Wed, 12 Aug 2020 11:27:05 +0200
>> > Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
>> >  
>> >> Add support for Protected Virtual Machine (PVM) tests. For starting a
>> >> PVM guest we must be able to generate a PVM image by using the
>> >> `genprotimg` tool from the s390-tools collection. This requires the
>> >> ability to pass a machine-specific host-key document, so the option
>> >> `--host-key-document` is added to the configure script.
>> >> 
>> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> >> ---
>> >>  configure               |  8 ++++++++
>> >>  s390x/Makefile          | 17 +++++++++++++++--
>> >>  s390x/selftest.parmfile |  1 +
>> >>  s390x/unittests.cfg     |  1 +
>> >>  scripts/s390x/func.bash | 18 ++++++++++++++++++
>> >>  5 files changed, 43 insertions(+), 2 deletions(-)
>> >>  create mode 100644 s390x/selftest.parmfile
>> >>  create mode 100644 scripts/s390x/func.bash
>> >> 
>> >> diff --git a/configure b/configure
>> >> index f9d030fd2f03..aa528af72534 100755
>> >> --- a/configure
>> >> +++ b/configure
>> >> @@ -18,6 +18,7 @@ u32_long=
>> >>  vmm="qemu"
>> >>  errata_force=0
>> >>  erratatxt="$srcdir/errata.txt"
>> >> +host_key_document=
>> >>  
>> >>  usage() {
>> >>      cat <<-EOF
>> >> @@ -40,6 +41,8 @@ usage() {
>> >>  	                           no environ is provided by the user (enabled by default)
>> >>  	    --erratatxt=FILE       specify a file to use instead of errata.txt. Use
>> >>  	                           '--erratatxt=' to ensure no file is used.
>> >> +	    --host-key-document=HOST_KEY_DOCUMENT
>> >> +	                           host-key-document to use (s390x only)  
>> >
>> > Maybe a bit more verbose? If I see only this option, I have no idea
>> > what it is used for and where to get it.  
>> 
>> “Specifies the machine-specific host-key document required to create a
>> PVM image using the `genprotimg` tool from the s390-tools collection
>> (s390x only)”
>> 
>> Better?
>
> "specify the machine-specific host-key document for creating a PVM
> image with 'genprotimg' (s390x only)"
>
> I think you can figure out where to get genprotimg if you actually know
> that you want it ;)
>
> (...)
>
>> >> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
>> >> new file mode 100644
>> >> index 000000000000..5c682cb47f73
>> >> --- /dev/null
>> >> +++ b/scripts/s390x/func.bash
>> >> @@ -0,0 +1,18 @@
>> >> +# Run Protected VM test
>> >> +function arch_cmd()
>> >> +{
>> >> +	local cmd=$1
>> >> +	local testname=$2
>> >> +	local groups=$3
>> >> +	local smp=$4
>> >> +	local kernel=$5
>> >> +	local opts=$6
>> >> +	local arch=$7
>> >> +	local check=$8
>> >> +	local accel=$9
>> >> +	local timeout=${10}
>> >> +
>> >> +	kernel=${kernel%.elf}.pv.bin
>> >> +	# do not run PV test cases by default
>> >> +	"$cmd" "${testname}_PV" "$groups pv nodefault" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"  
>> >
>> > If we don't run this test, can we maybe print some informative message
>> > like "PV tests not run; specify --host-key-document to enable" or so?
>> > (At whichever point that makes the most sense.)  
>> 
>> Currently, the output looks like this:
>> 
>> $ ./run_tests.sh    
>> PASS selftest-setup (14 tests)
>> SKIP selftest-setup_PV (test marked as manual run only)
>> PASS intercept (20 tests)
>> SKIP intercept_PV (test marked as manual run only)
>> …
>> 
>> And if you’re trying to run the PV tests without specifying the host-key
>> document it results in:
>> 
>> $ ./run_tests.sh -a
>> PASS selftest-setup (14 tests)
>> FAIL selftest-setup_PV 
>> PASS intercept (20 tests)
>> FAIL intercept_PV 
>> …
>> 
>> But if you like I can return a hint that the PVM image was not
>> generated. Should the PV test case then be skipped?
>
> Yes, I was expecting something like
>
> SKIP selftest-setup_PV (no host-key document specified)
> SKIP intercept_PV (no host-key document specified)
>
> so that you get a hint what you may want to set up.

Okay, I’ll add this and remove the marker that PV tests aren’t executed
by default. Because when someone builds the PV images, he will most
likely want to run the PV test cases as well.

>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest`
  2020-08-13  8:30   ` Andrew Jones
@ 2020-08-14 13:06     ` Marc Hartmayer
  2020-08-14 13:29       ` Andrew Jones
  0 siblings, 1 reply; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-14 13:06 UTC (permalink / raw)
  To: Andrew Jones, Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote:
>> This allows us, for example, to auto generate a new test case based on
>> an existing test case.
>> 
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  run_tests.sh            |  2 +-
>>  scripts/common.bash     | 13 +++++++++++++
>>  scripts/mkstandalone.sh |  2 +-
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/run_tests.sh b/run_tests.sh
>> index 24aba9cc3a98..23658392c488 100755
>> --- a/run_tests.sh
>> +++ b/run_tests.sh
>> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT
>>     # preserve stdout so that process_test_output output can write TAP to it
>>     exec 3>&1
>>     test "$tap_output" == "yes" && exec > /dev/null
>> -   for_each_unittest $config run_task
>> +   for_each_unittest $config run_task arch_cmd
>
> Let's just require that arch cmd hook be specified by the "$arch_cmd"
> variable. Then we don't need to pass it to for_each_unittest.

Where is it then specified?

>
>>  ) | postprocess_suite_output
>>  
>>  # wait until all tasks finish
>> diff --git a/scripts/common.bash b/scripts/common.bash
>> index f9c15fd304bd..62931a40b79a 100644
>> --- a/scripts/common.bash
>> +++ b/scripts/common.bash
>> @@ -1,8 +1,15 @@
>> +function arch_cmd()
>> +{
>> +	# Dummy function, can be overwritten by architecture dependent
>> +	# code
>> +	return
>> +}
>
> This dummy function appears unused and can be dropped.

So what is then used if the function is not defined by the architecture
specific code?

>
>>  
>>  function for_each_unittest()
>>  {
>>  	local unittests="$1"
>>  	local cmd="$2"
>> +	local arch_cmd="${3-}"
>>  	local testname
>>  	local smp
>>  	local kernel
>> @@ -19,6 +26,9 @@ function for_each_unittest()
>>  		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
>>  			if [ -n "${testname}" ]; then
>>  				"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>> +				if [ "${arch_cmd}" ]; then
>> +					"${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>> +				fi
>
> Rather than assuming we should run both $cmd ... and $arch_cmd $cmd ...,
> let's just run $arch_cmd $cmd ..., when it exists. If $arch_cmd wants to
> run $cmd ... first, then it can do so itself.

Yep, makes sense.

>
>>  			fi
>>  			testname=${BASH_REMATCH[1]}
>>  			smp=1
>> @@ -49,6 +59,9 @@ function for_each_unittest()
>>  	done
>>  	if [ -n "${testname}" ]; then
>>  		"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>> +		if [ "${arch_cmd}" ]; then
>> +			"${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>> +		fi
>>  	fi
>>  	exec {fd}<&-
>>  }
>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> index cefdec30cb33..3b18c0cf090b 100755
>> --- a/scripts/mkstandalone.sh
>> +++ b/scripts/mkstandalone.sh
>> @@ -128,4 +128,4 @@ fi
>>  
>>  mkdir -p tests
>>  
>> -for_each_unittest $cfg mkstandalone
>> +for_each_unittest $cfg mkstandalone arch_cmd
>> -- 
>> 2.25.4
>>
>
> In summary, I think this patch should just be
>
> diff --git a/scripts/common.bash b/scripts/common.bash
> index 9a6ebbd7f287..b409b0529ea6 100644
> --- a/scripts/common.bash
> +++ b/scripts/common.bash
> @@ -17,7 +17,7 @@ function for_each_unittest()
>  
>         while read -r -u $fd line; do
>                 if [[ "$line" =~ ^\[(.*)\]$ ]]; then
> -                       "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +                       "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"

If we remove the $arch_cmd variable we can directly use:

arch_cmd "$cmd" …

>                         testname=${BASH_REMATCH[1]}
>                         smp=1
>                         kernel=""
> @@ -45,6 +45,6 @@ function for_each_unittest()
>                         timeout=${BASH_REMATCH[1]}
>                 fi
>         done
> -       "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +       "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>         exec {fd}<&-
>  }
>  
>
> Thanks,
> drew
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest`
  2020-08-14 13:06     ` Marc Hartmayer
@ 2020-08-14 13:29       ` Andrew Jones
  2020-08-18  9:03         ` Marc Hartmayer
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-08-14 13:29 UTC (permalink / raw)
  To: Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Fri, Aug 14, 2020 at 03:06:36PM +0200, Marc Hartmayer wrote:
> On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
> > On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote:
> >> This allows us, for example, to auto generate a new test case based on
> >> an existing test case.
> >> 
> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> >> ---
> >>  run_tests.sh            |  2 +-
> >>  scripts/common.bash     | 13 +++++++++++++
> >>  scripts/mkstandalone.sh |  2 +-
> >>  3 files changed, 15 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/run_tests.sh b/run_tests.sh
> >> index 24aba9cc3a98..23658392c488 100755
> >> --- a/run_tests.sh
> >> +++ b/run_tests.sh
> >> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT
> >>     # preserve stdout so that process_test_output output can write TAP to it
> >>     exec 3>&1
> >>     test "$tap_output" == "yes" && exec > /dev/null
> >> -   for_each_unittest $config run_task
> >> +   for_each_unittest $config run_task arch_cmd
> >
> > Let's just require that arch cmd hook be specified by the "$arch_cmd"
> > variable. Then we don't need to pass it to for_each_unittest.
> 
> Where is it then specified?

Just using it that way in the source is enough. We should probably call
it $ARCH_CMD to indicate that it's a special variable. Also, we could
return it from a $(arch_cmd) function, which is how $(migration_cmd) and
$(timeout_cmd) work.

> 
> >
> >>  ) | postprocess_suite_output
> >>  
> >>  # wait until all tasks finish
> >> diff --git a/scripts/common.bash b/scripts/common.bash
> >> index f9c15fd304bd..62931a40b79a 100644
> >> --- a/scripts/common.bash
> >> +++ b/scripts/common.bash
> >> @@ -1,8 +1,15 @@
> >> +function arch_cmd()
> >> +{
> >> +	# Dummy function, can be overwritten by architecture dependent
> >> +	# code
> >> +	return
> >> +}
> >
> > This dummy function appears unused and can be dropped.
> 
> So what is then used if the function is not defined by the architecture
> specific code?

Nothing, which works fine

 $ arch_cmd=
 $ $arch_cmd echo foo   # just do 'echo foo'

However, with what I wrote above, we now need a common arch_cmd function.
Something like

 arch_cmd()
 {
   [ "$ARCH_CMD" ] && return "$ARCH_CMD"
 }

Which would allow us to write

$(arch_cmd) $cmd ...

That does the same thing as above, but it now follows the pattern of
migration_cmd and timeout_cmd.

Thanks,
drew


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest`
  2020-08-14 13:29       ` Andrew Jones
@ 2020-08-18  9:03         ` Marc Hartmayer
  2020-08-18  9:13           ` Andrew Jones
  0 siblings, 1 reply; 20+ messages in thread
From: Marc Hartmayer @ 2020-08-18  9:03 UTC (permalink / raw)
  To: Andrew Jones, Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Fri, Aug 14, 2020 at 03:29 PM +0200, Andrew Jones <drjones@redhat.com> wrote:
> On Fri, Aug 14, 2020 at 03:06:36PM +0200, Marc Hartmayer wrote:
>> On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
>> > On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote:
>> >> This allows us, for example, to auto generate a new test case based on
>> >> an existing test case.
>> >> 
>> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> >> ---
>> >>  run_tests.sh            |  2 +-
>> >>  scripts/common.bash     | 13 +++++++++++++
>> >>  scripts/mkstandalone.sh |  2 +-
>> >>  3 files changed, 15 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/run_tests.sh b/run_tests.sh
>> >> index 24aba9cc3a98..23658392c488 100755
>> >> --- a/run_tests.sh
>> >> +++ b/run_tests.sh
>> >> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT
>> >>     # preserve stdout so that process_test_output output can write TAP to it
>> >>     exec 3>&1
>> >>     test "$tap_output" == "yes" && exec > /dev/null
>> >> -   for_each_unittest $config run_task
>> >> +   for_each_unittest $config run_task arch_cmd
>> >
>> > Let's just require that arch cmd hook be specified by the "$arch_cmd"
>> > variable. Then we don't need to pass it to for_each_unittest.
>> 
>> Where is it then specified?
>
> Just using it that way in the source is enough. We should probably call
> it $ARCH_CMD to indicate that it's a special variable. Also, we could
> return it from a $(arch_cmd) function, which is how $(migration_cmd) and
> $(timeout_cmd) work.

My first approach was different…

First we source the (common) functions that could be overridden by
architecture dependent code, and then source the architecture dependent
code. But I’m not sure which approach is cleaner - if you prefer your
proposed solution with the global variables I can change it.

Thanks for the feedback!

[…snip]

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest`
  2020-08-18  9:03         ` Marc Hartmayer
@ 2020-08-18  9:13           ` Andrew Jones
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2020-08-18  9:13 UTC (permalink / raw)
  To: Marc Hartmayer
  Cc: kvm, Thomas Huth, David Hildenbrand, Janosch Frank,
	Cornelia Huck, Paolo Bonzini, Christian Borntraeger, linux-s390

On Tue, Aug 18, 2020 at 11:03:27AM +0200, Marc Hartmayer wrote:
> On Fri, Aug 14, 2020 at 03:29 PM +0200, Andrew Jones <drjones@redhat.com> wrote:
> > On Fri, Aug 14, 2020 at 03:06:36PM +0200, Marc Hartmayer wrote:
> >> On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
> >> > On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote:
> >> >> This allows us, for example, to auto generate a new test case based on
> >> >> an existing test case.
> >> >> 
> >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> >> >> ---
> >> >>  run_tests.sh            |  2 +-
> >> >>  scripts/common.bash     | 13 +++++++++++++
> >> >>  scripts/mkstandalone.sh |  2 +-
> >> >>  3 files changed, 15 insertions(+), 2 deletions(-)
> >> >> 
> >> >> diff --git a/run_tests.sh b/run_tests.sh
> >> >> index 24aba9cc3a98..23658392c488 100755
> >> >> --- a/run_tests.sh
> >> >> +++ b/run_tests.sh
> >> >> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT
> >> >>     # preserve stdout so that process_test_output output can write TAP to it
> >> >>     exec 3>&1
> >> >>     test "$tap_output" == "yes" && exec > /dev/null
> >> >> -   for_each_unittest $config run_task
> >> >> +   for_each_unittest $config run_task arch_cmd
> >> >
> >> > Let's just require that arch cmd hook be specified by the "$arch_cmd"
> >> > variable. Then we don't need to pass it to for_each_unittest.
> >> 
> >> Where is it then specified?
> >
> > Just using it that way in the source is enough. We should probably call
> > it $ARCH_CMD to indicate that it's a special variable. Also, we could
> > return it from a $(arch_cmd) function, which is how $(migration_cmd) and
> > $(timeout_cmd) work.
> 
> My first approach was different…
> 
> First we source the (common) functions that could be overridden by
> architecture dependent code, and then source the architecture dependent
> code. But I’m not sure which approach is cleaner - if you prefer your
> proposed solution with the global variables I can change it.

I prefer my proposed solution. It's not necessary to provide and
source an arch-neutral function that will never do anything. And,
it will never do anything, because the function is supposed to be
arch-specific. If an arch doesn't implement the function, then
we don't need to call anything at all.

Thanks,
drew

> 
> Thanks for the feedback!
> 
> […snip]
> 
> -- 
> Kind regards / Beste Grüße
>    Marc Hartmayer
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzender des Aufsichtsrats: Gregor Pillen 
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-08-18  9:14 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-12  9:27 [kvm-unit-tests RFC v2 0/4] s390x: Add Protected VM support Marc Hartmayer
2020-08-12  9:27 ` [kvm-unit-tests RFC v2 1/4] common.bash: run `cmd` only if a test case was found Marc Hartmayer
2020-08-13  7:40   ` Andrew Jones
2020-08-13 11:37     ` Marc Hartmayer
2020-08-12  9:27 ` [kvm-unit-tests RFC v2 2/4] scripts: add support for architecture dependent functions Marc Hartmayer
2020-08-13  7:49   ` Andrew Jones
2020-08-13 11:45     ` Marc Hartmayer
2020-08-13 12:07       ` Andrew Jones
2020-08-13 12:36         ` Marc Hartmayer
2020-08-12  9:27 ` [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest` Marc Hartmayer
2020-08-13  8:30   ` Andrew Jones
2020-08-14 13:06     ` Marc Hartmayer
2020-08-14 13:29       ` Andrew Jones
2020-08-18  9:03         ` Marc Hartmayer
2020-08-18  9:13           ` Andrew Jones
2020-08-12  9:27 ` [kvm-unit-tests RFC v2 4/4] s390x: add Protected VM support Marc Hartmayer
2020-08-13 11:56   ` Cornelia Huck
2020-08-13 13:08     ` Marc Hartmayer
2020-08-13 14:22       ` Cornelia Huck
2020-08-13 15:13         ` Marc Hartmayer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).