Re: [Qemu-devel] [PATCHv2 1/3] target-ppc: Split out SREGS get/put functions

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4342 bytes --]

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 <david@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]