[for-4.17 0/3] GitLab CI cleanup and boot time cpupools test

[for-4.17 0/3] GitLab CI cleanup and boot time cpupools test

[Michal Orzel]

This patch series performs a small cleanup before the release and adds
a test for validating boot time cpupools feature introduced in 4.17.

Notes for the release manager:
Benefits:
 - improved dom0less test coverage
 - tested feature that is introduced in 4.17
Risks:
 - CI pipeline failure

Michal Orzel (3):
  automation: qemu-alpine-arm64-gcc: Use kernel 5.19
  automation: qemu-smoke-arm64: Silence ifconfig error messages
  automation: Add a new job for testing boot time cpupools on arm64

 automation/gitlab-ci/build.yaml               | 11 ------
 automation/gitlab-ci/test.yaml                | 21 ++++++++++-
 automation/scripts/build                      |  3 +-
 automation/scripts/qemu-smoke-arm64.sh        | 35 +++++++++++++------
 .../kernel/5.9.9-arm64v8.dockerfile           | 34 ------------------
 5 files changed, 47 insertions(+), 57 deletions(-)
 delete mode 100644 automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile

-- 
2.25.1

[for-4.17 1/3] automation: qemu-alpine-arm64-gcc: Use kernel 5.19

[Michal Orzel]

After qemu-smoke-arm64 was changed to use kernel 5.19 we end up having
two kernel configurations. This is something not needed and maintaining
a single kernel version is always easier. Modify qemu-alpine-arm64-gcc
to use kernel 5.19 and remove kernel 5.9 from tests-artifacts.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 automation/gitlab-ci/build.yaml               | 11 ------
 automation/gitlab-ci/test.yaml                |  2 +-
 .../kernel/5.9.9-arm64v8.dockerfile           | 34 -------------------
 3 files changed, 1 insertion(+), 46 deletions(-)
 delete mode 100644 automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index d2f75a090c0f..720ce6e07ba0 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -586,17 +586,6 @@ alpine-3.12-arm64-rootfs-export:
   tags:
     - arm64
 
-kernel-5.9.9-arm64-export:
-  extends: .test-jobs-artifact-common
-  image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:5.9.9-arm64v8
-  script:
-    - mkdir binaries && cp /Image binaries/Image
-  artifacts:
-    paths:
-      - binaries/Image
-  tags:
-    - arm64
-
 kernel-5.19-arm64-export:
   extends: .test-jobs-artifact-common
   image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:5.19-arm64v8
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 2eb6c3866e2c..07209820b474 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -34,7 +34,7 @@ qemu-alpine-arm64-gcc:
   needs:
     - alpine-3.12-gcc-arm64
     - alpine-3.12-arm64-rootfs-export
-    - kernel-5.9.9-arm64-export
+    - kernel-5.19-arm64-export
     - qemu-system-aarch64-6.0.0-arm64-export
   artifacts:
     paths:
