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

[Thomas Huth <thuth@redhat.com>]

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.

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