1. Patchew
  2. Xen
  3. [PATCH 0/3] Add {adl,zen3p}-pvshim-* tests
  4. View patch

[PATCH 3/3] CI: Add {adl,zen3p}-pvshim-* tests

Andrew Cooper posted 3 patches 1 month ago
[PATCH 3/3] CI: Add {adl,zen3p}-pvshim-* tests
Posted by Andrew Cooper 1 month ago 
GitlabCI has no testing of Xen's PVH entrypoint.  Fix this.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Doug Goldstein <cardoe@cardoe.com>

OSSTest (which is disappearing imminently) found a pvshim bug in the
hyperlaunch series, and I found a second shortly after while trying to take
more of the series.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1505518838
---
 automation/gitlab-ci/test.yaml     | 16 ++++++++++++++++
 automation/scripts/qubes-x86-64.sh | 10 ++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index b27c2be17487..e76a37bef32d 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -240,6 +240,14 @@ adl-pci-hvm-x86-64-gcc-debug:
     - *x86-64-test-needs
     - alpine-3.18-gcc-debug
 
+adl-pvshim-x86-64-gcc-debug:
+  extends: .adl-x86-64
+  script:
+    - ./automation/scripts/qubes-x86-64.sh pvshim 2>&1 | tee ${LOGFILE}
+  needs:
+    - *x86-64-test-needs
+    - alpine-3.18-gcc-debug
+
 zen3p-smoke-x86-64-gcc-debug:
   extends: .zen3p-x86-64
   script:
@@ -272,6 +280,14 @@ zen3p-pci-hvm-x86-64-gcc-debug:
     - *x86-64-test-needs
     - alpine-3.18-gcc-debug
 
+zen3p-pvshim-x86-64-gcc-debug:
+  extends: .zen3p-x86-64
+  script:
+    - ./automation/scripts/qubes-x86-64.sh pvshim 2>&1 | tee ${LOGFILE}
+  needs:
+    - *x86-64-test-needs
+    - alpine-3.18-gcc-debug
+
 qemu-smoke-dom0-arm64-gcc:
   extends: .qemu-arm64
   script:
diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 4b6311efffa8..ace494b938d8 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -8,6 +8,7 @@ set -ex
 #  - dom0pvh-hvm    PVH dom0, HVM domU
 #  - pci-hvm        PV dom0,  HVM domU + PCI Passthrough
 #  - pci-pv         PV dom0,  PV domU + PCI Passthrough
+#  - pvshim         PV dom0,  PVSHIM domU
 #  - s3             PV dom0,  S3 suspend/resume
 test_variant=$1
 
@@ -20,8 +21,8 @@ domU_vif="'bridge=xenbr0',"
 domU_extra_cfg=
 
 case "${test_variant}" in
-    ### test: smoke test & smoke test PVH & smoke test HVM
-    ""|"dom0pvh"|"dom0pvh-hvm")
+    ### test: smoke test & smoke test PVH & smoke test HVM & smoke test PVSHIM
+    ""|"dom0pvh"|"dom0pvh-hvm"|"pvshim")
         passed="ping test passed"
         domU_check="
 ifconfig eth0 192.168.0.2
@@ -44,6 +45,11 @@ echo \"${passed}\"
 
         if [ "${test_variant}" = "dom0pvh-hvm" ]; then
             domU_type="hvm"
+        elif [ "${test_variant}" = "pvshim" ]; then
+            domU_type="pv"
+            domU_extra_cfg='
+pvshim = 1
+'
         fi
         ;;
 