diff --git a/automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile b/automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile
deleted file mode 100644
index 053d65a3454e..000000000000
--- a/automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile
+++ /dev/null
@@ -1,34 +0,0 @@
-FROM arm64v8/debian:unstable
-LABEL maintainer.name="The Xen Project" \
-      maintainer.email="xen-devel@lists.xenproject.org"
-
-ENV DEBIAN_FRONTEND=noninteractive
-ENV LINUX_VERSION=5.9.9
-ENV USER root
-
-RUN mkdir /build
-WORKDIR /build
-
-# build depends
-RUN apt-get update && \
-    apt-get --quiet --yes install \
-        build-essential \
-        libssl-dev \
-        bc \
-        curl \
-        flex \
-        bison \
-        && \
-    \
-    # Build the kernel
-    curl -fsSLO https://cdn.kernel.org/pub/linux/kernel/v5.x/linux-"$LINUX_VERSION".tar.xz && \
-    tar xvJf linux-"$LINUX_VERSION".tar.xz && \
-    cd linux-"$LINUX_VERSION" && \
-    make defconfig && \
-    make -j$(nproc) Image.gz && \
-    cp arch/arm64/boot/Image / && \
-    cd /build && \
-    rm -rf linux-"$LINUX_VERSION"* && \
-    apt-get autoremove -y && \
-    apt-get clean && \
-    rm -rf /var/lib/apt/lists* /tmp/* /var/tmp/*
-- 
2.25.1

[for-4.17 2/3] automation: qemu-smoke-arm64: Silence ifconfig error messages

[Michal Orzel]

During the ping test, dom1 tries to assign an ip to eth0 in a loop.
Before setting up the network interface by dom0, this results in
printing the following error message several times:
(XEN) DOM1: ifconfig: SIOCSIFADDR: No such device

Silence this by redirecting stderr/stdout to /dev/null as we do not
care about the output and we should not pollute the log file.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 automation/scripts/qemu-smoke-arm64.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/scripts/qemu-smoke-arm64.sh b/automation/scripts/qemu-smoke-arm64.sh
index c80d9b2aee00..7ac96027760d 100755
--- a/automation/scripts/qemu-smoke-arm64.sh
+++ b/automation/scripts/qemu-smoke-arm64.sh
@@ -6,7 +6,7 @@ test_variant=$1
 
 passed="passed"
 check="
-until ifconfig eth0 192.168.0.2 && ping -c 10 192.168.0.1; do
+until ifconfig eth0 192.168.0.2 &> /dev/null && ping -c 10 192.168.0.1; do
     sleep 30
 done
 echo \"${passed}\"
-- 
2.25.1

[for-4.17 3/3] automation: Add a new job for testing boot time cpupools on arm64

[Michal Orzel]

Add a new test job qemu-smoke-arm64-gcc-boot-cpupools that will execute
script qemu-smoke-arm64.sh to test boot time cpupools feature.
Enable CONFIG_BOOT_TIME_CPUPOOLS for the arm64 build and add a new test
case in qemu-smoke-arm64.sh that if selected will:
 - create a device tree cpupool node with cpu@1
 - assign created cpupool to domU0
 - add a check in dom0 xen.start to see if domU is assigned a Pool-1

Take the opportunity to refactor the qemu-smoke-arm64.sh script as
follows:
 - use domU_check to store the test's commands to be run from domU
 - use dom0_check to store the test's commands to be run from dom0
 - use fdtput instead of sed to perform dtb modifications
 - use more meaningful messages for "passed" variable. This way we can
   grep for messages reported either by domU or dom0 and get rid of
   assumption that tests can only be run from domU

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 automation/gitlab-ci/test.yaml         | 19 +++++++++++++++
 automation/scripts/build               |  3 ++-
 automation/scripts/qemu-smoke-arm64.sh | 33 +++++++++++++++++++-------
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 07209820b474..d899b3bdbf7a 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -100,6 +100,25 @@ qemu-smoke-arm64-gcc-staticmem:
   tags:
     - arm64
 
+qemu-smoke-arm64-gcc-boot-cpupools:
+  extends: .test-jobs-common
+  variables:
+    CONTAINER: debian:unstable-arm64v8
+  script:
+    - ./automation/scripts/qemu-smoke-arm64.sh boot-cpupools 2>&1 | tee qemu-smoke-arm64.log
+  needs:
+    - alpine-3.12-gcc-arm64
+    - alpine-3.12-arm64-rootfs-export
+    - kernel-5.19-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 2b9f2d2b541a..2f15ab3198e6 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -19,7 +19,8 @@ else
         echo "
 CONFIG_EXPERT=y
 CONFIG_UNSUPPORTED=y
-CONFIG_STATIC_MEMORY=y" > xen/.config
+CONFIG_STATIC_MEMORY=y
+CONFIG_BOOT_TIME_CPUPOOLS=y" > xen/.config
         make -j$(nproc) -C xen olddefconfig
     else
         make -j$(nproc) -C xen defconfig
diff --git a/automation/scripts/qemu-smoke-arm64.sh b/automation/scripts/qemu-smoke-arm64.sh
index 7ac96027760d..c2184850293c 100755
--- a/automation/scripts/qemu-smoke-arm64.sh
+++ b/automation/scripts/qemu-smoke-arm64.sh
@@ -4,20 +4,22 @@ set -ex
 
 test_variant=$1
 
-passed="passed"
-check="
+if [ -z "${test_variant}" ]; then
+    passed="ping test passed"
+    domU_check="
 until ifconfig eth0 192.168.0.2 &> /dev/null && ping -c 10 192.168.0.1; do
     sleep 30
 done
 echo \"${passed}\"
 "
+fi
 
 if [[ "${test_variant}" == "static-mem" ]]; then
     # Memory range that is statically allocated to DOM1
     domu_base="50000000"
     domu_size="10000000"
     passed="${test_variant} test passed"
-    check="
+    domU_check="
 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
@@ -42,11 +44,23 @@ curl -fsSLO https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
    -cpu cortex-a57 -machine type=virt \
    -m 1024 -smp 2 -display none \
    -machine dumpdtb=binaries/virt-gicv2.dtb
-# XXX disable pl061 to avoid Linux crash
-dtc -I dtb -O dts binaries/virt-gicv2.dtb > binaries/virt-gicv2.dts
-sed 's/compatible = "arm,pl061.*/status = "disabled";/g' binaries/virt-gicv2.dts > binaries/virt-gicv2-edited.dts
-dtc -I dts -O dtb binaries/virt-gicv2-edited.dts > binaries/virt-gicv2.dtb
 
+# XXX disable pl061 to avoid Linux crash
+fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
+
+if [[ "${test_variant}" == "boot-cpupools" ]]; then
+    # Create cpupool node and assign it to domU0
+    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
+    cpupool_phandle="0xffffff"
+    fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible xen,cpupool
+    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus $cpu_phandle
+    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle $cpupool_phandle
+    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/domU0 domain-cpupool $cpupool_phandle
+
+    # Check if domU0 (id=1) is assigned to Pool-1
+    passed="${test_variant} test passed"
+    dom0_check="if xl list -c 1 | grep -q Pool-1; then echo ${passed}; fi"
+fi
 
 # Busybox
 mkdir -p initrd
