[PATCH 0/5] automation: cleanup hardware based tests

[PATCH 0/5] automation: cleanup hardware based tests

[Marek Marczykowski-Górecki]

While working on tests for MSI-X, I did few cleanups of hw-based gitlab tests,
greatly reducing false positive messages in the test output.

After applying this series, the tests-artifacts/alpine/3.18 container needs to
be rebuilt.
Test run with container rebuilt (on my repo):
https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1027467761

Cc-ing Henry for release ack.
---
Cc: Henry Wang <Henry.Wang@arm.com>
Cc:  Doug Goldstein <cardoe@cardoe.com>
Cc:  Stefano Stabellini <sstabellini@kernel.org>

Marek Marczykowski-Górecki (5):
  automation: include real-time view of the domU console log too
  automation: hide timeout countdown in log
  automation: cleanup test alpine install
  automation: improve checking for MSI/MSI-X in PCI passthrough tests
  automation: extract QEMU log in relevant hardware tests

 automation/gitlab-ci/test.yaml                    |  2 +-
 automation/scripts/qubes-x86-64.sh                | 29 ++++++----------
 automation/tests-artifacts/alpine/3.18.dockerfile |  7 +---
 3 files changed, 15 insertions(+), 23 deletions(-)

base-commit: 3d2d9e90224c4f430a7ee1190fd3b871b99b0ba0
-- 
git-series 0.9.1

[PATCH 1/5] automation: include real-time view of the domU console log too

[Marek Marczykowski-Górecki]

Passthrough domU console log to the serial console in real time, not
only after the test. First of all, this gives domU console also in case
of test failure. But also, allows correlation between domU and dom0 or
Xen messages.

To avoid ambiguity, add log prefix with 'sed'.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 automation/scripts/qubes-x86-64.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 5f6052eef041..1e84e40a4afc 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -33,8 +33,6 @@ echo \"${passed}\"
 until grep -q \"${passed}\" /var/log/xen/console/guest-domU.log; do
     sleep 1
 done
-# get domU console content into test log
-tail -n 100 /var/log/xen/console/guest-domU.log
 echo \"${passed}\"
 "
 if [ "${test_variant}" = "dom0pvh" ]; then
@@ -59,8 +57,6 @@ echo deep > /sys/power/mem_sleep
 echo mem > /sys/power/state
 # now wait for resume
 sleep 5
-# get domU console content into test log
-tail -n 100 /var/log/xen/console/guest-domU.log
 xl list
 xl dmesg | grep 'Finishing wakeup from ACPI S3 state' || exit 1
 # check if domU is still alive
@@ -121,7 +117,6 @@ echo \"${passed}\"
 until grep -q \"^domU Welcome to Alpine Linux\" /var/log/xen/console/guest-domU.log; do
     sleep 1
 done
-tail -n 100 /var/log/xen/console/guest-domU.log
 "
 fi
 
@@ -169,6 +164,8 @@ ifconfig eth0 up
 ifconfig xenbr0 up
 ifconfig xenbr0 192.168.0.1
 
+# get domU console content into test log
+tail -F /var/log/xen/console/guest-domU.log 2>/dev/null | sed -e \"s/^/(domU) /\" &
 xl create /etc/xen/domU.cfg
 ${dom0_check}
 " > etc/local.d/xen.start
-- 
git-series 0.9.1

[PATCH 2/5] automation: hide timeout countdown in log

[Marek Marczykowski-Górecki]

grep+sleep message every 1s makes job log unnecessary hard to read.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
I know I can download serial log file, but that's 3 more clicks...
---
 automation/scripts/qubes-x86-64.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 1e84e40a4afc..5464d10fc343 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -222,10 +222,12 @@ if [ -n "$wait_and_wakeup" ]; then
     ssh $CONTROLLER wake
 fi
 
+set +x
 until grep "^Welcome to Alpine Linux" smoke.serial || [ $timeout -le 0 ]; do
     sleep 1;
     : $((--timeout))
 done
