[PATCH 0/9] Improve Coverity integration, fix some ARM findings

[Jan Kiszka]

So far we only built the x86 configuration for the Coverity scan. In
order to include also the currently supported ARM boards, some tricks
are needed. See patches 4 and 5 for background and the taken approach.

Now that the infrastructure is in place, I would suggest to include
static analysis into your contribution process. Only as recommended step
for contributors, but as required one before taking something that
touches code into master. Comments welcome.

Finally, this series comes with a couple of fixes for issues found by
Coverity in ARM. There are some more open, but nothing is critical.
Those who already have access to the result page can check my remarks.

Jan

Jan Kiszka (9):
ci: Beautify travis script
ci: Use script for building all configurations
ci: Do not perform regular build for Coverity scan
ci: Include all configurations in Coverity scan
ci: Select single configuration for Coverity scan via branch name
Add Coverity scan to README and contribution process
arm: Fix off-by-one in gic_probe_cpu_id
arm: Stop relying on little endian ordering for GICH_ELSRn scan
arm: Fix spi_in_cell for SPIs 32..63

.travis.yml | 28 ++++++++---------
CONTRIBUTING.md | 5 ++-
README.md | 13 ++++++--
ci/build-all-configs.sh | 52 +++++++++++++++++++++++++++++++
ci/coverity-scan-build.sh | 36 +++++++++++++++++++++
hypervisor/arch/arm/gic-common.c | 6 ++--
hypervisor/arch/arm/gic-v2.c | 8 ++---
hypervisor/arch/arm/include/asm/irqchip.h | 12 ++++---
8 files changed, 129 insertions(+), 31 deletions(-)
create mode 100755 ci/build-all-configs.sh
create mode 100644 ci/coverity-scan-build.sh

--
2.1.4

[Jan Kiszka]

This will ease the maintenance when we start to use it for the Coverity
build as well.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
.travis.yml | 7 +------
ci/build-all-configs.sh | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 6 deletions(-)
create mode 100755 ci/build-all-configs.sh

diff --git a/.travis.yml b/.travis.yml
index 341a40a..046097d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -31,12 +31,7 @@ install:
- popd

script:
- - cp ci/jailhouse-config-x86.h hypervisor/include/jailhouse/config.h
- - make KDIR=ci/linux/build-x86
- - cp ci/jailhouse-config-banana-pi.h hypervisor/include/jailhouse/config.h
- - make KDIR=ci/linux/build-banana-pi ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- clean all
- - cp ci/jailhouse-config-vexpress.h hypervisor/include/jailhouse/config.h
- - make KDIR=ci/linux/build-vexpress ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- clean all
+ - ci/build-all-configs.sh

addons:
coverity_scan:
diff --git a/ci/build-all-configs.sh b/ci/build-all-configs.sh
new file mode 100755
index 0000000..fb73bea
--- /dev/null
+++ b/ci/build-all-configs.sh
@@ -0,0 +1,35 @@
+#!/bin/bash
+#
+# Jailhouse, a Linux-based partitioning hypervisor
+#
+# Copyright (c) Siemens AG, 2015
+#
+# Authors:
+# Jan Kiszka <jan.k...@siemens.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2. See
+# the COPYING file in the top-level directory.
+#
+
+CONFIGS="x86 banana-pi vexpress"
+
+for CONFIG in $CONFIGS; do
+ echo
+ echo "*** Building configuration $CONFIG ***"
+
+ cp ci/jailhouse-config-$CONFIG.h hypervisor/include/jailhouse/config.h
+
+ case $CONFIG in
+ x86)
+ ARCH=x86_64
+ CROSS_COMPILE=
+ ;;
+ *)
+ ARCH=arm
+ CROSS_COMPILE=arm-linux-gnueabihf-
+ ;;
+ esac
+
+ make KDIR=ci/linux/build-$CONFIG ARCH=$ARCH \
+ CROSS_COMPILE=$CROSS_COMPILE clean all
+done
--
2.1.4

[Jan Kiszka]

There is no need to run the script step when we only want to collect
data via the coverity_scan add-on.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

.travis.yml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 046097d..a5b454e 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -31,7 +31,9 @@ install:
- popd

script:
- - ci/build-all-configs.sh
+ - if [ ${COVERITY_SCAN_BRANCH} != 1 ];
+ then ci/build-all-configs.sh;
+ fi

addons:
coverity_scan:
--
2.1.4

[Jan Kiszka]

