From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-s390-owner@vger.kernel.org>
Received: from us-smtp-delivery-1.mimecast.com ([207.211.31.120]:39725 "EHLO
        us-smtp-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org
        with ESMTP id S1727981AbgHSKmn (ORCPT
        <rfc822;linux-s390@vger.kernel.org>); Wed, 19 Aug 2020 06:42:43 -0400
Subject: Re: [kvm-unit-tests PATCH 4/4] s390x: add Protected VM support
References: <20200818130424.20522-1-mhartmay@linux.ibm.com>
 <20200818130424.20522-5-mhartmay@linux.ibm.com>
From: David Hildenbrand <david@redhat.com>
Message-ID: <6bd69c28-12df-f300-9cb2-9da1d80672f1@redhat.com>
Date: Wed, 19 Aug 2020 12:42:35 +0200
MIME-Version: 1.0
In-Reply-To: <20200818130424.20522-5-mhartmay@linux.ibm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-s390-owner@vger.kernel.org
List-ID: <linux-s390.vger.kernel.org>
To: Marc Hartmayer <mhartmay@linux.ibm.com>, kvm@vger.kernel.org
Cc: Thomas Huth <thuth@redhat.com>, Janosch Frank <frankja@linux.ibm.com>, Cornelia Huck <cohuck@redhat.com>, Andrew Jones <drjones@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, linux-s390@vger.kernel.org

On 18.08.20 15:04, Marc Hartmayer 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               |  9 +++++++++
>  s390x/Makefile          | 17 +++++++++++++++--
>  s390x/selftest.parmfile |  1 +
>  s390x/unittests.cfg     |  1 +
>  scripts/s390x/func.bash | 35 +++++++++++++++++++++++++++++++++++
>  5 files changed, 61 insertions(+), 2 deletions(-)
>  create mode 100644 s390x/selftest.parmfile
>  create mode 100644 scripts/s390x/func.bash
> 
> diff --git a/configure b/configure
> index f9d030fd2f03..0e64af58b3c1 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,9 @@ 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
> +	                           Specify the machine-specific host-key document for creating
> +	                           a PVM image with 'genprotimg' (s390x only)
>  EOF
>      exit 1
>  }
> @@ -92,6 +96,9 @@ while [[ "$1" = -* ]]; do
>  	    erratatxt=
>  	    [ "$arg" ] && erratatxt=$(eval realpath "$arg")
>  	    ;;
> +	--host-key-document)
> +	    host_key_document="$arg"
> +	    ;;
>  	--help)
>  	    usage
>  	    ;;
> @@ -205,6 +212,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'

This is ugly, can't we somehow create the parmfiles dynamically?

>  
>  [intercept]
> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
> new file mode 100644
> index 000000000000..b2d59d0d6f25
> --- /dev/null
> +++ b/scripts/s390x/func.bash
> @@ -0,0 +1,35 @@
> +# The file scripts/common.bash has to be the only file sourcing this
> +# arch helper file
> +source config.mak
> +
> +ARCH_CMD=arch_cmd_s390x
> +
> +function arch_cmd_s390x()
> +{
> +	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}
> +
> +	# run the normal test case
> +	"$cmd" "${testname}" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +

Why ${testname} instead of $testname ?

> +	# run PV test case
> +	kernel=${kernel%.elf}.pv.bin
> +	if [ ! -f "${kernel}" ]; then
> +		if [ -z "${HOST_KEY_DOCUMENT}" ]; then
> +			print_result 'SKIP' $testname '(no host-key document specified)'
> +			return 2
> +		fi
> +
> +		print_result 'SKIP' $testname '(PVM image was not created)'
> +		return 2
> +	fi
> +	"$cmd" "${testname}_PV" "$groups pv" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"

dito

> +}
> 


-- 
Thanks,

David / dhildenb