@@ -66,7 +80,7 @@ echo "#!/bin/sh
 mount -t proc proc /proc
 mount -t sysfs sysfs /sys
 mount -t devtmpfs devtmpfs /dev
-${check}
+${domU_check}
 /bin/sh" > initrd/init
 chmod +x initrd/init
 cd initrd
@@ -98,6 +112,7 @@ ifconfig xenbr0 up
 ifconfig xenbr0 192.168.0.1
 
 xl network-attach 1 type=vif
+${dom0_check}
 " > etc/local.d/xen.start
 chmod +x etc/local.d/xen.start
 echo "rc_verbose=yes" >> etc/rc.conf
@@ -148,5 +163,5 @@ timeout -k 1 240 \
     -bios /usr/lib/u-boot/qemu_arm64/u-boot.bin |& tee smoke.serial
 
 set -e
-(grep -q "^Welcome to Alpine Linux" smoke.serial && grep -q "DOM1: ${passed}" smoke.serial) || exit 1
+(grep -q "^Welcome to Alpine Linux" smoke.serial && grep -q "${passed}" smoke.serial) || exit 1
 exit 0
-- 
2.25.1

Re: [for-4.17 1/3] automation: qemu-alpine-arm64-gcc: Use kernel 5.19

[Luca Fancellu]

> On 2 Sep 2022, at 08:09, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> After qemu-smoke-arm64 was changed to use kernel 5.19 we end up having
> two kernel configurations. This is something not needed and maintaining
> a single kernel version is always easier. Modify qemu-alpine-arm64-gcc
> to use kernel 5.19 and remove kernel 5.9 from tests-artifacts.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Hi Michal,

I’m not an expert of gitlab-ci, but the changes looks ok to me:

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [for-4.17 2/3] automation: qemu-smoke-arm64: Silence ifconfig error messages

[Luca Fancellu]

> On 2 Sep 2022, at 08:09, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> During the ping test, dom1 tries to assign an ip to eth0 in a loop.
> Before setting up the network interface by dom0, this results in
> printing the following error message several times:
> (XEN) DOM1: ifconfig: SIOCSIFADDR: No such device
> 
> Silence this by redirecting stderr/stdout to /dev/null as we do not
> care about the output and we should not pollute the log file.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [for-4.17 3/3] automation: Add a new job for testing boot time cpupools on arm64

[Luca Fancellu]

> On 2 Sep 2022, at 08:09, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Add a new test job qemu-smoke-arm64-gcc-boot-cpupools that will execute
> script qemu-smoke-arm64.sh to test boot time cpupools feature.
> Enable CONFIG_BOOT_TIME_CPUPOOLS for the arm64 build and add a new test
> case in qemu-smoke-arm64.sh that if selected will:
> - create a device tree cpupool node with cpu@1
> - assign created cpupool to domU0
> - add a check in dom0 xen.start to see if domU is assigned a Pool-1
> 
> Take the opportunity to refactor the qemu-smoke-arm64.sh script as
> follows:
> - use domU_check to store the test's commands to be run from domU
> - use dom0_check to store the test's commands to be run from dom0
> - use fdtput instead of sed to perform dtb modifications
> - use more meaningful messages for "passed" variable. This way we can
>   grep for messages reported either by domU or dom0 and get rid of
>   assumption that tests can only be run from domU
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [for-4.17 3/3] automation: Add a new job for testing boot time cpupools on arm64

[Stefano Stabellini]