Adjust whitespaces, comment installation steps, use pushd/popd for
switching directories.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

.travis.yml | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 2211e51..341a40a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,7 +1,7 @@
#

# Jailhouse, a Linux-based partitioning hypervisor

#
-# Copyright (c) Siemens AG, 2014
+# Copyright (c) Siemens AG, 2014, 2015
#
# Authors:
# Jan Kiszka <jan.k...@siemens.com>
@@ -17,17 +17,18 @@ compiler:

env:
global:
- - secure: "HeNK8vFhW6656FoEc05SWwjorvMZWkUTIioHBJoX+s59uQ6MlgyPNnro8FrrcX/fNWLDNRrzl9U05423JVMWwjOuov6MgShDJkG/imadVO8Y+ZIsKdefAp/DsUhIhsnRNh8+bxUStdFAVatMvPf/ZWuEk0K3BC95yWK1VXE73ZQ="
-
+ - secure: "HeNK8vFhW6656FoEc05SWwjorvMZWkUTIioHBJoX+s59uQ6MlgyPNnro8FrrcX/fNWLDNRrzl9U05423JVMWwjOuov6MgShDJkG/imadVO8Y+ZIsKdefAp/DsUhIhsnRNh8+bxUStdFAVatMvPf/ZWuEk0K3BC95yWK1VXE73ZQ="

install:
+ # Install additional packages
- echo "deb http://archive.ubuntu.com/ubuntu utopic main restricted" | sudo tee -a /etc/apt/sources.list
- sudo apt-get update -qq
- sudo apt-get install -qq python-mako gcc-arm-linux-gnueabihf make
- - cd ci
+ # Install kernel build environment
+ - pushd ci
- wget -q http://www.kiszka.org/downloads/travis-ci/kernel-build.tar.xz
- tar xJf kernel-build.tar.xz
- - cd ..
+ - popd

script:
- cp ci/jailhouse-config-x86.h hypervisor/include/jailhouse/config.h
--
2.1.4

[Jan Kiszka]

Coverity only provides us as OSS project a single "stream", thus a
single configuration for our project. But we already have 3. However,
we can accumulate results to a certain degree with some tricks: We have
to ensure that the intermediate "make clean" runs are not tracked by
cov-build, the build tracker of Coverity.

That's why we overload the default scan-build script of Travis CI and
Coverity, obtain the original one from our script, patch that version
to run our build script in a way that we have control over what gets
tracked and what not. Nasty, but seems to work sufficiently for now.

In addition, we need to register the ARM cross-compiler via
cov-configure.

At this chance: "description" is no longer used by Coverity - drop it.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

.travis.yml | 6 +++---
ci/build-all-configs.sh | 14 +++++++++++++-
ci/coverity-scan-build.sh | 30 ++++++++++++++++++++++++++++++
3 files changed, 46 insertions(+), 4 deletions(-)
create mode 100644 ci/coverity-scan-build.sh

diff --git a/.travis.yml b/.travis.yml
index a5b454e..d7110df 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,8 +39,8 @@ addons:
coverity_scan:
project:
name: "siemens/jailhouse"
- description: "Build submitted via Travis CI"
notification_email: jan.k...@siemens.com
- build_command_prepend: "cp ci/jailhouse-config-x86.h hypervisor/include/jailhouse/config.h"
- build_command: "make KDIR=ci/linux/build-x86"
+ build_script_url: https://raw.githubusercontent.com/$TRAVIS_REPO_SLUG/$TRAVIS_BRANCH/ci/coverity-scan-build.sh
+ build_command_prepend: "cov-configure --comptype gcc --compiler arm-linux-gnueabihf-gcc --template"
+ build_command: "unused"
branch_pattern: coverity_scan
diff --git a/ci/build-all-configs.sh b/ci/build-all-configs.sh
index fb73bea..8c5ca6e 100755
--- a/ci/build-all-configs.sh
+++ b/ci/build-all-configs.sh
@@ -13,6 +13,12 @@

CONFIGS="x86 banana-pi vexpress"

+PREFIX=
+if [ "$1" == "--cov" ]; then
+ export COVERITY_UNSUPPORTED=1
+ PREFIX="cov-build --append-log --dir $2 $3"
+fi
+
for CONFIG in $CONFIGS; do
echo

echo "*** Building configuration $CONFIG ***"

@@ -30,6 +36,12 @@ for CONFIG in $CONFIGS; do
;;
esac

