From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47515) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ad614-0006k3-FQ for qemu-devel@nongnu.org; Mon, 07 Mar 2016 20:00:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ad612-0003Ds-Cd for qemu-devel@nongnu.org; Mon, 07 Mar 2016 20:00:50 -0500 Date: Tue, 8 Mar 2016 11:36:11 +1100 From: David Gibson Message-ID: <20160308003611.GP22546@voom.fritz.box> References: <1457317586-15122-1-git-send-email-david@gibson.dropbear.id.au> <1457317586-15122-4-git-send-email-david@gibson.dropbear.id.au> <20160307144134.416abcb9@bahia.huguette.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="tZCmRiovzb4sxAVa" Content-Disposition: inline In-Reply-To: <20160307144134.416abcb9@bahia.huguette.org> Subject: Re: [Qemu-devel] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab global List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: lvivier@redhat.com, thuth@redhat.com, aik@ozlabs.ru, agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org --tZCmRiovzb4sxAVa Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 07, 2016 at 02:41:34PM +0100, Greg Kurz wrote: > On Mon, 7 Mar 2016 13:26:26 +1100 > David Gibson wrote: >=20 > > fa48b43 "target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KV= M" > > purports to remove a hack in the handling of hash page tables (HPTs) > > managed by KVM instead of qemu. However, it actually went in the wrong > > direction. > >=20 > > That patch requires anything looking for an external HPT (that is one n= ot > > managed by the guest itself) to check both env->external_htab (for a qe= mu > > managed HPT) and kvmppc_kern_htab (for a KVM managed HPT). That's a > > problem because kvmppc_kern_htab is local to mmu-hash64.c, but some pla= ces > > which need to check for an external HPT are outside that, such as > > kvm_arch_get_registers(). The latter was subtly broken by the earlier > > patch such that gdbstub can no longer access memory. > >=20 > > Basically a KVM managed HPT is much more like a qemu managed HPT than i= t is > > like a guest managed HPT, so the original "hack" was actually on the ri= ght > > track. > >=20 > > This partially reverts fa48b43, so we again mark a KVM managed external= HPT > > by putting a special but non-NULL value in env->external_htab. It then > > goes further, using that marker to eliminate the kvmppc_kern_htab global > > entirely. The ppc_hash64_set_external_hpt() helper function is extended > > to set that marker if passed a NULL value (if you're setting an external > > HPT, but don't have an actual HPT to set, the assumption is that it must > > be a KVM managed HPT). > >=20 > > This also has some flow-on changes to the HPT access helpers, required = by > > the above changes. > >=20 > > Reported-by: Greg Kurz > > Signed-off-by: David Gibson > > Reviewed-by: Thomas Huth > > --- >=20 > Typo in comment in target-ppc/mmu-hash64.c (see below). >=20 > Apart from that, you get: >=20 > Reviewed-by: Greg Kurz >=20 > and also... >=20 > without this patch: >=20 > 0x00000000100009b8 in main (argc=3D, argv=3D) at fp.c:11 >=20 > with this patch: >=20 > 0x00000000100009b8 in main (argc=3D4, argv=3D0x3fffc7fcfd18) at fp.c:11 >=20 > Just to be sure, I've also tested with TCG: no regression. >=20 > Thanks for the fix ! >=20 > Tested-by: Greg Kurz Thanks. > > hw/ppc/spapr.c | 3 +-- > > hw/ppc/spapr_hcall.c | 10 +++++----- > > target-ppc/mmu-hash64.c | 40 ++++++++++++++++++---------------------- > > target-ppc/mmu-hash64.h | 9 +++------ > > 4 files changed, 27 insertions(+), 35 deletions(-) > >=20 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 72a018b..43708a2 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1091,7 +1091,7 @@ static void spapr_reallocate_hpt(sPAPRMachineStat= e *spapr, int shift, > > } > >=20 > > spapr->htab_shift =3D shift; > > - kvmppc_kern_htab =3D true; > > + spapr->htab =3D NULL; > > } else { > > /* kernel-side HPT not needed, allocate in userspace instead */ > > size_t size =3D 1ULL << shift; > > @@ -1106,7 +1106,6 @@ static void spapr_reallocate_hpt(sPAPRMachineStat= e *spapr, int shift, > >=20 > > memset(spapr->htab, 0, size); > > spapr->htab_shift =3D shift; > > - kvmppc_kern_htab =3D false; > >=20 > > for (i =3D 0; i < size / HASH_PTE_SIZE_64; i++) { > > DIRTY_HPTE(HPTE(spapr->htab, i)); > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > index 1733482..b2b1b93 100644 > > --- a/hw/ppc/spapr_hcall.c > > +++ b/hw/ppc/spapr_hcall.c > > @@ -122,17 +122,17 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAP= RMachineState *spapr, > > break; > > } > > } > > - ppc_hash64_stop_access(token); > > + ppc_hash64_stop_access(cpu, token); > > if (index =3D=3D 8) { > > return H_PTEG_FULL; > > } > > } else { > > token =3D ppc_hash64_start_access(cpu, pte_index); > > if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) { > > - ppc_hash64_stop_access(token); > > + ppc_hash64_stop_access(cpu, token); > > return H_PTEG_FULL; > > } > > - ppc_hash64_stop_access(token); > > + ppc_hash64_stop_access(cpu, token); > > } > >=20 > > ppc_hash64_store_hpte(cpu, pte_index + index, > > @@ -165,7 +165,7 @@ static RemoveResult remove_hpte(PowerPCCPU *cpu, ta= rget_ulong ptex, > > token =3D ppc_hash64_start_access(cpu, ptex); > > v =3D ppc_hash64_load_hpte0(cpu, token, 0); > > r =3D ppc_hash64_load_hpte1(cpu, token, 0); > > - ppc_hash64_stop_access(token); > > + ppc_hash64_stop_access(cpu, token); > >=20 > > if ((v & HPTE64_V_VALID) =3D=3D 0 || > > ((flags & H_AVPN) && (v & ~0x7fULL) !=3D avpn) || > > @@ -288,7 +288,7 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAP= RMachineState *spapr, > > token =3D ppc_hash64_start_access(cpu, pte_index); > > v =3D ppc_hash64_load_hpte0(cpu, token, 0); > > r =3D ppc_hash64_load_hpte1(cpu, token, 0); > > - ppc_hash64_stop_access(token); > > + ppc_hash64_stop_access(cpu, token); > >=20 > > if ((v & HPTE64_V_VALID) =3D=3D 0 || > > ((flags & H_AVPN) && (v & ~0x7fULL) !=3D avpn)) { > > diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c > > index 7b5200b..a9ae0b3 100644 > > --- a/target-ppc/mmu-hash64.c > > +++ b/target-ppc/mmu-hash64.c > > @@ -36,10 +36,11 @@ > > #endif > >=20 > > /* > > - * Used to indicate whether we have allocated htab in the > > - * host kernel > > + * Used to indicate that a CPU has it's hash page table (HPT) managed >=20 > s/it's/its Oh dear. That mispelling really annoys me, so it's an embarrassing error. Thanks for the catch. --=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 --tZCmRiovzb4sxAVa Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW3h57AAoJEGw4ysog2bOSinUP/0o9V13r4HoxbpiCD5eofw3V 2/ATuIFxAn1AC/Q0Mo0qOTu7AygfdyzOVm8QlyiOpPQKbiyBC4mZayBWQRe2mzlH RxruF00Y0LGiKWjIowOSt2DsXdQBbUXEhiFOUhmfbHQjr37k+5awwr/mtpAQje2J nCJHTMEYLtK4gln4iMW9VB0dYddnzovaK1WQ70yZttucy4oCaw644/1gT5LOLbWI eMPrHGkEHW8kkbt11XouFnHwXe8tr7RLGnwp8FM8aeF8GlM7ywM8Y6+Y76IofAp5 z9SonEh4YKgVQPJRs+O09eRFiwWcaYwVrz1gHydLLaLViZzxRaZXjFztuNQaHEq9 XDLTq4rVqrWgfL5K/t16XDaNkp5VMSJF4AoxV+6UZH0l3W43lGFLBhx5AaVpgJXA gKqSLc/evWfhb/6SXuTkSL72s+N2Z4pi9B+IcNU6EFTMP5Og2rIpIH4x1Q4g+RiA yuHBKyFquxr7ybckruwZHx2yv8XhNDhCuRO12pytlOpNf56CFrp2Uf+m9FpLhVmj TEJFr+LBAxL2PJFgkUffrLRYjcMKrJ606A5J4Kz3p4oOmIUKz2IfDRrDR+VhXKlS GUPBNYjkfpFNLzD61NND22L0eTdvt7eqEdA7b5ifQqclvsyCNuRSCGi5VxAvg2UL lPBgQ5PqwcdDY95yFaJg =MVMM -----END PGP SIGNATURE----- --tZCmRiovzb4sxAVa--