+set -x
 
 tail -n 100 smoke.serial
 
-- 
git-series 0.9.1

[PATCH 3/5] automation: cleanup test alpine install

[Marek Marczykowski-Górecki]

Remove parts of initramfs for the test system (domU, and in few tests
dom0 too) that are not not working and are not really needed in this
simple system.

This makes the test log much lighter on misleading error messages.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 automation/tests-artifacts/alpine/3.18.dockerfile | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/automation/tests-artifacts/alpine/3.18.dockerfile b/automation/tests-artifacts/alpine/3.18.dockerfile
index 32aa8e177847..333951d05e84 100644
--- a/automation/tests-artifacts/alpine/3.18.dockerfile
+++ b/automation/tests-artifacts/alpine/3.18.dockerfile
@@ -40,7 +40,6 @@ RUN \
   rc-update add udev && \
   rc-update add udev-trigger && \
   rc-update add udev-settle && \
-  rc-update add networking sysinit && \
   rc-update add loopback sysinit && \
   rc-update add bootmisc boot && \
   rc-update add devfs sysinit && \
@@ -48,18 +47,17 @@ RUN \
   rc-update add hostname boot && \
   rc-update add hwclock boot && \
   rc-update add hwdrivers sysinit && \
-  rc-update add killprocs shutdown && \
-  rc-update add modloop sysinit && \
   rc-update add modules boot && \
+  rc-update add killprocs shutdown && \
   rc-update add mount-ro shutdown && \
   rc-update add savecache shutdown && \
-  rc-update add sysctl boot && \
   rc-update add local default && \
   cp -a /sbin/init /init && \
   echo "ttyS0" >> /etc/securetty && \
   echo "hvc0" >> /etc/securetty && \
   echo "ttyS0::respawn:/sbin/getty -L ttyS0 115200 vt100" >> /etc/inittab && \
   echo "hvc0::respawn:/sbin/getty -L hvc0 115200 vt100" >> /etc/inittab && \
+  echo > /etc/modules && \
   passwd -d "root" root && \
   \
   # Create rootfs
-- 
git-series 0.9.1

[PATCH 4/5] automation: improve checking for MSI/MSI-X in PCI passthrough tests

[Marek Marczykowski-Górecki]

Checking /proc/interrupts is unreliable because different drivers set
different names there. Install pciutils and use lspci instead.
In fact, the /proc/interrupts content was confusing enough that
adl-pci-hvm had it wrong (MSI-X is in use there). Fix this too.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 automation/gitlab-ci/test.yaml                    |  2 +--
 automation/scripts/qubes-x86-64.sh                | 19 +++++-----------
 automation/tests-artifacts/alpine/3.18.dockerfile |  1 +-
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 4b836bf04784..61e642cce0cc 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -195,8 +195,6 @@ adl-pci-pv-x86-64-gcc-debug:
 
 adl-pci-hvm-x86-64-gcc-debug:
   extends: .adl-x86-64
-  variables:
-    PCIDEV_INTR: "MSI"
   script:
     - ./automation/scripts/qubes-x86-64.sh pci-hvm 2>&1 | tee ${LOGFILE}
   needs:
diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 5464d10fc343..842e6fae7204 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -90,23 +90,18 @@ on_reboot = "destroy"
 
     domU_check="
 set -x -e
-ip link set eth0 up
-timeout 30s udhcpc -i eth0
+interface=eth0
+ip link set \"\$interface\" up
+timeout 30s udhcpc -i \"\$interface\"
 pingip=\$(ip -o -4 r show default|cut -f 3 -d ' ')
 ping -c 10 \"\$pingip\"
 echo domU started
-cat /proc/interrupts
+pcidevice=\$(basename \$(readlink /sys/class/net/\$interface/device))
+lspci -vs \$pcidevice
 "
-    if [ "$PCIDEV_INTR" = "MSI-X" ]; then
+    if [ -n "$PCIDEV_INTR" ]; then
         domU_check="$domU_check