On Fri, 2 Sep 2022, Michal Orzel wrote:
> Add a new test job qemu-smoke-arm64-gcc-boot-cpupools that will execute
> script qemu-smoke-arm64.sh to test boot time cpupools feature.
> Enable CONFIG_BOOT_TIME_CPUPOOLS for the arm64 build and add a new test
> case in qemu-smoke-arm64.sh that if selected will:
>  - create a device tree cpupool node with cpu@1
>  - assign created cpupool to domU0
>  - add a check in dom0 xen.start to see if domU is assigned a Pool-1
> 
> Take the opportunity to refactor the qemu-smoke-arm64.sh script as
> follows:
>  - use domU_check to store the test's commands to be run from domU
>  - use dom0_check to store the test's commands to be run from dom0
>  - use fdtput instead of sed to perform dtb modifications
>  - use more meaningful messages for "passed" variable. This way we can
>    grep for messages reported either by domU or dom0 and get rid of
>    assumption that tests can only be run from domU
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>  automation/gitlab-ci/test.yaml         | 19 +++++++++++++++
>  automation/scripts/build               |  3 ++-
>  automation/scripts/qemu-smoke-arm64.sh | 33 +++++++++++++++++++-------
>  3 files changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index 07209820b474..d899b3bdbf7a 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -100,6 +100,25 @@ qemu-smoke-arm64-gcc-staticmem:
>    tags:
>      - arm64
>  
> +qemu-smoke-arm64-gcc-boot-cpupools:
> +  extends: .test-jobs-common
> +  variables:
> +    CONTAINER: debian:unstable-arm64v8
> +  script:
> +    - ./automation/scripts/qemu-smoke-arm64.sh boot-cpupools 2>&1 | tee qemu-smoke-arm64.log
> +  needs:
> +    - alpine-3.12-gcc-arm64
> +    - alpine-3.12-arm64-rootfs-export
> +    - kernel-5.19-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 2b9f2d2b541a..2f15ab3198e6 100755
> --- a/automation/scripts/build
> +++ b/automation/scripts/build
> @@ -19,7 +19,8 @@ else
>          echo "
>  CONFIG_EXPERT=y
>  CONFIG_UNSUPPORTED=y
> -CONFIG_STATIC_MEMORY=y" > xen/.config
> +CONFIG_STATIC_MEMORY=y
> +CONFIG_BOOT_TIME_CPUPOOLS=y" > xen/.config
>          make -j$(nproc) -C xen olddefconfig
>      else
>          make -j$(nproc) -C xen defconfig
> diff --git a/automation/scripts/qemu-smoke-arm64.sh b/automation/scripts/qemu-smoke-arm64.sh
> index 7ac96027760d..c2184850293c 100755
> --- a/automation/scripts/qemu-smoke-arm64.sh
> +++ b/automation/scripts/qemu-smoke-arm64.sh
> @@ -4,20 +4,22 @@ set -ex
>  
>  test_variant=$1
>  
> -passed="passed"
> -check="
> +if [ -z "${test_variant}" ]; then
> +    passed="ping test passed"
> +    domU_check="
>  until ifconfig eth0 192.168.0.2 &> /dev/null && ping -c 10 192.168.0.1; do
>      sleep 30
>  done
>  echo \"${passed}\"
>  "
> +fi
>  
>  if [[ "${test_variant}" == "static-mem" ]]; then
>      # Memory range that is statically allocated to DOM1
>      domu_base="50000000"
>      domu_size="10000000"
>      passed="${test_variant} test passed"
> -    check="
> +    domU_check="
>  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
> @@ -42,11 +44,23 @@ curl -fsSLO https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
>     -cpu cortex-a57 -machine type=virt \
>     -m 1024 -smp 2 -display none \
>     -machine dumpdtb=binaries/virt-gicv2.dtb
> -# XXX disable pl061 to avoid Linux crash
> -dtc -I dtb -O dts binaries/virt-gicv2.dtb > binaries/virt-gicv2.dts
> -sed 's/compatible = "arm,pl061.*/status = "disabled";/g' binaries/virt-gicv2.dts > binaries/virt-gicv2-edited.dts
> -dtc -I dts -O dtb binaries/virt-gicv2-edited.dts > binaries/virt-gicv2.dtb
>  
> +# XXX disable pl061 to avoid Linux crash
> +fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled

Currently this test fails with:

+ fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
+ [[ boot-cpupools == \b\o\o\t\-\c\p\u\p\o\o\l\s ]]
++ fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle
Error at 'phandle': FDT_ERR_NOTFOUND

Given my other comment below, I would leave this code as is.


> +if [[ "${test_variant}" == "boot-cpupools" ]]; then
> +    # Create cpupool node and assign it to domU0
> +    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
> +    cpupool_phandle="0xffffff"
> +    fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible xen,cpupool
> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus $cpu_phandle
> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle $cpupool_phandle
> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/domU0 domain-cpupool $cpupool_phandle
> +
> +    # Check if domU0 (id=1) is assigned to Pool-1
> +    passed="${test_variant} test passed"
> +    dom0_check="if xl list -c 1 | grep -q Pool-1; then echo ${passed}; fi"
> +fi

I would prefer to keep the device tree editing here to a minimum and
instead add boot-cpupool support in ImageBuilder and add CPUPOOL* config
options to the existing config file for ImageBuilder created in this
file below. This way, we keep this test cleaner and we help more the
user by proving a way to set boot-cpupools more easily in general, also
useful outside gitlab-ci.

Re: [for-4.17 3/3] automation: Add a new job for testing boot time cpupools on arm64

[Michal Orzel]

Hi Stefano,

On 03/09/2022 01:49, Stefano Stabellini wrote:
> 
> 
> Currently this test fails with:
> 
> + fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
> + [[ boot-cpupools == \b\o\o\t\-\c\p\u\p\o\o\l\s ]]
> ++ fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle
> Error at 'phandle': FDT_ERR_NOTFOUND
My bad. The qemu version used by CI does not generate phandles for cpus.
So the fix is very straightforward and requires putting custom phandle for cpu@1.

diff --git a/automation/scripts/qemu-smoke-arm64.sh b/automation/scripts/qemu-smoke-arm64.sh
index c2184850293c..158d5665d71d 100755
--- a/automation/scripts/qemu-smoke-arm64.sh
+++ b/automation/scripts/qemu-smoke-arm64.sh
@@ -50,8 +50,9 @@ fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
 
 if [[ "${test_variant}" == "boot-cpupools" ]]; then
     # Create cpupool node and assign it to domU0
-    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
+    cpu_phandle="0xfffffe"
     cpupool_phandle="0xffffff"
+    fdtput binaries/virt-gicv2.dtb -p -t x /cpus/cpu@1 phandle $cpu_phandle
     fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible xen,cpupool
     fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus $cpu_phandle
     fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle $cpupool_phandle

