Conversation
fe6b796 to
8025741
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6204 +/- ##
==========================================
+ Coverage 83.05% 83.09% +0.03%
==========================================
Files 277 277
Lines 31481 31506 +25
==========================================
+ Hits 26147 26180 +33
+ Misses 5334 5326 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7b15947 to
ecea4ba
Compare
Manciukic
added
the
Status: Awaiting review
update_mem_table handed the backend any region that had a file descriptor. Memory restored from a snapshot file has one but is mapped MAP_PRIVATE, so the backend would map its own copy of the file: writes on either side never reach the other, and the backend's land in the snapshot file. Require MAP_SHARED as well, and name the error for what it means. The test helper mapped its file private, which is the case now rejected; it maps shared, as memfd-backed guest memory does. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
Hot-adding a vhost-user block was accepted with a 204 whenever the transport allowed it, and failed only when the guest driver signalled DRIVER_OK and activation reached update_mem_table. Guest memory is a shared memfd mapping only when a vhost-user device is configured before boot; a VM booted without one has anonymous memory, and a VM restored from a snapshot file has a private mapping. Check the same condition up front, after the transport check so a VM that cannot hotplug at all still reports that first. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
Nothing exercised hot-adding a vhost-user block after boot, in either direction. Add the accepted case -- a VM booted with a vhost-user drive gains a second one, which the guest formats and writes through the backend -- and the two rejected ones: a VM booted without any vhost-user device, and a VM restored from a snapshot memory file. Hotplug needs PCIe and ACPI in the guest, so the tests are pinned like the rest of the hotplug suite. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
Record the new hot-add restriction in the hotplug limitations and in the CHANGELOG. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
ecea4ba to
19af4ff
Compare
|
Rebased onto current |
| /// Failed to read vhost eventfd: No memory region found | ||
| VhostUserNoMemoryRegion, | ||
| /// Guest memory is not a shared file mapping, so the backend cannot map it | ||
| VhostUserMemoryNotShareable, |
There was a problem hiding this comment.
I don't believe this is a valid error for vhost-user devices. We must not create them if we don't use sharable memfd for memory.
There was a problem hiding this comment.
Right now the only way preventing that is that we can't snapshot, but in theory the code could restore it. A modified snapshot could in theory contain it and this makes the check more generic and robust for the future.
| pub fn shared_file_offset(&self) -> Option<&FileOffset> { | ||
| self.inner | ||
| .file_offset() | ||
| .filter(|_| self.inner.flags() & libc::MAP_SHARED != 0) | ||
| } |
There was a problem hiding this comment.
how about just exposing is_shared function instead? This will convert the usage to just:
assert!(region.is_shared());
let Some(_file_offset) = region.file_offset() else {
panic!("...")
}
let (mmap_handle, mmap_offset) = (_file_offset.file().as_raw_fd(), _file_offset.start());There was a problem hiding this comment.
which is longer and more complicated than the code above? no thanks. Maybe I can add a separate is_shared helper but I don't really see the point.
| def test_hotplug_after_file_restore_rejected(uvm, microvm_factory): | ||
| """A vhost-user block cannot be hot-added to a VM restored from a snapshot file. |
There was a problem hiding this comment.
what is going on with this "hot-add" business? FC supports "hotplug", so let's use this term everywhere in code and in commits.
|
|
||
| // After the transport check, so a VM that cannot hotplug at all says so | ||
| // first. Without this the request succeeds and fails only at DRIVER_OK. | ||
| if config.is_vhost_user() && !vm.vhost_user_memory_shareable() { |
There was a problem hiding this comment.
Can we just reject it unconditionally for now?
There was a problem hiding this comment.
we have a release that supports it. Sure, it's dev preview, but if it works, why should we gate it?
| use crate::vstate::memory; | ||
| use crate::vstate::memory::{GuestAddress, GuestRegionMmapExt}; | ||
|
|
||
| pub(crate) fn create_mem(file: File, regions: &[(GuestAddress, usize)]) -> GuestMemoryMmap { |
There was a problem hiding this comment.
This seems to also be used in test_update_mem_table_rejects_unshareable_memory() L783, shall we rather make create_mem have a flags argument?
| VirtioDevices::Mmio(_) => return Err(VmmActionError::PciNotEnabled), | ||
| } | ||
|
|
||
| // After the transport check, so a VM that cannot hotplug at all says so |
There was a problem hiding this comment.
classic AI comment that I'd remove
| // first. Without this the request succeeds and fails only at DRIVER_OK. | ||
| if config.is_vhost_user() && !vm.vhost_user_memory_shareable() { | ||
| return Err(VmmActionError::NotSupported( | ||
| "vhost-user hot-add requires guest memory that a backend can map shared" |
There was a problem hiding this comment.
Would a more user friendly error be something like "vhost-user can only be hotplugged when a vhost-user device was present at boot"?
|
|
||
| @pin_pci(True) | ||
| @pin_guest_kernel(ACPI_GUEST_KERNELS) | ||
| def test_hotplug_vhost_user(uvm_vhost_user_booted_ro): |
There was a problem hiding this comment.
Shall these go where the other hotplug tests are?
not4s
left a comment
not4s
left a comment
There was a problem hiding this comment.
The description says that after restoring from snapshot, the backend writes reached the snapshot memory file. Is this possible given the memory file is a read-only descriptor on the normal path?
I tried to reproduce and the writable shared mmap failed with EACCES
| vm.ssh.check_output(f"mkfs.ext4 {dev}") | ||
| vm.ssh.check_output(f"mkdir -p /tmp/scratch && mount {dev} /tmp/scratch") | ||
| vm.ssh.check_output("echo vhost_user_hotplug > /tmp/scratch/probe") | ||
| assert ( |
There was a problem hiding this comment.
nit; should we unmount and remount before cat-ting or check the backing image after unmouning? The immediate cat content can come from the guest's page cache
|
|
||
|
|
||
| @pin_pci(True) | ||
| def test_hotplug_without_memfd_rejected(uvm): |
There was a problem hiding this comment.
The RuntimeError would also accept a 500 as well, so might be better to assert 400 for the rejection tests explicitly.
4 participants
Changes
Hot-adding a vhost-user block device (
PUT /drives/{id}withsocket) now fails with 400 when the backend cannot map guest memory. That is a microVM booted without any vhost-user drive, whose memory is anonymous and has no descriptor to hand over, or one restored from a snapshot memory file, whose mapping is private, so the backend would map its own copy of the file.The check runs after the transport check, so an MMIO VM still gets
PciNotEnabled.update_mem_tableapplies the same condition, so the vhost-user handshake refuses private file mappings too; both use one helper. Integration tests cover the accepted case, a VM booted with a vhost-user drive gaining a second one and writing through it, and both rejected cases. The CHANGELOG, the hotplug docs and the vhost-user block docs record the restriction.Reason
Until now the request succeeded with 204 and the device failed later at DRIVER_OK, with nothing in the API response to connect the two. In the snapshot case nothing failed at all: guest writes never reached the backend, and the backend's writes landed in the snapshot file.
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.