-grep -- '\\(-msi-x\\|PCI-MSI-X\\).*eth0' /proc/interrupts
-"
-    elif [ "$PCIDEV_INTR" = "MSI" ]; then
-        # depending on the kernel version and domain type, the MSI can be
-        # marked as '-msi', 'PCI-MSI', or 'PCI-MSI-<SBDF>'; be careful to not match
-        # -msi-x nor PCI-MSI-X
-        domU_check="$domU_check
-grep -- '\\(-msi \\|PCI-MSI\\( \\|-[^X]\\)\\).*eth0' /proc/interrupts
+lspci -vs \$pcidevice | fgrep '$PCIDEV_INTR: Enable+'
 "
     fi
     domU_check="$domU_check
diff --git a/automation/tests-artifacts/alpine/3.18.dockerfile b/automation/tests-artifacts/alpine/3.18.dockerfile
index 333951d05e84..5f521572b8fb 100644
--- a/automation/tests-artifacts/alpine/3.18.dockerfile
+++ b/automation/tests-artifacts/alpine/3.18.dockerfile
@@ -33,6 +33,7 @@ RUN \
   apk add pixman && \
   apk add curl && \
   apk add udev && \
+  apk add pciutils && \
   \
   # Xen
   cd / && \
-- 
git-series 0.9.1

[PATCH 5/5] automation: extract QEMU log in relevant hardware tests

[Marek Marczykowski-Górecki]

Let it be printed to the console too. QEMU and Linux messages have
different enough format that it should be possible to distinguish them.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 automation/scripts/qubes-x86-64.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 842e6fae7204..fe80a1c84308 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -109,6 +109,7 @@ echo \"${passed}\"
 "
 
     dom0_check="
+tail -F /var/log/xen/qemu-dm-domU.log &
 until grep -q \"^domU Welcome to Alpine Linux\" /var/log/xen/console/guest-domU.log; do
     sleep 1
 done
-- 
git-series 0.9.1

Re: [PATCH 2/5] automation: hide timeout countdown in log

[Andrew Cooper]

On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote:
> grep+sleep message every 1s makes job log unnecessary hard to read.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> I know I can download serial log file, but that's 3 more clicks...
> ---
>  automation/scripts/qubes-x86-64.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> index 1e84e40a4afc..5464d10fc343 100755
> --- a/automation/scripts/qubes-x86-64.sh
> +++ b/automation/scripts/qubes-x86-64.sh
> @@ -222,10 +222,12 @@ if [ -n "$wait_and_wakeup" ]; then
>      ssh $CONTROLLER wake
>  fi
>  
> +set +x
>  until grep "^Welcome to Alpine Linux" smoke.serial || [ $timeout -le 0 ]; do
>      sleep 1;
>      : $((--timeout))
>  done
> +set -x
>  
>  tail -n 100 smoke.serial
>  

This wants repeating in dom0_check= when looking for "Welcome to Alpine"
in guest-domU.log because the scrool is still visible in

https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/5235487317

Happy to fix on commit, as this is the only comment I have on the series.

Alternatively, would it be worth writing a "wait_until $msg $file"
function to abstract this away?

~Andrew

Re: [PATCH for-4.18 0/5] automation: cleanup hardware based tests

[Andrew Cooper]

On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote:
> While working on tests for MSI-X, I did few cleanups of hw-based gitlab tests,
> greatly reducing false positive messages in the test output.
>
> After applying this series, the tests-artifacts/alpine/3.18 container needs to
> be rebuilt.
> Test run with container rebuilt (on my repo):
> https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1027467761
>
> Cc-ing Henry for release ack.
> ---
> Cc: Henry Wang <Henry.Wang@arm.com>
> Cc:  Doug Goldstein <cardoe@cardoe.com>
> Cc:  Stefano Stabellini <sstabellini@kernel.org>
>
> Marek Marczykowski-Górecki (5):
>   automation: include real-time view of the domU console log too
>   automation: hide timeout countdown in log
>   automation: cleanup test alpine install
>   automation: improve checking for MSI/MSI-X in PCI passthrough tests
>   automation: extract QEMU log in relevant hardware tests

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Henry - this will be a good improvement to take.  It's only the test
logic, with Gitlab being happy with the result.

