Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add s390x support to QEMU backend #2309

Merged
merged 1 commit into from May 10, 2023

Conversation

LKHN
Copy link
Contributor

@LKHN LKHN commented Apr 28, 2023

These virtual devices added for s390x virtualization:

  • virtio-scsi as alias of virtio-scsi-ccw for SCSI controller
  • virtio-gpu as alias of virtio-gpu-ccw for VirtIO GPU Device
  • virtio-rng as alias of virtio-rng-ccw for VirtIO RNG
  • virtio-tablet as alias of virtio-tablet-ccw for input mouse interaction
  • virtio-keyboard as alias of virtio-keyboard-ccw for keyboard interaction

Disabled audio support since there is no audio virtual device available on this architecture (see qemu-kvm -device help)

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching backend/**.pm:

  • Consider running manual verification tests, especially for non-qemu backends

This is an automatically generated QA checklist based on modified files.

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

I suppose all of this is generally not very problematic because we don't use the QEMU backend for s390x test at SUSE (so it won't break existing use cases). However, tests need to be adapted (to pass again) and supposedly also extended (to cover all newly introduced code paths).

backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
@LKHN LKHN force-pushed the backend_qemu_support_s390x branch from cdd8be9 to 27c60d5 Compare May 2, 2023 17:20
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #2309 (27c60d5) into master (0688448) will decrease coverage by 0.02%.
The diff coverage is 91.66%.

❗ Current head 27c60d5 differs from pull request most recent head 5f43d43. Consider uploading reports for the commit 5f43d43 to get more accurate results

@@            Coverage Diff             @@
##           master    #2309      +/-   ##
==========================================
- Coverage   94.94%   94.93%   -0.02%     
==========================================
  Files         155      155              
  Lines       15223    15224       +1     
==========================================
- Hits        14454    14453       -1     
- Misses        769      771       +2     
Impacted Files Coverage Δ
backend/qemu.pm 99.68% <90.47%> (-0.32%) ⬇️
OpenQA/Qemu/Proc.pm 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@LKHN
Copy link
Contributor Author

LKHN commented May 3, 2023

@Martchus Thanks for the review.

I fixed the not passing tests and applied your suggestion. Thanks a lot!

Another issue that I tried to extend the tests with asserting QEMU parameters for s390x. However it fails due the mixed QEMU parameters from the previous tests. How can I generate the QEMU parameters which are use only variables I set for s390x?

Test - LKHN@8680ff6#diff-2c6d71750f92f3631c01e08b2695ed0a936acb082240f62d5c6162ddb5c04ce7
Error - https://github.com/LKHN/os-autoinst/actions/runs/4874753836/jobs/8696080601

@Martchus
Copy link
Contributor

Martchus commented May 3, 2023

Likely by just setting bmwqemu::vars{ARCH} = 's390x'; before invoking the function you need to test. You can also delete keys/values from that global hash map if they lead to cases you don't want to test.

@LKHN LKHN force-pushed the backend_qemu_support_s390x branch from 27c60d5 to 16e5a31 Compare May 5, 2023 15:11
@LKHN
Copy link
Contributor Author

LKHN commented May 5, 2023

It looks like, it passes now but no response yet from the codecov -https://app.codecov.io/gh/os-autoinst/os-autoinst/commit/16e5a31424afd5f4dbfa2845c7be237d21aa9ad0

I tried to extend the unit tests to cover the testing of these QEMU options:

When: ARCH => 's390x', QEMU_VIDEO_DEVICE => 'virtio-gpu', OFW => 0

  • EDID is present for virtio-gpu
  • -boot option not present.
  • virtio-keyboard is present.
  • no audio options present.
  • isa-fdc.fdtypeA=none is not present.
  • virtio-rng is present.
  • virtio-tablet is present.

What is left, is the testing of virtio-scsi as SCSICONTROLLER Which comes from OpenQA/Qemu/Proc.pm.

@Martchus @okurz Could tell me if this approach of the implementation tests is correct? If yes how can I test the SCSICONTROLLER too?

OpenQA/Qemu/Proc.pm Outdated Show resolved Hide resolved
@Martchus
Copy link
Contributor

Martchus commented May 8, 2023

but no response yet from the codecov

The CI check shows your diff is fully covered. I think that's good enough.

These virtual devices added for s390x virtualization:

- virtio-scsi as alias of virtio-scsi-ccw for SCSI controller
- virtio-gpu as alias of virtio-gpu-ccw for VirtIO GPU Device
- virtio-rng as alias of virtio-rng-ccw for VirtIO RNG
- virtio-tablet as alias of virtio-tablet-ccw for input mouse
  interaction
- virtio-keyboard as alias of virtio-keyboard-ccw for keyboard
  interaction

Disabled audio support since there is no audio virtual device available
on this architecture (see qemu-kvm -device help)

Signed-off-by: Elkhan Mammadli <elkhan.mammadli@protonmail.com>
@LKHN LKHN force-pushed the backend_qemu_support_s390x branch from 16e5a31 to 5f43d43 Compare May 8, 2023 11:36
@LKHN
Copy link
Contributor Author

LKHN commented May 10, 2023

@Martchus and @okurz , Thanks for the review, please let me know if we need any further changes.

@Martchus
Copy link
Contributor

No further changes are required. The PR hasn't been merged automatically because not all OBS checks have reported back. However, https://build.opensuse.org/package/show/devel:openQA:GitHub:os-autoinst:os-autoinst:PR-2309/os-autoinst shows that all builds were successful. So I'm merging this manually.

@Martchus Martchus merged commit c2d30be into os-autoinst:master May 10, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants