From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40268) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1amuoq-0002C2-EP for qemu-devel@nongnu.org; Sun, 03 Apr 2016 23:04:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1amuon-0001QK-6w for qemu-devel@nongnu.org; Sun, 03 Apr 2016 23:04:48 -0400 Date: Mon, 4 Apr 2016 12:59:01 +1000 From: David Gibson Message-ID: <20160404025901.GH16485@voom.fritz.box> References: <1459482767-12573-1-git-send-email-david@gibson.dropbear.id.au> <56FE4D4F.2050003@redhat.com> <20160404011056.GF16485@voom.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="cW+P/jduATWpL925" Content-Disposition: inline In-Reply-To: <20160404011056.GF16485@voom.fritz.box> Subject: Re: [Qemu-devel] [PATCH] target-ppc: Correct KVM synchronization for ppc_hash64_set_external_hpt() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: lvivier@redhat.com, peter.maydell@linaro.org, mdroth@linux.vnet.ibm.com, aik@ozlabs.ru, qemu-devel@nongnu.org, agraf@suse.de, qemu-ppc@nongnu.org --cW+P/jduATWpL925 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 04, 2016 at 11:10:56AM +1000, David Gibson wrote: > On Fri, Apr 01, 2016 at 12:28:31PM +0200, Paolo Bonzini wrote: > >=20 > >=20 > > On 01/04/2016 05:52, David Gibson wrote: > > > This seems like the right minimal fix in the qemu-2.6 timeframe to fix > > > the actual bug. However, longer term it seems like the correct thing > > > to do might be to set kvm_vcpu_dirty early in the reset path. Though= ts? > >=20 > > Isn't it done already? vl.c does: > >=20 > > pause_all_vcpus(); > > cpu_synchronize_all_states(); > > qemu_system_reset(VMRESET_REPORT); > > resume_all_vcpus(); >=20 >=20 > Huh.. now I'm really confused. Given this I would indeed have > expected kvm_vpcu_dirty to be set in the reset path, but that would > make the cpu_synchronize_state() causing the problem to act as a > no-op, so it shouldn't be causing the problem. >=20 > Investigating... Ok, I've found it. So, if you reset the system after it's up and running, the fragment above is indeed what will happen and I think everything will work correct from there. The problem occurs only on the very first reset in main() (vl.c:4624 as of de1d099): kvm_vcpu_dirty is false during this call. Specifically, although kvm_vcpu_dirty was initialized to true in kvm_vcpu_init(), it gets set to false in cpu_synchronize_post_init() and never set true again before the qemu_system_reset(). Seems to me we either need a cpu_synchronize_all_states() before that qemu_system_reset(), or we need to fold the cpu_synchronize_all_states() right into qemu_system_reset() itself. Opinion on which option is preferred? >=20 > >=20 > > Thanks, > >=20 > > Paolo > >=20 > > > diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c > > > index 72c4ab5..caf41ce 100644 > > > --- a/target-ppc/mmu-hash64.c > > > +++ b/target-ppc/mmu-hash64.c > > > @@ -283,8 +283,6 @@ void ppc_hash64_set_external_hpt(PowerPCCPU *cpu,= void *hpt, int shift, > > > CPUPPCState *env =3D &cpu->env; > > > Error *local_err =3D NULL; > > > =20 > > > - cpu_synchronize_state(CPU(cpu)); > > > - > > > if (hpt) { > > > env->external_htab =3D hpt; > > > } else { > >=20 >=20 --=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 --cW+P/jduATWpL925 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXAdh1AAoJEGw4ysog2bOSUFMP+QGgxPZO0ghwb2bhUEmo2I1V 4VCKbnfSNRO6PfsqbiJfx/Mg//v/tMbE70vvtvzGvB8BbSCuwkvK52BqFiqKOm+8 pzqy5GPoPx4/0VD3oczLg4z8hw7RubX87/x/r4geI6BwaTL9IJeuPc+1WVuFSkyw DDxpiiiMYlIg+KTKv8A8y4viOb/kDb8HeMy3Rb2vw7eVA1zeNcc+5IwHQYfcsWRm isZk68gaVVU7x+39cNHUADLoRHnDi2E6ZAsjR8rPxvBeYQgr7yQrzA65lT5dzZ7P 5jJUPC0x2h2euzykcgF6fwml7czLKn9200Z6DdX1amxyzklRUaWzxfOdcQ/WFs0x D4EorvjCuKtpQpp/jrtq9nqAHMdVWkgMsLbxVwQAjRGR8CzlFct1K+IrUVeghSrB igpk3cS1LePdS46J1vQUDGSKkHipvdhQU+rzUJi8TMtaTQMUuvVpo4YE2E31jhBb ysrwUmGc+zRL0otGPynEHSJOaX3KJ+Bb2Qn+X5XyVjyZQlxgxV6h1vyFvVCYtqQY YJ+dbsKCnUEkpxuJ7xk8RrLt6oehnPxztMWtpRXcsxpg5lGl1qpl3LZt7obH6lQ+ 3V7l45Eyx2jXZMqRVJwPHTe73kWJuJSr9k0SlBAKm9X/T1XyTRqsPLJbaF2aUWmq eVt5CqISQsHT5mWUEpYI =bSn8 -----END PGP SIGNATURE----- --cW+P/jduATWpL925--