From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44616) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abi0d-00070U-6E for qemu-devel@nongnu.org; Fri, 04 Mar 2016 00:10:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abi0b-00027U-Ad for qemu-devel@nongnu.org; Fri, 04 Mar 2016 00:10:39 -0500 Date: Fri, 4 Mar 2016 16:10:27 +1100 From: David Gibson Message-ID: <20160304051027.GA1620@voom.redhat.com> References: <1457059242-6673-1-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Do9hQ/bfCb4zBpLo" Content-Disposition: inline In-Reply-To: <1457059242-6673-1-git-send-email-david@gibson.dropbear.id.au> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH] target-ppc/pseries: Clean up handling of KVM managed external HPTs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: gkurz@linux.vnet.ibm.com, aik@ozlabs.ru Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org --Do9hQ/bfCb4zBpLo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 04, 2016 at 01:40:42PM +1100, David Gibson wrote: > fa48b43 "target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KVM" > purports to remove a hack in the handling of hash page tables (HPTs) > managed by KVM instead of qemu. However, it makes the wrong call. >=20 > That patch requires anything looking for an external HPT (that is one not > managed by the guest itself) to check both env->external_htab (for a qemu > 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 places > 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 it = is > like a guest managed HPT, so the original "hack" was actually on the right > track. >=20 > This partially reverts fa48b43, marking 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, and adding a ppc_hash64_set_external_hpt() helper function to > reduce the amount of intimate knowledge of the cpu code that the pseries > machine type needs to set this up correctly. >=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 Self NACK, sorry. Realised this has a stupid omission (and also partially overlaps with another patch I was working on). > --- > hw/ppc/spapr.c | 6 ++---- > hw/ppc/spapr_hcall.c | 10 +++++----- > target-ppc/mmu-hash64.c | 46 +++++++++++++++++++++++++------------------= --- > target-ppc/mmu-hash64.h | 9 ++++----- > 4 files changed, 36 insertions(+), 35 deletions(-) >=20 > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index e9d4abf..d8b749c 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1091,7 +1091,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState = *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(sPAPRMachineState = *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)); > @@ -1196,8 +1195,7 @@ static void spapr_cpu_reset(void *opaque) > =20 > env->spr[SPR_HIOR] =3D 0; > =20 > - env->external_htab =3D (uint8_t *)spapr->htab; > - env->htab_base =3D -1; > + ppc_hash64_set_external_hpt(cpu, spapr->htab); > /* > * htab_mask is the mask used to normalize hash value to PTEG index. > * htab_shift is log2 of hash table size. > 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, sPAPRM= achineState *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, targ= et_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, sPAPRM= achineState *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 9c58fbf..88d4296 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 > + * within the host kernel > */ > -bool kvmppc_kern_htab; > +#define MMU_HASH64_KVM_MANAGED_HPT ((void *)-1) > + > /* > * SLB handling > */ > @@ -259,6 +260,18 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, = target_ulong rb) > * 64-bit hash table MMU handling > */ > =20 > +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt) > +{ > + CPUPPCState *env =3D &cpu->env; > + > + if (hpt) { > + env->external_htab =3D hpt; > + } else { > + env->external_htab =3D MMU_HASH64_KVM_MANAGED_HPT; > + } > + env->htab_base =3D -1; > +} > + > static int ppc_hash64_pte_prot(PowerPCCPU *cpu, > ppc_slb_t *slb, ppc_hash_pte64_t pte) > { > @@ -353,25 +366,16 @@ uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, t= arget_ulong pte_index) > hwaddr pte_offset; > =20 > pte_offset =3D pte_index * HASH_PTE_SIZE_64; > - if (kvmppc_kern_htab) { > + if (cpu->env.external_htab =3D=3D MMU_HASH64_KVM_MANAGED_HPT) { > /* > * HTAB is controlled by KVM. Fetch the PTEG into a new buffer. > */ > token =3D kvmppc_hash64_read_pteg(cpu, pte_index); > - if (token) { > - return token; > - } > + } else if (cpu->env.external_htab) { > /* > - * pteg read failed, even though we have allocated htab via > - * kvmppc_reset_htab. > + * HTAB is controlled by QEMU. Just point to the internally > + * accessible PTEG. > */ > - return 0; > - } > - /* > - * HTAB is controlled by QEMU. Just point to the internally > - * accessible PTEG. > - */ > - if (cpu->env.external_htab) { > token =3D (uint64_t)(uintptr_t) cpu->env.external_htab + pte_off= set; > } else if (cpu->env.htab_base) { > token =3D cpu->env.htab_base + pte_offset; > @@ -379,9 +383,9 @@ uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, tar= get_ulong pte_index) > return token; > } > =20 > -void ppc_hash64_stop_access(uint64_t token) > +void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token) > { > - if (kvmppc_kern_htab) { > + if (cpu->env.external_htab =3D=3D MMU_HASH64_KVM_MANAGED_HPT) { > kvmppc_hash64_free_pteg(token); > } > } > @@ -410,11 +414,11 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cp= u, hwaddr hash, > && HPTE64_V_COMPARE(pte0, ptem)) { > pte->pte0 =3D pte0; > pte->pte1 =3D pte1; > - ppc_hash64_stop_access(token); > + ppc_hash64_stop_access(cpu, token); > return (pte_index + i) * HASH_PTE_SIZE_64; > } > } > - ppc_hash64_stop_access(token); > + ppc_hash64_stop_access(cpu, token); > /* > * We didn't find a valid entry. > */ > @@ -729,7 +733,7 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu, > { > CPUPPCState *env =3D &cpu->env; > =20 > - if (kvmppc_kern_htab) { > + if (env->external_htab =3D=3D MMU_HASH64_KVM_MANAGED_HPT) { > kvmppc_hash64_write_pte(env, pte_index, pte0, pte1); > return; > } > diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h > index e7d9925..f7b1f0d 100644 > --- a/target-ppc/mmu-hash64.h > +++ b/target-ppc/mmu-hash64.h > @@ -90,10 +90,9 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *= cpu, > #define HPTE64_V_1TB_SEG 0x4000000000000000ULL > #define HPTE64_V_VRMA_MASK 0x4001ffffff000000ULL > =20 > - > -extern bool kvmppc_kern_htab; > +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt); > uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index= ); > -void ppc_hash64_stop_access(uint64_t token); > +void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token); > =20 > static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu, > uint64_t token, int ind= ex) > @@ -102,7 +101,7 @@ static inline target_ulong ppc_hash64_load_hpte0(Powe= rPCCPU *cpu, > uint64_t addr; > =20 > addr =3D token + (index * HASH_PTE_SIZE_64); > - if (kvmppc_kern_htab || env->external_htab) { > + if (env->external_htab) { > return ldq_p((const void *)(uintptr_t)addr); > } else { > return ldq_phys(CPU(cpu)->as, addr); > @@ -116,7 +115,7 @@ static inline target_ulong ppc_hash64_load_hpte1(Powe= rPCCPU *cpu, > uint64_t addr; > =20 > addr =3D token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2; > - if (kvmppc_kern_htab || env->external_htab) { > + if (env->external_htab) { > return ldq_p((const void *)(uintptr_t)addr); > } else { > return ldq_phys(CPU(cpu)->as, addr); --=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 --Do9hQ/bfCb4zBpLo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW2RjDAAoJEGw4ysog2bOS3acQAKH3HmkGCg4P/UNWrFofvRxk w9bp1mFmXgxNk6LoBdOxyptZbetzUgLQ52AnTJAfe8FCivS/9HD0JcrSpENCNxus BtMIK4d2XWv3gWrX8WfHSQzwgWfnCxeTIMvaZ0ZcOXhQHjH+SoUGP3hdFrU0ETWe VHjq1aNKCwz9hSANnryffViqTWFFtAm8CdDbs4PnpDxryHIbivyT4h1qnnv94jbj uGpBEg8FI7s/aGLTb7wvWAOxgMkKxhE9TU8+8gQHIgsK/J0mKO5ZLSgn3g8P3lR4 Vtko8IeR+KFXsKS4snTQvK+UTrKR2WTaeT9g2mNixhbRjK4qeoqzNcfThyFElD78 IjU6ya0YE1Qi7cDSjoOjirlfsIPDFrs0pABVHXwQMiVyfOZtIeWjcwPlVdf1YfKM /fqMipn3Q6HfIU3TaoKK7OHnAdgArAqFDT7uihHBw4QYUGGFKShCVoByNy8YFHP9 b9oEFGq1nhHxF/5tSMUYU6aGsCC7qr8jfyC93jSBFs3tfKMGyxRiTICy6pl2b5JX AEkaLzTjXqasBz0bcfrHcx2aPdt9qOAN5XiRyXAqp2fT6pOr7x/skppKLNNAAORB NC8PMcbAyrJxdFdpxlP6WJDQ8B/mD4wA1fCREK1RW3vhnJF+vuXcW+1nA6x9mSMA KHLR3yCayNSjxXhURRcD =CyBP -----END PGP SIGNATURE----- --Do9hQ/bfCb4zBpLo--