From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45101) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xqk9q-0001nY-2x for qemu-devel@nongnu.org; Tue, 18 Nov 2014 09:53:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xqk9j-0000q0-UK for qemu-devel@nongnu.org; Tue, 18 Nov 2014 09:53:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51001) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xqk9j-0000po-N7 for qemu-devel@nongnu.org; Tue, 18 Nov 2014 09:53:23 -0500 Date: Tue, 18 Nov 2014 14:53:18 +0000 From: Stefan Hajnoczi Message-ID: <20141118145318.GG7837@stefanha-thinkpad.redhat.com> References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="VACxsDaSTfeluoxK" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] exec.c:invalidate_and_set_dirty() only checks whether first page in its range is dirty... List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Paolo Bonzini , QEMU Developers --VACxsDaSTfeluoxK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Nov 16, 2014 at 06:11:48PM +0000, Peter Maydell wrote: > I'm trying to track down a bug in ARM TCG where we: > * boot a guest > * run 'shutdown -r now' to trigger a reboot > * on reboot, crash when running userspace because the contents > of physical RAM have changed but the translated code from > before the shutdown was never invalidated >=20 > This is with a virtio-mmio block device as the disk. >=20 > Debugging indicates that when the post-reboot guest reloads > binaries from disk into ram we fail to invalidate the cached > translations. For the specific case I looked at, we have a > translation of code at ramaddr_t 0x806e000. The disk load > pulls 0x16000 bytes of data off disk to address 0x806a000. > Virtio correctly calls address_space_unmap(), which is supposed > to be what marks the ram range as dirty. It in turn calls > invalidate_and_set_clean(). However invalidate_and_set_clean() > just does this: >=20 > if (cpu_physical_memory_is_clean(addr)) { > /* invalidate code */ > tb_invalidate_phys_page_range(addr, addr + length, 0); > /* set dirty bit */ > cpu_physical_memory_set_dirty_range_nocode(addr, length); > } >=20 > So if the first page in the range (here 0x806a000) happens > to be dirty then we won't do anything, even if later pages > in the range do need to be invalidated. Also, we'll call > tb_invalidate_phys_page_range() with a start/end which may > be in different physical pages, which is forbidden by that > function's API. >=20 > I guess invalidate_and_set_clean() really needs to be > fixed to loop through each page in the range; does anybody > know how this is supposed to work (or why nobody's noticed > this bug before :-)) ? Not directly but I don't like this code because it's not atomic. I'll send patches soon for atomic test-and-set and test-and-clear. Hopefully it won't impact performance too much. What you've discovered seems like a plain old bug. It needs a loop. Stefan --VACxsDaSTfeluoxK Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUa11eAAoJEJykq7OBq3PIOn4H/0ZMZbgNaMPiAYMUuEVju71M snI3o6O74TisARNSSn9fWHfpwCk6eFsO7btTdCA77npa9J7Bs+4l3YZ6T6CIvRHv Tq8o1jnZ5fOY5W9THcsZkXdzHF6bJRZWODBD6qqLG03I5mbdJTjs2VLtEI3METf0 WAYTthXbDfSQIky44j9hWtga7tQlYvlGklIXoTslisDEiyom17kVCkRIPbEMUZR4 gVlRd2QEEhvxVABtdSGVW0IFLMSdpEydpthVpD3+evqLaAW3Vv03iUaFF2Ed+6ld Ss/zU00liG0eUVW6J3tI5R5yays7ECCGN8qKpZtUwax6OHKQZZXdmzKS105X/cI= =oeMU -----END PGP SIGNATURE----- --VACxsDaSTfeluoxK--