> 
> Given my other comment below, I would leave this code as is.
> 
> 
>> +if [[ "${test_variant}" == "boot-cpupools" ]]; then
>> +    # Create cpupool node and assign it to domU0
>> +    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
>> +    cpupool_phandle="0xffffff"
>> +    fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible xen,cpupool
>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus $cpu_phandle
>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle $cpupool_phandle
>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/domU0 domain-cpupool $cpupool_phandle
>> +
>> +    # Check if domU0 (id=1) is assigned to Pool-1
>> +    passed="${test_variant} test passed"
>> +    dom0_check="if xl list -c 1 | grep -q Pool-1; then echo ${passed}; fi"
>> +fi
> 
> I would prefer to keep the device tree editing here to a minimum and
> instead add boot-cpupool support in ImageBuilder and add CPUPOOL* config
> options to the existing config file for ImageBuilder created in this
> file below. This way, we keep this test cleaner and we help more the
> user by proving a way to set boot-cpupools more easily in general, also
> useful outside gitlab-ci.

I agree that ImageBuilder is a great tool. However, I would opt for keeping what I did because of the following:
- current release schedule (we could benefit from having a test for 4.17 feature instead of waiting for the corresponding
  change to be done in ImageBuilder first and tested),
- test is already prepared and requires just a trivial fix,
- we should not enforce users willing to add tests to gitlab-ci to always prepare the ImageBuilder changes first.
  ImageBuilder is not meant to support all the features strictly because some of them require too much
  end-user knowledge and digging into device tree (it should stay as simple as possible),
- all in all we need to have a way to modify the dtb and fdtput is certainly better than sed as it does not
  require additional steps for decompilation/compilation and its commands look more clean than using sed transformation.

Let me know what you think.

On a side note, I can add boot-time cpupools support in ImageBuilder to my TODO list so that we can check if this is something
ImageBuilder should support. If yes, we can modify this test after the release.
I can already think of the following IB config options that would need to be introduced to properly support boot-time cpupools:
CPUPOOL[number] = "<list_of_cpus> <scheduler>" - to create cpupools
NUM_CPUPOOLS = "<number>" - to keep the number of created cpupools
DOMU_CPUPOOL[number] = "CPUPOOL[number]" - to assing domU to one of the created cpupools
So we already have 3 new options and the number of required sanity checks I can think of is significant.
Even then, we could easily trigger a failure e.g. if user assigns cpus of different type and does not pass hmp-unsafe=1.


~Michal

Re: [for-4.17 3/3] automation: Add a new job for testing boot time cpupools on arm64

[Stefano Stabellini]

On Mon, 5 Sep 2022, Michal Orzel wrote:
> Hi Stefano,
> 
> On 03/09/2022 01:49, Stefano Stabellini wrote:
> > 
> > 
> > Currently this test fails with:
> > 
> > + fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
> > + [[ boot-cpupools == \b\o\o\t\-\c\p\u\p\o\o\l\s ]]
> > ++ fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle
> > Error at 'phandle': FDT_ERR_NOTFOUND
> My bad. The qemu version used by CI does not generate phandles for cpus.
> So the fix is very straightforward and requires putting custom phandle for cpu@1.
> 
> diff --git a/automation/scripts/qemu-smoke-arm64.sh b/automation/scripts/qemu-smoke-arm64.sh
> index c2184850293c..158d5665d71d 100755
> --- a/automation/scripts/qemu-smoke-arm64.sh
> +++ b/automation/scripts/qemu-smoke-arm64.sh
> @@ -50,8 +50,9 @@ fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
>  
>  if [[ "${test_variant}" == "boot-cpupools" ]]; then
>      # Create cpupool node and assign it to domU0
> -    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
> +    cpu_phandle="0xfffffe"
>      cpupool_phandle="0xffffff"
> +    fdtput binaries/virt-gicv2.dtb -p -t x /cpus/cpu@1 phandle $cpu_phandle
>      fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible xen,cpupool
>      fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus $cpu_phandle
>      fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle $cpupool_phandle
 
 
> > Given my other comment below, I would leave this code as is.
> > 
> > 
> >> +if [[ "${test_variant}" == "boot-cpupools" ]]; then
> >> +    # Create cpupool node and assign it to domU0
> >> +    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
> >> +    cpupool_phandle="0xffffff"
> >> +    fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible xen,cpupool
> >> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus $cpu_phandle
> >> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle $cpupool_phandle
> >> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/domU0 domain-cpupool $cpupool_phandle
> >> +
> >> +    # Check if domU0 (id=1) is assigned to Pool-1
> >> +    passed="${test_variant} test passed"
> >> +    dom0_check="if xl list -c 1 | grep -q Pool-1; then echo ${passed}; fi"
> >> +fi
> > 
> > I would prefer to keep the device tree editing here to a minimum and
> > instead add boot-cpupool support in ImageBuilder and add CPUPOOL* config
> > options to the existing config file for ImageBuilder created in this
> > file below. This way, we keep this test cleaner and we help more the
> > user by proving a way to set boot-cpupools more easily in general, also
> > useful outside gitlab-ci.
> 
> I agree that ImageBuilder is a great tool. However, I would opt for keeping what I did because of the following:
> - current release schedule (we could benefit from having a test for 4.17 feature instead of waiting for the corresponding
>   change to be done in ImageBuilder first and tested),
> - test is already prepared and requires just a trivial fix,
> - we should not enforce users willing to add tests to gitlab-ci to always prepare the ImageBuilder changes first.
>   ImageBuilder is not meant to support all the features strictly because some of them require too much
>   end-user knowledge and digging into device tree (it should stay as simple as possible),
> - all in all we need to have a way to modify the dtb and fdtput is certainly better than sed as it does not
>   require additional steps for decompilation/compilation and its commands look more clean than using sed transformation.
>
> Let me know what you think.
>
> On a side note, I can add boot-time cpupools support in ImageBuilder to my TODO list so that we can check if this is something
> ImageBuilder should support. If yes, we can modify this test after the release.


