From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42667) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acksY-0006Xp-4H for qemu-devel@nongnu.org; Sun, 06 Mar 2016 21:26:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1acksU-0007y9-SQ for qemu-devel@nongnu.org; Sun, 06 Mar 2016 21:26:38 -0500 Date: Mon, 7 Mar 2016 13:23:47 +1100 From: David Gibson Message-ID: <20160307022347.GA22546@voom.fritz.box> References: <1457069753-13123-1-git-send-email-david@gibson.dropbear.id.au> <1457069753-13123-2-git-send-email-david@gibson.dropbear.id.au> <56D95C75.3050009@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="9amGYk9869ThD9tj" Content-Disposition: inline In-Reply-To: <56D95C75.3050009@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/2] 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: Thomas Huth Cc: lvivier@redhat.com, qemu-devel@nongnu.org, aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, agraf@suse.de, qemu-ppc@nongnu.org, gkurz@linux.vnet.ibm.com --9amGYk9869ThD9tj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 04, 2016 at 10:59:17AM +0100, Thomas Huth wrote: > On 04.03.2016 06:35, 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 | 15 +++++++++++++++ > > target-ppc/kvm_ppc.h | 6 ++++++ > > target-ppc/mmu-hash64.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > target-ppc/mmu-hash64.h | 6 ++++++ > > target-ppc/mmu_helper.c | 13 ++++++------- > > 6 files changed, 77 insertions(+), 18 deletions(-) > >=20 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index e9d4abf..a88e3af 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 d67c169..99b3231 100644 > > --- a/target-ppc/kvm.c > > +++ b/target-ppc/kvm.c > > @@ -2537,3 +2537,18 @@ int kvmppc_enable_hwrng(void) > > =20 > > return kvmppc_enable_hcall(kvm_state, H_RANDOM); > > } > > + > > +int kvmppc_update_sdr1(PowerPCCPU *cpu) > > +{ > > + CPUState *cs =3D CPU(cpu); > > + > > + if (!kvm_enabled()) { > > + return 0; /* nothing to do */ > > + } >=20 > Since you're updating more than SDR1 below ... Could you maybe add a > "assert(cs->kvm_vcpu_dirty)" here, just to make sure that nobody > accidentally ever calls this function without synchronizing the > registers first? Hmm.. that's a good idea, but it doesn't work so well with the one below. >=20 > > + /* The normal KVM_PUT_RUNTIME_STATE doesn't include SDR1, which is > > + * why we need an explicit update for it. KVM_PUT_RESET_STATE is > > + * overkill, but this is a pretty rare operation, so it's simpler > > + * than writing a special purpose updater */ > > + return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE); > > +} >=20 > That indeed looks like a big overkill ... what about extracting the > block from kvm_arch_put_registers() which sets the the "struct > kvm_sregs" via KVM_SET_SREGS into a separate function, and then call > that function here instead? > kvm_arch_put_registers() is IMHO way too big anyway, so separating some > code into external functions there would improve readability there, too. Yeah, fair enough. I'll split that up in a prelim patch for v2. --=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 --9amGYk9869ThD9tj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW3OYzAAoJEGw4ysog2bOS2dMQAOY7Iv3j2B8GmWtKrXiF/OYB a5DF3BLfU2AvwpsPKq4QhlyGfbmhXfXEaeVOEoWD3Tbm/0BiNztobJv3h2SpyhEq ssHfKMsISadFWRUouhsosl+lcYA9Lzwo1uOWvpaNzzMhTr3nli0OlgtXRlTqdwR0 PyKJDVun4QWjiNSEkFyE+DM+b8jWfPiVYgO1Ei9znhFkaqaLmyXQTVzNrEYf4rLn OF1mBCgAE/vwJ0IKFmgdcFzqj8zPPL50F9mkuGMlfJue/9TB2SF+k5BvoVsI20ge /oeGFDmaVvCM3UT2Er0DGROdiLaRoLc7lpcreU0SBj6OM4UEUNRM+nitP8Ug8BKd QkNANdlsqFDXOeFl+RxjiCmt+yfsDR2ckleazmmRfd6RTXlBZ4OqoD2Db7BqUL7j KFoUdMvulLuuFqvcfaEpjcUMXepoJ5qkJi3euCvF0r/AzF2B8KxYQS/JmijL2k5z fRNSwze2sBLIZXOfKmrgmVVAT0GAm9l7r7CEnMYT/u/UegWBO8z5gHahs+JEup62 SeDF5fcHFTusqtan2/4su1778HKQ7EOl2LiFw9uP1v0VPWc/YKukPuEU9Y6Lxyp+ XT5qYyCqakvVztXAvZxNNHmCJiPeGgw2JDYpJMw7r3JKgOxbL3h9Hn5nE5je1yG3 +Elg3RbpjfJ+uGy809Jt =8cFF -----END PGP SIGNATURE----- --9amGYk9869ThD9tj--