From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52845) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaGnd-0004mL-2u for qemu-devel@nongnu.org; Tue, 24 Mar 2015 00:50:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaGna-0008HR-Q5 for qemu-devel@nongnu.org; Tue, 24 Mar 2015 00:50:45 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:49417) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaGna-0008H5-4d for qemu-devel@nongnu.org; Tue, 24 Mar 2015 00:50:42 -0400 Date: Tue, 24 Mar 2015 15:51:28 +1100 From: David Gibson Message-ID: <20150324045128.GA25043@voom.fritz.box> References: <1424883128-9841-1-git-send-email-dgilbert@redhat.com> <1424883128-9841-37-git-send-email-dgilbert@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="rmZDAlAp7pllCg/D" Content-Disposition: inline In-Reply-To: <1424883128-9841-37-git-send-email-dgilbert@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 36/45] Postcopy: Use helpers to map pages during migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, amit.shah@redhat.com, pbonzini@redhat.com, yanghy@cn.fujitsu.com --rmZDAlAp7pllCg/D Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 25, 2015 at 04:51:59PM +0000, Dr. David Alan Gilbert (git) wrot= e: > From: "Dr. David Alan Gilbert" >=20 > In postcopy, the destination guest is running at the same time > as it's receiving pages; as we receive new pages we must put > them into the guests address space atomically to avoid a running > CPU accessing a partially written page. >=20 > Use the helpers in postcopy-ram.c to map these pages. >=20 > Note, gcc 4.9.2 is giving me false uninitialized warnings in ram_load's > switch, so anything conditionally set at the start of the switch needs > initializing; filed as > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D64614 >=20 > Signed-off-by: Dr. David Alan Gilbert > --- > arch_init.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++++------= ------ > 1 file changed, 111 insertions(+), 26 deletions(-) >=20 > diff --git a/arch_init.c b/arch_init.c > index acf65e1..7a1c9ea 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -1479,9 +1479,39 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t add= r, void *host) > return 0; > } > =20 > +/* > + * Helper for host_from_stream_offset in the succesful case. > + * Returns the host pointer for the given block and offset > + * calling the postcopy hook and filling in *rb. > + */ > +static void *host_with_postcopy_hook(MigrationIncomingState *mis, > + ram_addr_t offset, > + RAMBlock *block, > + RAMBlock **rb) > +{ > + if (rb) { > + *rb =3D block; > + } > + > + postcopy_hook_early_receive(mis, > + (offset + block->offset) >> TARGET_PAGE_BITS); What does postcopy_hook_early_receive() do? > + return memory_region_get_ram_ptr(block->mr) + offset; > +} > + > +/* > + * Read a RAMBlock ID from the stream f, find the host address of the > + * start of that block and add on 'offset' > + * > + * f: Stream to read from > + * mis: MigrationIncomingState > + * offset: Offset within the block > + * flags: Page flags (mostly to see if it's a continuation of previous b= lock) > + * rb: Pointer to RAMBlock* that gets filled in with the RB we find > + */ > static inline void *host_from_stream_offset(QEMUFile *f, > + MigrationIncomingState *mis, > ram_addr_t offset, > - int flags) > + int flags, RAMBlock **rb) > { > static RAMBlock *block =3D NULL; > char id[256]; > @@ -1492,8 +1522,7 @@ static inline void *host_from_stream_offset(QEMUFil= e *f, > error_report("Ack, bad migration stream!"); > return NULL; > } > - > - return memory_region_get_ram_ptr(block->mr) + offset; > + return host_with_postcopy_hook(mis, offset, block, rb); > } > =20 > len =3D qemu_get_byte(f); > @@ -1503,7 +1532,7 @@ static inline void *host_from_stream_offset(QEMUFil= e *f, > QTAILQ_FOREACH(block, &ram_list.blocks, next) { > if (!strncmp(id, block->idstr, sizeof(id)) && > block->max_length > offset) { > - return memory_region_get_ram_ptr(block->mr) + offset; > + return host_with_postcopy_hook(mis, offset, block, rb); > } > } > =20 > @@ -1537,6 +1566,15 @@ static int ram_load(QEMUFile *f, void *opaque, int= version_id) > { > int flags =3D 0, ret =3D 0; > static uint64_t seq_iter; > + /* > + * System is running in postcopy mode, page inserts to host memory m= ust be > + * atomic > + */ > + MigrationIncomingState *mis =3D migration_incoming_get_current(); > + bool postcopy_running =3D postcopy_state_get(mis) >=3D > + POSTCOPY_INCOMING_LISTENING; > + void *postcopy_host_page =3D NULL; > + bool postcopy_place_needed =3D false; > =20 > seq_iter++; > =20 > @@ -1545,14 +1583,57 @@ static int ram_load(QEMUFile *f, void *opaque, in= t version_id) > } > =20 > while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { > + RAMBlock *rb =3D 0; /* =3D0 needed to silence compiler */ > ram_addr_t addr, total_ram_bytes; > - void *host; > + void *host =3D 0; > + void *page_buffer =3D 0; > uint8_t ch; > + bool all_zero =3D false; > =20 > addr =3D qemu_get_be64(f); > flags =3D addr & ~TARGET_PAGE_MASK; > addr &=3D TARGET_PAGE_MASK; > =20 > + if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE | > + RAM_SAVE_FLAG_XBZRLE)) { > + host =3D host_from_stream_offset(f, mis, addr, flags, &rb); > + if (!host) { > + error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); > + ret =3D -EINVAL; > + break; > + } > + if (!postcopy_running) { > + page_buffer =3D host; > + } else { > + /* > + * Postcopy requires that we place whole host pages atom= ically. > + * To make it atomic, the data is read into a temporary = page > + * that's moved into place later. > + * The migration protocol uses, possibly smaller, targe= t-pages > + * however the source ensures it always sends all the co= mponents > + * of a host page in order. > + */ > + if (!postcopy_host_page) { > + postcopy_host_page =3D postcopy_get_tmp_page(mis); > + } > + page_buffer =3D postcopy_host_page + > + ((uintptr_t)host & ~qemu_host_page_mask); > + /* If all TP are zero then we can optimise the place */ > + if (!((uintptr_t)host & ~qemu_host_page_mask)) { > + all_zero =3D true; > + } > + > + /* > + * If it's the last part of a host page then we place th= e host > + * page > + */ So it's assumed in a bunch of places that the other end will send target pages in order until you have a whole host page. Which is fine, but it would be nice to have some tests to verify that, just in case somebody changes that behaviour in future, then wonders why everything broke. As I suggested on the earlier patch, I think some of this stuff might become less clunky, if you had a "receive_host_page" subfunction with its own inner loop. > + postcopy_place_needed =3D (((uintptr_t)host + TARGET_PAG= E_SIZE) & > + ~qemu_host_page_mask) =3D=3D 0; > + } > + } else { > + postcopy_place_needed =3D false; > + } > + > switch (flags & ~RAM_SAVE_FLAG_CONTINUE) { > case RAM_SAVE_FLAG_MEM_SIZE: > /* Synchronize RAM block list */ > @@ -1590,32 +1671,27 @@ static int ram_load(QEMUFile *f, void *opaque, in= t version_id) > } > break; > case RAM_SAVE_FLAG_COMPRESS: > - host =3D host_from_stream_offset(f, addr, flags); > - if (!host) { > - error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); > - ret =3D -EINVAL; > - break; > - } > - > ch =3D qemu_get_byte(f); > - ram_handle_compressed(host, ch, TARGET_PAGE_SIZE); > - break; > - case RAM_SAVE_FLAG_PAGE: > - host =3D host_from_stream_offset(f, addr, flags); > - if (!host) { > - error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); > - ret =3D -EINVAL; > - break; > + if (!postcopy_running) { > + ram_handle_compressed(host, ch, TARGET_PAGE_SIZE); > + } else { > + memset(page_buffer, ch, TARGET_PAGE_SIZE); > + if (ch) { > + all_zero =3D false; It's a bit nasty that the non-postcopy cases are now out-of-line with the postcopy cases in-line. I think it would be nicer to alter the helper functions so that they work on a supplied buffer, rather than directly on the ram state. Then you should be able to use the same helpers for either case, with just different postcopy logic before and after the actual page load. > + } > } > + break; > =20 > - qemu_get_buffer(f, host, TARGET_PAGE_SIZE); > + case RAM_SAVE_FLAG_PAGE: > + all_zero =3D false; > + qemu_get_buffer(f, page_buffer, TARGET_PAGE_SIZE); > break; > + > case RAM_SAVE_FLAG_XBZRLE: > - host =3D host_from_stream_offset(f, addr, flags); > - if (!host) { > - error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); > - ret =3D -EINVAL; > - break; > + all_zero =3D false; > + if (postcopy_running) { > + error_report("XBZRLE RAM block in postcopy mode @%zx\n",= addr); > + return -EINVAL; > } > =20 > if (load_xbzrle(f, addr, host) < 0) { > @@ -1637,6 +1713,15 @@ static int ram_load(QEMUFile *f, void *opaque, int= version_id) > ret =3D -EINVAL; > } > } > + > + if (postcopy_place_needed) { > + /* This gets called at the last target page in the host page= */ > + ret =3D postcopy_place_page(mis, host + TARGET_PAGE_SIZE - > + qemu_host_page_size, > + postcopy_host_page, > + (addr + rb->offset) >> TARGET_PAGE= _BITS, > + all_zero); > + } > if (!ret) { > ret =3D qemu_file_get_error(f); > } --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --rmZDAlAp7pllCg/D Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVEO1QAAoJEGw4ysog2bOSiBwQAIyl12sNzxB1fnaI+dbTvcRR CFJHsM9LMkDmj4MX6fuQer0SKhHGFH0uk/nkYc9UUdjqCFSLZ1wzA0EqgJJVnTZr rj60SF76gTfOsNCn0AcpxzLVBeh3skdvCdWO9NCyPBHuSnc6Q71D+hnRqnGIXebR X8TCoWPtA9XSWKsRS0+1ARgKIHTwmaIMe6uWJNnY7A90tGf0pHgyyJT7/lOmZVUc JUwpSZTvgoW/MTbjDkZ0VxbHmEAMNAq8KaYvagVEYsT+2+dJagENvm4k7rozTaF3 X2iNoIj7ANOGC9EyF5Q5vE4sESjWPJ3p3J0GXfM55AhaBrod/mrYsJP559YjJz3S g90zCCJorU0xP6ZDNCiKuYF46NN7c99s9BN3/dMtLIlef+lyQgi3jLWNJNddvfiH cuKEmbvpvPwpJVrKlgEUbP7JbgeLBH8lJJn1okObcgGLWDHW2zk25ICer3XOKPYp hJN6cbu++09ZAMpvHZZeFRQLWogw0bLjmyvOi7weeCOZvYPZziTLoDRVw1z72AtY XVW7KkZfT1fKgEjYGJ23qx9rQQdYtlbRLyXXD/eG7or52OFzJGUX8lamZqoMA8eX 76KtqrlvdRiK2u/AI9XzfF0k/yv6D6QQaUeLkv11xrq1ZSr1I0DvQeTtRKajWkH0 83V0zN2Guc2m69IUmlyf =1rqR -----END PGP SIGNATURE----- --rmZDAlAp7pllCg/D--