On 8/5/21 10:07 AM, David Hildenbrand wrote: > On 05.08.21 09:48, Philippe Mathieu-Daudé wrote: >> On 7/30/21 10:52 AM, David Hildenbrand wrote: >>> Currently, when someone (i.e., the VM) accesses discarded parts inside a >>> RAMBlock with a RamDiscardManager managing the corresponding mapped >>> memory >>> region, postcopy will request migration of the corresponding page >>> from the >>> source. The source, however, will never answer, because it refuses to >>> migrate such pages with undefined content ("logically unplugged"): the >>> pages are never dirty, and get_queued_page() will consequently skip >>> processing these postcopy requests. >>> >>> Especially reading discarded ("logically unplugged") ranges is >>> supposed to >>> work in some setups (for example with current virtio-mem), although it >>> barely ever happens: still, not placing a page would currently stall the >>> VM, as it cannot make forward progress. >>> >>> Let's check the state via the RamDiscardManager (the state e.g., >>> of virtio-mem is migrated during precopy) and avoid sending a request >>> that will never get answered. Place a fresh zero page instead to keep >>> the VM working. This is the same behavior that would happen >>> automatically without userfaultfd being active, when accessing virtual >>> memory regions without populated pages -- "populate on demand". >>> >>> For now, there are valid cases (as documented in the virtio-mem spec) >>> where >>> a VM might read discarded memory; in the future, we will disallow that. >>> Then, we might want to handle that case differently, e.g., warning the >>> user that the VM seems to be mis-behaving. >>> >>> Signed-off-by: David Hildenbrand <da...@redhat.com> >>> --- >>> migration/postcopy-ram.c | 31 +++++++++++++++++++++++++++---- >>> migration/ram.c | 21 +++++++++++++++++++++ >>> migration/ram.h | 1 + >>> 3 files changed, 49 insertions(+), 4 deletions(-) >>> >>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c >>> index 2e9697bdd2..38cdfc09c3 100644 >>> --- a/migration/postcopy-ram.c >>> +++ b/migration/postcopy-ram.c >>> @@ -671,6 +671,29 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd, >>> return ret; >>> } >>> +static int postcopy_request_page(MigrationIncomingState *mis, >>> RAMBlock *rb, >>> + ram_addr_t start, uint64_t haddr) >>> +{ >>> + void *aligned = (void *)(uintptr_t)(haddr & >>> -qemu_ram_pagesize(rb)); >> >> void *aligned = QEMU_ALIGN_PTR_DOWN(haddr, qemu_ram_pagesize(rb))); >> > > Does not compile as haddr is not a pointer.
I suppose the typeof() fails? /* n-byte align pointer down */ #define QEMU_ALIGN_PTR_DOWN(p, n) \ ((typeof(p))QEMU_ALIGN_DOWN((uintptr_t)(p), (n))) > void *aligned = QEMU_ALIGN_PTR_DOWN((void *)haddr, qemu_ram_pagesize(rb))); > > should work. What about void *aligned = QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb))); else void *aligned = (void *)QEMU_ALIGN_DOWN(haddr, qemu_ram_pagesize(rb))); I don't mind much the style you prefer, as long as it compiles :p But these QEMU_ALIGN_DOWN() macros make the review easier than (a & -b). > I can also add a patch to adjust a similar call in > migrate_send_rp_req_pages()! > > Thanks! >