-
Notifications
You must be signed in to change notification settings - Fork 115
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
[RFC] Add support for KVM_GET_XSAVE2
ioctls
#261
Closed
+70
−0
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Add support for
KVM_GET_XSAVE2
ioctls
Since Linux 5.17, the `kvm_xsave` struct has a flexible array member (FAM) at the end, which can be retrieved using the `KVM_GET_XSAVE2` ioctl [1]. What makes this FAM special is that the length is not stored in the header, but has to be retrieved via `KVM_CHECK_CAPABILITY(KVM_CAP_XSAVE2)`, which returns the total size of the `kvm_xsave` struct (e.g. the traditional 4096 byte header + the size of the FAM). Thus, to support `KVM_GET_XSAVE2`, we first need to check the capability on the VM fd, construct a FamStructWrapper of the correct size, and then call `KVM_GET_XSAVE2`. [1]: https://www.kernel.org/doc/html/latest/virt/kvm/api.html#kvm-get-xsave2 Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
commit 28b79530411ca55c2d48ae268faea356d41578db
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we really should get the required size internally.
VMM may check KVM caps beforehand (to fail faster). In that case, VMM can cache the XSAVE size somewhere and reuse it without calling KVM_CHECK_EXTENSIONS. For more flexibility of the VMM implementation, it could be better to keep
xsave_size()
out ofget_xsave2()
.Rather, not having
xsave_size()
in the first place might be better. In the first look, handling both cases (KVM_GET_XSAVE2 supported and not supported) looks nice. But a VMM supporting multiple kernel versions supporting KVM_GET_XSAVE and KVM_GET_XSAVE2 (like firecracker) should check the availability of the new API first any way, and then, if not available, it should fall back to KVM_GET_XSAVE. If we really want to absorb the differences of these two APIs, we should implement the fallback part internally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, yeah, I think I see what you mean. In the context of
get_xsave2
, having the fallback ofxsave_size()
to be 4096 when CAP_XSAVE2 isnt supported it fairly useless, because the KVM_GET_XSAVE2 call is gonna fail anyway. So it's kinda half-baked.The problem API wise is that we cannot let the user allocate the Xsave struct, because if they allocate a too-small one, then get_xsave2 will result in undefined behavior potentially (e.g. the kernel writing past the allocated buffer). We could mark it unsafe, but that's also not very nice imo.
And yeah, I guess if all Xsave structs are created from within kvm-ioctls, then its not even useful to expose xsave_size() as an API altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least for me, marking it
unsafe
isn't so bad.unsafe
marked functions always make me (hopefully all Rustaceans) pay my highest attention to it. Of course, safe wrapper is the best, but as being discussed in another thread, it may be impossible to wrap it safely due to the possible race condition.So my current humble suggestion is:
get_xsave2()
unsafeget_xsave2()
fail early if KVM_CAP_XSAVE2 isn't supportedcheck_extension()
get_xsave2_xsave()
which allows users to pass&mut Xsave
.get_xsave2_raw()
which allows users to pass a pointer tostruct kvm_xsave
.