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

[Julien Grall <julien@xxxxxxx>]

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).
> +
> +(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.
> +
> +(grep -q "BusyBox" qemu-staticmem.serial) || exit 1
Cheers,

--
Julien Grall