+ $PREFIX make KDIR=ci/linux/build-$CONFIG ARCH=$ARCH \
+ CROSS_COMPILE=$CROSS_COMPILE
+
+ # Keep the clean run out of sight for cov-build so that results are
+ # accumulated as far as possible. Multiple compilations of the same
+ # file will still leave only the last run in the results.
make KDIR=ci/linux/build-$CONFIG ARCH=$ARCH \
- CROSS_COMPILE=$CROSS_COMPILE clean all
+ CROSS_COMPILE=$CROSS_COMPILE clean
done
diff --git a/ci/coverity-scan-build.sh b/ci/coverity-scan-build.sh
new file mode 100644
index 0000000..b4f0d62
--- /dev/null
+++ b/ci/coverity-scan-build.sh
@@ -0,0 +1,30 @@

+#!/bin/bash
+#
+# Jailhouse, a Linux-based partitioning hypervisor
+#
+# Copyright (c) Siemens AG, 2015
+#
+# Authors:
+# Jan Kiszka <jan.k...@siemens.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2. See
+# the COPYING file in the top-level directory.
+#
+

+curl -s https://scan.coverity.com/scripts/travisci_build_coverity_scan.sh \
+ -o ci/travisci_build_coverity_scan.sh.orig
+
+# Patch the line that starts the build.
+# We need to control this step via our build script.
+sed 's/^COVERITY_UNSUPPORTED=1 cov-build --dir.*/ci\/build-all-configs.sh --cov \$RESULTS_DIR \$COV_BUILD_OPTIONS/' \
+ ci/travisci_build_coverity_scan.sh.orig > ci/travisci_build_coverity_scan.sh
+
+# Check if the patch applied, bail out if not.
+if diff -q ci/travisci_build_coverity_scan.sh.orig \
+ ci/travisci_build_coverity_scan.sh > /dev/null; then
+ echo "Unable to patch Coverity script!"
+ exit 1
+fi
+
+# Run the patched scanner script.
+. ci/travisci_build_coverity_scan.sh
--
2.1.4

[Jan Kiszka]

As scan results of generic files are overwritten with the last
configuration build, allow to pick a specific config for stand-alone
analysis. This comes at the price of overwriting results on the project
page but is still better than missing something subtle.

To differentiate the snapshot in Coverity, patch the description that is
attached to the upload.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

.travis.yml | 2 +-
ci/build-all-configs.sh | 5 +++++
ci/coverity-scan-build.sh | 8 +++++++-
3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index d7110df..618ce80 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -43,4 +43,4 @@ addons:
build_script_url: https://raw.githubusercontent.com/$TRAVIS_REPO_SLUG/$TRAVIS_BRANCH/ci/coverity-scan-build.sh

build_command_prepend: "cov-configure --comptype gcc --compiler arm-linux-gnueabihf-gcc --template"

build_command: "unused"
- branch_pattern: coverity_scan
+ branch_pattern: coverity_scan.*
diff --git a/ci/build-all-configs.sh b/ci/build-all-configs.sh
index 8c5ca6e..92207d7 100755
--- a/ci/build-all-configs.sh
+++ b/ci/build-all-configs.sh
@@ -13,6 +13,11 @@

CONFIGS="x86 banana-pi vexpress"

