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

[Julien Grall <julien@xxxxxxx>]

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.
[...]

> > > +# 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.
> > > +
> > > +(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 ... ".
Cheers,

--
Julien Grall