Re: [PATCH 2/2] automation: arm64: Create a test job for testing static allocation on qemu

[Stefano Stabellini <stefano.stabellini@xxxxxxx>]

On Fri, 8 Jul 2022, Xenia Ragiadakou wrote:
> On 7/8/22 02:05, Stefano Stabellini wrote:
> > On Thu, 7 Jul 2022, Julien Grall wrote:
> > > Hi Xenia,
> > > 
> > > On 07/07/2022 21:38, Xenia Ragiadakou wrote:
> > > > Add an arm subdirectory under automation/configs for the arm specific
> > > > configs
> > > > and add a config that enables static allocation.
> > > > 
> > > > Modify the build script to search for configs also in this subdirectory
> > > > and
> > > > to
> > > > keep the generated xen binary, suffixed with the config file name, as
> > > > artifact.
> > > > 
> > > > Create a test job that
> > > > - boots xen on qemu with a single direct mapped dom0less guest
> > > > configured
> > > > with
> > > > statically allocated memory
> > > > - verifies that the memory ranges reported in the guest's logs are the
> > > > same
> > > > with the provided static memory regions
> > > > 
> > > > For guest kernel, use the 5.9.9 kernel from the tests-artifacts
> > > > containers.
> > > > Use busybox-static package, to create the guest ramdisk.
> > > > To generate the u-boot script, use ImageBuilder.
> > > > Use the qemu from the tests-artifacts containers.
> > > > 
> > > > Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> > > > ---
> > > >    automation/configs/arm/static_mem          |   3 +
> > > >    automation/gitlab-ci/test.yaml             |  24 +++++
> > > >    automation/scripts/build                   |   4 +
> > > >    automation/scripts/qemu-staticmem-arm64.sh | 114
> > > > +++++++++++++++++++++
> > > >    4 files changed, 145 insertions(+)
> > > >    create mode 100644 automation/configs/arm/static_mem
> > > >    create mode 100755 automation/scripts/qemu-staticmem-arm64.sh
> > > > 
> > > > diff --git a/automation/configs/arm/static_mem
> > > > b/automation/configs/arm/static_mem
> > > > new file mode 100644
> > > > index 0000000000..84675ddf4e
> > > > --- /dev/null
> > > > +++ b/automation/configs/arm/static_mem
> > > > @@ -0,0 +1,3 @@
> > > > +CONFIG_EXPERT=y
> > > > +CONFIG_UNSUPPORTED=y
> > > > +CONFIG_STATIC_MEMORY=y
> > > > \ No newline at end of file
> > > 
> > > Any particular reason to build a new Xen rather enable
> > > CONFIG_STATIC_MEMORY in
> > > the existing build?
> > > 
> > > > diff --git a/automation/scripts/build b/automation/scripts/build
> > > > index 21b3bc57c8..9c6196d9bd 100755
> > > > --- a/automation/scripts/build
> > > > +++ b/automation/scripts/build
> > > > @@ -83,6 +83,7 @@ fi
> > > >    # Build all the configs we care about
> > > >    case ${XEN_TARGET_ARCH} in
> > > >        x86_64) arch=x86 ;;
> > > > +    arm64) arch=arm ;;
> > > >        *) exit 0 ;;
> > > >    esac
> > > >    @@ -93,4 +94,7 @@ for cfg in `ls ${cfg_dir}`; do
> > > >        rm -f xen/.config
> > > >        make -C xen KBUILD_DEFCONFIG=../../../../${cfg_dir}/${cfg}
> > > > defconfig
> > > >        make -j$(nproc) -C xen
> > > > +    if [[ ${arch} == "arm" ]]; then
> > > > +        cp xen/xen binaries/xen-${cfg}
> > > > +    fi
> > > 
> > > This feels a bit of a hack to be arm only. Can you explain why this is not
> > > enabled for x86 (other than this is not yet used)?
> > > 
> > > >    done
> > > > diff --git a/automation/scripts/qemu-staticmem-arm64.sh
> > > > b/automation/scripts/qemu-staticmem-arm64.sh
> > > > new file mode 100755
> > > > index 0000000000..5b89a151aa
> > > > --- /dev/null
> > > > +++ b/automation/scripts/qemu-staticmem-arm64.sh
> > > > @@ -0,0 +1,114 @@
> > > > +#!/bin/bash
> > > > +
> > > > +base=(0x50000000 0x100000000)
> > > > +size=(0x10000000 0x10000000)
> > > 
> > >  From the name, it is not clear what the base and size refers too. Looking
> > > a
> > > bit below, it seems to be referring to the domain memory. If so, I would
> > > suggest to comment and rename to "domu_{base, size}".
> > > 
> > > > +
> > > > +set -ex
> > > > +
> > > > +apt-get -qy update
> > > > +apt-get -qy install --no-install-recommends u-boot-qemu \
> > > > +                                            u-boot-tools \
> > > > +                                            device-tree-compiler \
> > > > +                                            cpio \
> > > > +                                            curl \
> > > > +                                            busybox-static
> > > > +
> > > > +# DomU Busybox
> > > > +cd binaries
> > > > +mkdir -p initrd
> > > > +mkdir -p initrd/bin
> > > > +mkdir -p initrd/sbin
> > > > +mkdir -p initrd/etc
> > > > +mkdir -p initrd/dev
> > > > +mkdir -p initrd/proc
> > > > +mkdir -p initrd/sys
> > > > +mkdir -p initrd/lib
> > > > +mkdir -p initrd/var
> > > > +mkdir -p initrd/mnt
> > > > +cp /bin/busybox initrd/bin/busybox
> > > > +initrd/bin/busybox --install initrd/bin
> > > > +echo "#!/bin/sh
> > > > +
> > > > +mount -t proc proc /proc
> > > > +mount -t sysfs sysfs /sys
> > > > +mount -t devtmpfs devtmpfs /dev
> > > > +/bin/sh" > initrd/init
> > > > +chmod +x initrd/init
> > > > +cd initrd
> > > > +find . | cpio --create --format='newc' | gzip > ../initrd.cpio.gz
> > > > +cd ../..
> > > > +
> > > > +# XXX QEMU looks for "efi-virtio.rom" even if it is unneeded
> > > > +curl -fsSLO
> > > > https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
> > > > +
> > > > +./binaries/qemu-system-aarch64 -nographic \
> > > > +    -M virtualization=true \
> > > > +    -M virt \
> > > > +    -M virt,gic-version=2 \
> > > > +    -cpu cortex-a57 \
> > > > +    -smp 2 \
> > > > +    -m 8G \
> > > > +    -M dumpdtb=binaries/virt-gicv2.dtb
> > > > +
> > > > +#dtc -I dtb -O dts binaries/virt-gicv2.dtb > binaries/virt-gicv2.dts
> > > > +
> > > > +# ImageBuilder
> > > > +rm -rf imagebuilder
> > > > +git clone https://gitlab.com/ViryaOS/imagebuilder
> > > > +
> > > > +echo "MEMORY_START=\"0x40000000\"
> > > > +MEMORY_END=\"0x0200000000\"
> > > > +
> > > > +DEVICE_TREE=\"virt-gicv2.dtb\"
> > > > +
> > > > +XEN=\"xen-static_mem\"
> > > > +XEN_CMD=\"console=dtuart earlyprintk xsm=dummy\"
> > > 
> > > AFAIK, earlyprintk is not an option for Xen on Arm (at least). It is also
> > > not
> > > clear why you need to pass xsm=dummy.
> > > 
> > > > +
> > > > +NUM_DOMUS=1
> > > > +DOMU_MEM[0]=512
> > > > +DOMU_VCPUS[0]=1
> > > > +DOMU_KERNEL[0]=\"Image\"
> > > > +DOMU_RAMDISK[0]=\"initrd.cpio.gz\"
> > > > +DOMU_CMD[0]=\"earlyprintk console=ttyAMA0\"
> > > > +DOMU_STATIC_MEM[0]=\"${base[0]} ${size[0]} ${base[1]} ${size[1]}\"
> > > > +
> > > > +UBOOT_SOURCE=\"boot.source\"
> > > > +UBOOT_SCRIPT=\"boot.scr\"" > binaries/imagebuilder_config
> > > > +
> > > > +bash imagebuilder/scripts/uboot-script-gen -t tftp -d binaries/ -c
> > > > binaries/imagebuilder_config
> > > > +
> > > > +# Run the test
> > > > +rm -f qemu-staticmem.serial
> > > > +set +e
> > > > +echo "  virtio scan; dhcp; tftpb 0x40000000 boot.scr; source
> > > > 0x40000000"| \
> > > > +timeout -k 1 60 ./binaries/qemu-system-aarch64 -nographic \
> > > > +    -M virtualization=true \
> > > > +    -M virt \
> > > > +    -M virt,gic-version=2 \
> > > > +    -cpu cortex-a57 \
> > > > +    -smp 2 \
> > > > +    -m 8G \
> > > > +    -no-reboot \
> > > > +    -device virtio-net-pci,netdev=vnet0 -netdev
> > > > user,id=vnet0,tftp=binaries
> > > > \
> > > > +    -bios /usr/lib/u-boot/qemu_arm64/u-boot.bin \
> > > > +    -dtb ./binaries/virt-gicv2.dtb \
> > > > +    |& tee qemu-staticmem.serial
> > > > +
> > > > +set -e
> > > 
> > > A lot of the code above is duplicated from qemu-smoke-arm64.sh. I think it
> > > would be better to consolidate in a single script. Looking briefly
> > > throught
> > > the existing scripts, it looks like it is possible to pass arguments (see
> > > qemu-smoke-x86-64.sh).
> >   One idea would be to make the script common and "source" a second
> > script or config file with just the ImageBuilder configuration because
> > it looks like it is the only thing different.
> > 
> > 
> > > > +
> > > > +(grep -q "Xen dom0less mode detected" qemu-staticmem.serial) || exit 1
> > > > +
> > > > +for ((i=0; i<${#base[@]}; i++))
> > > > +do
> > > > +    start="$(printf "0x%016x" ${base[$i]})"
> > > > +    end="$(printf "0x%016x" $((${base[$i]} + ${size[$i]} - 1)))"
> > > > +    grep -q "node   0: \[mem ${start}-${end}\]" qemu-staticmem.serial
> > > > +    if test $? -eq 1
> > > > +    then
> > > > +        exit 1
> > > > +    fi
> > > > +done
> > > 
> > > Please add a comment on top to explain what this is meant to do. However,
> > > I
> > > think we should avoid relying on output that we have written ourself. IOW,
> > > relying on Xen/Linux to always write the same message is risky because
> > > they
> > > can change at any time.
> > 
> > Especially if we make the script common, then we could just rely on the
> > existing check to see if the guest started correctly (no special check
> > for static memory).
> 
> In this case, how the test will verify that the static memory configuration
> has been taken into account and has not just been ignored?

There is no simple way that I can think of.

We can trust that it worked, without checking that the ranges were
actually static as requested.

We can parse the Xen output like you did, although if it changes then
the test will break.

Or we could add a script to detect and print specific output but I
don't know if there is something under /proc or /sys that we could
simply "cat" from bash to check it.

If there is a simple way to do this by cat'ing /proc or /sys then I
think that would be great. Otherwise I think we could do as you did in
this patch, which is not perfect but it works and if something changes
in the Xen output we'll detect it right away given that gitlab-ci is run
pre-commit.