| Message ID | 20200506124636.21876-1-mhartmay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,RFC] s390x: Add Protected VM support | expand |
On Wed, May 06, 2020 at 02:46:36PM +0200, 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> > --- > .gitignore | 1 + > configure | 8 ++++++++ > s390x/Makefile | 16 +++++++++++++--- > s390x/unittests.cfg | 20 ++++++++++++++++++++ > scripts/common.bash | 30 +++++++++++++++++++++++++++++- > 5 files changed, 71 insertions(+), 4 deletions(-) > > diff --git a/.gitignore b/.gitignore > index 784cb2ddbcb8..1fa5c0c0ea76 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -4,6 +4,7 @@ > *.o > *.flat > *.elf > +*.img > .pc > patches > .stgit-* > diff --git a/configure b/configure > index 5d2cd90cd180..29191f4b0994 100755 > --- a/configure > +++ b/configure > @@ -18,6 +18,7 @@ u32_long= > vmm="qemu" > errata_force=0 > erratatxt="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 > } > @@ -91,6 +94,9 @@ while [[ "$1" = -* ]]; do > --erratatxt) > erratatxt="$arg" > ;; > + --host-key-document) > + host_key_document="$arg" > + ;; > --help) > usage > ;; > @@ -207,6 +213,8 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks > ENVIRON_DEFAULT=$environ_default > ERRATATXT=$erratatxt > U32_LONG_FMT=$u32_long > +GENPROTIMG=genprotimg > +HOST_KEY_DOCUMENT=$host_key_document > EOF > > cat <<EOF > lib/config.h > diff --git a/s390x/Makefile b/s390x/Makefile > index ddb4b48ecbf9..a57655dcce10 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -17,12 +17,19 @@ tests += $(TEST_DIR)/stsi.elf > tests += $(TEST_DIR)/skrf.elf > tests += $(TEST_DIR)/smp.elf > tests += $(TEST_DIR)/sclp.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_img = $(patsubst %.elf,%.pv.img,$(tests)) > +else > +tests_pv_img = > +endif > + > +all: directories test_cases test_cases_binary test_cases_pv > > test_cases: $(tests) > test_cases_binary: $(tests_binary) > +test_cases_pv: $(tests_pv_img) > > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > @@ -68,8 +75,11 @@ FLATLIBS = $(libcflat) > %.bin: %.elf > $(OBJCOPY) -O binary $< $@ > > +%.pv.img: %.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 > + $(RM) $(TEST_DIR)/*.{o,elf,bin,img} $(TEST_DIR)/.*.d lib/s390x/.*.d > > generated-files = $(asm-offsets) > $(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files) > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg > index b307329354f6..6beaca45fb20 100644 > --- a/s390x/unittests.cfg > +++ b/s390x/unittests.cfg > @@ -16,6 +16,8 @@ > # # a test. The check line can contain multiple files > # # to check separated by a space but each check > # # parameter needs to be of the form <path>=<value> > +# pv_support = 0|1 # Optionally specify whether a test supports the > +# # execution as a PV guest. Maybe pv_supported vs. pv_support? > ############################################################################## > > [selftest-setup] > @@ -25,62 +27,80 @@ extra_params = -append 'test 123' > > [intercept] > file = intercept.elf > +pv_support = 1 > > [emulator] > file = emulator.elf > +pv_support = 1 > > [sieve] > file = sieve.elf > groups = selftest > # can take fairly long when KVM is nested inside z/VM > timeout = 600 > +pv_support = 1 > > [sthyi] > file = sthyi.elf > +pv_support = 1 > > [skey] > file = skey.elf > +pv_support = 1 > > [diag10] > file = diag10.elf > +pv_support = 1 > > [diag308] > file = diag308.elf > +pv_support = 1 > > [pfmf] > file = pfmf.elf > +pv_support = 1 > > [cmm] > file = cmm.elf > +pv_support = 1 > > [vector] > file = vector.elf > +pv_support = 1 > > [gs] > file = gs.elf > +pv_support = 1 > > [iep] > file = iep.elf > +pv_support = 1 > > [cpumodel] > file = cpumodel.elf > +pv_support = 1 > > [diag288] > file = diag288.elf > extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi > +pv_support = 1 > > [stsi] > file = stsi.elf > extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-0242ac130003 -smp 1,maxcpus=8 > +pv_support = 1 > > [smp] > file = smp.elf > smp = 2 > +pv_support = 1 > > [sclp-1g] > file = sclp.elf > extra_params = -m 1G > +pv_support = 1 > > [sclp-3g] > file = sclp.elf > extra_params = -m 3G > +pv_support = 1 > diff --git a/scripts/common.bash b/scripts/common.bash > index 9a6ebbd7f287..971d0661287c 100644 > --- a/scripts/common.bash > +++ b/scripts/common.bash > @@ -1,3 +1,20 @@ > +function pv_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.img > + # do not run the PV test cases by default > + "$cmd" "${testname}_PV" "$groups pv nodefault" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > +} > > function for_each_unittest() > { > @@ -12,12 +29,16 @@ function for_each_unittest() > local check > local accel > local timeout > + local pv_support > > exec {fd}<"$unittests" > > while read -r -u $fd line; do > if [[ "$line" =~ ^\[(.*)\]$ ]]; then > - "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + if [ "${pv_support}" == 1 ]; then > + pv_cmd "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + fi > + > testname=${BASH_REMATCH[1]} > smp=1 > kernel="" > @@ -27,6 +48,7 @@ function for_each_unittest() > check="" > accel="" > timeout="" > + pv_support="" > elif [[ $line =~ ^file\ *=\ *(.*)$ ]]; then > kernel=$TEST_DIR/${BASH_REMATCH[1]} > elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then > @@ -43,8 +65,14 @@ function for_each_unittest() > accel=${BASH_REMATCH[1]} > elif [[ $line =~ ^timeout\ *=\ *(.*)$ ]]; then > timeout=${BASH_REMATCH[1]} > + elif [[ $line =~ ^pv_support\ *=\ *(.*)$ ]]; then > + pv_support=${BASH_REMATCH[1]} > fi > done > + > "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + if [ "${pv_support}" == 1 ]; then > + pv_cmd "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + fi > exec {fd}<&- > } > -- > 2.17.0 > I don't think making the changes to scripts/common.bash will work for standalone tests. Why not do this stuff in s390x/run instead? Also, do you need the pv_support[ed] parameter? You could just do a [ -f "${kernel%.elf}.pv.img" ] to decide if you should run again with PV, right? Thanks, drew
On 5/6/20 2:46 PM, 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> > --- > .gitignore | 1 + > configure | 8 ++++++++ > s390x/Makefile | 16 +++++++++++++--- > s390x/unittests.cfg | 20 ++++++++++++++++++++ > scripts/common.bash | 30 +++++++++++++++++++++++++++++- > 5 files changed, 71 insertions(+), 4 deletions(-) > > diff --git a/.gitignore b/.gitignore > index 784cb2ddbcb8..1fa5c0c0ea76 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -4,6 +4,7 @@ > *.o > *.flat > *.elf > +*.img > .pc > patches > .stgit-* > diff --git a/configure b/configure > index 5d2cd90cd180..29191f4b0994 100755 > --- a/configure > +++ b/configure > @@ -18,6 +18,7 @@ u32_long= > vmm="qemu" > errata_force=0 > erratatxt="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 > } > @@ -91,6 +94,9 @@ while [[ "$1" = -* ]]; do > --erratatxt) > erratatxt="$arg" > ;; > + --host-key-document) > + host_key_document="$arg" > + ;; > --help) > usage > ;; > @@ -207,6 +213,8 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks > ENVIRON_DEFAULT=$environ_default > ERRATATXT=$erratatxt > U32_LONG_FMT=$u32_long > +GENPROTIMG=genprotimg > +HOST_KEY_DOCUMENT=$host_key_document > EOF > > cat <<EOF > lib/config.h > diff --git a/s390x/Makefile b/s390x/Makefile > index ddb4b48ecbf9..a57655dcce10 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -17,12 +17,19 @@ tests += $(TEST_DIR)/stsi.elf > tests += $(TEST_DIR)/skrf.elf > tests += $(TEST_DIR)/smp.elf > tests += $(TEST_DIR)/sclp.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_img = $(patsubst %.elf,%.pv.img,$(tests)) > +else > +tests_pv_img = > +endif > + > +all: directories test_cases test_cases_binary test_cases_pv > > test_cases: $(tests) > test_cases_binary: $(tests_binary) > +test_cases_pv: $(tests_pv_img) > > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > @@ -68,8 +75,11 @@ FLATLIBS = $(libcflat) > %.bin: %.elf > $(OBJCOPY) -O binary $< $@ > > +%.pv.img: %.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 > + $(RM) $(TEST_DIR)/*.{o,elf,bin,img} $(TEST_DIR)/.*.d lib/s390x/.*.d > > generated-files = $(asm-offsets) > $(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files) > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg > index b307329354f6..6beaca45fb20 100644 > --- a/s390x/unittests.cfg > +++ b/s390x/unittests.cfg > @@ -16,6 +16,8 @@ > # # a test. The check line can contain multiple files > # # to check separated by a space but each check > # # parameter needs to be of the form <path>=<value> > +# pv_support = 0|1 # Optionally specify whether a test supports the > +# # execution as a PV guest. > ############################################################################## > > [selftest-setup] > @@ -25,62 +27,80 @@ extra_params = -append 'test 123' > > [intercept] > file = intercept.elf > +pv_support = 1 So, let's do this discussion once more: Why would we need a opt-in for something which works on all our current tests? I'd much rather have a opt-out or just a bail-out when running the test like I already implemented for the storage key related tests... I don't see any benefit for this right now other than forcing me to add another line to this file that was not needed before..
On 06.05.20 16:03, Janosch Frank wrote: > On 5/6/20 2:46 PM, 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> >> --- >> .gitignore | 1 + >> configure | 8 ++++++++ >> s390x/Makefile | 16 +++++++++++++--- >> s390x/unittests.cfg | 20 ++++++++++++++++++++ >> scripts/common.bash | 30 +++++++++++++++++++++++++++++- >> 5 files changed, 71 insertions(+), 4 deletions(-) >> >> diff --git a/.gitignore b/.gitignore >> index 784cb2ddbcb8..1fa5c0c0ea76 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -4,6 +4,7 @@ >> *.o >> *.flat >> *.elf >> +*.img >> .pc >> patches >> .stgit-* >> diff --git a/configure b/configure >> index 5d2cd90cd180..29191f4b0994 100755 >> --- a/configure >> +++ b/configure >> @@ -18,6 +18,7 @@ u32_long= >> vmm="qemu" >> errata_force=0 >> erratatxt="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 >> } >> @@ -91,6 +94,9 @@ while [[ "$1" = -* ]]; do >> --erratatxt) >> erratatxt="$arg" >> ;; >> + --host-key-document) >> + host_key_document="$arg" >> + ;; >> --help) >> usage >> ;; >> @@ -207,6 +213,8 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks >> ENVIRON_DEFAULT=$environ_default >> ERRATATXT=$erratatxt >> U32_LONG_FMT=$u32_long >> +GENPROTIMG=genprotimg >> +HOST_KEY_DOCUMENT=$host_key_document >> EOF >> >> cat <<EOF > lib/config.h >> diff --git a/s390x/Makefile b/s390x/Makefile >> index ddb4b48ecbf9..a57655dcce10 100644 >> --- a/s390x/Makefile >> +++ b/s390x/Makefile >> @@ -17,12 +17,19 @@ tests += $(TEST_DIR)/stsi.elf >> tests += $(TEST_DIR)/skrf.elf >> tests += $(TEST_DIR)/smp.elf >> tests += $(TEST_DIR)/sclp.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_img = $(patsubst %.elf,%.pv.img,$(tests)) >> +else >> +tests_pv_img = >> +endif >> + >> +all: directories test_cases test_cases_binary test_cases_pv >> >> test_cases: $(tests) >> test_cases_binary: $(tests_binary) >> +test_cases_pv: $(tests_pv_img) >> >> CFLAGS += -std=gnu99 >> CFLAGS += -ffreestanding >> @@ -68,8 +75,11 @@ FLATLIBS = $(libcflat) >> %.bin: %.elf >> $(OBJCOPY) -O binary $< $@ >> >> +%.pv.img: %.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 >> + $(RM) $(TEST_DIR)/*.{o,elf,bin,img} $(TEST_DIR)/.*.d lib/s390x/.*.d >> >> generated-files = $(asm-offsets) >> $(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files) >> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg >> index b307329354f6..6beaca45fb20 100644 >> --- a/s390x/unittests.cfg >> +++ b/s390x/unittests.cfg >> @@ -16,6 +16,8 @@ >> # # a test. The check line can contain multiple files >> # # to check separated by a space but each check >> # # parameter needs to be of the form <path>=<value> >> +# pv_support = 0|1 # Optionally specify whether a test supports the >> +# # execution as a PV guest. >> ############################################################################## >> >> [selftest-setup] >> @@ -25,62 +27,80 @@ extra_params = -append 'test 123' >> >> [intercept] >> file = intercept.elf >> +pv_support = 1 > > So, let's do this discussion once more: > Why would we need a opt-in for something which works on all our current > tests? I'd much rather have a opt-out or just a bail-out when running > the test like I already implemented for the storage key related tests... > > I don't see any benefit for this right now other than forcing me to add > another line to this file that was not needed before.. > Exactly my thought. I would assume that most tests that properly test for feature availability should just work?
On 5/6/20 4:05 PM, David Hildenbrand wrote: > On 06.05.20 16:03, Janosch Frank wrote: >> On 5/6/20 2:46 PM, 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> >>> --- >>> .gitignore | 1 + >>> configure | 8 ++++++++ >>> s390x/Makefile | 16 +++++++++++++--- >>> s390x/unittests.cfg | 20 ++++++++++++++++++++ >>> scripts/common.bash | 30 +++++++++++++++++++++++++++++- >>> 5 files changed, 71 insertions(+), 4 deletions(-) >>> >>> diff --git a/.gitignore b/.gitignore >>> index 784cb2ddbcb8..1fa5c0c0ea76 100644 >>> --- a/.gitignore >>> +++ b/.gitignore >>> @@ -4,6 +4,7 @@ >>> *.o >>> *.flat >>> *.elf >>> +*.img >>> .pc >>> patches >>> .stgit-* >>> diff --git a/configure b/configure >>> index 5d2cd90cd180..29191f4b0994 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -18,6 +18,7 @@ u32_long= >>> vmm="qemu" >>> errata_force=0 >>> erratatxt="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 >>> } >>> @@ -91,6 +94,9 @@ while [[ "$1" = -* ]]; do >>> --erratatxt) >>> erratatxt="$arg" >>> ;; >>> + --host-key-document) >>> + host_key_document="$arg" >>> + ;; >>> --help) >>> usage >>> ;; >>> @@ -207,6 +213,8 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks >>> ENVIRON_DEFAULT=$environ_default >>> ERRATATXT=$erratatxt >>> U32_LONG_FMT=$u32_long >>> +GENPROTIMG=genprotimg >>> +HOST_KEY_DOCUMENT=$host_key_document >>> EOF >>> >>> cat <<EOF > lib/config.h >>> diff --git a/s390x/Makefile b/s390x/Makefile >>> index ddb4b48ecbf9..a57655dcce10 100644 >>> --- a/s390x/Makefile >>> +++ b/s390x/Makefile >>> @@ -17,12 +17,19 @@ tests += $(TEST_DIR)/stsi.elf >>> tests += $(TEST_DIR)/skrf.elf >>> tests += $(TEST_DIR)/smp.elf >>> tests += $(TEST_DIR)/sclp.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_img = $(patsubst %.elf,%.pv.img,$(tests)) >>> +else >>> +tests_pv_img = >>> +endif >>> + >>> +all: directories test_cases test_cases_binary test_cases_pv >>> >>> test_cases: $(tests) >>> test_cases_binary: $(tests_binary) >>> +test_cases_pv: $(tests_pv_img) >>> >>> CFLAGS += -std=gnu99 >>> CFLAGS += -ffreestanding >>> @@ -68,8 +75,11 @@ FLATLIBS = $(libcflat) >>> %.bin: %.elf >>> $(OBJCOPY) -O binary $< $@ >>> >>> +%.pv.img: %.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 >>> + $(RM) $(TEST_DIR)/*.{o,elf,bin,img} $(TEST_DIR)/.*.d lib/s390x/.*.d >>> >>> generated-files = $(asm-offsets) >>> $(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files) >>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg >>> index b307329354f6..6beaca45fb20 100644 >>> --- a/s390x/unittests.cfg >>> +++ b/s390x/unittests.cfg >>> @@ -16,6 +16,8 @@ >>> # # a test. The check line can contain multiple files >>> # # to check separated by a space but each check >>> # # parameter needs to be of the form <path>=<value> >>> +# pv_support = 0|1 # Optionally specify whether a test supports the >>> +# # execution as a PV guest. >>> ############################################################################## >>> >>> [selftest-setup] >>> @@ -25,62 +27,80 @@ extra_params = -append 'test 123' >>> >>> [intercept] >>> file = intercept.elf >>> +pv_support = 1 >> >> So, let's do this discussion once more: >> Why would we need a opt-in for something which works on all our current >> tests? I'd much rather have a opt-out or just a bail-out when running >> the test like I already implemented for the storage key related tests... >> >> I don't see any benefit for this right now other than forcing me to add >> another line to this file that was not needed before.. >> > > Exactly my thought. I would assume that most tests that properly test > for feature availability should just work? > Yes At the beginning of firmware development it was sometimes necessary to blacklist some tests, but now all of them run or bail-out.
On Wed, May 06, 2020 at 04:03 PM +0200, Janosch Frank <frankja@linux.ibm.com> wrote: > On 5/6/20 2:46 PM, 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> […snip…] >> [intercept] >> file = intercept.elf >> +pv_support = 1 > > So, let's do this discussion once more: > Why would we need a opt-in for something which works on all our current > tests? I'd much rather have a opt-out or just a bail-out when running > the test like I already implemented for the storage key related > tests... > > I don't see any benefit for this right now other than forcing me to add > another line to this file that was not needed before.. > Okay. So shall I add an option ’pv_not_supported’? Or simply assume that the actual test cases will handle it?
On 07.05.20 14:30, Marc Hartmayer wrote: > On Wed, May 06, 2020 at 04:03 PM +0200, Janosch Frank <frankja@linux.ibm.com> wrote: >> On 5/6/20 2:46 PM, 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> > > […snip…] > >>> [intercept] >>> file = intercept.elf >>> +pv_support = 1 >> >> So, let's do this discussion once more: >> Why would we need a opt-in for something which works on all our current >> tests? I'd much rather have a opt-out or just a bail-out when running >> the test like I already implemented for the storage key related >> tests... >> >> I don't see any benefit for this right now other than forcing me to add >> another line to this file that was not needed before.. >> > > Okay. So shall I add an option ’pv_not_supported’? Or simply assume that > the actual test cases will handle it? I would suggest to fix the testcases (e.g. do an early exit when we are running secure and we know it does not work)
On Wed, May 06, 2020 at 03:50 PM +0200, Andrew Jones <drjones@redhat.com> wrote: > On Wed, May 06, 2020 at 02:46:36PM +0200, 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> >> --- >> @@ -16,6 +16,8 @@ >> # # a test. The check line can contain multiple files >> # # to check separated by a space but each check >> # # parameter needs to be of the form <path>=<value> >> +# pv_support = 0|1 # Optionally specify whether a test supports the >> +# # execution as a PV guest. > > Maybe pv_supported vs. pv_support? pv_supported is better, thanks. >> >> exec {fd}<"$unittests" >> >> while read -r -u $fd line; do >> if [[ "$line" =~ ^\[(.*)\]$ ]]; then >> - "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" This line was accidentally removed. >> + if [ "${pv_support}" == 1 ]; then >> + pv_cmd "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + fi >> + >> testname=${BASH_REMATCH[1]} >> smp=1 >> kernel="" >> @@ -27,6 +48,7 @@ function for_each_unittest() >> check="" >> accel="" >> timeout="" >> + pv_support="" >> elif [[ $line =~ ^file\ *=\ *(.*)$ ]]; then >> kernel=$TEST_DIR/${BASH_REMATCH[1]} >> elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then >> @@ -43,8 +65,14 @@ function for_each_unittest() >> accel=${BASH_REMATCH[1]} >> elif [[ $line =~ ^timeout\ *=\ *(.*)$ ]]; then >> timeout=${BASH_REMATCH[1]} >> + elif [[ $line =~ ^pv_support\ *=\ *(.*)$ ]]; then >> + pv_support=${BASH_REMATCH[1]} >> fi >> done >> + >> "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + if [ "${pv_support}" == 1 ]; then >> + pv_cmd "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + fi >> exec {fd}<&- >> } >> -- >> 2.17.0 >> > > I don't think making the changes to scripts/common.bash will work for > standalone tests. With the fix above it works (tested on x86 and s390x). $ make standalone Written tests/selftest-setup. Written tests/intercept. Written tests/intercept_PV. Written tests/emulator. Written tests/emulator_PV. Written tests/sieve. Written tests/sieve_PV. … > Why not do this stuff in s390x/run instead? I will try. > Also, > do you need the pv_support[ed] parameter? You could just do a > [ -f "${kernel%.elf}.pv.img" ] to decide if you should run again > with PV, right? AFAIK, for the other test cases the kernel file is also not checked, but this would be an option - thanks. > > Thanks, > drew > Thanks for the feedback!
On Wed, May 06, 2020 at 03:50 PM +0200, Andrew Jones <drjones@redhat.com> wrote: > On Wed, May 06, 2020 at 02:46:36PM +0200, 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> >> --- […snip…] > + >> "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + if [ "${pv_support}" == 1 ]; then >> + pv_cmd "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + fi >> exec {fd}<&- >> } >> -- >> 2.17.0 >> > > I don't think making the changes to scripts/common.bash will work for > standalone tests. Why not do this stuff in s390x/run instead? Okay, I’ve looked into the code, and the reason for this approach is that I want to treat the PVM and the “normal” test case as two separate test cases, but using the same test configuration. I don’t see how I can achieve this by editing s390x/run and for the standalone case. Maybe this approach is already broken and I should simply add the PVM testcases as extra test cases to the unittest.cfg - but this would result in duplicated code in the configuration file. > Also, > do you need the pv_support[ed] parameter? You could just do a > [ -f "${kernel%.elf}.pv.img" ] to decide if you should run again > with PV, right? > > Thanks, > drew >
diff --git a/.gitignore b/.gitignore index 784cb2ddbcb8..1fa5c0c0ea76 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ *.o *.flat *.elf +*.img .pc patches .stgit-* diff --git a/configure b/configure index 5d2cd90cd180..29191f4b0994 100755 --- a/configure +++ b/configure @@ -18,6 +18,7 @@ u32_long= vmm="qemu" errata_force=0 erratatxt="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 } @@ -91,6 +94,9 @@ while [[ "$1" = -* ]]; do --erratatxt) erratatxt="$arg" ;; + --host-key-document) + host_key_document="$arg" + ;; --help) usage ;; @@ -207,6 +213,8 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks ENVIRON_DEFAULT=$environ_default ERRATATXT=$erratatxt U32_LONG_FMT=$u32_long +GENPROTIMG=genprotimg +HOST_KEY_DOCUMENT=$host_key_document EOF cat <<EOF > lib/config.h diff --git a/s390x/Makefile b/s390x/Makefile index ddb4b48ecbf9..a57655dcce10 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -17,12 +17,19 @@ tests += $(TEST_DIR)/stsi.elf tests += $(TEST_DIR)/skrf.elf tests += $(TEST_DIR)/smp.elf tests += $(TEST_DIR)/sclp.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_img = $(patsubst %.elf,%.pv.img,$(tests)) +else +tests_pv_img = +endif + +all: directories test_cases test_cases_binary test_cases_pv test_cases: $(tests) test_cases_binary: $(tests_binary) +test_cases_pv: $(tests_pv_img) CFLAGS += -std=gnu99 CFLAGS += -ffreestanding @@ -68,8 +75,11 @@ FLATLIBS = $(libcflat) %.bin: %.elf $(OBJCOPY) -O binary $< $@ +%.pv.img: %.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 + $(RM) $(TEST_DIR)/*.{o,elf,bin,img} $(TEST_DIR)/.*.d lib/s390x/.*.d generated-files = $(asm-offsets) $(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files) diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg index b307329354f6..6beaca45fb20 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -16,6 +16,8 @@ # # a test. The check line can contain multiple files # # to check separated by a space but each check # # parameter needs to be of the form <path>=<value> +# pv_support = 0|1 # Optionally specify whether a test supports the +# # execution as a PV guest. ############################################################################## [selftest-setup] @@ -25,62 +27,80 @@ extra_params = -append 'test 123' [intercept] file = intercept.elf +pv_support = 1 [emulator] file = emulator.elf +pv_support = 1 [sieve] file = sieve.elf groups = selftest # can take fairly long when KVM is nested inside z/VM timeout = 600 +pv_support = 1 [sthyi] file = sthyi.elf +pv_support = 1 [skey] file = skey.elf +pv_support = 1 [diag10] file = diag10.elf +pv_support = 1 [diag308] file = diag308.elf +pv_support = 1 [pfmf] file = pfmf.elf +pv_support = 1 [cmm] file = cmm.elf +pv_support = 1 [vector] file = vector.elf +pv_support = 1 [gs] file = gs.elf +pv_support = 1 [iep] file = iep.elf +pv_support = 1 [cpumodel] file = cpumodel.elf +pv_support = 1 [diag288] file = diag288.elf extra_params=-device diag288,id=watchdog0 --watchdog-action inject-nmi +pv_support = 1 [stsi] file = stsi.elf extra_params=-name kvm-unit-test --uuid 0fb84a86-727c-11ea-bc55-0242ac130003 -smp 1,maxcpus=8 +pv_support = 1 [smp] file = smp.elf smp = 2 +pv_support = 1 [sclp-1g] file = sclp.elf extra_params = -m 1G +pv_support = 1 [sclp-3g] file = sclp.elf extra_params = -m 3G +pv_support = 1 diff --git a/scripts/common.bash b/scripts/common.bash index 9a6ebbd7f287..971d0661287c 100644 --- a/scripts/common.bash +++ b/scripts/common.bash @@ -1,3 +1,20 @@ +function pv_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.img + # do not run the PV test cases by default + "$cmd" "${testname}_PV" "$groups pv nodefault" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" +} function for_each_unittest() { @@ -12,12 +29,16 @@ function for_each_unittest() local check local accel local timeout + local pv_support exec {fd}<"$unittests" while read -r -u $fd line; do if [[ "$line" =~ ^\[(.*)\]$ ]]; then - "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + if [ "${pv_support}" == 1 ]; then + pv_cmd "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + fi + testname=${BASH_REMATCH[1]} smp=1 kernel="" @@ -27,6 +48,7 @@ function for_each_unittest() check="" accel="" timeout="" + pv_support="" elif [[ $line =~ ^file\ *=\ *(.*)$ ]]; then kernel=$TEST_DIR/${BASH_REMATCH[1]} elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then @@ -43,8 +65,14 @@ function for_each_unittest() accel=${BASH_REMATCH[1]} elif [[ $line =~ ^timeout\ *=\ *(.*)$ ]]; then timeout=${BASH_REMATCH[1]} + elif [[ $line =~ ^pv_support\ *=\ *(.*)$ ]]; then + pv_support=${BASH_REMATCH[1]} fi done + "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + if [ "${pv_support}" == 1 ]; then + pv_cmd "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + fi exec {fd}<&- }
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> --- .gitignore | 1 + configure | 8 ++++++++ s390x/Makefile | 16 +++++++++++++--- s390x/unittests.cfg | 20 ++++++++++++++++++++ scripts/common.bash | 30 +++++++++++++++++++++++++++++- 5 files changed, 71 insertions(+), 4 deletions(-)