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

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 7/8/22 10:46, Julien Grall wrote:
> On 08/07/2022 08:17, Xenia Ragiadakou wrote:
> > Hi Julien,
> Hi Xenia,
>
> > On 7/8/22 01:26, 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
> > IIUC, the xen binary (built with the arm64_defconfig) is used by the 
> > two other arm64 test jobs qemu-smoke-arm64-gcc and 
> > qemu-alpine-arm64-gcc. I did not want to change its configuration.
> > If there is no issue with changing its configuration, I can enable 
> > static memory and use this one. 
> I would expect a Xen built to CONFIG_STATIC_MEMORY to continue to work 
> in normal case. So it should be fine to enable by default.
> > But to be honest, I would like to be able also to test with custom 
> > configs.
> That's fine. But in this case...
>
> > > > 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)?
> > I did not want to change the existing behavior for x86.
>
> ... I don't think this should be restricted to arm. I would also 
> consider to do this change separately to avoid mixing infrastructure 
> change and new test.
Sure.

> [...]
>
> > > > +# 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.
> > It is not clear to me either :). I will remove them.
> Where was this command line copied from? If it is an Arm documentation 
> (or script), then they should be corrected.
Don't worry :) I was using them when debugging a script for parsing the 
xen cmdline and I dragged them around. Sorry about that.
> > > > +
> > > > +(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.
> > The kernel is taken from a test-artifact container, so, IIUC, it won't 
> > change.
> This statement is correct today. However, we may decide to update the 
> kernel or test multiple kernels (with different ouput).
> In the first case, it would be a matter of updating the script. This is 
> annoying but not too bad. In the second case, we would need to have "if 
> version X ... else if version Y ... ".
The particular test was relying and had a dependency on this kernel.
If the test is merged into the qemu-smoke-arm64.sh, the check above will 
leave and it will be tested whether the guest makes it to the busybox, 
based on the busybox logs, which also may change at any time.
--
Xenia