Re: [Qemu-ppc] [PATCH 1/8] target/ppc: Add ppc_store_lpcr() helper

[Cédric Le Goater]

On 05/03/2018 08:21 AM, David Gibson wrote:
> There are some fields in the cpu state which need to be updated when the
> LPCR register is changed, which is done by ppc_hash64_update_rmls() and
> ppc_hash64_update_vrma().  Code which alters env->spr[SPR_LPCR] needs to
> call them afterwards to make sure the state is up to date.
> 
> That's easy to get wrong.  The normal way of dealing with sitautions like
> that is to use a helper which both updates the basic register value and the
> derived state.
> 
> So, do that.
> 
> Signed-off-by: David Gibson <address@hidden>

Reviewed-by: Cédric Le Goater <address@hidden>

Although, I am not sure mmu-hash64.c is the right file for the ppc_store_lpcr()
helper. This is minor.

C.

> ---
>  target/ppc/mmu-hash64.c     | 15 +++++++++++----
>  target/ppc/mmu-hash64.h     |  3 +--
>  target/ppc/translate_init.c |  6 +-----
>  3 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 7e0adecfd9..a1db20e3a8 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -942,7 +942,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, 
> target_ulong ptex,
>      cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
>  }
>  
> -void ppc_hash64_update_rmls(PowerPCCPU *cpu)
> +static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      uint64_t lpcr = env->spr[SPR_LPCR];
> @@ -977,7 +977,7 @@ void ppc_hash64_update_rmls(PowerPCCPU *cpu)
>      }
>  }
>  
> -void ppc_hash64_update_vrma(PowerPCCPU *cpu)
> +static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      const PPCHash64SegmentPageSizes *sps = NULL;
> @@ -1028,9 +1028,9 @@ void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>      slb->sps = sps;
>  }
>  
> -void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> +void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>  {
> -    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    CPUPPCState *env = &cpu->env;
>      uint64_t lpcr = 0;
>  
>      /* Filter out bits */
> @@ -1096,6 +1096,13 @@ void helper_store_lpcr(CPUPPCState *env, target_ulong 
> val)
>      ppc_hash64_update_vrma(cpu);
>  }
>  
> +void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +
> +    ppc_store_lpcr(cpu, val);
> +}
> +
>  void ppc_hash64_init(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index f6349ccdb3..53dcec5b93 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -17,8 +17,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
>                                 target_ulong pte0, target_ulong pte1);
>  unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
>                                            uint64_t pte0, uint64_t pte1);
> -void ppc_hash64_update_vrma(PowerPCCPU *cpu);
> -void ppc_hash64_update_rmls(PowerPCCPU *cpu);
> +void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val);
>  void ppc_hash64_init(PowerPCCPU *cpu);
>  void ppc_hash64_finalize(PowerPCCPU *cpu);
>  #endif
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index c83c910a29..3fd380dad6 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8940,15 +8940,11 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, 
> PPCVirtualHypervisor *vhyp)
>      /* We should be followed by a CPU reset but update the active value
>       * just in case...
>       */
> -    env->spr[SPR_LPCR] = lpcr->default_value;
> +    ppc_store_lpcr(cpu, lpcr->default_value);
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
>      env->spr[SPR_AMOR] = amor->default_value = 0xffffffffffffffffull;
>  
> -    /* Update some env bits based on new LPCR value */
> -    ppc_hash64_update_rmls(cpu);
> -    ppc_hash64_update_vrma(cpu);
> -
>      /* Tell KVM that we're in PAPR mode */
>      if (kvm_enabled()) {
>          kvmppc_set_papr(cpu);
>