Yeah, ImageBuilder doesn't necessarely need to support every feature.
However, a tool (if not ImageBuilder, Lopper, or a new ImageBuilder
script) should support CPUPOOLs to enable the user.

You are right that ImageBuilder is not necessarely tied with gitlab-ci.
This is especially true once we start doing more interface-level
testing, such as hypercalls fuzzing with XTF. We are not going to be
able to use ImageBuilder to trigger every possible device tree boot time
combination, especially the ones that are invalid. We want to be able to
test Xen with invalid device tree input as well.

In addition to interface-level testing, we need user-level testing to
test features the way we expect a user to use them. This is what
ImageBuilder is for and that is why it has been used today in gitlab-ci.
On ARM today we only have user-level testing in gitlab-ci, but I'd love
to have more interface-level testing, which will surely require more
device tree manipulations outside of ImageBuilder.

- user-level tests -> ImageBuilder, common valid configurations
- interface-level tests -> not ImageBuilder, various valid and invalid
                           configurations, maybe automatically generated?
                           Device tree manipulations expected in gitlab-ci.


In my view, this test belongs to the "user-level test" category, this is
why I would prefer if it was done using the same tool that we expect the
user to use. Ideally, it would be ImageBuilder because that is the tool
that we have used so far (but it could be a new script under
ImageBuilder or Lopper).

But I understand deadlines, release schedule, etc., so if you think it
cannot be done properly using ImageBuilder in 2-3 days, then I would
take this patch as is, and we can revisit it in the future as you
suggested. I am OK with that too.

 
> I can already think of the following IB config options that would need to be introduced to properly support boot-time cpupools:
> CPUPOOL[number] = "<list_of_cpus> <scheduler>" - to create cpupools
> NUM_CPUPOOLS = "<number>" - to keep the number of created cpupools
> DOMU_CPUPOOL[number] = "CPUPOOL[number]" - to assing domU to one of the created cpupools
> So we already have 3 new options and the number of required sanity checks I can think of is significant.
> Even then, we could easily trigger a failure e.g. if user assigns cpus of different type and does not pass hmp-unsafe=1.


As we both wrote above, ImageBuilder doesn't need to support every
feature and every possible way a feature can be used. Only the most
common ones. I don't think we want to support the case where a user
assigns cpus of different type.

I think you are right that these are the interesting options at the
ImageBuilder level:

NUM_CPUPOOLS="<number>"
CPUPOOL[number] = "<list_of_cpus> <scheduler>"
DOMU_CPUPOOL[number] = "CPUPOOL[number]"

The last one could also be:

DOMU_CPUPOOL[number] = "CPUPOOL<number>" # e.g. DOMU_CPUPOOL[1] = "CPUPOOL2"

Re: [for-4.17 3/3] automation: Add a new job for testing boot time cpupools on arm64

[Michal Orzel]

Hi Stefano,

