On Mon, Sep 11, 2023 at 03:48:38PM -0400, Peter Xu wrote: > On Sat, Sep 09, 2023 at 03:57:44PM +0100, Joao Martins wrote: > > > Should I continue to treat them as zero pages written with > > > save_zero_page_to_file ? > > > > MCE had already been forward to the guest, so guest is supposed to not be > > using > > the page (nor rely on its contents). Hence destination ought to just see a > > zero > > page. So what you said seems like the best course of action. > > > > > Or should I consider the case of an ongoing compression > > > use and create a new code compressing an empty page with > > > save_compress_page() ? > > > > > The compress code looks to be a tentative compression (not guaranteed > > IIUC), so > > I am not sure it needs any more logic that just adding at the top of > > ram_save_target_page_legacy() as Peter suggested? > > > > > And what about an RDMA memory region impacted by a memory error ? > > > This is an important aspect. > > > Does anyone know how this situation is dealt with ? And how it should be > > > handled > > > in Qemu ? > > > > > > > If you refer to guest RDMA MRs that is just guest RAM, not sure we are even > > aware of those from qemu. But if you refer to the RDMA transport that sits > > below > > the Qemu file (or rather acts as an implementation of QemuFile), so > > handling in > > ram_save_target_page_legacy() already seems to cover it. > > I'm also not familiar enough with RDMA, but it looks tricky indeed. AFAIU > it's leveraging RDMA_CONTROL_COMPRESS for zero pages for now (with > RDMACompress.value==0), so it doesn't seem to be using generic migration > protocols. > > If we want to fix all places well, one way to consider is to introduce > migration_buffer_is_zero(), which can be a wrapper for buffer_is_zero() by > default, but also returns true for poisoned pages before reading the > buffer. Then we use it in all three places: > > - For compression, in do_compress_ram_page() > - For RDMA, in qemu_rdma_write_one()
Ah, this may not be enough.. sorry. It seems this is only one path that RDMA will use to save a target page, for (!rdma->pin_all || !block->is_ram_block) && !block->remote_keys[chunk]. RDMA seems to also possible to merge buffers if virtually continuous (qemu_rdma_buffer_mergable()), so IIUC it may not trigger an immediate access to the guest page until later if it finds continuous pages and skip even more logic. I suspect that's also problematic for poisoned pages so we should not allow any merged buffer to contain a poisoned page. Not sure how complicated will it be to fix rdma specifically, copy again two rdma developers. One option is we state the issue in rdma and fix non-rdma first. Looks like rdma needs its own fix anyway. > - For generic migration, in save_zero_page_to_file() (your current patch) > > I suppose then all cases will be fixed. We need to make sure we'll always > use migration_buffer_is_zero() as the 1st thing to call when QEMU wants to > migrate a target page. Maybe it'll worth a comment above that function. > > Thanks, > > -- > Peter Xu -- Peter Xu
