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 loongarch cpu support for virt-manager #629

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

lixianglai
Copy link
Contributor

The virt-manager code is currently not perfect and lacks corresponding test cases. However, according to the suggestion, I first create a Pr for everyone to review. The following is a more detailed description:

#628

@lixianglai lixianglai force-pushed the loongarch branch 2 times, most recently from 0799bb0 to 61ed66c Compare February 28, 2024 08:15
virtinst/guest.py Outdated Show resolved Hide resolved
virtinst/domain/cpu.py Outdated Show resolved Hide resolved
virtinst/devices/disk.py Outdated Show resolved Hide resolved
virtinst/guest.py Outdated Show resolved Hide resolved
virtinst/guest.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.80%. Comparing base (eee8d09) to head (2eaa3aa).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #629   +/-   ##
=======================================
  Coverage   99.80%   99.80%           
=======================================
  Files          79       79           
  Lines       12431    12440    +9     
=======================================
+ Hits        12407    12416    +9     
  Misses         24       24           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andreabolognani
Copy link
Contributor

Oh and we're missing at least <rng> and <memballoon>. There's an allowlist deciding whether those are to be added by default, and it needs to be extended to include loongarch64 guests.

@lixianglai
Copy link
Contributor Author

Oh and we're missing at least <rng> and <memballoon>. There's an allowlist deciding whether those are to be added by default, and it needs to be extended to include loongarch64 guests.

Thanks,I'll add them in the guest.

virtinst/guest.py Outdated Show resolved Hide resolved
virtinst/guest.py Show resolved Hide resolved
tests/data/capabilities/kvm-loongarch64.xml Show resolved Hide resolved
@andreabolognani
Copy link
Contributor

@lixianglai no more comments from my side, everything looks good. Of course we want to get loongarch64 support merged into libvirt first.

@lixianglai
Copy link
Contributor Author

@lixianglai no more comments from my side, everything looks good. Of course we want to get loongarch64 support merged into libvirt first.

Thank you very much! I will be pushing a new version of libvirt patch this week.

@andreabolognani
Copy link
Contributor

@crobinso libvirt support has now been merged, so as far as I'm concerned there are no longer any blockers to go ahead with this.

@lixianglai
Copy link
Contributor Author

@crobinso Sorry to bother you, does this PR meet the entry requirements? Do I need to do anything?

@lixianglai lixianglai requested a review from crobinso April 16, 2024 09:32
tests/test_cli.py Outdated Show resolved Hide resolved
Define the judgment function of
loongarch architecture.

Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
Add adaptations for loongarch with the following features:
Default video Support
UEFI prefer
Usb tablet and usb keyboard
rng and memballoon
sound device
Usb controller

Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
Add some basic test cases for loongarch.

Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
@lixianglai
Copy link
Contributor Author

@andreabolognani @crobinso
Hi Andrea and crobinso:
In this submission, I added a new patch, which adds optional bios type firmware for loongarch64 architecture, which essentially uses the -bios parameter to load QEMU_EFI.fd to achieve the purpose of using virt-manager internal snapshot.

I am not sure if this is within the norm, so I look forward to your comments and responses.
virt-manager-bios

@andreabolognani
Copy link
Contributor

@lixianglai thanks for updating the PR!

In this submission, I added a new patch, which adds optional bios type firmware for loongarch64 architecture, which essentially uses the -bios parameter to load QEMU_EFI.fd to achieve the purpose of using virt-manager internal snapshot.

This doesn't look right at all.

While it's true that the edk2 binary can be loaded using QEMU's -bios option, the firmware descriptor specification very clearly states that the "bios" interface type is intended for "Traditional x86 BIOS interface. For example, firmware built from the SeaBIOS project", and that's the meaning both libvirt and virt-manager assign to it.

It's unfortunate that virt-manger currently doesn't support internal snapshots of running VMs when using UEFI, but hacking around this limitation by having incorrect information presented to the user and passed to libvirt is not an acceptable way to address that. Please drop the new commit.

Everything else looks good to me.

Related to the overall effort, when you have a minute can you please weight in on this thread with information about support for TPM on loongarch64? That'd be very helpful. Thanks in advance!

@lixianglai
Copy link
Contributor Author

@lixianglai thanks for updating the PR!

In this submission, I added a new patch, which adds optional bios type firmware for loongarch64 architecture, which essentially uses the -bios parameter to load QEMU_EFI.fd to achieve the purpose of using virt-manager internal snapshot.

This doesn't look right at all.

While it's true that the edk2 binary can be loaded using QEMU's -bios option, the firmware descriptor specification very clearly states that the "bios" interface type is intended for "Traditional x86 BIOS interface. For example, firmware built from the SeaBIOS project", and that's the meaning both libvirt and virt-manager assign to it.

It's unfortunate that virt-manger currently doesn't support internal snapshots of running VMs when using UEFI, but hacking around this limitation by having incorrect information presented to the user and passed to libvirt is not an acceptable way to address that. Please drop the new commit.

Oh, that's a shame. Does the community have any plans or ideas to solve the problem of not being able to create snapshots when using UEFI firmware? Since the variable storage is configured read-only when flashmode is stateless, there is no write security problem in this mode. Can we allow the creation of internal snapshots in this mode?

Everything else looks good to me.

Related to the overall effort, when you have a minute can you please weight in on this thread with information about support for TPM on loongarch64? That'd be very helpful. Thanks in advance!

Ok,I will reply to it as soon as possible。

@phrdina
Copy link
Member

phrdina commented Jun 18, 2024

Just a note about snapshots to clarify current state.

When UEFI is used the issue with internal snapshots for running VM is that QEMU will try to store memory state in the varstore as that is the first storage where it can be saved. That's the reason why libvirt and because of that virt-manager don't support creating internal snapshot of running VM.

Upstream virt-manager (not part of any release yet) supports creating external snapshots that work with running VM with UEFI and that's what upstream virt-manager will prefer now if supported by libvirt.

@andreabolognani
Copy link
Contributor

@phrdina thanks for providing this update! It's indeed amazing news. I've just tried it with my loongarch64 VM and it worked flawlessly :)

Do you think you could merge this PR now? AFAICT all of @crobinso's original feedback has been addressed, so I don't think it's necessary to wait for him to look at it again if me and you both are happy with the current version.

@phrdina
Copy link
Member

phrdina commented Jun 18, 2024

Will look at it today and if I don't see anything will merge it as well.

@phrdina phrdina merged commit 9ede5d2 into virt-manager:main Jun 18, 2024
8 checks passed
@andreabolognani
Copy link
Contributor

Thanks @phrdina! And thanks @lixianglai for your patience and excellent work both here and on the libvirt side :)

@lixianglai
Copy link
Contributor Author

@andreabolognani @phrdina @crobinso Thank you very much for your help in reviewing the code and giving your opinions, and also thank you for your recognition of my work !!!

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.

4 participants