On 06/09/2022 01:21, Stefano Stabellini wrote:
> 
> On Mon, 5 Sep 2022, Michal Orzel wrote:
>> Hi Stefano,
>>
>> On 03/09/2022 01:49, Stefano Stabellini wrote:
>>>
>>>
>>> Currently this test fails with:
>>>
>>> + fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
>>> + [[ boot-cpupools == \b\o\o\t\-\c\p\u\p\o\o\l\s ]]
>>> ++ fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle
>>> Error at 'phandle': FDT_ERR_NOTFOUND
>> My bad. The qemu version used by CI does not generate phandles for cpus.
>> So the fix is very straightforward and requires putting custom phandle for cpu@1.
>>
>> diff --git a/automation/scripts/qemu-smoke-arm64.sh b/automation/scripts/qemu-smoke-arm64.sh
>> index c2184850293c..158d5665d71d 100755
>> --- a/automation/scripts/qemu-smoke-arm64.sh
>> +++ b/automation/scripts/qemu-smoke-arm64.sh
>> @@ -50,8 +50,9 @@ fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
>>
>>  if [[ "${test_variant}" == "boot-cpupools" ]]; then
>>      # Create cpupool node and assign it to domU0
>> -    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
>> +    cpu_phandle="0xfffffe"
>>      cpupool_phandle="0xffffff"
>> +    fdtput binaries/virt-gicv2.dtb -p -t x /cpus/cpu@1 phandle $cpu_phandle
>>      fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible xen,cpupool
>>      fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus $cpu_phandle
>>      fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle $cpupool_phandle
> 
> 
>>> Given my other comment below, I would leave this code as is.
>>>
>>>
>>>> +if [[ "${test_variant}" == "boot-cpupools" ]]; then
>>>> +    # Create cpupool node and assign it to domU0
>>>> +    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
>>>> +    cpupool_phandle="0xffffff"
>>>> +    fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible xen,cpupool
>>>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus $cpu_phandle
>>>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle $cpupool_phandle
>>>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/domU0 domain-cpupool $cpupool_phandle
>>>> +
>>>> +    # Check if domU0 (id=1) is assigned to Pool-1
>>>> +    passed="${test_variant} test passed"
>>>> +    dom0_check="if xl list -c 1 | grep -q Pool-1; then echo ${passed}; fi"
>>>> +fi
>>>
>>> I would prefer to keep the device tree editing here to a minimum and
>>> instead add boot-cpupool support in ImageBuilder and add CPUPOOL* config
>>> options to the existing config file for ImageBuilder created in this
>>> file below. This way, we keep this test cleaner and we help more the
>>> user by proving a way to set boot-cpupools more easily in general, also
>>> useful outside gitlab-ci.
>>
>> I agree that ImageBuilder is a great tool. However, I would opt for keeping what I did because of the following:
>> - current release schedule (we could benefit from having a test for 4.17 feature instead of waiting for the corresponding
>>   change to be done in ImageBuilder first and tested),
>> - test is already prepared and requires just a trivial fix,
>> - we should not enforce users willing to add tests to gitlab-ci to always prepare the ImageBuilder changes first.
>>   ImageBuilder is not meant to support all the features strictly because some of them require too much
>>   end-user knowledge and digging into device tree (it should stay as simple as possible),
>> - all in all we need to have a way to modify the dtb and fdtput is certainly better than sed as it does not
>>   require additional steps for decompilation/compilation and its commands look more clean than using sed transformation.
>>
>> Let me know what you think.
>>
>> On a side note, I can add boot-time cpupools support in ImageBuilder to my TODO list so that we can check if this is something
>> ImageBuilder should support. If yes, we can modify this test after the release.
> 
> 
> Yeah, ImageBuilder doesn't necessarely need to support every feature.
> However, a tool (if not ImageBuilder, Lopper, or a new ImageBuilder
> script) should support CPUPOOLs to enable the user.
> 
> You are right that ImageBuilder is not necessarely tied with gitlab-ci.
> This is especially true once we start doing more interface-level
> testing, such as hypercalls fuzzing with XTF. We are not going to be
> able to use ImageBuilder to trigger every possible device tree boot time
> combination, especially the ones that are invalid. We want to be able to
> test Xen with invalid device tree input as well.
> 
> In addition to interface-level testing, we need user-level testing to
> test features the way we expect a user to use them. This is what
> ImageBuilder is for and that is why it has been used today in gitlab-ci.
> On ARM today we only have user-level testing in gitlab-ci, but I'd love
> to have more interface-level testing, which will surely require more
> device tree manipulations outside of ImageBuilder.
> 
> - user-level tests -> ImageBuilder, common valid configurations
> - interface-level tests -> not ImageBuilder, various valid and invalid
>                            configurations, maybe automatically generated?
>                            Device tree manipulations expected in gitlab-ci.
> 
> 
> In my view, this test belongs to the "user-level test" category, this is
> why I would prefer if it was done using the same tool that we expect the
> user to use. Ideally, it would be ImageBuilder because that is the tool
> that we have used so far (but it could be a new script under
> ImageBuilder or Lopper).
> 
> But I understand deadlines, release schedule, etc., so if you think it
> cannot be done properly using ImageBuilder in 2-3 days, then I would
> take this patch as is, and we can revisit it in the future as you
> suggested. I am OK with that too.

Ok, let me see if I can come up with a clean solution in the ImageBuilder.

~Michal

Re: [for-4.17 1/3] automation: qemu-alpine-arm64-gcc: Use kernel 5.19

[Ayan Kumar Halder]

Hi Michal,

On 02/09/2022 08:09, Michal Orzel wrote:
> After qemu-smoke-arm64 was changed to use kernel 5.19 we end up having
> two kernel configurations. This is something not needed and maintaining
> a single kernel version is always easier. Modify qemu-alpine-arm64-gcc
> to use kernel 5.19 and remove kernel 5.9 from tests-artifacts.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Ayan Kumar Halder <ayankuma@amd.com>

Question unrelated to the patch.

When do we decide that we need to upgrade the kernel version ?

