[kvm-unit-tests PATCH 0/6] Documentation misc fixes

[kvm-unit-tests PATCH 0/6] Documentation misc fixes

[Alex Bennée]

Hi,

I broke these out of my earlier MTTCG test series as they are not
strictly related. The libcflat/pci fixes are a result of trying to
cross-compile arm32 binaries on my arm64 box with a arm-none-abi
compiler. I've also tidied up some documentation (along with a
controversial move to Markdown ;-). And finally the run_script now
follows the convention of passing arguments after -- to the child
process.

Alex Benn?e (6):
  libcflat: add PRI(dux)32 format types
  lib/pci: fix BAR format strings
  docs: move README to README.md and symlink
  docs: mention checkpatch in the README
  docs: mention modifying env vars in README
  run_tests: allow passing of options to QEMU

 Makefile               |  1 +
 README                 | 69 +-----------------------------------
 README.md              | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
 configure              | 13 +++++++
 lib/libcflat.h         |  9 +++++
 lib/pci.c              |  4 +--
 run_tests.sh           | 13 +++++--
 scripts/functions.bash |  7 ++--
 8 files changed, 137 insertions(+), 75 deletions(-)
 mode change 100644 => 120000 README
 create mode 100644 README.md

-- 
2.11.0

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Alex Bennée]

So we can have portable formatting of uint32_t types. However there is
a catch. Different compilers can use legally subtly different types
though so we need to probe the compiler defined intdef.h first.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
---
 Makefile       |  1 +
 configure      | 13 +++++++++++++
 lib/libcflat.h |  9 +++++++++
 3 files changed, 23 insertions(+)

diff --git a/Makefile b/Makefile
index a32333b..9822d9a 100644
--- a/Makefile
+++ b/Makefile
@@ -55,6 +55,7 @@ CFLAGS += $(fomit_frame_pointer)
 CFLAGS += $(fno_stack_protector)
 CFLAGS += $(fno_stack_protector_all)
 CFLAGS += $(wno_frame_address)
+CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)
 
 CXXFLAGS += $(CFLAGS)
 
diff --git a/configure b/configure
index 995c8fa..127868c 100755
--- a/configure
+++ b/configure
@@ -109,6 +109,18 @@ if [ -f $testdir/run ]; then
     ln -fs $testdir/run $testdir-run
 fi
 
+# check if uint32_t needs a long format modifier
+cat << EOF > lib_test.c
+#include <inttypes.h>
+EOF
+
+$cross_prefix$cc lib_test.c -E | grep "typedef" | grep "long" | grep "uint32_t" &> /dev/null
+exit=$?
+if [ $exit -eq 0 ]; then
+    u32_long=true
+fi
+rm -f lib_test.c
+
 # check for dependent 32 bit libraries
 if [ "$arch" != "arm" ]; then
 cat << EOF > lib_test.c
@@ -155,4 +167,5 @@ TEST_DIR=$testdir
 FIRMWARE=$firmware
 ENDIAN=$endian
 PRETTY_PRINT_STACKS=$pretty_print_stacks
+U32_LONG_FMT=$u32_long
 EOF
diff --git a/lib/libcflat.h b/lib/libcflat.h
index 380395f..e80fc50 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -58,12 +58,21 @@ typedef _Bool		bool;
 #define true  1
 
 #if __SIZEOF_LONG__ == 8
+#  define __PRI32_PREFIX
 #  define __PRI64_PREFIX	"l"
 #  define __PRIPTR_PREFIX	"l"
 #else
+#if defined(__U32_LONG_FMT__)
+#  define __PRI32_PREFIX        "l"
+#else
+#  define __PRI32_PREFIX
+#endif
 #  define __PRI64_PREFIX	"ll"
 #  define __PRIPTR_PREFIX
 #endif
+#define PRId32  __PRI32_PREFIX	"d"
+#define PRIu32  __PRI32_PREFIX	"u"
+#define PRIx32  __PRI32_PREFIX	"x"
 #define PRId64  __PRI64_PREFIX	"d"
 #define PRIu64  __PRI64_PREFIX	"u"
 #define PRIx64  __PRI64_PREFIX	"x"
-- 
2.11.0

[kvm-unit-tests PATCH 2/6] lib/pci: fix BAR format strings

[Alex Bennée]

Using %x as a format string is not portable across 32/64 bit builds.
Use explicit PRIx32 format strings like the 64 bit version above.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
---
 lib/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index 6416191..597d8f2 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -67,7 +67,7 @@ bool pci_setup_msi(struct pci_dev *dev, uint64_t msi_addr, uint32_t msi_data)
 		pci_config_writel(addr, offset + PCI_MSI_DATA_32, msi_data);
 		printf("MSI: dev 0x%x init 32bit address: ", addr);
 	}
-	printf("addr=0x%lx, data=0x%x\n", msi_addr, msi_data);
+	printf("addr=0x%" PRIx64 ", data=0x%" PRIx32 "\n", msi_addr, msi_data);
 
 	msi_control |= PCI_MSI_FLAGS_ENABLE;
 	pci_config_writew(addr, offset + PCI_MSI_FLAGS, msi_control);