-- 
2.39.5
Re: [PATCH 3/3] CI: Add {adl,zen3p}-pvshim-* tests
Posted by Andrew Cooper 1 month ago 
On 21/10/2024 3:35 pm, Andrew Cooper wrote:
> GitlabCI has no testing of Xen's PVH entrypoint.  Fix this.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Anthony PERARD <anthony.perard@vates.tech>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Doug Goldstein <cardoe@cardoe.com>
>
> OSSTest (which is disappearing imminently) found a pvshim bug in the
> hyperlaunch series, and I found a second shortly after while trying to take
> more of the series.
>
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1505518838
> ---
>  automation/gitlab-ci/test.yaml     | 16 ++++++++++++++++
>  automation/scripts/qubes-x86-64.sh | 10 ++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index b27c2be17487..e76a37bef32d 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -240,6 +240,14 @@ adl-pci-hvm-x86-64-gcc-debug:
>      - *x86-64-test-needs
>      - alpine-3.18-gcc-debug
>  
> +adl-pvshim-x86-64-gcc-debug:
> +  extends: .adl-x86-64
> +  script:
> +    - ./automation/scripts/qubes-x86-64.sh pvshim 2>&1 | tee ${LOGFILE}
> +  needs:
> +    - *x86-64-test-needs
> +    - alpine-3.18-gcc-debug
> +
>  zen3p-smoke-x86-64-gcc-debug:
>    extends: .zen3p-x86-64
>    script:
> @@ -272,6 +280,14 @@ zen3p-pci-hvm-x86-64-gcc-debug:
>      - *x86-64-test-needs
>      - alpine-3.18-gcc-debug
>  
> +zen3p-pvshim-x86-64-gcc-debug:
> +  extends: .zen3p-x86-64
> +  script:
> +    - ./automation/scripts/qubes-x86-64.sh pvshim 2>&1 | tee ${LOGFILE}
> +  needs:
> +    - *x86-64-test-needs
> +    - alpine-3.18-gcc-debug
> +
>  qemu-smoke-dom0-arm64-gcc:
>    extends: .qemu-arm64
>    script:
> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> index 4b6311efffa8..ace494b938d8 100755
> --- a/automation/scripts/qubes-x86-64.sh
> +++ b/automation/scripts/qubes-x86-64.sh
> @@ -8,6 +8,7 @@ set -ex
>  #  - dom0pvh-hvm    PVH dom0, HVM domU
>  #  - pci-hvm        PV dom0,  HVM domU + PCI Passthrough
>  #  - pci-pv         PV dom0,  PV domU + PCI Passthrough
> +#  - pvshim         PV dom0,  PVSHIM domU
>  #  - s3             PV dom0,  S3 suspend/resume
>  test_variant=$1
>  
> @@ -20,8 +21,8 @@ domU_vif="'bridge=xenbr0',"
>  domU_extra_cfg=
>  
>  case "${test_variant}" in
> -    ### test: smoke test & smoke test PVH & smoke test HVM
> -    ""|"dom0pvh"|"dom0pvh-hvm")
> +    ### test: smoke test & smoke test PVH & smoke test HVM & smoke test PVSHIM
> +    ""|"dom0pvh"|"dom0pvh-hvm"|"pvshim")
>          passed="ping test passed"
>          domU_check="
>  ifconfig eth0 192.168.0.2
> @@ -44,6 +45,11 @@ echo \"${passed}\"
>  
>          if [ "${test_variant}" = "dom0pvh-hvm" ]; then
>              domU_type="hvm"
> +        elif [ "${test_variant}" = "pvshim" ]; then
> +            domU_type="pv"
> +            domU_extra_cfg='
> +pvshim = 1
> +'
>          fi
>          ;;
>  

Bah - serves me right for some last minute refactoring.  The domain type
should be pvh for pvshim=1 to work.

New pipeline:
https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1505540810

~Andrew
Re: [PATCH 3/3] CI: Add {adl,zen3p}-pvshim-* tests
Posted by Andrew Cooper 1 month ago 
On 21/10/2024 3:41 pm, Andrew Cooper wrote:
> On 21/10/2024 3:35 pm, Andrew Cooper wrote:
>> GitlabCI has no testing of Xen's PVH entrypoint.  Fix this.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>> CC: Anthony PERARD <anthony.perard@vates.tech>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Michal Orzel <michal.orzel@amd.com>
>> CC: Doug Goldstein <cardoe@cardoe.com>
>>
>> OSSTest (which is disappearing imminently) found a pvshim bug in the
>> hyperlaunch series, and I found a second shortly after while trying to take
>> more of the series.
>>
>> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1505518838
>> ---
>>  automation/gitlab-ci/test.yaml     | 16 ++++++++++++++++
>>  automation/scripts/qubes-x86-64.sh | 10 ++++++++--
>>  2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
>> index b27c2be17487..e76a37bef32d 100644
>> --- a/automation/gitlab-ci/test.yaml
>> +++ b/automation/gitlab-ci/test.yaml
>> @@ -240,6 +240,14 @@ adl-pci-hvm-x86-64-gcc-debug:
>>      - *x86-64-test-needs
>>      - alpine-3.18-gcc-debug
>>  
>> +adl-pvshim-x86-64-gcc-debug:
>> +  extends: .adl-x86-64
>> +  script:
>> +    - ./automation/scripts/qubes-x86-64.sh pvshim 2>&1 | tee ${LOGFILE}
>> +  needs:
>> +    - *x86-64-test-needs
>> +    - alpine-3.18-gcc-debug
>> +
>>  zen3p-smoke-x86-64-gcc-debug:
>>    extends: .zen3p-x86-64
>>    script:
>> @@ -272,6 +280,14 @@ zen3p-pci-hvm-x86-64-gcc-debug:
>>      - *x86-64-test-needs
>>      - alpine-3.18-gcc-debug
>>  
>> +zen3p-pvshim-x86-64-gcc-debug:
>> +  extends: .zen3p-x86-64
>> +  script:
>> +    - ./automation/scripts/qubes-x86-64.sh pvshim 2>&1 | tee ${LOGFILE}
>> +  needs:
>> +    - *x86-64-test-needs
>> +    - alpine-3.18-gcc-debug
>> +
>>  qemu-smoke-dom0-arm64-gcc:
>>    extends: .qemu-arm64
>>    script:
>> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
>> index 4b6311efffa8..ace494b938d8 100755
>> --- a/automation/scripts/qubes-x86-64.sh
>> +++ b/automation/scripts/qubes-x86-64.sh
>> @@ -8,6 +8,7 @@ set -ex
>>  #  - dom0pvh-hvm    PVH dom0, HVM domU
>>  #  - pci-hvm        PV dom0,  HVM domU + PCI Passthrough
>>  #  - pci-pv         PV dom0,  PV domU + PCI Passthrough
>> +#  - pvshim         PV dom0,  PVSHIM domU
>>  #  - s3             PV dom0,  S3 suspend/resume
>>  test_variant=$1
>>  
>> @@ -20,8 +21,8 @@ domU_vif="'bridge=xenbr0',"
>>  domU_extra_cfg=
>>  
>>  case "${test_variant}" in
>> -    ### test: smoke test & smoke test PVH & smoke test HVM
>> -    ""|"dom0pvh"|"dom0pvh-hvm")
>> +    ### test: smoke test & smoke test PVH & smoke test HVM & smoke test PVSHIM
>> +    ""|"dom0pvh"|"dom0pvh-hvm"|"pvshim")
>>          passed="ping test passed"
>>          domU_check="
>>  ifconfig eth0 192.168.0.2
>> @@ -44,6 +45,11 @@ echo \"${passed}\"
>>  
>>          if [ "${test_variant}" = "dom0pvh-hvm" ]; then
>>              domU_type="hvm"
>> +        elif [ "${test_variant}" = "pvshim" ]; then
>> +            domU_type="pv"
>> +            domU_extra_cfg='
>> +pvshim = 1
>> +'
>>          fi
>>          ;;
>>  
> Bah - serves me right for some last minute refactoring.  The domain type
> should be pvh for pvshim=1 to work.
>
> New pipeline:
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1505540810