> ---
>   automation/gitlab-ci/build.yaml               | 11 ------
>   automation/gitlab-ci/test.yaml                |  2 +-
>   .../kernel/5.9.9-arm64v8.dockerfile           | 34 -------------------
>   3 files changed, 1 insertion(+), 46 deletions(-)
>   delete mode 100644 automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile
>
> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
> index d2f75a090c0f..720ce6e07ba0 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -586,17 +586,6 @@ alpine-3.12-arm64-rootfs-export:
>     tags:
>       - arm64
>   
> -kernel-5.9.9-arm64-export:
> -  extends: .test-jobs-artifact-common
> -  image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:5.9.9-arm64v8
> -  script:
> -    - mkdir binaries && cp /Image binaries/Image
> -  artifacts:
> -    paths:
> -      - binaries/Image
> -  tags:
> -    - arm64
> -
>   kernel-5.19-arm64-export:
>     extends: .test-jobs-artifact-common
>     image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:5.19-arm64v8
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index 2eb6c3866e2c..07209820b474 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -34,7 +34,7 @@ qemu-alpine-arm64-gcc:
>     needs:
>       - alpine-3.12-gcc-arm64
>       - alpine-3.12-arm64-rootfs-export
> -    - kernel-5.9.9-arm64-export
> +    - kernel-5.19-arm64-export
>       - qemu-system-aarch64-6.0.0-arm64-export
>     artifacts:
>       paths:
> diff --git a/automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile b/automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile
> deleted file mode 100644
> index 053d65a3454e..000000000000
> --- a/automation/tests-artifacts/kernel/5.9.9-arm64v8.dockerfile
> +++ /dev/null
> @@ -1,34 +0,0 @@
> -FROM arm64v8/debian:unstable
> -LABEL maintainer.name="The Xen Project" \
> -      maintainer.email="xen-devel@lists.xenproject.org"
> -
> -ENV DEBIAN_FRONTEND=noninteractive
> -ENV LINUX_VERSION=5.9.9
> -ENV USER root
> -
> -RUN mkdir /build
> -WORKDIR /build
> -
> -# build depends
> -RUN apt-get update && \
> -    apt-get --quiet --yes install \
> -        build-essential \
> -        libssl-dev \
> -        bc \
> -        curl \
> -        flex \
> -        bison \
> -        && \
> -    \
> -    # Build the kernel
> -    curl -fsSLO https://cdn.kernel.org/pub/linux/kernel/v5.x/linux-"$LINUX_VERSION".tar.xz && \
> -    tar xvJf linux-"$LINUX_VERSION".tar.xz && \
> -    cd linux-"$LINUX_VERSION" && \
> -    make defconfig && \
> -    make -j$(nproc) Image.gz && \
> -    cp arch/arm64/boot/Image / && \
> -    cd /build && \
> -    rm -rf linux-"$LINUX_VERSION"* && \
> -    apt-get autoremove -y && \
> -    apt-get clean && \
> -    rm -rf /var/lib/apt/lists* /tmp/* /var/tmp/*

Re: [for-4.17 2/3] automation: qemu-smoke-arm64: Silence ifconfig error messages

[Ayan Kumar Halder]

On 02/09/2022 08:09, Michal Orzel wrote:
> During the ping test, dom1 tries to assign an ip to eth0 in a loop.
> Before setting up the network interface by dom0, this results in
> printing the following error message several times:
> (XEN) DOM1: ifconfig: SIOCSIFADDR: No such device
>
> Silence this by redirecting stderr/stdout to /dev/null as we do not
> care about the output and we should not pollute the log file.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Ayan Kumar Halder <ayankuma@amd.com>
> ---
>   automation/scripts/qemu-smoke-arm64.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/automation/scripts/qemu-smoke-arm64.sh b/automation/scripts/qemu-smoke-arm64.sh
> index c80d9b2aee00..7ac96027760d 100755
> --- a/automation/scripts/qemu-smoke-arm64.sh
> +++ b/automation/scripts/qemu-smoke-arm64.sh
> @@ -6,7 +6,7 @@ test_variant=$1
>   
>   passed="passed"
>   check="
> -until ifconfig eth0 192.168.0.2 && ping -c 10 192.168.0.1; do
> +until ifconfig eth0 192.168.0.2 &> /dev/null && ping -c 10 192.168.0.1; do
>       sleep 30
>   done
>   echo \"${passed}\"

Re: [for-4.17 1/3] automation: qemu-alpine-arm64-gcc: Use kernel 5.19

[Michal Orzel]

Hi Ayan,

On 08/09/2022 11:07, Ayan Kumar Halder wrote:
> Hi Michal,
> 
> On 02/09/2022 08:09, Michal Orzel wrote:
>> After qemu-smoke-arm64 was changed to use kernel 5.19 we end up having
>> two kernel configurations. This is something not needed and maintaining
>> a single kernel version is always easier. Modify qemu-alpine-arm64-gcc
>> to use kernel 5.19 and remove kernel 5.9 from tests-artifacts.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> Reviewed-by: Ayan Kumar Halder <ayankuma@amd.com>
> 
> Question unrelated to the patch.
> 
> When do we decide that we need to upgrade the kernel version ?
Usually when we need a feature from the newer kernel version.
Upgrade to 5.19 was done so that we can test dom0less enhanced
as this kernel introduced late xenstore initialization.

~Michal