[RFC] Use guest memory collection and other refactors #5452
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
Manciukic
force-pushed
the
refactor/guest-memory-collection
branch
from
47c4116 to
c6010f6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5452 +/- ##
==========================================
- Coverage 82.78% 82.68% -0.10%
==========================================
Files 263 264 +1
Lines 27292 27456 +164
==========================================
+ Hits 22594 22703 +109
- Misses 4698 4753 +55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Manciukic
force-pushed
the
refactor/guest-memory-collection
branch
from
c6010f6 to
b847d28
Compare
ShadowCurse
reviewed
Sep 24, 2025
Copy the implementation downstream while we wait for upstream. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
This refactor enables us to add more information to the GuestMemory. To start off, I am adding KVM slot number and type of memory. So far we only support Dram (normal memory), but we will also add Hotpluggable region type in the future. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
As we will add other guest memory regions, last_addr() will not return what we actually want: the last addr of the DRAM region. Use the region type introduced in an earlier refactor to find the DRAM region and use that instead. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
The way we populate the PVH and E820 memmap is by redoing all the math about the regions. Replace that duplicated logic with an iteration over the guest memory, filtering by Dram type (as we don't want to expose other regions we will add in the future). Furthermore, clamp the first 1MB as it's reserved for the system. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
Split the actual allocation from the calculations to find the right ranges. This will be used to create the hotpluggable region in the future. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
Manciukic
force-pushed
the
refactor/guest-memory-collection
branch
from
b847d28 to
c742637
Compare
Move the function to discard guest memory under the GuestMemoryExtension trait. This is done so that it could be reused in other parts of the code. Furthermore, this also changes it to use try_access, which is the correct way to access a range of guest memory as it deals with the possibility of it spanning multiple regions. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
This operation is completely useless after we mmap-ed anonymous private zero pages on top of the file mapping. Also, add a TODO for memfd-backed memory, as it's currently not supported using madvise. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
This flag was used for determining how to discard memory, but we previously changed that logic to look at the region mmap and perform the right action depending on that. This commit removes all references to this flag. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
Manciukic
force-pushed
the
refactor/guest-memory-collection
branch
from
c742637 to
fc5c4b0
Compare
ShadowCurse
reviewed
Sep 26, 2025
Comment on lines
+377
to
+378
| if self.common.next_slot > self.common.max_memslots { | ||
| Err(VmError::NotEnoughMemorySlots(self.common.max_memslots)) |
There was a problem hiding this comment.
if check fails the self.common.next_slot should be decremented by num_slots.
Alternatevely:
let last_slot = self.common.next_slot + num_slots;
if self.common.max_memslots < last_slot { return Err(...) }
let new_slot = self.common_next_slot;
self.common_next_slot = last_slot;
return Ok(new_slot);There was a problem hiding this comment.
Yeah, I can keep the order of check/increase as it was before. In any case this error is fatal and leads to the microvm not starting.
| let start_addr = dram_region | ||
| .start_addr() | ||
| .unchecked_add(super::layout::SYSTEM_MEM_SIZE); | ||
| let mem_size = dram_region.last_addr().unchecked_offset_from(start_addr) + 1; |
There was a problem hiding this comment.
shouldn't this be simply dram_region.size()?
And start_address technically should be a DRAM_MEM_START always as well.
There was a problem hiding this comment.
No, start_addr is the start of the non-system memory, which is a SYSTEM_MEM_SIZE offset from the start of the region.
I could change it to:
let mem_size = dram_region.len() - SYSTEM_MEM_SIZE;
which is actually simpler.
src/vmm/src/vstate/memory.rs
Outdated
Comment on lines
130
to
135
| libc::mmap( | ||
| phys_address.cast(), | ||
| len, | ||
| libc::PROT_READ | libc::PROT_WRITE, | ||
| libc::MAP_FIXED | libc::MAP_ANONYMOUS | libc::MAP_PRIVATE, | ||
| -1, |
There was a problem hiding this comment.
Can munmap be used here to ensure range is not usable?
There was a problem hiding this comment.
this is for the balloon device, so we want to be able to keep accessing the memory. Since when restoring we have a private map of a file, we just patch an anon mapping on top of it to deallocate the previous mapping and any memory associated with it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
vm-memory) to store KVM slotremove_rangeutil function to free memory from balloon utils toGuestMemoryExtensiontraitfallocate). I didn't want to increase the scope by adding a new syscall to the seccomp filter to support something we don't actually formally support.Reason
This refactors are needed for the virtio-mem branch.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.