Fix live migration when AMX is enabled #46
Merged
Conversation
This file contains hidden or 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
olivereanderson
force-pushed
the
fix-xstate-amx
branch
2 times, most recently
from
ea1dd7a to
159bedc
Compare
phip1611
requested changes
Dec 1, 2025
Member
phip1611
left a comment
phip1611
left a comment
There was a problem hiding this comment.
thanks for working on this and fixing the issue :) I left some remarks
olivereanderson
force-pushed
the
fix-xstate-amx
branch
8 times, most recently
from
6c344af to
bbd008d
Compare
olivereanderson
force-pushed
the
fix-xstate-amx
branch
from
bbd008d to
dd47163
Compare
olivereanderson
commented
Dec 2, 2025
olivereanderson
force-pushed
the
fix-xstate-amx
branch
from
dd47163 to
4e60410
Compare
olivereanderson
force-pushed
the
fix-xstate-amx
branch
2 times, most recently
from
1d70fb6 to
7ba61cc
Compare
phip1611
requested changes
Dec 2, 2025
Member
phip1611
left a comment
phip1611
left a comment
There was a problem hiding this comment.
Very good work! I think this is a clear improvement! Thanks for working on this. I left a few remarks
In order for guests to use AMX it is necessary to ask the kernel to enable the related state components for guests. While cloud hypervisor already does this, we would prefer to extract the logic into a stand alone (reusable) function. In this commit we only introduce the error type that will later be part of the enable_amx_state_components function's signature. Signed-Off-by: Oliver Anderson <[email protected]> On-behalf-of: SAP [email protected]
olivereanderson
force-pushed
the
fix-xstate-amx
branch
from
7ba61cc to
115cfae
Compare
phip1611
reviewed
Dec 2, 2025
phip1611
approved these changes
Dec 2, 2025
Member
phip1611
left a comment
phip1611
left a comment
There was a problem hiding this comment.
LGTM now, thanks! I left one remark but I already approve it
We introduce a static enable_amx_state_components method on the XSaveState struct that will be used in a follow up commit. We will also extend the logic of what this method does in the near future. Signed-Off-by: Oliver Anderson <[email protected]> On-behalf-of: SAP [email protected]
We use the central amx_state_components enabling function in the CpuManager constructor. This way we can make changes to the AMX related state components functionality without needing to update the CpuManager's constructor. Signed-Off-by: Oliver Anderson <[email protected]> On-behalf-of: SAP <[email protected]>
We will need to query KVM for the size of the xsave struct in a follow up commit. This commit introduces the necessary method on the hypervisor trait to do that. Signed-Off-by: Oliver Anderson <[email protected]> On-behalf-of: SAP [email protected]
AMX requires dynamically enabling certain large state components which leads to an increase in the size of kvm_xsave. This was not taken into account by Cloud hypervisor until now. We solve this by refactoring `XSaveState` to directly wrap `kvm::Xsave` and always ensure (via a OnceLocked static variable) that all operations on the wrapped xsave state obtain an instance of the right size. Signed-Off-by: Oliver Anderson <[email protected]> On-behalf-of: SAP [email protected]
olivereanderson
force-pushed
the
fix-xstate-amx
branch
from
115cfae to
1a214ba
Compare
tpressure
approved these changes
Dec 2, 2025
This was referenced Dec 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Closes cloud-hypervisor#5800
Variable length Xsave state
When XSAVE state components are dynamically enabled, the de-facto length of
kvm_xsavechanges and this needs to be accounted for. I do this by updating the pre-existingXSaveStatestruct to wrap the FAM-structXsavefrom thekvm_bindingscrate directly and I track the FAM length in a Oncelocked static variable.How this relates to the upcoming CPU profiles feature
This fix is necessary to properly test the upcoming CPU Profiles feature, but the
XSaveState::enable_amx_state_componentsmethod introduced here will also be called during CPU profile generation and also prior to checking CPU compatibility when receiving a live migration command.Risks
There are a few things we should keep in mind with regards to this fix:
kvm_bindingscrate is sound. I am not entirely convinced that this will be the case in the eyes of the Rust abstract machine, but I think we can investigate that more in the future.cargo nextestruns each test in a separate process hence as long as that is used for running the unit/integration tests this should not be a problem.XsaveStatewill now heap allocate whenever AMX is enabled. Each initialization will require roughly 1KB of memory on the heap. We have however determined that this will never be called in any hot paths.How this PR has been tested
This was tested by running an AMX workload (loading and mutiplying some matrices, then printing the result in a loop)
inside a guest on a Sapphire rapids server and then migrating it to a Granite rapids server. The migration was successful and the program kept running.