On Mon, Mar 07, 2016 at 12:22:04PM +0100, Thomas Huth wrote:
> On 07.03.2016 03:26, David Gibson wrote:
> > Currently the getting and setting of Power MMU registers (sregs) take up
> > 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 in
> > practice, this is pretty hard to read.
> >
> > This patch splits these out into helper functions for clarity. No
> > functional change is expected.
> >
> > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> > ---
> > target-ppc/kvm.c | 421
> > ++++++++++++++++++++++++++++++-------------------------
> > 1 file changed, 228 insertions(+), 193 deletions(-)
> >
> > 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 = POWERPC_CPU(cs);
> > @@ -920,39 +958,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> > }
> >
> > if (cap_segstate && (level >= KVM_PUT_RESET_STATE)) {
> > - struct kvm_sregs sregs;
> > -
> > - sregs.pvr = env->spr[SPR_PVR];
> > -
> > - sregs.u.s.sdr1 = env->spr[SPR_SDR1];
> > -
> > - /* Sync SLB */
> > -#ifdef TARGET_PPC64
> > - for (i = 0; i < ARRAY_SIZE(env->slb); i++) {
> > - sregs.u.s.ppc64.slb[i].slbe = env->slb[i].esid;
> > - if (env->slb[i].esid & SLB_ESID_V) {
> > - sregs.u.s.ppc64.slb[i].slbe |= i;
> > - }
> > - sregs.u.s.ppc64.slb[i].slbv = env->slb[i].vsid;
> > - }
> > -#endif
> > -
> > - /* Sync SRs */
> > - for (i = 0; i < 16; i++) {
> > - sregs.u.s.ppc32.sr[i] = env->sr[i];
> > - }
> > -
> > - /* Sync BATs */
> > - for (i = 0; i < 8; i++) {
> > - /* Beware. We have to swap upper and lower bits here */
> > - sregs.u.s.ppc32.dbat[i] = ((uint64_t)env->DBAT[0][i] << 32)
> > - | env->DBAT[1][i];
> > - sregs.u.s.ppc32.ibat[i] = ((uint64_t)env->IBAT[0][i] << 32)
> > - | env->IBAT[1][i];
> > - }
> > -
> > - ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> > - if (ret) {
> > + ret = kvmppc_put_books_sregs(cpu);
> > + if (ret < 0) {
> > return ret;
> > }
>
> Nit: Technically you've changed the check for the return code from
> "ret != 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, int
> > vector, int ivor)
> > env->excp_vectors[vector] = env->spr[ivor] + env->spr[SPR_BOOKE_IVPR];
> > }
> >
> > +static int kvmppc_get_booke_sregs(PowerPCCPU *cpu)
> > +{
> > + CPUPPCState *env = &cpu->env;
> > + struct kvm_sregs sregs;
> > + int ret;
> ...
> > +
> > + if (sregs.u.e.features & KVM_SREGS_E_ARCH206_MMU) {
> > + env->spr[SPR_BOOKE_MAS0] = sregs.u.e.mas0;
> > + env->spr[SPR_BOOKE_MAS1] = sregs.u.e.mas1;
> > + env->spr[SPR_BOOKE_MAS2] = sregs.u.e.mas2;
> > + env->spr[SPR_BOOKE_MAS3] = sregs.u.e.mas7_3 & 0xffffffff;
> > + env->spr[SPR_BOOKE_MAS4] = sregs.u.e.mas4;
> > + env->spr[SPR_BOOKE_MAS6] = sregs.u.e.mas6;
> > + env->spr[SPR_BOOKE_MAS7] = sregs.u.e.mas7_3 >> 32;
> > + env->spr[SPR_MMUCFG] = sregs.u.e.mmucfg;
> > + env->spr[SPR_BOOKE_TLB0CFG] = sregs.u.e.tlbcfg[0];
> > + env->spr[SPR_BOOKE_TLB1CFG] = sregs.u.e.tlbcfg[1];
> > + }
>
> Cosmetical nit: That closing curly bracket should not be indented by 8
> spaces, but by 4.
Oops, thanks for the catch.
> 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.
Done, thanks.
--
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
signature.asc
Description: PGP signature
