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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Xenia

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Xenia Ragiadakou
> Sent: Friday, July 8, 2022 5:54 PM
> To: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Doug Goldstein <cardoe@xxxxxxxxxx>
> Subject: Re: [PATCH 2/2] automation: arm64: Create a test job for testing
> static allocation on qemu
> 
> Hi Stefano,
> 
> 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

Although you said booting a single direct mapped dom0less guest
configured with statically allocated memory here, later in code, you are
only enabling statically allocated memory in the ImageBuilder script,
missing the direct-map property.
 
> >>> - 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]}\"
> >>> +

You would like to add  DOMU_DIRECT_MAP[0] = 1 to enable direct-map.

> >>> +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?
> 

If only statically allocated memory is enabled, guest memory address will still 
be mapped
to GUEST_RAM_BASE(0x40000000)

> >>> +
> >>> +(grep -q "BusyBox" qemu-staticmem.serial) || exit 1
> >
> 
> --
> Xenia

---
Cheers,
Penny Zheng