[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 2/2] automation: arm64: Create a test job for testing static allocation on qemu
On Tue, 26 Jul 2022, Xenia Ragiadakou wrote: > Enable CONFIG_STATIC_MEMORY in the existing arm64 build. > > Create a new test job, called qemu-smoke-arm64-gcc-staticmem. > > Adjust qemu-smoke-arm64.sh script to accomodate the static memory test as a > new test variant. The test variant is determined based on the first argument > passed to the script. For testing static memory, the argument is 'static-mem'. > > The test configures DOM1 with a static memory region and adds a check in the > init script. > The check consists in comparing the contents of the /proc/device-tree > memory entry with the static memory range with which DOM1 was configured. > If the memory layout is correct, a message gets printed by DOM1. > > At the end of the qemu run, the script searches for the specific message > in the logs and fails if not found. > > Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx> This looks good, I only have a couple of minor comments about code style and a small suggestion for improvements. > --- > > Changes in v2: > - enable CONFIG_STATIC_MEMORY for all arm64 builds > - use the existing qemu-smoke-arm64.sh with an argument passed for > distinguishing between tests, instead of creating a new script > - test does not rely on kernel logs but prints a message itself on success > - remove the part that enables custom configs for arm64 builds > - add comments > - adapt commit message > > automation/gitlab-ci/test.yaml | 18 +++++++++++ > automation/scripts/build | 8 +++++ > automation/scripts/qemu-smoke-arm64.sh | 42 ++++++++++++++++++++++++-- > 3 files changed, 65 insertions(+), 3 deletions(-) > > diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml > index 26bdbcc3ea..6f9f64a8da 100644 > --- a/automation/gitlab-ci/test.yaml > +++ b/automation/gitlab-ci/test.yaml > @@ -80,6 +80,24 @@ qemu-smoke-arm64-gcc: > tags: > - arm64 > > +qemu-smoke-arm64-gcc-staticmem: > + extends: .test-jobs-common > + variables: > + CONTAINER: debian:unstable-arm64v8 > + script: > + - ./automation/scripts/qemu-smoke-arm64.sh static-mem 2>&1 | tee > qemu-smoke-arm64.log > + needs: > + - debian-unstable-gcc-arm64 > + - kernel-5.9.9-arm64-export > + - qemu-system-aarch64-6.0.0-arm64-export > + artifacts: > + paths: > + - smoke.serial > + - '*.log' > + when: always > + tags: > + - arm64 > + > qemu-smoke-arm32-gcc: > extends: .test-jobs-common > variables: > diff --git a/automation/scripts/build b/automation/scripts/build > index 21b3bc57c8..1941763315 100755 > --- a/automation/scripts/build > +++ b/automation/scripts/build > @@ -18,6 +18,14 @@ else > make -j$(nproc) -C xen defconfig > fi > > +if [[ "${XEN_TARGET_ARCH}" = "arm64" ]]; then > + echo " > +CONFIG_EXPERT=y > +CONFIG_UNSUPPORTED=y > +CONFIG_STATIC_MEMORY=y" > xen/.config > + make -j$(nproc) -C xen olddefconfig > +fi I think we should do this within the not "${RANDCONFIG}" == "y" case above to avoid calling make defconfig needlessly. > + > # Save the config file before building because build failure causes the > script > # to exit early -- bash is invoked with -e. > cp xen/.config xen-config > diff --git a/automation/scripts/qemu-smoke-arm64.sh > b/automation/scripts/qemu-smoke-arm64.sh > index 53086a5ac7..48513a7832 100755 > --- a/automation/scripts/qemu-smoke-arm64.sh > +++ b/automation/scripts/qemu-smoke-arm64.sh > @@ -2,6 +2,15 @@ > > set -ex > > +test_variant=$1 > + > +if [[ "${test_variant}" == "static-mem" ]]; then > + # Memory range that is statically allocated to DOM1 > + domu_base="50000000" > + domu_size="10000000" > + passed="static-mem test passed" > +fi I think we should set passed to "BusyBox" for the regular case so that we don't need an if/else at the bottom to check for passed > # Install QEMU > export DEBIAN_FRONTENT=noninteractive > apt-get -qy update > @@ -42,8 +51,22 @@ echo "#!/bin/sh > > mount -t proc proc /proc > mount -t sysfs sysfs /sys > -mount -t devtmpfs devtmpfs /dev > -/bin/sh" > initrd/init > +mount -t devtmpfs devtmpfs /dev" > initrd/init > + > +if [[ "${test_variant}" == "static-mem" ]]; then > + # Verify that DOM1 booted with the expected memory layout by checking the > + # contents of the memory entry in /proc/device-tree. > + echo " > +current=\$(hexdump -e '16/1 \"%02x\"' > /proc/device-tree/memory@${domu_base}/reg 2>/dev/null) > +expected=\"$(printf \"%016x%016x\" 0x${domu_base} 0x${domu_size})\" This works but it is hard to read. I am suggesting the follow to improve readability: --- echo "#!/bin/sh mount -t proc proc /proc mount -t sysfs sysfs /sys mount -t devtmpfs devtmpfs /dev current=\$(hexdump -e '16/1 \"%02x\"' /proc/device-tree/memory@${domu_base}/reg 2>/dev/null) expected=\"$(printf \"%016x%016x\" 0x${domu_base} 0x${domu_size})\" if [[ ${expected} == \"\${current}\" ]]; then echo \"${passed}\" fi " > initrd/init --- I know that it is going to be a couple of extra instructions on the non-static case to run inside QEMU but the advantage is that the init script is identical between the two cases which makes the code easier. > +if [[ ${expected} == \"\${current}\" ]]; then > + echo \"${passed}\" > +fi" >> initrd/init > +fi > + > +echo " > +/bin/sh" >> initrd/init Is this needed? > chmod +x initrd/init > cd initrd > find . | cpio --create --format='newc' | gzip > ../binaries/initrd > @@ -68,6 +91,12 @@ DOMU_MEM[0]="256" > LOAD_CMD="tftpb" > UBOOT_SOURCE="boot.source" > UBOOT_SCRIPT="boot.scr"' > binaries/config > + > +if [[ "${test_variant}" == "static-mem" ]]; then > + echo " > +DOMU_STATIC_MEM[0]=\"0x${domu_base} 0x${domu_size}\"" >> binaries/config please keep it on a single line for readability (even if it will push it above 80 chars): echo "DOMU_STATIC_MEM[0]=\"0x${domu_base} 0x${domu_size}\"" >> binaries/config > +fi > + > rm -rf imagebuilder > git clone https://gitlab.com/ViryaOS/imagebuilder > bash imagebuilder/scripts/uboot-script-gen -t tftp -d binaries/ -c > binaries/config > @@ -89,5 +118,12 @@ timeout -k 1 240 \ > -bios /usr/lib/u-boot/qemu_arm64/u-boot.bin |& tee smoke.serial > > set -e > -(grep -q "^BusyBox" smoke.serial && grep -q "DOM1: BusyBox" smoke.serial) || > exit 1 > +grep -q "^BusyBox" smoke.serial || exit 1 > + > +if [[ "${test_variant}" == "static-mem" ]]; then > + grep -q "DOM1: ${passed}" smoke.serial || exit 1 > +else > + grep -q "DOM1: BusyBox" smoke.serial || exit 1 > +fi No need for this if/else/fi if we set $passed in both cases at the top
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |