Skip to content

github/workflows: Enable test-in-svsm for NOCC#823

Merged
joergroedel merged 6 commits intococonut-svsm:mainfrom
luigix25:ci_test
Oct 16, 2025
Merged

github/workflows: Enable test-in-svsm for NOCC#823
joergroedel merged 6 commits intococonut-svsm:mainfrom
luigix25:ci_test

Conversation

@luigix25
Copy link
Collaborator

@luigix25 luigix25 commented Sep 26, 2025

Enable test-in-svsm for NOCC and run them in the CI to make sure that a PR doesn't break tests.

Suggested-by: Stefano Garzarella sgarzare@redhat.com

@luigix25 luigix25 changed the title github/workflows: Build test images github/workflows: Enable test-in-svsm for NOCC Sep 29, 2025
@luigix25
Copy link
Collaborator Author

luigix25 commented Sep 29, 2025

I modified this PR, now it's possible to run test-in-svsm for nocc.
I modified the CI so that it executes those tests, unfortunately all tests can be performed:
all tests requiring snp capabilities or mmio have to be skipped.

/cc @osteffenrh who is the author of the original script

@luigix25
Copy link
Collaborator Author

update: if some of the tests doesn't pass (and panics the kernel) the GH action will run until timeout (180s).
Investigating..

Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, this is nice, maybe it will slow down a bit our CI, but makes sense to me to run tests at least in native, hoping one day we will have SNP/TDX machines for our CI.

@stefano-garzarella
Copy link
Member

update: if some of the tests doesn't pass (and panics the kernel) the GH action will run until timeout (180s). Investigating..

@luigix25 IIRC @osteffenrh added the timeout in scripts/test-qemu-nocc-boot.sh because QEMU was never exiting with just the boot. But with test-in-svsm IIRC, QEMU will exit at the end of the tests, so maybe we don't need that script anymore. (And if we need, maybe we need to update the name)

@luigix25
Copy link
Collaborator Author

update: if some of the tests doesn't pass (and panics the kernel) the GH action will run until timeout (180s). Investigating..

@luigix25 IIRC @osteffenrh added the timeout in scripts/test-qemu-nocc-boot.sh because QEMU was never exiting with just the boot. But with test-in-svsm IIRC, QEMU will exit at the end of the tests, so maybe we don't need that script anymore. (And if we need, maybe we need to update the name)

If everything goes well, then test-in-svsm exits correctly, but if something goes wrong it panics and qemu remains open. I'm modifying Oliver's script to detect a panic.

@stefano-garzarella
Copy link
Member

update: if some of the tests doesn't pass (and panics the kernel) the GH action will run until timeout (180s). Investigating..

@luigix25 IIRC @osteffenrh added the timeout in scripts/test-qemu-nocc-boot.sh because QEMU was never exiting with just the boot. But with test-in-svsm IIRC, QEMU will exit at the end of the tests, so maybe we don't need that script anymore. (And if we need, maybe we need to update the name)

If everything goes well, then test-in-svsm exits correctly, but if something goes wrong it panics and qemu remains open. I'm modifying Oliver's script to detect a panic.

Strange, in the panic handler we have platform::halt(); (

platform::halt();
), so I'm expecting QEMU to exit on hlt, no?

@stefano-garzarella
Copy link
Member

update: if some of the tests doesn't pass (and panics the kernel) the GH action will run until timeout (180s). Investigating..

@luigix25 IIRC @osteffenrh added the timeout in scripts/test-qemu-nocc-boot.sh because QEMU was never exiting with just the boot. But with test-in-svsm IIRC, QEMU will exit at the end of the tests, so maybe we don't need that script anymore. (And if we need, maybe we need to update the name)

If everything goes well, then test-in-svsm exits correctly, but if something goes wrong it panics and qemu remains open. I'm modifying Oliver's script to detect a panic.

Strange, in the panic handler we have platform::halt(); (

platform::halt();

), so I'm expecting QEMU to exit on hlt, no?

no, so maybe we should call testing:exit(), we need to write in the QEMU_EXIT_PORT

@luigix25
Copy link
Collaborator Author

Strange, in the panic handler we have platform::halt(); (

platform::halt();

), so I'm expecting QEMU to exit on hlt, no?

no, so maybe we should call testing:exit(), we need to write in the QEMU_EXIT_PORT

Exactly, you have to write in the QEMU_EXIT_PORT. You mean calling it every time the kernel panics?

@stefano-garzarella
Copy link
Member

no, so maybe we should call testing:exit(), we need to write in the QEMU_EXIT_PORT

Exactly, you have to write in the QEMU_EXIT_PORT. You mean calling it every time the kernel panics?

IMO we should do something similar to debug_break(), so add a new function to call there, where we do what we have in kernel/src/testing.rs::exit(). (Maybe you can factor out the write to QEMU_EXIT_PORT in a pub function and call it on both exit() and panic handler.

@osteffenrh
Copy link
Collaborator

@luigix25 IIRC @osteffenrh added the timeout in scripts/test-qemu-nocc-boot.sh because QEMU was never exiting with just the boot.

Also the tests (or SVSM) might run into an infinite loop or never terminate for some other reason.

@joergroedel joergroedel self-requested a review September 30, 2025 15:08
@joergroedel joergroedel added the in-review PR is under active review and not yet approved label Sep 30, 2025
@stefano-garzarella stefano-garzarella added the needs-rebase The PR needs to be rebased to the latest upstream branch label Oct 1, 2025
@stefano-garzarella
Copy link
Member

@luigix25 we just merged #775 so I think we can now rebase and enable also virtio-drivers feature, right?

@luigix25
Copy link
Collaborator Author

luigix25 commented Oct 1, 2025

@luigix25 we just merged #775 so I think we can now rebase and enable also virtio-drivers feature, right?

Thanks! yep, I am already working on this and some minor fixes

@luigix25
Copy link
Collaborator Author

luigix25 commented Oct 1, 2025

Rebased on latest upstream, picking up MMIO native PR.

A write to the QEMU exit port is performed whenever the test exits, successfully or panic.
This is to allow QEMU to close.

@luigix25 luigix25 removed the needs-rebase The PR needs to be rebased to the latest upstream branch label Oct 1, 2025
@luigix25 luigix25 force-pushed the ci_test branch 2 times, most recently from 2e8f39d to 5b45026 Compare October 1, 2025 12:42
@luigix25
Copy link
Collaborator Author

luigix25 commented Oct 6, 2025

Addressed Stefano's comments:

  • I moved qemu_write_exit to kernel/src/testing.rs
  • Removed the stub, it is not necessary anymore.
  • Refactored qemu_write_exit: there is no need to differentiate per platform anymore.
  • Added comment explaining the QEMU value
  • Introduced a commit for QEMU exit value

@luigix25
Copy link
Collaborator Author

luigix25 commented Oct 6, 2025

  • Reordered commits according to Stefano's suggestions.
  • Reworded a commit message
  • Very minor changes to bash and yaml

Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a minor comment.

…uid`

Test `test_has_memory_encryption_info_cpuid` can't be run on `native`
platform.

Add a check to prevent its execution.

Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
`exit` function, that writes to the QEMU port, uses GHCB unconditionally
that is SNP only.

Use `SVSM_PLATFORM` abstraction to perform the io operation according
to the platform used.

Tested only on native and SNP.

Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
QEMU, when a value is written in the exit port, returns `(value << 1) | 1`.
This means that writing 0, it will return 1. This would create an ambiguity
because 1 could also mean a failure in QEMU launch (i.e. error in the
parameters).

Change success value to 0x10.

Suggested-by: Oliver Steffen <osteffen@redhat.com>
Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
…c handler

When an assert is hit during testing, it can be convenient to close
QEMU immediately. This allows test failures to be detected quickly
using specific return values.

This requires writing to the QEMU exit port.

Factor out `qemu_write_exit` from the `exit` function, allowing it to
be reused in both the panic handler and the `exit` function.

Them, add a call to `qemu_write_exit` in the panic handler to terminate
QEMU with a failure code when a panic occurs during tests.

Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
Add necessary parameters to Makefile and `test-in-svsm` script to run
tests in a nocc environment.

Usage: `TEST_ARGS=--nocc make test-in-svsm`

Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
Build and run test images in the CI to make sure that a PR doesn't break
tests.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
@luigix25
Copy link
Collaborator Author

luigix25 commented Oct 7, 2025

  • Moved the enum to the next commit
  • All commit messages are not longer than 75 columns

Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joergroedel I think we can merge this, but I'll wait for your review.

Note the CI time is not affected by this PR, since running tests is just 2 sec (see Run test-in-svsm in QEMU in https://github.com/coconut-svsm/svsm/actions/runs/18306479849/job/52124795547?pr=823)

@stefano-garzarella stefano-garzarella added ready-to-merge PR is ready for merging into main branch and removed in-review PR is under active review and not yet approved labels Oct 7, 2025
@luigix25
Copy link
Collaborator Author

luigix25 commented Oct 7, 2025

Thanks for the review!!

@joergroedel joergroedel merged commit 32c1e6f into coconut-svsm:main Oct 16, 2025
4 checks passed
@luigix25 luigix25 deleted the ci_test branch October 16, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge PR is ready for merging into main branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants