Skip to content

arm64: Add support for enabling SVE in vm guests#6691

Merged
rbradford merged 1 commit intocloud-hypervisor:mainfrom
uran0sH:enable-sve
Sep 4, 2024
Merged

arm64: Add support for enabling SVE in vm guests#6691
rbradford merged 1 commit intocloud-hypervisor:mainfrom
uran0sH:enable-sve

Conversation

@uran0sH
Copy link
Contributor

@uran0sH uran0sH commented Aug 24, 2024 •
edited
Loading

This change then adds the first --cpus features option sve that when passed will enable SVE usage for guests (needs a 5.2+ kernel) or exits with failure.

Fix: #6678

@uran0sH uran0sH requested a review from a team as a code owner August 24, 2024 14:42
@uran0sH uran0sH force-pushed the enable-sve branch 2 times, most recently from bff2602 to 47774b9 Compare August 24, 2024 14:44
@thomasbarrett
Copy link
Contributor

thomasbarrett commented Aug 25, 2024

Nice! Thanks for working on this @uran0sH. The changes look pretty straight-forward. I believe that the arm CI runner is currently down (we are working on getting a replacement). I will test this out on my machine for now.

@uran0sH uran0sH force-pushed the enable-sve branch 2 times, most recently from 6b70e0a to 4fec8fe Compare August 25, 2024 07:36
@uran0sH uran0sH changed the title Add support for enabling SVE in vm guests arm64: Add support for enabling SVE in vm guests Aug 26, 2024
@uran0sH
Copy link
Contributor Author

uran0sH commented Aug 30, 2024 •
edited
Loading

Nice! Thanks for working on this @uran0sH. The changes look pretty straight-forward. I believe that the arm CI runner is currently down (we are working on getting a replacement). I will test this out on my machine for now.

I submit a newer version. Maybe you can use the latest version to test. Thanks

@liuw liuw requested a review from thomasbarrett August 30, 2024 17:28
@thomasbarrett
Copy link
Contributor

thomasbarrett commented Aug 31, 2024 •
edited
Loading

I did some testing on my own machine and can confirm that this works on a arm machine with sve2 support:

sudo cloud-hypervisor \
--serial tty \
--console pty \
--cpus boot=144,features=sve \
--memory size=32G,hugepages=on,hugepage_size=1G \
--kernel /var/lib/cloud-hypervisor/hypervisor-fw \
--disk path=/var/lib/cloud-hypervisor/jammy-server-cloudimg-arm64.raw

Before enabling this feature:

    Flags:                fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fph
                          p asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 a
                          simddp sha512 asimdfhm dit uscat ilrcpc flagm ssbs sb 
                          dcpodp flagm2 frint i8mm bf16 dgh rng bti

After enabling this feature:

    Flags:                fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fph
                          p asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 a
                          simddp sha512 sve asimdfhm dit uscat ilrcpc flagm ssbs
                           sb dcpodp sve2 sveaes svepmull svebitperm svesha3 fla
                          gm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti

The net new flags added with this feature are:

sve, sve2, sveaes, svepmull, svebitperm, svesha3, svei8mm, svebf16

I also ran some arm sve benchmarks that I found to run to actually use the new instruction set.

thomasbarrett
thomasbarrett approved these changes Aug 31, 2024
View reviewed changes
Copy link
Contributor

@thomasbarrett thomasbarrett left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @uran0sH!

rbradford
rbradford reviewed Aug 31, 2024
View reviewed changes
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

Thanks for the change - just some one request about rearranging the check logic.

A more general question - is there any cost to always enabling SVE if the host supports it? - that would mean we could do away with all the config side logic. In the CH project we generally aim to have as few controls as possible and aim to to the best by default.

@thomasbarrett Thanks for validating.

rbradford
rbradford approved these changes Sep 1, 2024
View reviewed changes
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

Changes look good - thank you! My question still remains - is there any harm with automatically enabling SVE (and removing the config logic) if the host supports it?

@uran0sH
Copy link
Contributor Author

uran0sH commented Sep 2, 2024

Changes look good - thank you! My question still remains - is there any harm with automatically enabling SVE (and removing the config logic) if the host supports it?

I think that there is no harm with automatically enabling SVE. What do you think about it? @thomasbarrett

@thomasbarrett
Copy link
Contributor

thomasbarrett commented Sep 2, 2024 •
edited
Loading

Changes look good - thank you! My question still remains - is there any harm with automatically enabling SVE (and removing the config logic) if the host supports it?

I think that there is no harm with automatically enabling SVE. What do you think about it? @thomasbarrett

I agree that it makes sense / is safe to automatically enable.

liuw
liuw reviewed Sep 2, 2024
View reviewed changes
vmm/src/cpu.rs
&& sve_supported
&& vm
.as_any()
.downcast_ref::<hypervisor::kvm::KvmVm>()
Copy link
Member

@liuw liuw Sep 2, 2024

Choose a reason for hiding this comment

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

@jinankjain I think there is quite a bit of KVM-ism in this aarch64 code path. I don't think we want to block the progress on the KVM front though. We can refactor the code when our support is added.

Copy link
Member

@jinankjain jinankjain Sep 3, 2024

Choose a reason for hiding this comment

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

Yes this is already cleaned up in my tree for ARM support on MSHV. I just need to post those patches.

Copy link
Member

@rbradford rbradford Sep 3, 2024

Choose a reason for hiding this comment

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

If SVE is enabled where available by default then this can be moved to the hypervisor crate.

@uran0sH
Copy link
Contributor Author

uran0sH commented Sep 3, 2024 •
edited by rbradford
Loading

@rbradford But if users want to close this feature, how should we do? And it seems that amx also can be enabled automatically. Maybe we can merge this pr first, and discuss it in a new issue?

@rbradford
Copy link
Member

rbradford commented Sep 3, 2024

@rbradford But if users want to close this feature, how should we do? And it seems that amx also can be enabled automatically. Maybe we can merge this pr first, and discuss it in a new issue?

Since there is no downsides to always enabling SVE I would like to turn it on by default. I don't think it makes sense to draw parallels with AMX.

@uran0sH
vmm: Add support for enabling SVE in vm guests
b4f8d08 
This change enables SVE automatically if the host support SVE/SVE2.

Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
@rbradford rbradford added this pull request to the merge queue Sep 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 3, 2024
@rbradford rbradford merged commit d2a364c into cloud-hypervisor:main Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liuw liuw liuw left review comments

@jinankjain jinankjain jinankjain left review comments

@rbradford rbradford rbradford approved these changes

+1 more reviewer

@thomasbarrett thomasbarrett thomasbarrett approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Cloud Hypervisor Roadmap
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

arm64: enable sve support

5 participants

@uran0sH @thomasbarrett @rbradford @liuw @jinankjain