From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48588) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aixug-0008SO-R0 for qemu-devel@nongnu.org; Thu, 24 Mar 2016 01:34:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aixuf-00056T-3A for qemu-devel@nongnu.org; Thu, 24 Mar 2016 01:34:30 -0400 Date: Thu, 24 Mar 2016 16:35:35 +1100 From: David Gibson Message-ID: <20160324053535.GC23586@voom.redhat.com> 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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="1MovezAcxqLtNama" Content-Disposition: inline In-Reply-To: <56F173E9.2060702@redhat.com> Subject: Re: [Qemu-devel] [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: Laurent Vivier Cc: thuth@redhat.com, aik@ozlabs.ru, qemu-devel@nongnu.org, agraf@suse.de, qemu-ppc@nongnu.org, gkurz@linux.vnet.ibm.com --1MovezAcxqLtNama Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 22, 2016 at 05:33:45PM +0100, Laurent Vivier wrote: > Hi David, >=20 > using kvm-unit-tests, I've found a side effect of your patches: the MSR > is cleared (and perhaps some others). >=20 > I was trying to test my patch on top of QEMU master: >=20 > "ppc64: set MSR_SF bit" > http://patchwork.ozlabs.org/patch/598198/ >=20 > and it was not working anymore. >=20 > By bisecting, I've found this commit. >=20 > I think "cpu_synchronize_state()" in "ppc_hash64_set_external_hpt()" > restores the MSR from KVM whereas the one from QEMU has not been saved, > because cpu_synchronize_all_post_reset() is called later. >=20 > So it is cleared. >=20 > You can test this by applying the MSR_SF patch and using the "emulator" > test of kvm-unit-tests (the "emulator: 64bit" test case) Ugh, you're right of course. But, I'm having a bit of trouble figuring out how to fix it propertly. >=20 > Laurent >=20 > On 07/03/2016 03:26, David Gibson wrote: > > When a Power cpu with 64-bit hash MMU has it's hash page table (HPT) > > pointer updated by a write to the SDR1 register we need to update some > > derived variables. Likewise, when the cpu is configured for an external > > HPT (one not in the guest memory space) some derived variables need to = be > > updated. > >=20 > > Currently the logic for this is (partially) duplicated in ppc_store_sdr= 1() > > and in spapr_cpu_reset(). In future we're going to need it in some oth= er > > places, so make some common helpers for this update. > >=20 > > In addition the new ppc_hash64_set_external_hpt() helper also updates > > SDR1 in KVM - it's not updated by the normal runtime KVM <-> qemu CPU > > synchronization. In a sense this belongs logically in the > > ppc_hash64_set_sdr1() helper, but that is called from > > kvm_arch_get_registers() so can't itself call cpu_synchronize_state() > > without infinite recursion. In practice this doesn't matter because > > the only other caller is TCG specific. > >=20 > > Currently there aren't situations where updating SDR1 at runtime in KVM > > matters, but there are going to be in future. > >=20 > > Signed-off-by: David Gibson > > --- > > hw/ppc/spapr.c | 13 ++----------- > > target-ppc/kvm.c | 2 +- > > target-ppc/kvm_ppc.h | 6 ++++++ > > target-ppc/mmu-hash64.c | 43 +++++++++++++++++++++++++++++++++++++++++= ++ > > target-ppc/mmu-hash64.h | 6 ++++++ > > target-ppc/mmu_helper.c | 13 ++++++------- > > 6 files changed, 64 insertions(+), 19 deletions(-) > >=20 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 64c4acc..72a018b 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1196,17 +1196,8 @@ 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; > > - /* > > - * htab_mask is the mask used to normalize hash value to PTEG inde= x. > > - * htab_shift is log2 of hash table size. > > - * We have 8 hpte per group, and each hpte is 16 bytes. > > - * ie have 128 bytes per hpte entry. > > - */ > > - env->htab_mask =3D (1ULL << (spapr->htab_shift - 7)) - 1; > > - env->spr[SPR_SDR1] =3D (target_ulong)(uintptr_t)spapr->htab | > > - (spapr->htab_shift - 18); > > + ppc_hash64_set_external_hpt(cpu, spapr->htab, spapr->htab_shift, > > + &error_fatal); > > } > > =20 > > static void spapr_create_nvram(sPAPRMachineState *spapr) > > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > > index 8a762e8..380dff6 100644 > > --- a/target-ppc/kvm.c > > +++ b/target-ppc/kvm.c > > @@ -867,7 +867,7 @@ static int kvm_put_vpa(CPUState *cs) > > } > > #endif /* TARGET_PPC64 */ > > =20 > > -static int kvmppc_put_books_sregs(PowerPCCPU *cpu) > > +int kvmppc_put_books_sregs(PowerPCCPU *cpu) > > { > > CPUPPCState *env =3D &cpu->env; > > struct kvm_sregs sregs; > > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > > index fd64c44..fc79312 100644 > > --- a/target-ppc/kvm_ppc.h > > +++ b/target-ppc/kvm_ppc.h > > @@ -55,6 +55,7 @@ void kvmppc_hash64_write_pte(CPUPPCState *env, target= _ulong pte_index, > > target_ulong pte0, target_ulong pte1); > > bool kvmppc_has_cap_fixup_hcalls(void); > > int kvmppc_enable_hwrng(void); > > +int kvmppc_put_books_sregs(PowerPCCPU *cpu); > > =20 > > #else > > =20 > > @@ -246,6 +247,11 @@ static inline int kvmppc_enable_hwrng(void) > > { > > return -1; > > } > > + > > +static inline int kvmppc_put_books_sregs(PowerPCCPU *cpu) > > +{ > > + abort(); > > +} > > #endif > > =20 > > #ifndef CONFIG_KVM > > diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c > > index 9c58fbf..7b5200b 100644 > > --- a/target-ppc/mmu-hash64.c > > +++ b/target-ppc/mmu-hash64.c > > @@ -258,6 +258,49 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env= , target_ulong rb) > > /* > > * 64-bit hash table MMU handling > > */ > > +void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value, > > + Error **errp) > > +{ > > + CPUPPCState *env =3D &cpu->env; > > + target_ulong htabsize =3D value & SDR_64_HTABSIZE; > > + > > + env->spr[SPR_SDR1] =3D value; > > + if (htabsize > 28) { > > + error_setg(errp, > > + "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1= ", > > + htabsize); > > + htabsize =3D 28; > > + } > > + env->htab_mask =3D (1ULL << (htabsize + 18 - 7)) - 1; > > + env->htab_base =3D value & SDR_64_HTABORG; > > +} > > + > > +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift, > > + Error **errp) > > +{ > > + CPUPPCState *env =3D &cpu->env; > > + Error *local_err =3D NULL; > > + > > + cpu_synchronize_state(CPU(cpu)); > > + > > + env->external_htab =3D hpt; > > + ppc_hash64_set_sdr1(cpu, (target_ulong)(uintptr_t)hpt | (shift - 1= 8), > > + &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + /* Not strictly necessary, but makes it clearer that an external > > + * htab is in use when debugging */ > > + env->htab_base =3D -1; > > + > > + if (kvm_enabled()) { > > + if (kvmppc_put_books_sregs(cpu) < 0) { > > + error_setg(errp, "Unable to update SDR1 in KVM"); > > + } > > + } > > +} > > =20 > > static int ppc_hash64_pte_prot(PowerPCCPU *cpu, > > ppc_slb_t *slb, ppc_hash_pte64_t pte) > > diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h > > index e7d9925..138cd7e 100644 > > --- a/target-ppc/mmu-hash64.h > > +++ b/target-ppc/mmu-hash64.h > > @@ -92,6 +92,12 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU= *cpu, > > =20 > > =20 > > extern bool kvmppc_kern_htab; > > + > > +void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value, > > + Error **errp); > > +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift, > > + Error **errp); > > + > > uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_ind= ex); > > void ppc_hash64_stop_access(uint64_t token); > > =20 > > diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c > > index e5ec8d6..fcb2cc5 100644 > > --- a/target-ppc/mmu_helper.c > > +++ b/target-ppc/mmu_helper.c > > @@ -2005,15 +2005,14 @@ void ppc_store_sdr1(CPUPPCState *env, target_ul= ong value) > > env->spr[SPR_SDR1] =3D value; > > #if defined(TARGET_PPC64) > > if (env->mmu_model & POWERPC_MMU_64) { > > - target_ulong htabsize =3D value & SDR_64_HTABSIZE; > > + PowerPCCPU *cpu =3D ppc_env_get_cpu(env); > > + Error *local_err =3D NULL; > > =20 > > - if (htabsize > 28) { > > - fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx > > - " stored in SDR1\n", htabsize); > > - htabsize =3D 28; > > + ppc_hash64_set_sdr1(cpu, value, &local_err); > > + if (local_err) { > > + error_report_err(local_err); > > + error_free(local_err); > > } > > - env->htab_mask =3D (1ULL << (htabsize + 18 - 7)) - 1; > > - env->htab_base =3D value & SDR_64_HTABORG; > > } else > > #endif /* defined(TARGET_PPC64) */ > > { > >=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 --1MovezAcxqLtNama Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW83ynAAoJEGw4ysog2bOSamMP/33kb3x/nrEs2vOcmlwDOu2s F5SIuU/m62YlAD7XD1lvSQz82rHsjknAXDq4QFhgemGvrQnE/tO0Kvt5+dCi0did gbYmg61r3ZRiD08U4NS7hX3BqGVoVx1Z8RzFPULiY+d18TvMwaUB4zSfjjPT+pH/ VnaxhUsCovGsBEMWfLB4wNGA1cqJiNHhj5kT1pZ1Q8WI45qG9mTm/QAWyMa1ORph Tr+RAIqsHqD2fqb18kGhqNkLlSms8svD2LbzYL5a654KUpcrn1wF4uKogflxRX3l RR4I2lqQyV3amMhKHaFhkrKCfa3ZGGOoKVaa//OukxwMZFXiYzM/b2KXca3CaSec +/shF4njsW6utIKK8yRORGc4wv+qhi5RPdCrhu7JxdfTIxe2tVqtTT584xj3ZYYX Lvfc+9jUUp80Qtj+TmI8s4mAEjrdajM0Yk69rwDN8AF/itFDo92ZDTbV+m8Utu03 Nk48a5qFPx28rjLGHaeBLtHlM8IWOBpkvisI9O0ZRIBwBOhhVuPZ4aisua2v/Yds GpgTq+WF1AlhJiXQGquRLjyKSccKkn0mc0/ZwQv+2nReeHlO3YqSCU/emE2I6INm 72g/XEXzELUvXcb9ifWiXFi4GoInBOd4s/ZNkBp3vxE0fYTyQgk/0Ke/MuM+WTdF 2r6QeYjo29GiylmmAnLd =40qS -----END PGP SIGNATURE----- --1MovezAcxqLtNama--