Re: [PATCH v1 1/4] CI: unify x86 XTF test runner

[dmkhn@proton.me]

On Wed, Apr 16, 2025 at 05:16:49PM -0700, Stefano Stabellini wrote:
> On Wed, 16 Apr 2025, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Add test runner script qemu-xtf.sh which is allows any XTF x86 test to be
> > easily executed. Test runner is invoked from the qemu-smoke* jobs with the
> > hardcoded parameters.
> >
> > Each x86 XTF job lead time is reduced a bit since only the test-related code
> > is built, not the entire XTF project.
> >
> > Add .gitignore to avoid committing test artifacts by mistake.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> >  automation/gitlab-ci/test.yaml                |   8 +-
> >  automation/scripts/.gitignore                 |   6 +
> >  .../scripts/include/configs/xtf-x86-64-config |   0
> >  automation/scripts/include/xtf-runner         | 134 ++++++++++++++++++
> >  automation/scripts/include/xtf-x86-64         |  31 ++++
> >  automation/scripts/qemu-smoke-x86-64.sh       |  26 ----
> >  automation/scripts/qemu-xtf.sh                |  26 ++++
> >  7 files changed, 201 insertions(+), 30 deletions(-)
> >  create mode 100644 automation/scripts/.gitignore
> >  create mode 100644 automation/scripts/include/configs/xtf-x86-64-config
> >  create mode 100644 automation/scripts/include/xtf-runner
> >  create mode 100644 automation/scripts/include/xtf-x86-64
> >  delete mode 100755 automation/scripts/qemu-smoke-x86-64.sh
> >  create mode 100755 automation/scripts/qemu-xtf.sh
> >
> > diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> > index 5ce445b78f..3adc841335 100644
> > --- a/automation/gitlab-ci/test.yaml
> > +++ b/automation/gitlab-ci/test.yaml
> > @@ -659,28 +659,28 @@ qemu-alpine-x86_64-gcc:
> >  qemu-smoke-x86-64-gcc:
> >    extends: .qemu-smoke-x86-64
> >    script:
> > -    - ./automation/scripts/qemu-smoke-x86-64.sh pv 2>&1 | tee ${LOGFILE}
> > +    - ./automation/scripts/qemu-xtf.sh x86-64 pv64 example 2>&1 | tee ${LOGFILE}
> >    needs:
> >      - debian-12-x86_64-gcc-debug
> >
> >  qemu-smoke-x86-64-clang:
> >    extends: .qemu-smoke-x86-64
> >    script:
> > -    - ./automation/scripts/qemu-smoke-x86-64.sh pv 2>&1 | tee ${LOGFILE}
> > +    - ./automation/scripts/qemu-xtf.sh x86-64 pv64 example 2>&1 | tee ${LOGFILE}
> >    needs:
> >      - debian-12-x86_64-clang-debug
> >
> >  qemu-smoke-x86-64-gcc-pvh:
> >    extends: .qemu-smoke-x86-64
> >    script:
> > -    - ./automation/scripts/qemu-smoke-x86-64.sh pvh 2>&1 | tee ${LOGFILE}
> > +    - ./automation/scripts/qemu-xtf.sh x86-64 hvm64 example 2>&1 | tee ${LOGFILE}
> >    needs:
> >      - debian-12-x86_64-gcc-debug
> >
> >  qemu-smoke-x86-64-clang-pvh:
> >    extends: .qemu-smoke-x86-64
> >    script:
> > -    - ./automation/scripts/qemu-smoke-x86-64.sh pvh 2>&1 | tee ${LOGFILE}
> > +    - ./automation/scripts/qemu-xtf.sh x86-64 hvm64 example 2>&1 | tee ${LOGFILE}
> >    needs:
> >      - debian-12-x86_64-clang-debug
> >
> > diff --git a/automation/scripts/.gitignore b/automation/scripts/.gitignore
> > new file mode 100644
> > index 0000000000..2f2d6e1ebd
> > --- /dev/null
> > +++ b/automation/scripts/.gitignore
> > @@ -0,0 +1,6 @@
> > +!include
> > +
> > +binaries
> > +smoke.serial
> > +xen
> > +xtf*/
> 
> I am not sure this works as intended: I thought the paths are relative
> to the location of the .gitignore file, but I could be wrong.