@@ -237,7 +237,7 @@ void pci_bar_print(struct pci_dev *dev, int bar_num)
 		printf("BAR#%d,%d [%" PRIx64 "-%" PRIx64 " ",
 		       bar_num, bar_num + 1, start, end);
 	} else {
-		printf("BAR#%d [%02x-%02x ",
+		printf("BAR#%d [%" PRIx32 "-%" PRIx32 " ",
 		       bar_num, (uint32_t)start, (uint32_t)end);
 	}
 
-- 
2.11.0

[kvm-unit-tests PATCH 3/6] docs: move README to README.md and symlink

[Alex Bennée]

This allows a slightly nicer formatting of the text when displayed on
some repository hosts. We keep a symlink from README for the
old-school purists.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
---
 README    | 69 +----------------------------------------------------
 README.md | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 68 deletions(-)
 mode change 100644 => 120000 README
 create mode 100644 README.md

diff --git a/README b/README
deleted file mode 100644
index f8f196d..0000000
--- a/README
+++ /dev/null
@@ -1,68 +0,0 @@
-Welcome to kvm-unit-tests
-
-See http://www.linux-kvm.org/page/KVM-unit-tests for a high-level
-description of this project, as well as running tests and adding
-tests HOWTOs.
-
-This directory contains sources for a kvm test suite.
-
-To create the test images do
-  ./configure
-  make
-in this directory. Test images are created in ./<ARCH>/*.flat
-
-Then use the runner script to detect the correct invocation and
-invoke the test, e.g.
-  ./x86-run ./x86/msr.flat
-or
-  ./run_tests.sh
-to run them all.
-
-To select a specific qemu binary, specify the QEMU=<path>
-environment variable, e.g.
-  QEMU=/tmp/qemu/x86_64-softmmu/qemu-system-x86_64 ./x86-run ./x86/msr.flat
-
-To create and use standalone tests do
-  ./configure
-  make standalone
-  (send tests/some-test somewhere)
-  (go to somewhere)
-  ./some-test
-
-'make install' will install all tests in PREFIX/share/kvm-unit-tests/tests,
-each as a standalone test.
-
-Directory structure:
-.:		configure script, top-level Makefile, and run_tests.sh
-./scripts:	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
-
-See <ARCH>/README for architecture specific documentation.
-
-CONTRIBUTING:
-=============
-
-Style
------
-
-Currently there is a mix of indentation styles so any changes to
-existing files should be consistent with the existing style. For new
-files:
-
-  - C: please use standard linux-with-tabs
-  - Shell: use TABs for indentation
-
-Patches
--------
-
-Patches are welcome at the KVM mailing list <kvm@vger.kernel.org>.
-
-Please prefix messages with: [kvm-unit-tests PATCH]
-
-You can add the following to .git/config to do this automatically for you:
-
-[format]
-	subjectprefix = kvm-unit-tests PATCH
-
diff --git a/README b/README
new file mode 120000
index 0000000..42061c0
--- /dev/null
+++ b/README
@@ -0,0 +1 @@
+README.md
\ No newline at end of file
diff --git a/README.md b/README.md
new file mode 100644
index 0000000..5027b62
--- /dev/null
+++ b/README.md
@@ -0,0 +1,81 @@
+# Welcome to kvm-unit-tests
+
+See http://www.linux-kvm.org/page/KVM-unit-tests for a high-level
+description of this project, as well as running tests and adding
+tests HOWTOs.
+
+# Building the tests
+
+This directory contains sources for a kvm test suite.
+
+To create the test images do:
+
+    ./configure
+    make
+
+in this directory. Test images are created in ./<ARCH>/*.flat
+
+## Standalone tests
+
+The tests can be built as standalone
+To create and use standalone tests do:
+
+    ./configure
+    make standalone
+    (send tests/some-test somewhere)
+    (go to somewhere)
+    ./some-test
+
+'make install' will install all tests in PREFIX/share/kvm-unit-tests/tests,
+each as a standalone test.
+
+
+# Running the tests
+
+Then use the runner script to detect the correct invocation and
+invoke the test:
+
+    ./x86-run ./x86/msr.flat
+or:
+
+    ./run_tests.sh
+
+to run them all.
+
+To select a specific qemu binary, specify the QEMU=<path>
+environment variable:
+
+    QEMU=/tmp/qemu/x86_64-softmmu/qemu-system-x86_64 ./x86-run ./x86/msr.flat
+
+# Contributing
+
+## Directory structure
+
+    .:				configure script, top-level Makefile, and run_tests.sh
+    ./scripts:		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
+
+See <ARCH>/README for architecture specific documentation.
+
+## Style
+
+Currently there is a mix of indentation styles so any changes to
+existing files should be consistent with the existing style. For new
+files:
+
+  - C: please use standard linux-with-tabs
+  - Shell: use TABs for indentation
+
+## Patches
+
+Patches are welcome at the KVM mailing list <kvm@vger.kernel.org>.
+
+Please prefix messages with: [kvm-unit-tests PATCH]
+
+You can add the following to .git/config to do this automatically for you:
+
+    [format]
+        subjectprefix = kvm-unit-tests PATCH
+
-- 
2.11.0

[kvm-unit-tests PATCH 4/6] docs: mention checkpatch in the README

[Alex Bennée]

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
---
 README.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/README.md b/README.md
index 5027b62..9462824 100644
--- a/README.md
+++ b/README.md
@@ -79,3 +79,4 @@ You can add the following to .git/config to do this automatically for you:
     [format]
         subjectprefix = kvm-unit-tests PATCH
 
+Please run the kernel's ./scripts/checkpatch.pl on new patches
-- 
2.11.0

[kvm-unit-tests PATCH 5/6] docs: mention modifying env vars in README

[Alex Bennée]

I had started adding a series of flags to control the run-time
behaviour of the tests but it was pointed out env vars can already do
that. Mention them in the README so others can find out to.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
---
 README.md | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/README.md b/README.md
index 9462824..fa3a445 100644
--- a/README.md
+++ b/README.md
@@ -47,6 +47,14 @@ environment variable:
 
     QEMU=/tmp/qemu/x86_64-softmmu/qemu-system-x86_64 ./x86-run ./x86/msr.flat
 
+To force the acceleration mode:
+
+    ACCEL=tcg ./run_tests.sh
+
+To extend or disable the timeouts:
+
+    TIMEOUT=0 ./run_tests.sh
+
 # Contributing
 
 ## Directory structure
-- 
2.11.0

[kvm-unit-tests PATCH 6/6] run_tests: allow passing of options to QEMU

[Alex Bennée]

This allows additional options to be passed to QEMU. It follows the
convention of passing parameters after a -- to the child process. In
my case I'm using it to toggle MTTCG on an off:

  ./run_tests.sh -- --accel tcg,thread=multi

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>

---
v1
  - changes from -o to --
  - fixed whitespace damage
---
 README.md              |  6 ++++++
 run_tests.sh           | 13 +++++++++++--
 scripts/functions.bash |  7 ++++---
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/README.md b/README.md
index fa3a445..1bd6dcb 100644
--- a/README.md
+++ b/README.md
@@ -55,6 +55,12 @@ To extend or disable the timeouts:
 
     TIMEOUT=0 ./run_tests.sh
 
+Any arguments past the end-of-arguments marker (--) is passed on down
+to the QEMU invocation. This can of course be combined with the other
+modifiers:
+
+    ACCEL=tcg ./run_tests.sh -v -- --accel tcg,thread=multi
+
 # Contributing
 
 ## Directory structure
diff --git a/run_tests.sh b/run_tests.sh
index 254129d..3270fba 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -13,7 +13,7 @@ function usage()
 {
 cat <<EOF
 
-Usage: $0 [-g group] [-h] [-v]
+Usage: $0 [-g group] [-h] [-v] [-- QEMU options]
 
     -g: Only execute tests in the given group
     -h: Output this help text
@@ -22,6 +22,8 @@ Usage: $0 [-g group] [-h] [-v]
 Set the environment variable QEMU=/path/to/qemu-system-ARCH to
 specify the appropriate qemu binary for ARCH-run.
 
+All options specified after -- are passed on to QEMU.
+
 EOF
 }
 
@@ -29,6 +31,7 @@ RUNTIME_arch_run="./$TEST_DIR/run"
 source scripts/runtime.bash
 
 while getopts "g:hv" opt; do
+
     case $opt in
         g)
             only_group=$OPTARG
@@ -46,6 +49,12 @@ while getopts "g:hv" opt; do
     esac
 done
 
+# Any options left for QEMU?
+shift $((OPTIND-1))
+if [ "$#" -gt  0 ]; then
+    extra_opts="$@"
+fi
+
 RUNTIME_log_stderr () { cat >> test.log; }
 RUNTIME_log_stdout () {
     if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
@@ -59,4 +68,4 @@ RUNTIME_log_stdout () {
 config=$TEST_DIR/unittests.cfg
 rm -f test.log
 printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
-for_each_unittest $config run
+for_each_unittest $config run "$extra_opts"
diff --git a/scripts/functions.bash b/scripts/functions.bash
index ee9143c..60fbc6a 100644
--- a/scripts/functions.bash
+++ b/scripts/functions.bash
@@ -3,10 +3,11 @@ function for_each_unittest()
 {
 	local unittests="$1"
 	local cmd="$2"
+	local extra_opts=$3
 	local testname
 	local smp
 	local kernel
-	local opts
+	local opts=$extra_opts
 	local groups
 	local arch
 	local check
@@ -21,7 +22,7 @@ function for_each_unittest()
 			testname=${BASH_REMATCH[1]}
 			smp=1
 			kernel=""
-			opts=""
+			opts=$extra_opts
 			groups=""
 			arch=""
 			check=""
@@ -32,7 +33,7 @@ function for_each_unittest()
 		elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
 			smp=${BASH_REMATCH[1]}
 		elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
-			opts=${BASH_REMATCH[1]}
+			opts="$opts ${BASH_REMATCH[1]}"
 		elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
 			groups=${BASH_REMATCH[1]}
 		elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
-- 
2.11.0

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Paolo Bonzini]

On 11/01/2017 17:28, Alex Benn?e wrote:
> So we can have portable formatting of uint32_t types. However there is
> a catch. Different compilers can use legally subtly different types
> though so we need to probe the compiler defined intdef.h first.

Interesting, what platform has long uint32_t?  I thought the issue was
whether 64-bit is long or "long long".

Paolo

> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
>  Makefile       |  1 +
>  configure      | 13 +++++++++++++
>  lib/libcflat.h |  9 +++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index a32333b..9822d9a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -55,6 +55,7 @@ CFLAGS += $(fomit_frame_pointer)
>  CFLAGS += $(fno_stack_protector)
>  CFLAGS += $(fno_stack_protector_all)
>  CFLAGS += $(wno_frame_address)
> +CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)
>  
>  CXXFLAGS += $(CFLAGS)
>  
> diff --git a/configure b/configure
> index 995c8fa..127868c 100755
> --- a/configure
> +++ b/configure
> @@ -109,6 +109,18 @@ if [ -f $testdir/run ]; then
>      ln -fs $testdir/run $testdir-run
>  fi
>  
> +# check if uint32_t needs a long format modifier
> +cat << EOF > lib_test.c
> +#include <inttypes.h>
> +EOF
> +
> +$cross_prefix$cc lib_test.c -E | grep "typedef" | grep "long" | grep "uint32_t" &> /dev/null
> +exit=$?
> +if [ $exit -eq 0 ]; then
> +    u32_long=true
> +fi
> +rm -f lib_test.c
> +
>  # check for dependent 32 bit libraries
>  if [ "$arch" != "arm" ]; then
>  cat << EOF > lib_test.c
> @@ -155,4 +167,5 @@ TEST_DIR=$testdir
>  FIRMWARE=$firmware
>  ENDIAN=$endian
>  PRETTY_PRINT_STACKS=$pretty_print_stacks
> +U32_LONG_FMT=$u32_long
>  EOF
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 380395f..e80fc50 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -58,12 +58,21 @@ typedef _Bool		bool;
>  #define true  1
>  
>  #if __SIZEOF_LONG__ == 8
> +#  define __PRI32_PREFIX
>  #  define __PRI64_PREFIX	"l"
>  #  define __PRIPTR_PREFIX	"l"
>  #else
> +#if defined(__U32_LONG_FMT__)
> +#  define __PRI32_PREFIX        "l"
> +#else
> +#  define __PRI32_PREFIX
> +#endif
>  #  define __PRI64_PREFIX	"ll"
>  #  define __PRIPTR_PREFIX
>  #endif
> +#define PRId32  __PRI32_PREFIX	"d"
> +#define PRIu32  __PRI32_PREFIX	"u"
> +#define PRIx32  __PRI32_PREFIX	"x"
>  #define PRId64  __PRI64_PREFIX	"d"
>  #define PRIu64  __PRI64_PREFIX	"u"
>  #define PRIx64  __PRI64_PREFIX	"x"
> 

[kvm-unit-tests PATCH 4/6] docs: mention checkpatch in the README

[Paolo Bonzini]

On 11/01/2017 17:28, Alex Benn?e wrote:
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
>  README.md | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/README.md b/README.md
> index 5027b62..9462824 100644
> --- a/README.md
> +++ b/README.md
> @@ -79,3 +79,4 @@ You can add the following to .git/config to do this automatically for you:
>      [format]
>          subjectprefix = kvm-unit-tests PATCH
>  
> +Please run the kernel's ./scripts/checkpatch.pl on new patches

Does it really work well on kvm-unit-tests?

Paolo

[kvm-unit-tests PATCH 6/6] run_tests: allow passing of options to QEMU

[Paolo Bonzini]

On 11/01/2017 17:28, Alex Benn?e wrote:
> This allows additional options to be passed to QEMU. It follows the
> convention of passing parameters after a -- to the child process. In
> my case I'm using it to toggle MTTCG on an off:
> 
>   ./run_tests.sh -- --accel tcg,thread=multi
> 
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> 
> ---
> v1
>   - changes from -o to --
>   - fixed whitespace damage
> ---
>  README.md              |  6 ++++++
>  run_tests.sh           | 13 +++++++++++--
>  scripts/functions.bash |  7 ++++---
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/README.md b/README.md
> index fa3a445..1bd6dcb 100644
> --- a/README.md
> +++ b/README.md
> @@ -55,6 +55,12 @@ To extend or disable the timeouts:
>  
>      TIMEOUT=0 ./run_tests.sh
>  
> +Any arguments past the end-of-arguments marker (--) is passed on down
> +to the QEMU invocation. This can of course be combined with the other
> +modifiers:
> +
> +    ACCEL=tcg ./run_tests.sh -v -- --accel tcg,thread=multi
> +
>  # Contributing
>  
>  ## Directory structure
> diff --git a/run_tests.sh b/run_tests.sh
> index 254129d..3270fba 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -13,7 +13,7 @@ function usage()
>  {
>  cat <<EOF
>  
> -Usage: $0 [-g group] [-h] [-v]
> +Usage: $0 [-g group] [-h] [-v] [-- QEMU options]
>  
>      -g: Only execute tests in the given group
>      -h: Output this help text
> @@ -22,6 +22,8 @@ Usage: $0 [-g group] [-h] [-v]
>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
>  specify the appropriate qemu binary for ARCH-run.
>  
> +All options specified after -- are passed on to QEMU.
> +
>  EOF
>  }
>  
> @@ -29,6 +31,7 @@ RUNTIME_arch_run="./$TEST_DIR/run"
>  source scripts/runtime.bash
>  
>  while getopts "g:hv" opt; do
> +
>      case $opt in
>          g)
>              only_group=$OPTARG
> @@ -46,6 +49,12 @@ while getopts "g:hv" opt; do
>      esac
>  done
>  
> +# Any options left for QEMU?
> +shift $((OPTIND-1))
> +if [ "$#" -gt  0 ]; then
> +    extra_opts="$@"
> +fi
> +
>  RUNTIME_log_stderr () { cat >> test.log; }
>  RUNTIME_log_stdout () {
>      if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
> @@ -59,4 +68,4 @@ RUNTIME_log_stdout () {
>  config=$TEST_DIR/unittests.cfg
>  rm -f test.log
>  printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
> -for_each_unittest $config run
> +for_each_unittest $config run "$extra_opts"
> diff --git a/scripts/functions.bash b/scripts/functions.bash
> index ee9143c..60fbc6a 100644
> --- a/scripts/functions.bash
> +++ b/scripts/functions.bash
> @@ -3,10 +3,11 @@ function for_each_unittest()
>  {
>  	local unittests="$1"
>  	local cmd="$2"
> +	local extra_opts=$3
>  	local testname
>  	local smp
>  	local kernel
> -	local opts
> +	local opts=$extra_opts
>  	local groups
>  	local arch
>  	local check
> @@ -21,7 +22,7 @@ function for_each_unittest()
>  			testname=${BASH_REMATCH[1]}
>  			smp=1
>  			kernel=""
> -			opts=""
> +			opts=$extra_opts
>  			groups=""
>  			arch=""
>  			check=""
> @@ -32,7 +33,7 @@ function for_each_unittest()
>  		elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
>  			smp=${BASH_REMATCH[1]}
>  		elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
> -			opts=${BASH_REMATCH[1]}
> +			opts="$opts ${BASH_REMATCH[1]}"
>  		elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
>  			groups=${BASH_REMATCH[1]}
>  		elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
> 

Great idea!

Paolo

[kvm-unit-tests PATCH 4/6] docs: mention checkpatch in the README

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/01/2017 17:28, Alex Benn?e wrote:
>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>> ---
>>  README.md | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/README.md b/README.md
>> index 5027b62..9462824 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -79,3 +79,4 @@ You can add the following to .git/config to do this automatically for you:
>>      [format]
>>          subjectprefix = kvm-unit-tests PATCH
>>
>> +Please run the kernel's ./scripts/checkpatch.pl on new patches
>
> Does it really work well on kvm-unit-tests?

Well I keep getting pulled up on kernel coding style on my reviews so if
we mean it we should enforce it.

--
Alex Benn?e

[kvm-unit-tests PATCH 2/6] lib/pci: fix BAR format strings

[Paolo Bonzini]

On 11/01/2017 17:28, Alex Benn?e wrote:
> Using %x as a format string is not portable across 32/64 bit builds.
> Use explicit PRIx32 format strings like the 64 bit version above.
> 
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
>  lib/pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 6416191..597d8f2 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -67,7 +67,7 @@ bool pci_setup_msi(struct pci_dev *dev, uint64_t msi_addr, uint32_t msi_data)
>  		pci_config_writel(addr, offset + PCI_MSI_DATA_32, msi_data);
>  		printf("MSI: dev 0x%x init 32bit address: ", addr);
>  	}
> -	printf("addr=0x%lx, data=0x%x\n", msi_addr, msi_data);
> +	printf("addr=0x%" PRIx64 ", data=0x%" PRIx32 "\n", msi_addr, msi_data);

Applying only the %lx -> %"PRIx64" change for now (though it's also in
Andrew's 32-bit compilation fixes patch).

Paolo
>  
>  	msi_control |= PCI_MSI_FLAGS_ENABLE;
>  	pci_config_writew(addr, offset + PCI_MSI_FLAGS, msi_control);
> @@ -237,7 +237,7 @@ void pci_bar_print(struct pci_dev *dev, int bar_num)
>  		printf("BAR#%d,%d [%" PRIx64 "-%" PRIx64 " ",
>  		       bar_num, bar_num + 1, start, end);
>  	} else {
> -		printf("BAR#%d [%02x-%02x ",
> +		printf("BAR#%d [%" PRIx32 "-%" PRIx32 " ",
>  		       bar_num, (uint32_t)start, (uint32_t)end);
>  	}
>  
> 

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/01/2017 17:28, Alex Benn?e wrote:
>> So we can have portable formatting of uint32_t types. However there is
>> a catch. Different compilers can use legally subtly different types
>> though so we need to probe the compiler defined intdef.h first.
>
> Interesting, what platform has long uint32_t?  I thought the issue was
> whether 64-bit is long or "long long".

I haven't run into that one. This came up with the arm-none-eabi-gcc on
my overdrive01 box. According to the toolchain guys there is no particular
reason a 32bit compiler can't use long for its natural word length.

The native compiler on Debian armhf doesn't do this but the
arm-none-eabi-gcc compilers on both 64bit and 32bit Debian need this.

>
> Paolo
>
>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>> ---
>>  Makefile       |  1 +
>>  configure      | 13 +++++++++++++
>>  lib/libcflat.h |  9 +++++++++
>>  3 files changed, 23 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index a32333b..9822d9a 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -55,6 +55,7 @@ CFLAGS += $(fomit_frame_pointer)
>>  CFLAGS += $(fno_stack_protector)
>>  CFLAGS += $(fno_stack_protector_all)
>>  CFLAGS += $(wno_frame_address)
>> +CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)
>>
>>  CXXFLAGS += $(CFLAGS)
>>
>> diff --git a/configure b/configure
>> index 995c8fa..127868c 100755
>> --- a/configure
>> +++ b/configure
>> @@ -109,6 +109,18 @@ if [ -f $testdir/run ]; then
>>      ln -fs $testdir/run $testdir-run
>>  fi
>>
>> +# check if uint32_t needs a long format modifier
>> +cat << EOF > lib_test.c
>> +#include <inttypes.h>
>> +EOF
>> +
>> +$cross_prefix$cc lib_test.c -E | grep "typedef" | grep "long" | grep "uint32_t" &> /dev/null
>> +exit=$?
>> +if [ $exit -eq 0 ]; then
>> +    u32_long=true
>> +fi
>> +rm -f lib_test.c
>> +
>>  # check for dependent 32 bit libraries
>>  if [ "$arch" != "arm" ]; then
>>  cat << EOF > lib_test.c
>> @@ -155,4 +167,5 @@ TEST_DIR=$testdir
>>  FIRMWARE=$firmware
>>  ENDIAN=$endian
>>  PRETTY_PRINT_STACKS=$pretty_print_stacks
>> +U32_LONG_FMT=$u32_long
>>  EOF
>> diff --git a/lib/libcflat.h b/lib/libcflat.h
>> index 380395f..e80fc50 100644
>> --- a/lib/libcflat.h
>> +++ b/lib/libcflat.h
>> @@ -58,12 +58,21 @@ typedef _Bool		bool;
>>  #define true  1
>>
>>  #if __SIZEOF_LONG__ == 8
>> +#  define __PRI32_PREFIX
>>  #  define __PRI64_PREFIX	"l"
>>  #  define __PRIPTR_PREFIX	"l"
>>  #else
>> +#if defined(__U32_LONG_FMT__)
>> +#  define __PRI32_PREFIX        "l"
>> +#else
>> +#  define __PRI32_PREFIX
>> +#endif
>>  #  define __PRI64_PREFIX	"ll"
>>  #  define __PRIPTR_PREFIX
>>  #endif
>> +#define PRId32  __PRI32_PREFIX	"d"
>> +#define PRIu32  __PRI32_PREFIX	"u"
>> +#define PRIx32  __PRI32_PREFIX	"x"
>>  #define PRId64  __PRI64_PREFIX	"d"
>>  #define PRIu64  __PRI64_PREFIX	"u"
>>  #define PRIx64  __PRI64_PREFIX	"x"
>>


--
Alex Benn?e

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Andrew Jones]

On Thu, Jan 12, 2017 at 01:29:24PM +0100, Paolo Bonzini wrote:
> 
> 
> On 11/01/2017 17:28, Alex Benn?e wrote:
> > So we can have portable formatting of uint32_t types. However there is
> > a catch. Different compilers can use legally subtly different types
> > though so we need to probe the compiler defined intdef.h first.
> 
> Interesting, what platform has long uint32_t?  I thought the issue was
> whether 64-bit is long or "long long".
> 
> Paolo
> 
> > Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> > ---
> >  Makefile       |  1 +
> >  configure      | 13 +++++++++++++
> >  lib/libcflat.h |  9 +++++++++
> >  3 files changed, 23 insertions(+)
> > 
> > diff --git a/Makefile b/Makefile
> > index a32333b..9822d9a 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -55,6 +55,7 @@ CFLAGS += $(fomit_frame_pointer)
> >  CFLAGS += $(fno_stack_protector)
> >  CFLAGS += $(fno_stack_protector_all)
> >  CFLAGS += $(wno_frame_address)
> > +CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)
> >  
> >  CXXFLAGS += $(CFLAGS)
> >  
> > diff --git a/configure b/configure
> > index 995c8fa..127868c 100755
> > --- a/configure
> > +++ b/configure
> > @@ -109,6 +109,18 @@ if [ -f $testdir/run ]; then
> >      ln -fs $testdir/run $testdir-run
> >  fi
> >  
> > +# check if uint32_t needs a long format modifier
> > +cat << EOF > lib_test.c
> > +#include <inttypes.h>
> > +EOF
> > +
> > +$cross_prefix$cc lib_test.c -E | grep "typedef" | grep "long" | grep "uint32_t" &> /dev/null

This won't work with cross compilers that don't have inttypes.h in their
path (like mine). How about something like

 cat << EOF > lib_test.c
 __UINT32_TYPE__
 EOF
 u32_long="`aarch64-linux-gnu-gcc lib_test.c -E | awk '/unsigned/ && $2 == "long"'`"

Although I feel there should be a compiler macro way to do this without
a need for configure/makefile trickery at all...

Thanks,
drew


> > +exit=$?
> > +if [ $exit -eq 0 ]; then
> > +    u32_long=true
> > +fi
> > +rm -f lib_test.c
> > +
> >  # check for dependent 32 bit libraries
> >  if [ "$arch" != "arm" ]; then
> >  cat << EOF > lib_test.c
> > @@ -155,4 +167,5 @@ TEST_DIR=$testdir
> >  FIRMWARE=$firmware
> >  ENDIAN=$endian
> >  PRETTY_PRINT_STACKS=$pretty_print_stacks
> > +U32_LONG_FMT=$u32_long
> >  EOF
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index 380395f..e80fc50 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -58,12 +58,21 @@ typedef _Bool		bool;
> >  #define true  1
> >  
> >  #if __SIZEOF_LONG__ == 8
> > +#  define __PRI32_PREFIX
> >  #  define __PRI64_PREFIX	"l"
> >  #  define __PRIPTR_PREFIX	"l"
> >  #else
> > +#if defined(__U32_LONG_FMT__)
> > +#  define __PRI32_PREFIX        "l"
> > +#else
> > +#  define __PRI32_PREFIX
> > +#endif
> >  #  define __PRI64_PREFIX	"ll"
> >  #  define __PRIPTR_PREFIX
> >  #endif
> > +#define PRId32  __PRI32_PREFIX	"d"
> > +#define PRIu32  __PRI32_PREFIX	"u"
> > +#define PRIx32  __PRI32_PREFIX	"x"
> >  #define PRId64  __PRI64_PREFIX	"d"
> >  #define PRIu64  __PRI64_PREFIX	"u"
> >  #define PRIx64  __PRI64_PREFIX	"x"
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[kvm-unit-tests PATCH 3/6] docs: move README to README.md and symlink

[Andrew Jones]

On Wed, Jan 11, 2017 at 04:28:38PM +0000, Alex Benn?e wrote:
> This allows a slightly nicer formatting of the text when displayed on
> some repository hosts. We keep a symlink from README for the
> old-school purists.
> 
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
>  README    | 69 +----------------------------------------------------
>  README.md | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 68 deletions(-)
>  mode change 100644 => 120000 README
>  create mode 100644 README.md

Please use 'format-patch -M' to create patches when renaming files.
I'm not sure I care about having a .md version or not, but only
one README* file in the root dir would be my preference, i.e. if we
want a .md version, then let's just have that, i.e no symlink.

Thanks,
drew

[kvm-unit-tests PATCH 4/6] docs: mention checkpatch in the README

[Andrew Jones]

On Thu, Jan 12, 2017 at 01:29:52PM +0100, Paolo Bonzini wrote:
> 
> 
> On 11/01/2017 17:28, Alex Benn?e wrote:
> > Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> > ---
> >  README.md | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/README.md b/README.md
> > index 5027b62..9462824 100644
> > --- a/README.md
> > +++ b/README.md
> > @@ -79,3 +79,4 @@ You can add the following to .git/config to do this automatically for you:
> >      [format]
> >          subjectprefix = kvm-unit-tests PATCH
> >  
> > +Please run the kernel's ./scripts/checkpatch.pl on new patches
> 
> Does it really work well on kvm-unit-tests?

I use it on new code (liberally ignoring stuff I don't think
is worth conforming to), but there's no chance it'll work on
old files (many lib files and pretty much all x86 files). So
we can't really enforce for x86 developers, as we prefer
consistency over kernel style. That said, I think mentioning
in the README that new files could use it (at least for guidance)
is a good idea.

Thanks,
drew

[kvm-unit-tests PATCH 4/6] docs: mention checkpatch in the README

[Andrew Jones]

On Thu, Jan 12, 2017 at 12:35:03PM +0000, Alex Benn?e wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 11/01/2017 17:28, Alex Benn?e wrote:
> >> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> >> ---
> >>  README.md | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/README.md b/README.md
> >> index 5027b62..9462824 100644
> >> --- a/README.md
> >> +++ b/README.md
> >> @@ -79,3 +79,4 @@ You can add the following to .git/config to do this automatically for you:
> >>      [format]
> >>          subjectprefix = kvm-unit-tests PATCH
> >>
> >> +Please run the kernel's ./scripts/checkpatch.pl on new patches

s/patches/files

All patches should be new :-)

drew

> >
> > Does it really work well on kvm-unit-tests?
> 
> Well I keep getting pulled up on kernel coding style on my reviews so if
> we mean it we should enforce it.
> 
> --
> Alex Benn?e
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[kvm-unit-tests PATCH 5/6] docs: mention modifying env vars in README

[Andrew Jones]

On Wed, Jan 11, 2017 at 04:28:40PM +0000, Alex Benn?e wrote:
> I had started adding a series of flags to control the run-time
> behaviour of the tests but it was pointed out env vars can already do
> that. Mention them in the README so others can find out to.
> 
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
>  README.md | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/README.md b/README.md
> index 9462824..fa3a445 100644
> --- a/README.md
> +++ b/README.md
> @@ -47,6 +47,14 @@ environment variable:
>  
>      QEMU=/tmp/qemu/x86_64-softmmu/qemu-system-x86_64 ./x86-run ./x86/msr.flat
>  
> +To force the acceleration mode:
> +
> +    ACCEL=tcg ./run_tests.sh
> +
> +To extend or disable the timeouts:
> +
> +    TIMEOUT=0 ./run_tests.sh
> +

This is a nice addition to the README, but please add more detail.

To force the use of TCG:
 ACCEL=tcg ./run_tests.sh

To force failure when KVM is not present:
 ACCEL=kvm ./run_tests.sh

To modify the timeout:
 TIMEOUT=$DURATION ./run_tests.sh # man timeout(1) for duration format
 TIMEOUT=0 ./run_tests.sh # disable the timeout

or something like that...

thanks,
drew


>  # Contributing
>  
>  ## Directory structure
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Alex Bennée]

Andrew Jones <drjones@redhat.com> writes:

> On Thu, Jan 12, 2017 at 01:29:24PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 11/01/2017 17:28, Alex Benn?e wrote:
>> > So we can have portable formatting of uint32_t types. However there is
>> > a catch. Different compilers can use legally subtly different types
>> > though so we need to probe the compiler defined intdef.h first.
>>
>> Interesting, what platform has long uint32_t?  I thought the issue was
>> whether 64-bit is long or "long long".
>>
>> Paolo
>>
>> > Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>> > ---
>> >  Makefile       |  1 +
>> >  configure      | 13 +++++++++++++
>> >  lib/libcflat.h |  9 +++++++++
>> >  3 files changed, 23 insertions(+)
>> >
>> > diff --git a/Makefile b/Makefile
>> > index a32333b..9822d9a 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -55,6 +55,7 @@ CFLAGS += $(fomit_frame_pointer)
>> >  CFLAGS += $(fno_stack_protector)
>> >  CFLAGS += $(fno_stack_protector_all)
>> >  CFLAGS += $(wno_frame_address)
>> > +CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)
>> >
>> >  CXXFLAGS += $(CFLAGS)
>> >
>> > diff --git a/configure b/configure
>> > index 995c8fa..127868c 100755
>> > --- a/configure
>> > +++ b/configure
>> > @@ -109,6 +109,18 @@ if [ -f $testdir/run ]; then
>> >      ln -fs $testdir/run $testdir-run
>> >  fi
>> >
>> > +# check if uint32_t needs a long format modifier
>> > +cat << EOF > lib_test.c
>> > +#include <inttypes.h>
>> > +EOF
>> > +
>> > +$cross_prefix$cc lib_test.c -E | grep "typedef" | grep "long" | grep "uint32_t" &> /dev/null
>
> This won't work with cross compilers that don't have inttypes.h in their
> path (like mine). How about something like

Hmm good point, in my case inttypes.h came from the libnewlib-dev package.

>
>  cat << EOF > lib_test.c
>  __UINT32_TYPE__
>  EOF
>  u32_long="`aarch64-linux-gnu-gcc lib_test.c -E | awk '/unsigned/ && $2 == "long"'`"

Hmm it is not clear from the docs if this is a GCCism. If it is do we
care? Also the docs do say:

"Some of these macros may not be defined on particular systems if GCC does not provide a stdint.h header on those systems. "

> Although I feel there should be a compiler macro way to do this without
> a need for configure/makefile trickery at all...

I did ask our toolchain bods. They started going on about potential
solutions using _Generic but I fear that might be worse in this case!

>
> Thanks,
> drew
>
>
>> > +exit=$?
>> > +if [ $exit -eq 0 ]; then
>> > +    u32_long=true
>> > +fi
>> > +rm -f lib_test.c
>> > +
>> >  # check for dependent 32 bit libraries
>> >  if [ "$arch" != "arm" ]; then
>> >  cat << EOF > lib_test.c
>> > @@ -155,4 +167,5 @@ TEST_DIR=$testdir
>> >  FIRMWARE=$firmware
>> >  ENDIAN=$endian
>> >  PRETTY_PRINT_STACKS=$pretty_print_stacks
>> > +U32_LONG_FMT=$u32_long
>> >  EOF
>> > diff --git a/lib/libcflat.h b/lib/libcflat.h
>> > index 380395f..e80fc50 100644
>> > --- a/lib/libcflat.h
>> > +++ b/lib/libcflat.h
>> > @@ -58,12 +58,21 @@ typedef _Bool		bool;
>> >  #define true  1
>> >
>> >  #if __SIZEOF_LONG__ == 8
>> > +#  define __PRI32_PREFIX
>> >  #  define __PRI64_PREFIX	"l"
>> >  #  define __PRIPTR_PREFIX	"l"
>> >  #else
>> > +#if defined(__U32_LONG_FMT__)
>> > +#  define __PRI32_PREFIX        "l"
>> > +#else
>> > +#  define __PRI32_PREFIX
>> > +#endif
>> >  #  define __PRI64_PREFIX	"ll"
>> >  #  define __PRIPTR_PREFIX
>> >  #endif
>> > +#define PRId32  __PRI32_PREFIX	"d"
>> > +#define PRIu32  __PRI32_PREFIX	"u"
>> > +#define PRIx32  __PRI32_PREFIX	"x"
>> >  #define PRId64  __PRI64_PREFIX	"d"
>> >  #define PRIu64  __PRI64_PREFIX	"u"
>> >  #define PRIx64  __PRI64_PREFIX	"x"
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
Alex Benn?e

[kvm-unit-tests PATCH 6/6] run_tests: allow passing of options to QEMU

[Andrew Jones]

On Wed, Jan 11, 2017 at 04:28:41PM +0000, Alex Benn?e wrote:
> This allows additional options to be passed to QEMU. It follows the
> convention of passing parameters after a -- to the child process. In
> my case I'm using it to toggle MTTCG on an off:
> 
>   ./run_tests.sh -- --accel tcg,thread=multi
> 
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> 
> ---
> v1
>   - changes from -o to --
>   - fixed whitespace damage
> ---
>  README.md              |  6 ++++++
>  run_tests.sh           | 13 +++++++++++--
>  scripts/functions.bash |  7 ++++---
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/README.md b/README.md
> index fa3a445..1bd6dcb 100644
> --- a/README.md
> +++ b/README.md
> @@ -55,6 +55,12 @@ To extend or disable the timeouts:
>  
>      TIMEOUT=0 ./run_tests.sh
>  
> +Any arguments past the end-of-arguments marker (--) is passed on down
> +to the QEMU invocation. This can of course be combined with the other
> +modifiers:
> +
> +    ACCEL=tcg ./run_tests.sh -v -- --accel tcg,thread=multi
> +
>  # Contributing
>  
>  ## Directory structure
> diff --git a/run_tests.sh b/run_tests.sh
> index 254129d..3270fba 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -13,7 +13,7 @@ function usage()
>  {
>  cat <<EOF
>  
> -Usage: $0 [-g group] [-h] [-v]
> +Usage: $0 [-g group] [-h] [-v] [-- QEMU options]
>  
>      -g: Only execute tests in the given group
>      -h: Output this help text
> @@ -22,6 +22,8 @@ Usage: $0 [-g group] [-h] [-v]
>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
>  specify the appropriate qemu binary for ARCH-run.
>  
> +All options specified after -- are passed on to QEMU.
> +
>  EOF
>  }
>  
> @@ -29,6 +31,7 @@ RUNTIME_arch_run="./$TEST_DIR/run"
>  source scripts/runtime.bash
>  
>  while getopts "g:hv" opt; do
> +

stray blank line added?

>      case $opt in
>          g)
>              only_group=$OPTARG
> @@ -46,6 +49,12 @@ while getopts "g:hv" opt; do
>      esac
>  done
>  
> +# Any options left for QEMU?
> +shift $((OPTIND-1))
> +if [ "$#" -gt  0 ]; then
> +    extra_opts="$@"
> +fi

We can unconditionally do the extra_opts="$@", extra_opts will just
be null in the case there aren't more args, like it was before.

> +
>  RUNTIME_log_stderr () { cat >> test.log; }
>  RUNTIME_log_stdout () {
>      if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
> @@ -59,4 +68,4 @@ RUNTIME_log_stdout () {
>  config=$TEST_DIR/unittests.cfg
>  rm -f test.log
>  printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
> -for_each_unittest $config run
> +for_each_unittest $config run "$extra_opts"
> diff --git a/scripts/functions.bash b/scripts/functions.bash
> index ee9143c..60fbc6a 100644
> --- a/scripts/functions.bash
> +++ b/scripts/functions.bash
> @@ -3,10 +3,11 @@ function for_each_unittest()
>  {
>  	local unittests="$1"
>  	local cmd="$2"
> +	local extra_opts=$3
>  	local testname
>  	local smp
>  	local kernel
> -	local opts
> +	local opts=$extra_opts
>  	local groups
>  	local arch
>  	local check
> @@ -21,7 +22,7 @@ function for_each_unittest()
>  			testname=${BASH_REMATCH[1]}
>  			smp=1
>  			kernel=""
> -			opts=""
> +			opts=$extra_opts
>  			groups=""
>  			arch=""
>  			check=""
> @@ -32,7 +33,7 @@ function for_each_unittest()
>  		elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
>  			smp=${BASH_REMATCH[1]}
>  		elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
> -			opts=${BASH_REMATCH[1]}
> +			opts="$opts ${BASH_REMATCH[1]}"

How do QEMU opts work with respect to precedence? If the later
opts override the earlier (I think they do), then we should put
opts after rematch here, because the user explicitly added those
options to the command line, and therefore probably prefers them.

>  		elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
>  			groups=${BASH_REMATCH[1]}
>  		elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
> -- 
> 2.11.0
>

Thanks for this patch! I'm looking forward to making use of it for
testing Peter's EL2 series with/without "-machine virtualization=on"

drew

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Andrew Jones]

On Thu, Jan 12, 2017 at 05:18:25PM +0000, Alex Benn?e wrote:
> 
> Andrew Jones <drjones@redhat.com> writes:
> 
> > On Thu, Jan 12, 2017 at 01:29:24PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 11/01/2017 17:28, Alex Benn?e wrote:
> >> > So we can have portable formatting of uint32_t types. However there is
> >> > a catch. Different compilers can use legally subtly different types
> >> > though so we need to probe the compiler defined intdef.h first.
> >>
> >> Interesting, what platform has long uint32_t?  I thought the issue was
> >> whether 64-bit is long or "long long".
> >>
> >> Paolo
> >>
> >> > Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> >> > ---
> >> >  Makefile       |  1 +
> >> >  configure      | 13 +++++++++++++
> >> >  lib/libcflat.h |  9 +++++++++
> >> >  3 files changed, 23 insertions(+)
> >> >
> >> > diff --git a/Makefile b/Makefile
> >> > index a32333b..9822d9a 100644
> >> > --- a/Makefile
> >> > +++ b/Makefile
> >> > @@ -55,6 +55,7 @@ CFLAGS += $(fomit_frame_pointer)
> >> >  CFLAGS += $(fno_stack_protector)
> >> >  CFLAGS += $(fno_stack_protector_all)
> >> >  CFLAGS += $(wno_frame_address)
> >> > +CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)
> >> >
> >> >  CXXFLAGS += $(CFLAGS)
> >> >
> >> > diff --git a/configure b/configure
> >> > index 995c8fa..127868c 100755
> >> > --- a/configure
> >> > +++ b/configure
> >> > @@ -109,6 +109,18 @@ if [ -f $testdir/run ]; then
> >> >      ln -fs $testdir/run $testdir-run
> >> >  fi
> >> >
> >> > +# check if uint32_t needs a long format modifier
> >> > +cat << EOF > lib_test.c
> >> > +#include <inttypes.h>
> >> > +EOF
> >> > +
> >> > +$cross_prefix$cc lib_test.c -E | grep "typedef" | grep "long" | grep "uint32_t" &> /dev/null
> >
> > This won't work with cross compilers that don't have inttypes.h in their
> > path (like mine). How about something like
> 
> Hmm good point, in my case inttypes.h came from the libnewlib-dev package.
> 
> >
> >  cat << EOF > lib_test.c
> >  __UINT32_TYPE__
> >  EOF
> >  u32_long="`aarch64-linux-gnu-gcc lib_test.c -E | awk '/unsigned/ && $2 == "long"'`"
> 
> Hmm it is not clear from the docs if this is a GCCism. If it is do we
> care? Also the docs do say:

I think we're pretty bound to gcc already. I guess until somebody picks
up the effort to get clang, or whatever, compiling runnable
kvm-unit-tests, and commits to maintaining it, then I wouldn't worry too
much about it...

> 
> "Some of these macros may not be defined on particular systems if GCC does not provide a stdint.h header on those systems. "

We already expect stdint.h, we include it from libcflat.h.

> 
> > Although I feel there should be a compiler macro way to do this without
> > a need for configure/makefile trickery at all...
> 
> I did ask our toolchain bods. They started going on about potential
> solutions using _Generic but I fear that might be worse in this case!

Yikes :-)

drew

> 
> >
> > Thanks,
> > drew
> >
> >
> >> > +exit=$?
> >> > +if [ $exit -eq 0 ]; then
> >> > +    u32_long=true
> >> > +fi
> >> > +rm -f lib_test.c
> >> > +
> >> >  # check for dependent 32 bit libraries
> >> >  if [ "$arch" != "arm" ]; then
> >> >  cat << EOF > lib_test.c
> >> > @@ -155,4 +167,5 @@ TEST_DIR=$testdir
> >> >  FIRMWARE=$firmware
> >> >  ENDIAN=$endian
> >> >  PRETTY_PRINT_STACKS=$pretty_print_stacks
> >> > +U32_LONG_FMT=$u32_long
> >> >  EOF
> >> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> >> > index 380395f..e80fc50 100644
> >> > --- a/lib/libcflat.h
> >> > +++ b/lib/libcflat.h
> >> > @@ -58,12 +58,21 @@ typedef _Bool		bool;
> >> >  #define true  1
> >> >
> >> >  #if __SIZEOF_LONG__ == 8
> >> > +#  define __PRI32_PREFIX
> >> >  #  define __PRI64_PREFIX	"l"
> >> >  #  define __PRIPTR_PREFIX	"l"
> >> >  #else
> >> > +#if defined(__U32_LONG_FMT__)
> >> > +#  define __PRI32_PREFIX        "l"
> >> > +#else
> >> > +#  define __PRI32_PREFIX
> >> > +#endif
> >> >  #  define __PRI64_PREFIX	"ll"
> >> >  #  define __PRIPTR_PREFIX
> >> >  #endif
> >> > +#define PRId32  __PRI32_PREFIX	"d"
> >> > +#define PRIu32  __PRI32_PREFIX	"u"
> >> > +#define PRIx32  __PRI32_PREFIX	"x"
> >> >  #define PRId64  __PRI64_PREFIX	"d"
> >> >  #define PRIu64  __PRI64_PREFIX	"u"
> >> >  #define PRIx64  __PRI64_PREFIX	"x"
> >> >
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe kvm" in
> >> the body of a message to majordomo at vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> Alex Benn?e

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Paolo Bonzini]

On 12/01/2017 18:18, Alex Benn?e wrote:
>> Although I feel there should be a compiler macro way to do this without
>> a need for configure/makefile trickery at all...
> 
> I did ask our toolchain bods. They started going on about potential
> solutions using _Generic but I fear that might be worse in this case!

I don't think _Generic can do string concatenation, can it?

inttypes.h is not part of the set of freestanding headers, only stdint.h
is.  Who is providing stdint.h in your case?

Paolo

[kvm-unit-tests PATCH 6/6] run_tests: allow passing of options to QEMU

[Paolo Bonzini]

On 12/01/2017 18:32, Andrew Jones wrote:
>>  
>> +# Any options left for QEMU?
>> +shift $((OPTIND-1))
>> +if [ "$#" -gt  0 ]; then
>> +    extra_opts="$@"
>> +fi
> We can unconditionally do the extra_opts="$@", extra_opts will just
> be null in the case there aren't more args, like it was before.

extra_opts is not an array, so this would mess up options that contain
spaces.  (Alex's patch in general, not your tweak).

Paolo

>> +
>>  RUNTIME_log_stderr () { cat >> test.log; }
>>  RUNTIME_log_stdout () {
>>      if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
>> @@ -59,4 +68,4 @@ RUNTIME_log_stdout () {
>>  config=$TEST_DIR/unittests.cfg
>>  rm -f test.log
>>  printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
>> -for_each_unittest $config run
>> +for_each_unittest $config run "$extra_opts"

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/01/2017 18:18, Alex Benn?e wrote:
>>> Although I feel there should be a compiler macro way to do this without
>>> a need for configure/makefile trickery at all...
>>
>> I did ask our toolchain bods. They started going on about potential
>> solutions using _Generic but I fear that might be worse in this case!
>
> I don't think _Generic can do string concatenation, can it?
>
> inttypes.h is not part of the set of freestanding headers, only stdint.h
> is.  Who is providing stdint.h in your case?

/usr/lib/gcc/arm-none-eabi/5.4.1/include/stdint.h

is part of the compiler package although it can just include the lib
stdint.h if it is there:

  #ifndef _GCC_WRAP_STDINT_H
  #if __STDC_HOSTED__
  # if defined __cplusplus && __cplusplus >= 201103L
  #  undef __STDC_LIMIT_MACROS
  #  define __STDC_LIMIT_MACROS
  #  undef __STDC_CONSTANT_MACROS
  #  define __STDC_CONSTANT_MACROS
  # endif
  # include_next <stdint.h>
  #else
  # include "stdint-gcc.h"
  #endif
  #define _GCC_WRAP_STDINT_H
  #endif

So using inttypes we would get:

>arm-none-eabi-gcc ./test.c -E | grep typedef | grep uint32
typedef long unsigned int __uint32_t;
typedef __uint32_t uint32_t ;


--
Alex Benn?e

[kvm-unit-tests PATCH 1/6] libcflat: add PRI(dux)32 format types

[Andrew Jones]

On Thu, Jan 12, 2017 at 05:56:29PM +0100, Andrew Jones wrote:
> On Thu, Jan 12, 2017 at 01:29:24PM +0100, Paolo Bonzini wrote:
> > 
> > 
> > On 11/01/2017 17:28, Alex Benn?e wrote:
> > > So we can have portable formatting of uint32_t types. However there is
> > > a catch. Different compilers can use legally subtly different types
> > > though so we need to probe the compiler defined intdef.h first.
> > 
> > Interesting, what platform has long uint32_t?  I thought the issue was
> > whether 64-bit is long or "long long".
> > 
> > Paolo
> > 
> > > Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> > > ---
> > >  Makefile       |  1 +
> > >  configure      | 13 +++++++++++++
> > >  lib/libcflat.h |  9 +++++++++
> > >  3 files changed, 23 insertions(+)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index a32333b..9822d9a 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -55,6 +55,7 @@ CFLAGS += $(fomit_frame_pointer)
> > >  CFLAGS += $(fno_stack_protector)
> > >  CFLAGS += $(fno_stack_protector_all)
> > >  CFLAGS += $(wno_frame_address)
> > > +CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)
> > >  
> > >  CXXFLAGS += $(CFLAGS)
> > >  
> > > diff --git a/configure b/configure
> > > index 995c8fa..127868c 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -109,6 +109,18 @@ if [ -f $testdir/run ]; then
> > >      ln -fs $testdir/run $testdir-run
> > >  fi
> > >  
> > > +# check if uint32_t needs a long format modifier
> > > +cat << EOF > lib_test.c
> > > +#include <inttypes.h>
> > > +EOF
> > > +
> > > +$cross_prefix$cc lib_test.c -E | grep "typedef" | grep "long" | grep "uint32_t" &> /dev/null
> 
> This won't work with cross compilers that don't have inttypes.h in their
> path (like mine). How about something like

Paolo,

I see you merged the above. As I stated in this mail, that doesn't work
for my environment, neither arm nor powerpc cross-compile for me now...

Please replace with something like I proposed below. We can probably
replace the awk with a grep though.

Thanks,
drew


> 
>  cat << EOF > lib_test.c
>  __UINT32_TYPE__
>  EOF
>  u32_long="`aarch64-linux-gnu-gcc lib_test.c -E | awk '/unsigned/ && $2 == "long"'`"
> 
> Although I feel there should be a compiler macro way to do this without
> a need for configure/makefile trickery at all...
> 
> Thanks,
> drew
> 
> 
> > > +exit=$?
> > > +if [ $exit -eq 0 ]; then
> > > +    u32_long=true
> > > +fi
> > > +rm -f lib_test.c
> > > +
> > >  # check for dependent 32 bit libraries
> > >  if [ "$arch" != "arm" ]; then
> > >  cat << EOF > lib_test.c
> > > @@ -155,4 +167,5 @@ TEST_DIR=$testdir
> > >  FIRMWARE=$firmware
> > >  ENDIAN=$endian
> > >  PRETTY_PRINT_STACKS=$pretty_print_stacks
> > > +U32_LONG_FMT=$u32_long
> > >  EOF
> > > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > > index 380395f..e80fc50 100644
> > > --- a/lib/libcflat.h
> > > +++ b/lib/libcflat.h
> > > @@ -58,12 +58,21 @@ typedef _Bool		bool;
> > >  #define true  1
> > >  
> > >  #if __SIZEOF_LONG__ == 8
> > > +#  define __PRI32_PREFIX
> > >  #  define __PRI64_PREFIX	"l"
> > >  #  define __PRIPTR_PREFIX	"l"
> > >  #else
> > > +#if defined(__U32_LONG_FMT__)
> > > +#  define __PRI32_PREFIX        "l"
> > > +#else
> > > +#  define __PRI32_PREFIX
> > > +#endif
> > >  #  define __PRI64_PREFIX	"ll"
> > >  #  define __PRIPTR_PREFIX
> > >  #endif
> > > +#define PRId32  __PRI32_PREFIX	"d"
> > > +#define PRIu32  __PRI32_PREFIX	"u"
> > > +#define PRIx32  __PRI32_PREFIX	"x"
> > >  #define PRId64  __PRI64_PREFIX	"d"
> > >  #define PRIu64  __PRI64_PREFIX	"u"
> > >  #define PRIx64  __PRI64_PREFIX	"x"
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[kvm-unit-tests PATCH 6/6] run_tests: allow passing of options to QEMU

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/01/2017 18:32, Andrew Jones wrote:
>>>
>>> +# Any options left for QEMU?
>>> +shift $((OPTIND-1))
>>> +if [ "$#" -gt  0 ]; then
>>> +    extra_opts="$@"
>>> +fi
>> We can unconditionally do the extra_opts="$@", extra_opts will just
>> be null in the case there aren't more args, like it was before.
>
> extra_opts is not an array, so this would mess up options that contain
> spaces.  (Alex's patch in general, not your tweak).

Is it worth treating extra_opts as an array?

>
> Paolo
>
>>> +
>>>  RUNTIME_log_stderr () { cat >> test.log; }
>>>  RUNTIME_log_stdout () {
>>>      if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
>>> @@ -59,4 +68,4 @@ RUNTIME_log_stdout () {
>>>  config=$TEST_DIR/unittests.cfg
>>>  rm -f test.log
>>>  printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
>>> -for_each_unittest $config run
>>> +for_each_unittest $config run "$extra_opts"


--
Alex Benn?e