And from the same bit of refactoring, a mismatch between
domU_extra_{cfg,config}.  Consolidated on the latter.

https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/8143613752

~Andrew
Re: [PATCH 3/3] CI: Add {adl,zen3p}-pvshim-* tests
Posted by Stefano Stabellini 1 month ago 
On Mon, 21 Oct 2024, Andrew Cooper wrote:
> > Bah - serves me right for some last minute refactoring.  The domain type
> > should be pvh for pvshim=1 to work.
> >
> > New pipeline:
> > https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1505540810
> 
> And from the same bit of refactoring, a mismatch between
> domU_extra_{cfg,config}.  Consolidated on the latter.
> 
> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/8143613752

I reviewed the commit c80e9241209cc9ed7f66c3f45275f837ddafc21c from your
branch instead. See below.


> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index b27c2be174..e76a37bef3 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -240,6 +240,14 @@ adl-pci-hvm-x86-64-gcc-debug:
>      - *x86-64-test-needs
>      - alpine-3.18-gcc-debug
>  
> +adl-pvshim-x86-64-gcc-debug:
> +  extends: .adl-x86-64
> +  script:
> +    - ./automation/scripts/qubes-x86-64.sh pvshim 2>&1 | tee ${LOGFILE}
> +  needs:
> +    - *x86-64-test-needs
> +    - alpine-3.18-gcc-debug
> +
>  zen3p-smoke-x86-64-gcc-debug:
>    extends: .zen3p-x86-64
>    script:
> @@ -272,6 +280,14 @@ zen3p-pci-hvm-x86-64-gcc-debug:
>      - *x86-64-test-needs
>      - alpine-3.18-gcc-debug
>  
> +zen3p-pvshim-x86-64-gcc-debug:
> +  extends: .zen3p-x86-64
> +  script:
> +    - ./automation/scripts/qubes-x86-64.sh pvshim 2>&1 | tee ${LOGFILE}
> +  needs:
> +    - *x86-64-test-needs
> +    - alpine-3.18-gcc-debug
> +
>  qemu-smoke-dom0-arm64-gcc:
>    extends: .qemu-arm64
>    script:
> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> index 76fbafac84..95090eb12c 100755
> --- a/automation/scripts/qubes-x86-64.sh
> +++ b/automation/scripts/qubes-x86-64.sh
> @@ -8,6 +8,7 @@ set -ex
>  #  - dom0pvh-hvm    PVH dom0, HVM domU
>  #  - pci-hvm        PV dom0,  HVM domU + PCI Passthrough
>  #  - pci-pv         PV dom0,  PV domU + PCI Passthrough
> +#  - pvshim         PV dom0,  PVSHIM domU
>  #  - s3             PV dom0,  S3 suspend/resume
>  test_variant=$1
>  
> @@ -20,8 +21,8 @@ domU_vif="'bridge=xenbr0',"
>  domU_extra_config=
>  
>  case "${test_variant}" in
> -    ### test: smoke test & smoke test PVH & smoke test HVM
> -    ""|"dom0pvh"|"dom0pvh-hvm")
> +    ### test: smoke test & smoke test PVH & smoke test HVM & smoke test PVSHIM
> +    ""|"dom0pvh"|"dom0pvh-hvm"|"pvshim")
>          passed="ping test passed"
>          domU_check="
>  ifconfig eth0 192.168.0.2
> @@ -44,6 +45,11 @@ echo \"${passed}\"
>  
>          if [ "${test_variant}" = "dom0pvh-hvm" ]; then
>              domU_type="hvm"
> +        elif [ "${test_variant}" = "pvshim" ]; then
> +            domU_type="pvh"

This is not necessary since PVH is already the default. In theory, it is
harmless, but it caused me to do a double-take because I initially
thought I was missing something, given that PVH is expected to be the
default at this point.


> +            domU_extra_config='
> +pvshim = 1
> +'

Is there a reason this cannot be:

    domU_extra_config='pvshim = 1'

?

These are just minor cosmetics.


>          fi
>          ;;
>
Re: [PATCH 3/3] CI: Add {adl,zen3p}-pvshim-* tests
Posted by Andrew Cooper 1 month ago 
On 21/10/2024 11:28 pm, Stefano Stabellini wrote:
> On Mon, 21 Oct 2024, Andrew Cooper wrote:
>> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
>> index 76fbafac84..95090eb12c 100755
>> --- a/automation/scripts/qubes-x86-64.sh
>> +++ b/automation/scripts/qubes-x86-64.sh
>> @@ -44,6 +45,11 @@ echo \"${passed}\"
>>  
>>          if [ "${test_variant}" = "dom0pvh-hvm" ]; then
>>              domU_type="hvm"
>> +        elif [ "${test_variant}" = "pvshim" ]; then
>> +            domU_type="pvh"
> This is not necessary since PVH is already the default. In theory, it is
> harmless, but it caused me to do a double-take because I initially
> thought I was missing something, given that PVH is expected to be the
> default at this point.

That was more fallout of refactoring.

My first attempt (which worked) involved writing a whole domU_config=
block, and then I decided that was a bad thing.

Selecting PVShim without also forcing type to PVH is a little fragile if
anyone changes the domU_type default.  I think it's cleaner to keep both
together, even if it happens to be re-selecting the default.

>> +            domU_extra_config='
>> +pvshim = 1
>> +'
> Is there a reason this cannot be:
>
>     domU_extra_config='pvshim = 1'
>
> ?
>
> These are just minor cosmetics.

Again, refactoring from before pulling out domU_type.

I'm not fussed - I can shrink this to one line.

~Andrew
Re: [PATCH 3/3] CI: Add {adl,zen3p}-pvshim-* tests
Posted by Marek Marczykowski-Górecki 1 month ago 
On Tue, Oct 22, 2024 at 02:25:54PM +0100, Andrew Cooper wrote:
> On 21/10/2024 11:28 pm, Stefano Stabellini wrote:
> > On Mon, 21 Oct 2024, Andrew Cooper wrote:
> >> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> >> index 76fbafac84..95090eb12c 100755
> >> --- a/automation/scripts/qubes-x86-64.sh
> >> +++ b/automation/scripts/qubes-x86-64.sh
> >> @@ -44,6 +45,11 @@ echo \"${passed}\"
> >>  
> >>          if [ "${test_variant}" = "dom0pvh-hvm" ]; then
> >>              domU_type="hvm"
> >> +        elif [ "${test_variant}" = "pvshim" ]; then
> >> +            domU_type="pvh"
> > This is not necessary since PVH is already the default. In theory, it is
> > harmless, but it caused me to do a double-take because I initially
> > thought I was missing something, given that PVH is expected to be the
> > default at this point.
> 
> That was more fallout of refactoring.
> 
> My first attempt (which worked) involved writing a whole domU_config=
> block, and then I decided that was a bad thing.
> 
> Selecting PVShim without also forcing type to PVH is a little fragile if
> anyone changes the domU_type default.  I think it's cleaner to keep both
> together, even if it happens to be re-selecting the default.

IMO it's okay to leave re-setting the default here explicitly.

> >> +            domU_extra_config='
> >> +pvshim = 1
> >> +'
> > Is there a reason this cannot be:
> >
> >     domU_extra_config='pvshim = 1'
> >
> > ?
> >
> > These are just minor cosmetics.
> 
> Again, refactoring from before pulling out domU_type.
> 
> I'm not fussed - I can shrink this to one line.

Either is fine for me. The short one is more readable now, while the
long one is easier to extend in the future.

Anyway, for the series (c80e9241209cc9ed7f66c3f45275f837ddafc21c and
previous two):
Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.