git allows .gitignore at any level in the source tree so that lenghty
paths could be avoided.

> 
> 
> > diff --git a/automation/scripts/include/configs/xtf-x86-64-config b/automation/scripts/include/configs/xtf-x86-64-config
> > new file mode 100644
> > index 0000000000..e69de29bb2
> > diff --git a/automation/scripts/include/xtf-runner b/automation/scripts/include/xtf-runner
> > new file mode 100644
> > index 0000000000..55b7b34b89
> > --- /dev/null
> > +++ b/automation/scripts/include/xtf-runner
> > @@ -0,0 +1,134 @@
> > +#!/bin/bash
> > +#
> > +# XTF test utilities.
> > +#
> > +# Environment variables:
> > +#   BOOT_MSG: Expected boot message
> > +#   FW_PREFIX: Firmware images path including '/' at the end
> > +#   PASSED: XTF test printout in case of a pass
> > +#   QEMU_PREFIX: QEMU path including '/' at the end
> > +#   TEST_LOG: Output log file
> > +#   UBOOT_CMD: U-Boot command line
> > +#   WORKDIR: Test working directory
> > +#   XEN_BINARY: Xen binary location
> > +#   XEN_CONSOLE: Xen console device name
> > +#   XTF_SRC_CONFIG: XTF config file
> > +#   XTF_SRC_BRANCH: XTF branch
> > +#   XTF_SRC_URI: XTF source code URI
> > +
> > +function die()
> > +{
> > +    set +x
> > +    echo "FATAL: $*" >&2
> > +    exit 1
> > +}
> > +
> > +[ -z "$TOP" ] && die "\$TOP is not set"
> > +
> > +# Output log file
> > +TEST_LOG="${TEST_LOG:-smoke.serial}"
> > +# XTF test printout in case of a pass
> > +PASSED="${PASSED:-Test result: SUCCESS}"
> > +# Expected boot message
> > +BOOT_MSG="${BOOT_MSG:-Latest ChangeSet: }"
> > +# Test working directory
> > +WORKDIR="${WORKDIR:-binaries}"
> > +# XTF source code
> > +XTF_SRC_CONFIG="${XTF_CONFIG:-${TOP}/include/configs/xtf-${ARCH}-config}"
> 
> Should this be  XTF_SRC_CONFIG="${XTF_SRC_CONFIG:-  ?

Thanks for the catch!