Re: [PATCH 2/5] automation: hide timeout countdown in log

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On Wed, Oct 18, 2023 at 06:45:03PM +0100, Andrew Cooper wrote:
> On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote:
> > grep+sleep message every 1s makes job log unnecessary hard to read.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > I know I can download serial log file, but that's 3 more clicks...
> > ---
> >  automation/scripts/qubes-x86-64.sh | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> > index 1e84e40a4afc..5464d10fc343 100755
> > --- a/automation/scripts/qubes-x86-64.sh
> > +++ b/automation/scripts/qubes-x86-64.sh
> > @@ -222,10 +222,12 @@ if [ -n "$wait_and_wakeup" ]; then
> >      ssh $CONTROLLER wake
> >  fi
> >  
> > +set +x
> >  until grep "^Welcome to Alpine Linux" smoke.serial || [ $timeout -le 0 ]; do
> >      sleep 1;
> >      : $((--timeout))
> >  done
> > +set -x
> >  
> >  tail -n 100 smoke.serial
> >  
> 
> This wants repeating in dom0_check= when looking for "Welcome to Alpine"
> in guest-domU.log because the scrool is still visible in
> 
> https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/5235487317
> 
> Happy to fix on commit, as this is the only comment I have on the series.

Fine with me, thanks!

> Alternatively, would it be worth writing a "wait_until $msg $file"
> function to abstract this away?

Can refactor it later, maybe when adding more tests.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH for-4.18 0/5] automation: cleanup hardware based tests

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Wed, 18 Oct 2023, Andrew Cooper wrote:
> On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote:
> > While working on tests for MSI-X, I did few cleanups of hw-based gitlab tests,
> > greatly reducing false positive messages in the test output.
> >
> > After applying this series, the tests-artifacts/alpine/3.18 container needs to
> > be rebuilt.
> > Test run with container rebuilt (on my repo):
> > https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1027467761
> >
> > Cc-ing Henry for release ack.
> > ---
> > Cc: Henry Wang <Henry.Wang@arm.com>
> > Cc:  Doug Goldstein <cardoe@cardoe.com>
> > Cc:  Stefano Stabellini <sstabellini@kernel.org>
> >
> > Marek Marczykowski-Górecki (5):
> >   automation: include real-time view of the domU console log too
> >   automation: hide timeout countdown in log
> >   automation: cleanup test alpine install
> >   automation: improve checking for MSI/MSI-X in PCI passthrough tests
> >   automation: extract QEMU log in relevant hardware tests
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> Henry - this will be a good improvement to take.  It's only the test
> logic, with Gitlab being happy with the result.

I'll leave it to Henry.

Andrew, if/when you end up committing the series, please also update the
container,

Re: [PATCH 4/5] automation: improve checking for MSI/MSI-X in PCI passthrough tests

[Andrew Cooper]

On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote:
> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> index 5464d10fc343..842e6fae7204 100755
> --- a/automation/scripts/qubes-x86-64.sh
> +++ b/automation/scripts/qubes-x86-64.sh
> @@ -90,23 +90,18 @@ on_reboot = "destroy"
>  
>      domU_check="
>  set -x -e
> -ip link set eth0 up
> -timeout 30s udhcpc -i eth0
> +interface=eth0
> +ip link set \"\$interface\" up
> +timeout 30s udhcpc -i \"\$interface\"
>  pingip=\$(ip -o -4 r show default|cut -f 3 -d ' ')
>  ping -c 10 \"\$pingip\"
>  echo domU started
> -cat /proc/interrupts
> +pcidevice=\$(basename \$(readlink /sys/class/net/\$interface/device))
> +lspci -vs \$pcidevice