+# only build a specific config if the branch selects it
+if [ ${TRAVIS_BRANCH#coverity_scan-} != ${TRAVIS_BRANCH} ]; then
+ CONFIGS=${TRAVIS_BRANCH#coverity_scan-}
+fi
+
PREFIX=
if [ "$1" == "--cov" ]; then
export COVERITY_UNSUPPORTED=1
diff --git a/ci/coverity-scan-build.sh b/ci/coverity-scan-build.sh
index b4f0d62..72e0486 100644
--- a/ci/coverity-scan-build.sh
+++ b/ci/coverity-scan-build.sh
@@ -17,10 +17,16 @@ curl -s https://scan.coverity.com/scripts/travisci_build_coverity_scan.sh \

# Patch the line that starts the build.

# We need to control this step via our build script.

sed 's/^COVERITY_UNSUPPORTED=1 cov-build --dir.*/ci\/build-all-configs.sh --cov \$RESULTS_DIR \$COV_BUILD_OPTIONS/' \

- ci/travisci_build_coverity_scan.sh.orig > ci/travisci_build_coverity_scan.sh
+ ci/travisci_build_coverity_scan.sh.orig > ci/travisci_build_coverity_scan.sh.step1
+
+# Path the branch name into the description.
+sed 's/^ --form description=.*/ --form description="Travis CI build (branch: \$TRAVIS_BRANCH)" \\/' \
+ ci/travisci_build_coverity_scan.sh.step1 > ci/travisci_build_coverity_scan.sh

# Check if the patch applied, bail out if not.

if diff -q ci/travisci_build_coverity_scan.sh.orig \
+ ci/travisci_build_coverity_scan.sh.step1 > /dev/null || \
+ diff -q ci/travisci_build_coverity_scan.sh.step1 \
ci/travisci_build_coverity_scan.sh > /dev/null; then

echo "Unable to patch Coverity script!"

exit 1
--
2.1.4

[Jan Kiszka]

Integrate the code scan via Coverity in our documentation and the also
the contribution process.

Not all patches may require a scan prior to posting, thus only recommend
this step for contributors for now. A scan will now always be performed
for code changes before accepting them into master.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

CONTRIBUTING.md | 5 ++++-
README.md | 13 ++++++++++---
2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index d4d6e27..a7907f6 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -27,6 +27,8 @@ Contribution Checklist
- test patches sufficiently (obvious, but...) [**required**]
- no regressions are caused in affected code
- seemingly unaffected architectures still build (use Travis CI e.g.)
+ - static code analyzer finds no new defects (register a github fork with
+ Travis CI and Coverity for free scanning) [*recommended*]
- the world is still spinning

- add signed-off to all patches [**required**]
@@ -60,7 +62,8 @@ Contribution Integration Process

2. accepted patches merged into next branch

-3. further testing done by community, including CI build tests
+3. further testing done by community, including CI build tests and code
+ analyzer runs

4. if no new problems or discussions showed up, acceptance into master
* grace period for master: about 3 days
diff --git a/README.md b/README.md
index 19efc8e..b1a5a1b 100644
--- a/README.md
+++ b/README.md
@@ -25,8 +25,8 @@ WARNING: This is work in progress! Don't expect things to be complete in any
dimension. Use at your own risk. And keep the reset button in reach.


-Community
----------
+Community Resources
+-------------------

Project home:

@@ -48,7 +48,7 @@ Mailing list:
- Archives
- http://news.gmane.org/gmane.linux.jailhouse

-Continuous Integration:
+Continuous integration:

- https://travis-ci.org/siemens/jailhouse

@@ -56,6 +56,13 @@ Continuous Integration:
-  on master
-  on next

+Static code analysis:
+
+ - https://scan.coverity.com/projects/4114
+
+ - Status:
+ -  on coverity_scan
+
See the [contribution documentation](CONTRIBUTING.md) for details
on how to write Jailhouse patches and propose them for upstream integration.

--
2.1.4

[Jan Kiszka]

Arrange ELSR0 and ELSR1 in an array of unsigned longs to avoid that the
code breaks on a big endian target.

This addresses Coverity finding CID 21112.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

hypervisor/arch/arm/gic-v2.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hypervisor/arch/arm/gic-v2.c b/hypervisor/arch/arm/gic-v2.c
index 0e345c6..0113f12 100644
--- a/hypervisor/arch/arm/gic-v2.c
+++ b/hypervisor/arch/arm/gic-v2.c
@@ -228,12 +228,12 @@ static int gic_inject_irq(struct per_cpu *cpu_data, struct pending_irq *irq)
int i;
int first_free = -1;
u32 lr;
- u64 elsr;
+ unsigned long elsr[2];

- elsr = mmio_read32(gich_base + GICH_ELSR0);
- elsr |= (u64)mmio_read32(gich_base + GICH_ELSR1) << 32;
+ elsr[0] = mmio_read32(gich_base + GICH_ELSR0);
+ elsr[1] = mmio_read32(gich_base + GICH_ELSR1);
for (i = 0; i < gic_num_lr; i++) {
- if (test_bit(i, (unsigned long *)&elsr)) {
+ if (test_bit(i, elsr)) {
/* Entry is available */
if (first_free == -1)
first_free = i;
--
2.1.4

[Jan Kiszka]

We support up to 8 CPUs, not 9. Avoid future overflows by using the
actual size of target_cpu_map as limit.

This addresses Coverity finding CID 21114.

Adjust comment wordings at this chance as well.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

hypervisor/arch/arm/gic-common.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hypervisor/arch/arm/gic-common.c b/hypervisor/arch/arm/gic-common.c
index 6fc1d27..f9cb098 100644
--- a/hypervisor/arch/arm/gic-common.c
+++ b/hypervisor/arch/arm/gic-common.c
@@ -243,14 +243,14 @@ static int handle_sgir_access(struct per_cpu *cpu_data,
* Get the CPU interface ID for this cpu. It can be discovered by reading
* the banked value of the PPI and IPI TARGET registers
* Patch 2bb3135 in Linux explains why the probe may need to scans the first 8
- * registers: some early implementation returned 0 for the first TARGETS
- * registributor.
+ * registers: some early implementation returned 0 for the first ITARGETSR
+ * registers.
* Since those didn't have virtualization extensions, we can safely ignore that
* case.
*/
int gic_probe_cpu_id(unsigned int cpu)
{
- if (cpu > 8)
+ if (cpu >= ARRAY_SIZE(target_cpu_map))
return -EINVAL;

target_cpu_map[cpu] = mmio_read32(gicd_base + GICD_ITARGETSR);
--
2.1.4

[Jan Kiszka]

The cell configuration format restricts us to 64 SPIs this far. Make
sure that we properly test the range of 32 to 63 and avoid overflows due
to 32-bit word width. As Jailhouse provides no __aeabi_llsr, extract
high and low words first, then scan within 32 bits.

This addresses Coverity finding CID 21110.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

hypervisor/arch/arm/include/asm/irqchip.h | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hypervisor/arch/arm/include/asm/irqchip.h b/hypervisor/arch/arm/include/asm/irqchip.h
index 0f573fd..1f1ae54 100644
--- a/hypervisor/arch/arm/include/asm/irqchip.h
+++ b/hypervisor/arch/arm/include/asm/irqchip.h
@@ -105,14 +105,16 @@ int irqchip_set_pending(struct per_cpu *cpu_data, u32 irq_id, bool try_inject);
static inline bool spi_in_cell(struct cell *cell, unsigned int spi)
{
/* FIXME: Change the configuration to a bitmask range */
- u64 spi_mask;
+ u32 spi_mask;

- if (spi > 64)
+ if (spi >= 64)
return false;
+ else if (spi >= 32)
+ spi_mask = cell->arch.spis >> 32;
+ else
+ spi_mask = cell->arch.spis;

- spi_mask = cell->arch.spis;
-
- return spi_mask & (1 << spi);
+ return spi_mask & (1 << (spi & 31));
}

#endif /* __ASSEMBLY__ */
--
2.1.4

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

BTW, we received a new board, a Jetson TK1 by NVIDIA. As it has full
IOMMU support and runs well with upstream U-Boot and Linux, this will be
the next step for enhancing the ARM port. No promises on the time line,
though.

README.md | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/README.md b/README.md
index b1a5a1b..9b5ccd6 100644
--- a/README.md
+++ b/README.md
@@ -93,6 +93,7 @@ x86 architecture:
- at least 2 logical CPUs

- x86-64 Linux kernel (tested against >= 3.14)
+
- VT-d IOMMU usage (DMAR) has to be disabled in the Linux kernel, e.g. via
the command line parameter:

@@ -101,6 +102,25 @@ x86 architecture:
- To exploit the faster x2APIC, interrupt remapping needs to be on in the
kernel (check for CONFIG_IRQ_REMAP)

+ARM architecture:
+
+ - Abstract:
+
+ - ARMv7 with virtualization extensions
+
+ - Appropriate boot loader support (typically U-Boot)
+ - Linux is started in HYP mode
+ - PSCI support for CPU offlining
+
+ - at least 2 logical CPUs
+
+ - Board support:
+
+ - Banana Pi (see also [below](#setup-on-banana-pi-arm-board))
+
+ - ARM Versatile Express with Cortex-A15 or A7 cores
+ (includes ARM Fast Model)
+

Build & Installation
--------------------
--
2.1.4

[Henning Schild]

On Mon, 9 Feb 2015 08:19:33 +0100
Jan Kiszka <jan.k...@siemens.com> wrote:

> Now that the infrastructure is in place, I would suggest to include
> static analysis into your contribution process. Only as recommended
> step for contributors, but as required one before taking something
> that touches code into master. Comments welcome.

I agree that coverity is a useful tool and that we should be using it
on all our code. But it should really be optional for contributors. It
took you a while to figure out how to use it and we still have to see
how stable the current approach is. At the moment contributors do not
need to register an account anywhere, for coverity people would have to
register with github and/or coverity.

Henning