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

Change default QEMU CPU level to qemu64 on Windows amd64 #19518

Merged

Conversation

arixmkii
Copy link
Contributor

@arixmkii arixmkii commented Aug 4, 2023

Originally max level with exceptions was used, because netavark could not work with qemu64, because there were needed SIMD instructions blocked. Now it works w/o any issues. What could have changed in between:

  • qemu64 could have been adjusted to expose more features for this basic CPU;
  • Rust compiler could have been improved to generate multiple versions of binary paths to choose the suitable most performant one available at runtime;
  • Optimization flags might have changed for netavark;
  • Some or all of the above.

This has been verified to work on:

SW versions:

  • QEMU 8.1.0-rc2 (this is a safe choice, because the QEMU support on Windows is yet to be released)
  • Podman 4.7.0-dev

All test were run with SMP 1 and SMP 4. max level in Hyper-V was always getting stuck during boot sequence (in different stages for SMP 1 and SMP 4 (SMP 1 was completing slightly more actions, but never fully booted the VM)).

I don't have any AMD machines for testing, but I expect qemu64 to be more stable for AMD as well as it should hide almost all of the specifics of the CPU in question.

No release note, as there were no releases for Windows QEMU yet.

No tests as this is only the default settings changed and no logic changes.

Does this PR introduce a user-facing change?

None

[NO NEW TESTS NEEDED]

@rhatdan
Copy link
Member

rhatdan commented Aug 5, 2023

@baude @n1hility @gbraad @ashley-cui PTAL

@gbraad
Copy link
Member

gbraad commented Aug 5, 2023

I'll try to test this before my PTO

@arixmkii
Copy link
Contributor Author

arixmkii commented Aug 8, 2023

@gbraad if you want to test it end to end you will need as well adding:

Both PRs are rebased and applies nicely to the latest code. I also have the notes on how to init machine for Windows and QEMU (which additional parameters in cmd line are needed comparing to WSL or QEMU on other platforms) available here.

@arixmkii
Copy link
Contributor Author

Do we have any updates on this?

@arixmkii arixmkii force-pushed the qemu_win_settings_qemu64 branch from 70ee148 to 8293a49 Compare September 22, 2023 13:53
@flouthoc
Copy link
Collaborator

I'll try to test this before my PTO

@gbraad Friendly ping.

@arixmkii
Copy link
Contributor Author

arixmkii commented Sep 28, 2023

I have it in a branch with 2 other commits https://github.com/arixmkii/podman/tree/qcw-2023-09-30-3 This branch then can be tested with machine e2e tests (documentation update included in the topmost commit)

In isolation the change could be tests without rebuilds just following instructions on how to run Podman with QEMU on Windows from here https://github.com/containers/podman/blob/36f8e78d718e1a47727dce798e9bbe4881c53099/docs/tutorials/qemu-remote-tutorial.md#qemu-1 One will need to adjust -cpu part.

@rhatdan
Copy link
Member

rhatdan commented Oct 2, 2023

@gbraad @baude @ashley-cui PTAL

@n1hility
Copy link
Member

n1hility commented Oct 5, 2023

LGTM

@arixmkii arixmkii force-pushed the qemu_win_settings_qemu64 branch from 8293a49 to fd084d5 Compare October 9, 2023 18:08
@packit-as-a-service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lsm5
Copy link
Member

lsm5 commented Oct 10, 2023

Ephemeral COPR build failed. @containers/packit-build please check.

ignore eln for now

@rhatdan
Copy link
Member

rhatdan commented Oct 10, 2023

@baude waiting on you.

@TomSweeneyRedHat
Copy link
Member

Re-Ping @baude

@arixmkii arixmkii force-pushed the qemu_win_settings_qemu64 branch from fd084d5 to 4c249a2 Compare October 18, 2023 21:38
@baude
Copy link
Member

baude commented Dec 4, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023
@baude
Copy link
Member

baude commented Dec 4, 2023

/approve

Copy link
Contributor

openshift-ci bot commented Dec 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arixmkii, baude

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit cbb3e4d into containers:main Dec 4, 2023
99 checks passed
@arixmkii arixmkii deleted the qemu_win_settings_qemu64 branch March 1, 2024 13:54
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 31, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants