diff mbox series

ci: add yet another HW runner

Message ID 20250314030628.96166-1-marmarek@invisiblethingslab.com (mailing list archive)
State New
Headers show
Series ci: add yet another HW runner | expand

Commit Message

Marek Marczykowski-Górecki March 14, 2025, 3:06 a.m. UTC
This is AMD Zen2 (Ryzen 5 4500U specifically), in a HP Probook 445 G7.

This one has working S3, so add a test for it here.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

The suspend test added here currently fails on staging[1], but passes on
staging-4.19[2]. So the regression wants fixing before committing this
patch.
For this to work, the runner needs to be added to relevant repositories
(especially hardware/xen one). Somebody with appropriate access need to
go to Settings->CI/CD->Runners and click "enable for this project" on
hal9001 runner.

[1] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408437140
[2] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408943441
---
 automation/gitlab-ci/test.yaml | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Stefano Stabellini March 14, 2025, 9:19 p.m. UTC | #1
On Fri, 14 Mar 2025, Marek Marczykowski-Górecki wrote:
> This is AMD Zen2 (Ryzen 5 4500U specifically), in a HP Probook 445 G7.
> 
> This one has working S3, so add a test for it here.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> The suspend test added here currently fails on staging[1], but passes on
> staging-4.19[2]. So the regression wants fixing before committing this
> patch.

We could commit the patch now without the s3 test.

I don't know what the x86 maintainers think about fixing the suspend
bug, but one idea would be to run a bisection between 4.20 and 4.19.


> For this to work, the runner needs to be added to relevant repositories
> (especially hardware/xen one). Somebody with appropriate access need to
> go to Settings->CI/CD->Runners and click "enable for this project" on
> hal9001 runner.

I did that now


> [1] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408437140
> [2] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408943441
> ---
>  automation/gitlab-ci/test.yaml | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index 9530e36e9aaa..6b8e1b830e3d 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -155,6 +155,16 @@
>    tags:
>      - qubes-hw2
>  
> +.zen2-x86-64:
> +  # it's really similar to the above
> +  extends: .adl-x86-64
> +  variables:
> +    PCIDEV: "01:00.0"
> +    PCIDEV_INTR: "MSI-X"
> +    CONSOLE_OPTS: "console=com1 com1=115200,8n1,pci,msi,04:00.0"
> +  tags:
> +    - qubes-hw1
> +
>  .zen3p-x86-64:
>    # it's really similar to the above
>    extends: .adl-x86-64
> @@ -301,6 +311,22 @@ adl-tools-tests-pvh-x86-64-gcc-debug:
>      - *x86-64-test-needs
>      - alpine-3.18-gcc-debug
>  
> +zen2-smoke-x86-64-gcc-debug:
> +  extends: .zen2-x86-64
> +  script:
> +    - ./automation/scripts/qubes-x86-64.sh 2>&1 | tee ${LOGFILE}
> +  needs:
> +    - *x86-64-test-needs
> +    - alpine-3.18-gcc-debug
> +
> +zen2-suspend-x86-64-gcc-debug:
> +  extends: .zen2-x86-64
> +  script:
> +    - ./automation/scripts/qubes-x86-64.sh s3 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:
> -- 
> 2.48.1
>
Marek Marczykowski-Górecki March 14, 2025, 10:23 p.m. UTC | #2
On Fri, Mar 14, 2025 at 02:19:19PM -0700, Stefano Stabellini wrote:
> On Fri, 14 Mar 2025, Marek Marczykowski-Górecki wrote:
> > This is AMD Zen2 (Ryzen 5 4500U specifically), in a HP Probook 445 G7.
> > 
> > This one has working S3, so add a test for it here.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > 
> > The suspend test added here currently fails on staging[1], but passes on
> > staging-4.19[2]. So the regression wants fixing before committing this
> > patch.
> >
> > [1] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408437140
> > [2] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408943441
> 
> We could commit the patch now without the s3 test.
> 
> I don't know what the x86 maintainers think about fixing the suspend
> bug, but one idea would be to run a bisection between 4.20 and 4.19.

I'm on it already, but it's annoying. Lets convert this thread to
discussion about the issue:

So, I bisected it between staging-4.19 and master. The breakage is
somewhere between (inclusive):
eb21ce14d709 x86/boot: Rewrite EFI/MBI2 code partly in C
and
47990ecef286 x86/boot: Improve MBI2 structure check

But, the first one breaks booting on this system and it remains broken
until the second commit (or its parent) - at which point S3 is already
broken. So, there is a range of 71 commits that may be responsible...

But then, based on a matrix chat and Jan's observation I've tried
reverting f75780d26b2f "xen: move per-cpu area management into common
code" just on top of 47990ecef286, and that fixed suspend.
Applying "xen/percpu: don't initialize percpu on resume" on top of
47990ecef286 fixes suspend too.
But applying it on top of master
(91772f8420dfa2fcfe4db68480c216db5b79c512 specifically) does not fix it,
but the failure mode is different than without the patch - system resets
on S3 resume, with no crash message on the serial console (even with
sync_console), instead of hanging.
And one more data point: reverting f75780d26b2f on top of master is the
same as applying "xen/percpu: don't initialize percpu on resume" on
master - system reset on S3 resume.
So, it looks like there are more issues...
Marek Marczykowski-Górecki March 14, 2025, 11:53 p.m. UTC | #3
On Fri, Mar 14, 2025 at 11:23:28PM +0100, Marek Marczykowski-Górecki wrote:
> On Fri, Mar 14, 2025 at 02:19:19PM -0700, Stefano Stabellini wrote:
> > On Fri, 14 Mar 2025, Marek Marczykowski-Górecki wrote:
> > > This is AMD Zen2 (Ryzen 5 4500U specifically), in a HP Probook 445 G7.
> > > 
> > > This one has working S3, so add a test for it here.
> > > 
> > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > ---
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > 
> > > The suspend test added here currently fails on staging[1], but passes on
> > > staging-4.19[2]. So the regression wants fixing before committing this
> > > patch.
> > >
> > > [1] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408437140
> > > [2] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408943441
> > 
> > We could commit the patch now without the s3 test.
> > 
> > I don't know what the x86 maintainers think about fixing the suspend
> > bug, but one idea would be to run a bisection between 4.20 and 4.19.
> 
> I'm on it already, but it's annoying. Lets convert this thread to
> discussion about the issue:
> 
> So, I bisected it between staging-4.19 and master. The breakage is
> somewhere between (inclusive):
> eb21ce14d709 x86/boot: Rewrite EFI/MBI2 code partly in C
> and
> 47990ecef286 x86/boot: Improve MBI2 structure check
> 
> But, the first one breaks booting on this system and it remains broken
> until the second commit (or its parent) - at which point S3 is already
> broken. So, there is a range of 71 commits that may be responsible...
> 
> But then, based on a matrix chat and Jan's observation I've tried
> reverting f75780d26b2f "xen: move per-cpu area management into common
> code" just on top of 47990ecef286, and that fixed suspend.
> Applying "xen/percpu: don't initialize percpu on resume" on top of
> 47990ecef286 fixes suspend too.
> But applying it on top of master
> (91772f8420dfa2fcfe4db68480c216db5b79c512 specifically) does not fix it,
> but the failure mode is different than without the patch - system resets
> on S3 resume, with no crash message on the serial console (even with
> sync_console), instead of hanging.
> And one more data point: reverting f75780d26b2f on top of master is the
> same as applying "xen/percpu: don't initialize percpu on resume" on
> master - system reset on S3 resume.
> So, it looks like there are more issues...

Another bisection round and I have the second culprit:

    8e60d47cf011 x86/iommu: avoid MSI address and data writes if IRT index hasn't changed

With master+"xen/percpu: don't initialize percpu on resume"+revert of
8e60d47cf011 suspend works again on this AMD system.
Andrew Cooper March 15, 2025, 12:02 a.m. UTC | #4
On 14/03/2025 11:53 pm, Marek Marczykowski-Górecki wrote:
> On Fri, Mar 14, 2025 at 11:23:28PM +0100, Marek Marczykowski-Górecki wrote:
>> On Fri, Mar 14, 2025 at 02:19:19PM -0700, Stefano Stabellini wrote:
>>> On Fri, 14 Mar 2025, Marek Marczykowski-Górecki wrote:
>>>> This is AMD Zen2 (Ryzen 5 4500U specifically), in a HP Probook 445 G7.
>>>>
>>>> This one has working S3, so add a test for it here.
>>>>
>>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>>> ---
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>
>>>> The suspend test added here currently fails on staging[1], but passes on
>>>> staging-4.19[2]. So the regression wants fixing before committing this
>>>> patch.
>>>>
>>>> [1] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408437140
>>>> [2] https://gitlab.com/xen-project/people/marmarek/xen/-/jobs/9408943441
>>> We could commit the patch now without the s3 test.
>>>
>>> I don't know what the x86 maintainers think about fixing the suspend
>>> bug, but one idea would be to run a bisection between 4.20 and 4.19.
>> I'm on it already, but it's annoying. Lets convert this thread to
>> discussion about the issue:
>>
>> So, I bisected it between staging-4.19 and master. The breakage is
>> somewhere between (inclusive):
>> eb21ce14d709 x86/boot: Rewrite EFI/MBI2 code partly in C
>> and
>> 47990ecef286 x86/boot: Improve MBI2 structure check
>>
>> But, the first one breaks booting on this system and it remains broken
>> until the second commit (or its parent) - at which point S3 is already
>> broken. So, there is a range of 71 commits that may be responsible...
>>
>> But then, based on a matrix chat and Jan's observation I've tried
>> reverting f75780d26b2f "xen: move per-cpu area management into common
>> code" just on top of 47990ecef286, and that fixed suspend.
>> Applying "xen/percpu: don't initialize percpu on resume" on top of
>> 47990ecef286 fixes suspend too.
>> But applying it on top of master
>> (91772f8420dfa2fcfe4db68480c216db5b79c512 specifically) does not fix it,
>> but the failure mode is different than without the patch - system resets
>> on S3 resume, with no crash message on the serial console (even with
>> sync_console), instead of hanging.
>> And one more data point: reverting f75780d26b2f on top of master is the
>> same as applying "xen/percpu: don't initialize percpu on resume" on
>> master - system reset on S3 resume.
>> So, it looks like there are more issues...
> Another bisection round and I have the second culprit:
>
>     8e60d47cf011 x86/iommu: avoid MSI address and data writes if IRT index hasn't changed
>
> With master+"xen/percpu: don't initialize percpu on resume"+revert of
> 8e60d47cf011 suspend works again on this AMD system.

That's not surprising in the slightest.

Caching hardware values in Xen isn't safe across S3, which QubesOS has
found time and time again, and for which we still have outstanding bugs.

S3 turns most of the system off.  RAM gets preserved, but devices and
plenty of internal registers don't.

~Andrew
diff mbox series

Patch

diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 9530e36e9aaa..6b8e1b830e3d 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -155,6 +155,16 @@ 
   tags:
     - qubes-hw2
 
+.zen2-x86-64:
+  # it's really similar to the above
+  extends: .adl-x86-64
+  variables:
+    PCIDEV: "01:00.0"
+    PCIDEV_INTR: "MSI-X"
+    CONSOLE_OPTS: "console=com1 com1=115200,8n1,pci,msi,04:00.0"
+  tags:
+    - qubes-hw1
+
 .zen3p-x86-64:
   # it's really similar to the above
   extends: .adl-x86-64
@@ -301,6 +311,22 @@  adl-tools-tests-pvh-x86-64-gcc-debug:
     - *x86-64-test-needs
     - alpine-3.18-gcc-debug
 
+zen2-smoke-x86-64-gcc-debug:
+  extends: .zen2-x86-64
+  script:
+    - ./automation/scripts/qubes-x86-64.sh 2>&1 | tee ${LOGFILE}
+  needs:
+    - *x86-64-test-needs
+    - alpine-3.18-gcc-debug
+
+zen2-suspend-x86-64-gcc-debug:
+  extends: .zen2-x86-64
+  script:
+    - ./automation/scripts/qubes-x86-64.sh s3 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: