From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40272) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1actEs-0005TP-Aj for qemu-devel@nongnu.org; Mon, 07 Mar 2016 06:22:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1actEn-0005Bc-TF for qemu-devel@nongnu.org; Mon, 07 Mar 2016 06:22:14 -0500 References: <1457317586-15122-1-git-send-email-david@gibson.dropbear.id.au> <1457317586-15122-2-git-send-email-david@gibson.dropbear.id.au> From: Thomas Huth Message-ID: <56DD645C.8010309@redhat.com> Date: Mon, 7 Mar 2016 12:22:04 +0100 MIME-Version: 1.0 In-Reply-To: <1457317586-15122-2-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCHv2 1/3] target-ppc: Split out SREGS get/put functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson , gkurz@linux.vnet.ibm.com, aik@ozlabs.ru Cc: lvivier@redhat.com, qemu-ppc@nongnu.org, agraf@suse.de, qemu-devel@nongnu.org On 07.03.2016 03:26, David Gibson wrote: > Currently the getting and setting of Power MMU registers (sregs) take u= p > large inline chunks of the kvm_arch_get_registers() and > kvm_arch_put_registers() functions. Especially since there are two > variants (for Book-E and Book-S CPUs), only one of which will be used i= n > practice, this is pretty hard to read. >=20 > This patch splits these out into helper functions for clarity. No > functional change is expected. >=20 > Signed-off-by: David Gibson > --- > target-ppc/kvm.c | 421 ++++++++++++++++++++++++++++++-----------------= -------- > 1 file changed, 228 insertions(+), 193 deletions(-) >=20 > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index d67c169..8a762e8 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c ... > int kvm_arch_put_registers(CPUState *cs, int level) > { > PowerPCCPU *cpu =3D POWERPC_CPU(cs); > @@ -920,39 +958,8 @@ int kvm_arch_put_registers(CPUState *cs, int level= ) > } > =20 > if (cap_segstate && (level >=3D KVM_PUT_RESET_STATE)) { > - struct kvm_sregs sregs; > - > - sregs.pvr =3D env->spr[SPR_PVR]; > - > - sregs.u.s.sdr1 =3D env->spr[SPR_SDR1]; > - > - /* Sync SLB */ > -#ifdef TARGET_PPC64 > - for (i =3D 0; i < ARRAY_SIZE(env->slb); i++) { > - sregs.u.s.ppc64.slb[i].slbe =3D env->slb[i].esid; > - if (env->slb[i].esid & SLB_ESID_V) { > - sregs.u.s.ppc64.slb[i].slbe |=3D i; > - } > - sregs.u.s.ppc64.slb[i].slbv =3D env->slb[i].vsid; > - } > -#endif > - > - /* Sync SRs */ > - for (i =3D 0; i < 16; i++) { > - sregs.u.s.ppc32.sr[i] =3D env->sr[i]; > - } > - > - /* Sync BATs */ > - for (i =3D 0; i < 8; i++) { > - /* Beware. We have to swap upper and lower bits here */ > - sregs.u.s.ppc32.dbat[i] =3D ((uint64_t)env->DBAT[0][i] << = 32) > - | env->DBAT[1][i]; > - sregs.u.s.ppc32.ibat[i] =3D ((uint64_t)env->IBAT[0][i] << = 32) > - | env->IBAT[1][i]; > - } > - > - ret =3D kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs); > - if (ret) { > + ret =3D kvmppc_put_books_sregs(cpu); > + if (ret < 0) { > return ret; > } Nit: Technically you've changed the check for the return code from "ret !=3D 0" to "ret < 0", so this is a small functional change. But practically, it should not matter, since the ioctl is not supposed to return values > 0, I think. > } > @@ -1014,12 +1021,197 @@ static void kvm_sync_excp(CPUPPCState *env, in= t vector, int ivor) > env->excp_vectors[vector] =3D env->spr[ivor] + env->spr[SPR_BOOKE= _IVPR]; > } > =20 > +static int kvmppc_get_booke_sregs(PowerPCCPU *cpu) > +{ > + CPUPPCState *env =3D &cpu->env; > + struct kvm_sregs sregs; > + int ret; ... > + > + if (sregs.u.e.features & KVM_SREGS_E_ARCH206_MMU) { > + env->spr[SPR_BOOKE_MAS0] =3D sregs.u.e.mas0; > + env->spr[SPR_BOOKE_MAS1] =3D sregs.u.e.mas1; > + env->spr[SPR_BOOKE_MAS2] =3D sregs.u.e.mas2; > + env->spr[SPR_BOOKE_MAS3] =3D sregs.u.e.mas7_3 & 0xffffffff; > + env->spr[SPR_BOOKE_MAS4] =3D sregs.u.e.mas4; > + env->spr[SPR_BOOKE_MAS6] =3D sregs.u.e.mas6; > + env->spr[SPR_BOOKE_MAS7] =3D sregs.u.e.mas7_3 >> 32; > + env->spr[SPR_MMUCFG] =3D sregs.u.e.mmucfg; > + env->spr[SPR_BOOKE_TLB0CFG] =3D sregs.u.e.tlbcfg[0]; > + env->spr[SPR_BOOKE_TLB1CFG] =3D sregs.u.e.tlbcfg[1]; > + } Cosmetical nit: That closing curly bracket should not be indented by 8 spaces, but by 4. Apart from these two nits, the patch looks good to me, so feel free to add my "Reviewed-by" once you've fixed add least the cosmetical nit. Thomas