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.
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