> 
> 
> > +# Build an XTF test binary.
> > +# $1 Test variant.
> > +# $2 Test name.
> > +function xtf_build_binary()
> > +{
> > +    local xtf_variant=$1
> > +    local xtf_name=$2
> > +    local xtf_dir="xtf-${ARCH}"
> > +
> > +    # Crude check for local testing
> > +    if [ ! -d ${xtf_dir}/.git ]; then
> > +        git clone ${XTF_SRC_URI} ${xtf_dir} -b ${XTF_SRC_BRANCH}
> > +    fi
> > +
> > +    make \
> > +        -C ${xtf_dir} \
> > +        -j$(nproc) \
> > +        $(tr '\n' ' ' < ${XTF_SRC_CONFIG}) \
> > +        TESTS=tests/${xtf_name}
> > +
> > +    export XTF_NAME="${xtf_name}"
> > +    export XTF_VARIANT="${xtf_variant}"
> > +    export XTF_WORKDIR="$(readlink -f ${xtf_dir})"
> > +    export XTF_BINARY="${XTF_WORKDIR}/tests/${xtf_name}/test-${xtf_variant}-${xtf_name}"
> > +}
> > +
> > +# Build Xen command line for running an XTF test.
> > +# $1 Test variant.
> > +# $2 Test name.
> > +function xtf_build_cmdline()
> > +{
> > +    local xtf_variant=$1
> > +    local xtf_name=$2
> > +    declare -a cmdline=()
> > +
> > +    cmdline+=("loglvl=all noreboot console_timestamps=boot")
> > +    cmdline+=("console=${XEN_CONSOLE}")
> > +
> > +    # NB: OK to have hvm64, which is x86-only variant
> > +    if [[ $xtf_variant == "hvm64" ]]; then
> > +        cmdline+=("dom0-iommu=none dom0=pvh")
> > +    fi
> > +
> > +    export XEN_CMDLINE="${cmdline[@]}"
> > +}
> > +
> > +# Build an XTF test environment.
> > +# $1 Test variant.
> > +# $2 Test name.
> > +function xtf_build_test()
> > +{
> > +    local v=$1
> > +    local xtf_name=$2
> > +    local xtf_variant=""
> > +
> > +    for x in ${XTF_SRC_VARIANTS}; do
> > +        if [[ "${x}" == "${v}" ]]; then
> > +            xtf_variant=${v}
> > +            break
> > +        fi
> > +    done
> > +    if [[ -z $xtf_variant ]]; then
> > +        die "unsupported test variant '$1', supported variants: ${XTF_SRC_VARIANTS}"
> > +    fi
> > +
> > +    xtf_build_binary ${xtf_variant} ${xtf_name}
> > +    xtf_build_cmdline ${xtf_variant} ${xtf_name}
> > +}
> > +
> > +# Execute an XTF test.
> > +function xtf_run_test()
> > +{
> > +    rm -f ${TEST_LOG}
> > +    export BOOT_MSG PASSED TEST_CMD TEST_LOG UBOOT_CMD
> > +    ${TOP}/console.exp | sed 's/\r\+$//'
> > +}
> > +
> > +# Setup environment and run an XTF test.
> > +# $1 Test variant.
> > +# $2 Test name.
> > +function xtf_test()
> > +{
> > +    # Out: FW_*, QEMU_*, XEN_{BINARY,CONSOLE}, XTF_SRC_*
> > +    xtf_arch_prepare
> > +
> > +    # In: XTF_SRC_*
> > +    # OUt: XTF_{BINARY,NAME,VARIANT,WORKDIR} and XEN_CMDLINE
> > +    xtf_build_test $@
> > +
> > +    # In: FW_*, QEMU_*, XTF_*, XEN_*
> > +    # Out: BOOT_MSG, PASSED, TEST_{CMD,LOG}, UBOOT_CMD
> > +    xtf_arch_setup
> > +
> > +    # In: BOOT_MSG, PASSED, TEST_{CMD,LOG}, UBOOT_CMD
> > +    xtf_run_test
> > +}
> > diff --git a/automation/scripts/include/xtf-x86-64 b/automation/scripts/include/xtf-x86-64
> > new file mode 100644
> > index 0000000000..edddf18b38
> > --- /dev/null
> > +++ b/automation/scripts/include/xtf-x86-64
> > @@ -0,0 +1,31 @@
> > +#!/bin/bash
> > +#
> > +# XTF test utilities (x86_64).
> > +#
> > +
> > +# Arch-specific environment overrides.
> > +function xtf_arch_prepare()
> > +{
> > +    export FW_PREFIX="${FW_PREFIX:-}"
> > +    export QEMU_PREFIX="${QEMU_PREFIX:-}"
> 
> It looks like QEMU_PREFIX is only used within this file (and other
> similar files in later patches). If so, maybe for the sake of reducing
> global variables and variables in general we could get rid of
> QEMU_PREFIX.

Turned out that Arm and x86 CI jobs use different paths to QEMU binaries.
dev host may have some other location, that's why I parameterized QEMU
location.

> 
> 
> > +    export XEN_BINARY="${XEN_BINARY:-${WORKDIR}/xen}"
> > +    export XEN_CONSOLE="${XEN_CONSOLE:-com1}"
> 
> Instead of XEN_CONSOLE, I'd suggest to have an arch-specific variable
> for Xen command line options where we can put the console as well as
> other things. However, it is also totally fine as is.

I initially did that, but then there's a big portion of command line parameters
which is shared across all supported architectures, with console= pointing to
arch-specific console device.

I will add arch-specific variable and drop XEN_CONSOLE.

> 
> In fact, I think you went above and beyond in terms of generic
> abstractions, and I think it would have been OK to go with fewer
> variables, and more ad-hoc custom code.
> 
> 
> > +    export XTF_SRC_BRANCH="${XTF_SRC_BRANCH:-master}"
> > +    export XTF_SRC_URI="${XTF_SRC_URI:-https://xenbits.xen.org/git-http/xtf.git}"
> > +    export XTF_SRC_VARIANTS="hvm64 pv64"
> > +}
> > +
> > +# Perform arch-specific XTF environment setup.
> > +function xtf_arch_setup()
> > +{
> > +    export TEST_CMD="${QEMU_PREFIX}qemu-system-x86_64 \
> > +        -no-reboot \
> > +        -nographic \
> > +        -monitor none \
> > +        -serial stdio \
> > +        -m 512 \
> > +        -kernel ${XEN_BINARY} \
> > +        -initrd ${XTF_BINARY} \
> > +        -append \"${XEN_CMDLINE}\" \
> > +    "
> > +}
> > diff --git a/automation/scripts/qemu-smoke-x86-64.sh b/automation/scripts/qemu-smoke-x86-64.sh
> > deleted file mode 100755
> > index da0c26cc2f..0000000000
> > --- a/automation/scripts/qemu-smoke-x86-64.sh
> > +++ /dev/null
> > @@ -1,26 +0,0 @@
> > -#!/bin/bash
> > -
> > -set -ex -o pipefail
> > -
> > -# variant should be either pv or pvh
> > -variant=$1
> > -
> > -# Clone and build XTF
> > -git clone https://xenbits.xen.org/git-http/xtf.git
> > -cd xtf && make -j$(nproc) && cd -
> > -
> > -case $variant in
> > -    pvh) k=test-hvm64-example    extra="dom0-iommu=none dom0=pvh" ;;
> > -    *)   k=test-pv64-example     extra= ;;
> > -esac
> > -
> > -rm -f smoke.serial
> > -export TEST_CMD="qemu-system-x86_64 -nographic -kernel binaries/xen \
> > -        -initrd xtf/tests/example/$k \
> > -        -append \"loglvl=all console=com1 noreboot console_timestamps=boot $extra\" \
> > -        -m 512 -monitor none -serial stdio"
> > -
> > -export TEST_LOG="smoke.serial"
> > -export PASSED="Test result: SUCCESS"
> > -
> > -./automation/scripts/console.exp | sed 's/\r\+$//'
> > diff --git a/automation/scripts/qemu-xtf.sh b/automation/scripts/qemu-xtf.sh
> > new file mode 100755
> > index 0000000000..2e16d4aece
> > --- /dev/null
> > +++ b/automation/scripts/qemu-xtf.sh
> > @@ -0,0 +1,26 @@
> > +#!/bin/bash
> > +#
> > +# XTF test runner (QEMU).
> > +#
> > +
> > +set -e -o pipefail
> > +
> > +if [ $# -lt 3 ]; then
> > +    echo "Usage: $(basename $0) ARCH XTF-VARIANT XTF-NAME"
> > +    exit 1
> > +fi
> > +
> > +export ARCH="$1"
> > +shift
> > +
> > +export TOP="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)"
> > +if [ ! -f "${TOP}/include/xtf-${ARCH}" ]; then
> > +    echo "unsupported architecture '${ARCH}'" >&2
> > +    exit 1
> > +fi
> 
> This seems a bit complex.. maybe we can assume that we are in xen.git/ ?

Will do that.


> 
> 
> > +set -x
> > +source ${TOP}/include/xtf-runner
> > +source ${TOP}/include/xtf-${ARCH}
> 
> So here we could do:
> 
> source automation/scripts/include/xtf-runner
> 
> ? Or is it important to be able to detect and calculate the directory?
> If so, why not use TOP=$(pwd) or TOP=${PWD} or TOP=$(dirname "$0")?
> By the way, if we need a variable for the top directory I would call it
> XEN_ROOT instead of TOP.
> 
> I see the patches are using both TOP and WORKDIR, maybe we can get rid
> of one of them? This is only a matter of taste but I think it would be
> simpler with fewer variables and most of  the others look unavoidable.

I will try to eliminate TOP uses.

> 
> Everything else looks OK to me.
> 
> 
> > +xtf_test $@
> > --
> > 2.34.1
> >
> >