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: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> Sent: Monday, July 11, 2022 11:29 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; 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 Penny,
> 
> On 11/7/22 12:02, Penny Zheng wrote:
> > 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.
> 
> The imagebuilder configuration option DOMU_DIRECT_MAP defaults to 1.
> 
> But I could add DOMU_DIRECT_MAP[0]=1 in the script to make it clearer.
> 

Oh, sorry about that. Then everything is set clearly for me~~~

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