Actually, I know I said I had no more comments, and maybe this is one
for further cleanup rather than for now, but wouldn't we be much better
using a heredoc?

read -r -d '' domU_check <<"EOF"

lorem ipsum, no \ escaping " or $

EOF

If nothing else it would make the innards of the more readable as a
script fragment, and less likely go to wrong with variable expansion in
the wrong context.

~Andrew

Re: [PATCH 4/5] automation: improve checking for MSI/MSI-X in PCI passthrough tests

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]

On Thu, Oct 19, 2023 at 12:30:22AM +0100, Andrew Cooper wrote:
> On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote:
> > diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> > index 5464d10fc343..842e6fae7204 100755
> > --- a/automation/scripts/qubes-x86-64.sh
> > +++ b/automation/scripts/qubes-x86-64.sh
> > @@ -90,23 +90,18 @@ on_reboot = "destroy"
> >  
> >      domU_check="
> >  set -x -e
> > -ip link set eth0 up
> > -timeout 30s udhcpc -i eth0
> > +interface=eth0
> > +ip link set \"\$interface\" up
> > +timeout 30s udhcpc -i \"\$interface\"
> >  pingip=\$(ip -o -4 r show default|cut -f 3 -d ' ')
> >  ping -c 10 \"\$pingip\"
> >  echo domU started
> > -cat /proc/interrupts
> > +pcidevice=\$(basename \$(readlink /sys/class/net/\$interface/device))
> > +lspci -vs \$pcidevice
> 
> Actually, I know I said I had no more comments, and maybe this is one
> for further cleanup rather than for now, but wouldn't we be much better
> using a heredoc?
> 
> read -r -d '' domU_check <<"EOF"
> 
> lorem ipsum, no \ escaping " or $
> 
> EOF
> 
> If nothing else it would make the innards of the more readable as a
> script fragment, and less likely go to wrong with variable expansion in
> the wrong context.

Some of those snippets have intentionally unescaped $ (as in - have it
expanded in place), and IMO having different method depending whether
you want to expand variables inside or not will be more error-prone.
Syntax highlighting in an editor makes it rather easy to spot unescaped
$ or such (and then decide whether it was intentional or not).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH for-4.18 0/5] automation: cleanup hardware based tests

[Henry Wang]

Hi,

> On Oct 19, 2023, at 06:32, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 18 Oct 2023, Andrew Cooper wrote:
>> On 06/10/2023 3:05 am, Marek Marczykowski-Górecki wrote:
>>> While working on tests for MSI-X, I did few cleanups of hw-based gitlab tests,
>>> greatly reducing false positive messages in the test output.
>>> 
>>> After applying this series, the tests-artifacts/alpine/3.18 container needs to
>>> be rebuilt.
>>> Test run with container rebuilt (on my repo):
>>> https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1027467761
>>> 
>>> Cc-ing Henry for release ack.
>>> ---
>>> Cc: Henry Wang <Henry.Wang@arm.com>
>>> Cc:  Doug Goldstein <cardoe@cardoe.com>
>>> Cc:  Stefano Stabellini <sstabellini@kernel.org>
>>> 
>>> Marek Marczykowski-Górecki (5):
>>>  automation: include real-time view of the domU console log too
>>>  automation: hide timeout countdown in log
>>>  automation: cleanup test alpine install
>>>  automation: improve checking for MSI/MSI-X in PCI passthrough tests
>>>  automation: extract QEMU log in relevant hardware tests
>> 
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>> Henry - this will be a good improvement to take.  It's only the test
>> logic, with Gitlab being happy with the result.
> 
> I'll leave it to Henry.

I am happy to include this as in 4.17 we discussed that CI changes won’t be
blocked.

Release-acked-by: Henry Wang <Henry.Wang@arm.com>


> Andrew, if/when you end up committing the series, please also update the
> container,

Please any of you two do that :) Thank you.

Kind regards,
Henry