From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56442) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aknIU-0003Jj-EG for qemu-devel@nongnu.org; Tue, 29 Mar 2016 02:38:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aknIT-0000x0-73 for qemu-devel@nongnu.org; Tue, 29 Mar 2016 02:38:38 -0400 Date: Tue, 29 Mar 2016 17:39:45 +1100 From: David Gibson Message-ID: <20160329063945.GH15156@voom.fritz.box> References: <1457317586-15122-1-git-send-email-david@gibson.dropbear.id.au> <1457317586-15122-3-git-send-email-david@gibson.dropbear.id.au> <56F173E9.2060702@redhat.com> <20160324053535.GC23586@voom.redhat.com> <56F3A857.6060400@redhat.com> <20160325101359.3c6c95be@bahia.huguette.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="QDd5rp1wjxlDmy9q" Content-Disposition: inline In-Reply-To: <20160325101359.3c6c95be@bahia.huguette.org> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 2/3] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Laurent Vivier , thuth@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --QDd5rp1wjxlDmy9q Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 25, 2016 at 10:13:59AM +0100, Greg Kurz wrote: > Hi Laurent, >=20 > On Thu, 24 Mar 2016 09:41:59 +0100 > Laurent Vivier wrote: >=20 > > On 24/03/2016 06:35, David Gibson wrote: > > > On Tue, Mar 22, 2016 at 05:33:45PM +0100, Laurent Vivier wrote: =20 > > >> Hi David, > > >> > > >> using kvm-unit-tests, I've found a side effect of your patches: the = MSR > > >> is cleared (and perhaps some others). > > >> > > >> I was trying to test my patch on top of QEMU master: > > >> > > >> "ppc64: set MSR_SF bit" > > >> http://patchwork.ozlabs.org/patch/598198/ > > >> > > >> and it was not working anymore. > > >> > > >> By bisecting, I've found this commit. > > >> > > >> I think "cpu_synchronize_state()" in "ppc_hash64_set_external_hpt()" > > >> restores the MSR from KVM whereas the one from QEMU has not been sav= ed, > > >> because cpu_synchronize_all_post_reset() is called later. > > >> > > >> So it is cleared. > > >> > > >> You can test this by applying the MSR_SF patch and using the "emulat= or" > > >> test of kvm-unit-tests (the "emulator: 64bit" test case) =20 > > >=20 > > > Ugh, you're right of course. But, I'm having a bit of trouble > > > figuring out how to fix it propertly. =20 > >=20 > > Perhaps you can just remove the cpu_synchronize_state()? > >=20 > > As this is in the reset phase (spapr_cpu_reset()), I think the content > > of the QEMU side registers are correct, and they will be synchronized at > > the end of the reset phase. > >=20 >=20 > I think this is right because qemu_system_reset() is called either: > - during system startup: >=20 > cpu_synchronize_all_post_init(); =3D> push regs to KVM, kvm_vcpu_di= rty =3D false > ... > qemu_system_reset(VMRESET_SILENT); >=20 > QEMU still have good values for the registers though, since KVM hasn't ru= n yet. >=20 > But ppc_hash64_set_external_hpt()->cpu_synchronize_state() will indeed pu= ll > the registers from KVM and clear MSR_SF, since we have kvm_vcpu_dirty =3D= =3D false. >=20 > - or from main_loop_should_exit() and we have: >=20 > cpu_synchronize_all_states(); > qemu_system_reset(VMRESET_REPORT); > and > cpu_synchronize_all_states(); > qemu_system_reset(VMRESET_SILENT); >=20 > In which case ppc_hash64_set_external_hpt()->cpu_synchronize_state() isn't > needed. >=20 > Makes sense ? Ugh. So, I think this is the simplest short term fix, and I've applied a change to ppc-for-2.6 removing the cpu_synchronize_state(). But, longer term I don't think this is really right. Removing the cpu_synchronize_state() corrects using this in the reset path, but breaks it if it is used outside the reset path. At the moment we don't do that, but I have code which will do so in my HPT resizing branch. Obviously I can fix that by putting cpu_synchronize_state() in the caller, but it still seems a bit odd having a put_sregs(). Really, it seems to me that kvm_vcpu_dirty should be set to true (either via cpu_synchronize_state() or directly) *before* the core CPU reset path, in the same way that it is set to true in kvm_init_vcpu(). --=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 --QDd5rp1wjxlDmy9q Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW+iMxAAoJEGw4ysog2bOSqdwQAKhW/giPwh0O3QYalms8vWUb tlTtnlRYCDSN3YKkCPEi5D88KHU1azADAS/ZWu88rHw3tslOYSW4x2r4hNrP+kZX oiuyhizsQd8BOo5U8MPD9eJWzTHbS7uVW76zpk7jeIJ5GLvy4SXAzcbBZdVLW7Co iDJCOz0wRlm5lqcc2FsNs01fI8uPfRxYL5lv+tP3bxwWM9spCdvHCyjjuSN8yYhb 9TXyVFU/rOCv0SVe6oOv6uyq/5R5WoA1bdBLnl/YR/sObW7G0MZPLskBV89xxwJQ FemPpspUXuP+n0sit54ew/YXxt8VF809JU5cxUudo5tDzsk7+WoCi1HOx0wTFn6A r+D6Vef40/SlPYyeaKuLnVoestVM2az1A+J2UwZy8NUMntuafGUMPkJYd6wnRTNf rQH4zgveiGJDX7ASLW6AcPNgQk7sqVZwakFrHCqjDPpUFAx08P4YritPGkiPWyfS jDc+++mdYAKRSD3vx4HVZqJcjOqe/GTGaw1Bb3WOjfHsdyS5oBCyPk457AFoEirZ WM2YNI6RkAf09E0uLzptizEyJVeP313AkFbafSgbyNjIMmFX/VK105hJlYIFxUo+ 4usx1i/5EnHBVDz8hgnOnBETRMWBBHlNnX+sIyuLwZASrOZWCd1iPS7GfyO4GNH7 zohd82/SeKWc6jlH9reM =5KDG -----END PGP SIGNATURE----- --QDd5rp1wjxlDmy9q--