From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43329) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XokVw-0006q9-1y for qemu-devel@nongnu.org; Wed, 12 Nov 2014 21:52:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XokVu-0000Vo-8w for qemu-devel@nongnu.org; Wed, 12 Nov 2014 21:52:04 -0500 Received: from ozlabs.org ([2401:3900:2:1::2]:43170) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XokVt-0000Vb-N0 for qemu-devel@nongnu.org; Wed, 12 Nov 2014 21:52:02 -0500 Date: Thu, 13 Nov 2014 13:53:03 +1100 From: David Gibson Message-ID: <20141113025303.GB7291@voom.fritz.box> References: <1412358473-31398-1-git-send-email-dgilbert@redhat.com> <1412358473-31398-41-git-send-email-dgilbert@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ZfOjI3PrQbgiZnxM" Content-Disposition: inline In-Reply-To: <1412358473-31398-41-git-send-email-dgilbert@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 40/47] 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, lilei@linux.vnet.ibm.com, quintela@redhat.com, cristian.klein@cs.umu.se, qemu-devel@nongnu.org, amit.shah@redhat.com, yanghy@cn.fujitsu.com --ZfOjI3PrQbgiZnxM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 03, 2014 at 06:47:46PM +0100, 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 > Signed-off-by: Dr. David Alan Gilbert > --- > arch_init.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++= ------ > 1 file changed, 87 insertions(+), 9 deletions(-) >=20 > diff --git a/arch_init.c b/arch_init.c > index 2f4345a..0ba627b 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -1458,9 +1458,20 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t add= r, void *host) > return 0; > } > =20 > +/* > + * 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]; > @@ -1471,8 +1482,11 @@ static inline void *host_from_stream_offset(QEMUFi= le *f, > error_report("Ack, bad migration stream!"); > return NULL; > } > + if (rb) { > + *rb =3D block; > + } > =20 > - return memory_region_get_ram_ptr(block->mr) + offset; > + goto gotit; This is an ugly use of goto - it looks kind of like the exception handling goto idiom, but it's not. I think it would be nicer to make the code fragment after gotit into a helper function. > } > =20 > len =3D qemu_get_byte(f); > @@ -1480,12 +1494,22 @@ static inline void *host_from_stream_offset(QEMUF= ile *f, > id[len] =3D 0; > =20 > QTAILQ_FOREACH(block, &ram_list.blocks, next) { > - if (!strncmp(id, block->idstr, sizeof(id))) > - return memory_region_get_ram_ptr(block->mr) + offset; > + if (!strncmp(id, block->idstr, sizeof(id))) { > + if (rb) { > + *rb =3D block; > + } > + goto gotit; > + } > } > =20 > error_report("Can't find block %s!", id); > return NULL; > + > +gotit: > + postcopy_hook_early_receive(mis, > + (offset + (*rb)->offset) >> TARGET_PAGE_BITS); > + return memory_region_get_ram_ptr(block->mr) + offset; > + > } > =20 > /* > @@ -1515,6 +1539,13 @@ static int ram_load(QEMUFile *f, void *opaque, int= version_id) > ram_addr_t addr; > int flags, 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 mis->postcopy_ram_state >=3D > + POSTCOPY_RAM_INCOMING_LISTENING; > =20 > seq_iter++; > =20 > @@ -1523,6 +1554,7 @@ static int ram_load(QEMUFile *f, void *opaque, int = version_id) > } > =20 > while (!ret) { > + RAMBlock *rb =3D 0; /* =3D0 needed to silence compiler */ > addr =3D qemu_get_be64(f); > =20 > flags =3D addr & ~TARGET_PAGE_MASK; > @@ -1570,7 +1602,7 @@ static int ram_load(QEMUFile *f, void *opaque, int = version_id) > void *host; > uint8_t ch; > =20 > - host =3D host_from_stream_offset(f, addr, flags); > + 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; > @@ -1578,20 +1610,66 @@ static int ram_load(QEMUFile *f, void *opaque, in= t version_id) > } > =20 > ch =3D qemu_get_byte(f); > - ram_handle_compressed(host, ch, TARGET_PAGE_SIZE); > + if (!postcopy_running) { > + ram_handle_compressed(host, ch, TARGET_PAGE_SIZE); > + } else { > + if (!ch) { > + ret =3D postcopy_place_zero_page(mis, host, > + (addr + rb->offset) >> TARGET_PAGE_BITS); > + } else { > + void *tmp; > + tmp =3D postcopy_get_tmp_page(mis, (addr + rb->offse= t) >> > + TARGET_PAGE_BITS); > + > + if (!tmp) { > + return -ENOMEM; > + } > + memset(tmp, ch, TARGET_PAGE_SIZE); > + ret =3D postcopy_place_page(mis, host, tmp, > + (addr + rb->offset) >> TARGET_PAGE_BITS); > + } > + if (ret) { > + error_report("ram_load: Failure in postcopy compress= @" > + "%zx/%p;%s+%zx", > + addr, host, rb->idstr, rb->offset); > + return ret; > + } > + } Might be nicer to fold this logic into ram_handle_compressed(), since there's no obvious reason it should not be used for the postcopy path. > } else if (flags & RAM_SAVE_FLAG_PAGE) { > void *host; > =20 > - host =3D host_from_stream_offset(f, addr, flags); > + 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; > } > =20 > - qemu_get_buffer(f, host, TARGET_PAGE_SIZE); > + if (!postcopy_running) { > + qemu_get_buffer(f, host, TARGET_PAGE_SIZE); > + } else { > + void *tmp =3D postcopy_get_tmp_page(mis, (addr + rb->off= set) >> > + TARGET_PAGE_BITS= ); > + > + if (!tmp) { > + return -ENOMEM; > + } > + qemu_get_buffer(f, tmp, TARGET_PAGE_SIZE); > + ret =3D postcopy_place_page(mis, host, tmp, > + (addr + rb->offset) >> TARGET_PAGE_BITS); > + if (ret) { > + error_report("ram_load: Failure in postcopy simple" > + "@%zx/%p;%s+%zx", > + addr, host, rb->idstr, rb->offset); > + return ret; > + } > + } > } else if (flags & RAM_SAVE_FLAG_XBZRLE) { > - void *host =3D host_from_stream_offset(f, addr, flags); > + if (postcopy_running) { > + error_report("XBZRLE RAM block in postcopy mode @%zx\n",= addr); > + return -EINVAL; > + } Hrm, there doesn't seem like an inherent reason XBZRLE shouldn't be possible in postcopy. Obviously a temporary buffer would be necessary. > + void *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; --=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 --ZfOjI3PrQbgiZnxM Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUZB0PAAoJEGw4ysog2bOSzzcP/j7uD56z73sqMtkZyRrfoMWb tvNmvQPaDYaszkjFOTlt1GfcQpzsIrRagznRQbj+ekQhK1FQ+TenDqf8hXJm/bcz NqXpxp6PtwVKqvaLlrQdDgNwRQh2/nPyGTFuJVWzplrkJqZlZE4/lJURW1Hecxs5 CE5jMIy3HRqCTw1UW8lByz49/8E3G7kw/NGRzKKc4/AE7h29A5OaSO1tOynN6tkz 2wGTnNmUNur9Ucv09hhNbUUSctn8L0FC5lMcVr/gVkWS/eRQlTWmXkrpk2LyNljd GQQMG+SaYZvqslnEpYfm9F9/Q18RqRAynoNToD0i/jAwCrI2zu+aVFzhahC5J/uA p9MuGmlfFuW71W0ifhK/9cdSyRPO5qMHFbKw4NuYcXlESxr6PTufAkjAITXEZI8W JppzK8KP6i5t9F1Uq4zabwmgEVrb5Kgfkr5RFfNYac2xsIozgFF2K3bfijzdDx3L labltDyPZGxkoa2mNqqAse1e/zEF4Cf4XeQIQKLCzIb4lrwu6AHAGPpUicSDAWBH 8Rb/clGOk8OdXcalQe+DkLO/Dr9C5rsSOwAwad9zvSx8QxU8+8ZGy9vLylDeRQyE eWbb1u6iJGa3peDkQq1ZWlKvU7dcLItsZkELAtJ3bEzk2r3GcsZoTvNE+xcHzjaI h07+5KhSjbpQSJ9Xo435 =t5/2 -----END PGP SIGNATURE----- --ZfOjI3PrQbgiZnxM--