Re: [Qemu-ppc] [QEMU-PPC] [PATCH V4 06/11] target/ppc: Remove the function ppc_hash64_set_sdr1()

[Suraj Jitindar Singh]

On Fri, 2017-02-24 at 15:59 +1100, David Gibson wrote:
> On Fri, Feb 24, 2017 at 12:05:12PM +1100, Suraj Jitindar Singh wrote:
> > 
> > The function ppc_hash64_set_sdr1 basically checked the htabsize and
> > set an
> > error if it was too big, otherwise it just stored the value in
> > SPR_SDR1.
> > 
> > Given that the only function which calls ppc_hash64_set_sdr1() is
> > ppc_store_sdr1(), why not handle the checking in ppc_store_sdr1()
> > to avoid
> > the extra function call. Note that ppc_store_sdr1() already stores
> > the
> > value in SPR_SDR1 anyway, so we were doing it twice.
> > 
> > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> This is a sensible cleanup to follow on from my series.  One nit..
> 
> 
> 
> > 
> > 
> > ---
> > 
> > V3 -> V4:
> >  - Split patch from next patch in series
> >  - Removed unnecessary function instead of renaming it
> > ---
> >  target/ppc/mmu-hash64.c | 18 ------------------
> >  target/ppc/mmu-hash64.h |  3 ---
> >  target/ppc/mmu_helper.c | 16 +++++++++-------
> >  3 files changed, 9 insertions(+), 28 deletions(-)
> > 
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index 0c6a1e7..7e91290 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -289,24 +289,6 @@ target_ulong helper_load_slb_vsid(CPUPPCState
> > *env, target_ulong rb)
> >      return rt;
> >  }
> >  
> > -/*
> > - * 64-bit hash table MMU handling
> > - */
> > -void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> > -                         Error **errp)
> > -{
> > -    CPUPPCState *env = &cpu->env;
> > -    target_ulong htabsize = value & SDR_64_HTABSIZE;
> > -
> > -    if (htabsize > 28) {
> > -        error_setg(errp,
> > -                   "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in
> > SDR1",
> > -                   htabsize);
> > -        return;
> > -    }
> > -    env->spr[SPR_SDR1] = value;
> > -}
> > -
> >  static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
> >                                 ppc_slb_t *slb, ppc_hash_pte64_t
> > pte)
> >  {
> > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> > index 9c33e9d..a2a6748 100644
> > --- a/target/ppc/mmu-hash64.h
> > +++ b/target/ppc/mmu-hash64.h
> > @@ -109,9 +109,6 @@ static inline hwaddr
> > ppc_hash64_hpt_mask(PowerPCCPU *cpu)
> >      return (1ULL << ((cpu->env.spr[SPR_SDR1] & SDR_64_HTABSIZE) +
> > 18 - 7)) - 1;
> >  }
> >  
> > -void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> > -                         Error **errp);
> > -
> >  struct ppc_hash_pte64 {
> >      uint64_t pte0, pte1;
> >  };
> > diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> > index 0176ab6..eb5c35f 100644
> > --- a/target/ppc/mmu_helper.c
> > +++ b/target/ppc/mmu_helper.c
> > @@ -2006,13 +2006,15 @@ void ppc_store_sdr1(CPUPPCState *env,
> > target_ulong value)
> >      assert(!cpu->vhyp);
> >  #if defined(TARGET_PPC64)
> >      if (env->mmu_model & POWERPC_MMU_64) {
> > -        PowerPCCPU *cpu = ppc_env_get_cpu(env);
> > -        Error *local_err = NULL;
> > -
> > -        ppc_hash64_set_sdr1(cpu, value, &local_err);
> > -        if (local_err) {
> > -            error_report_err(local_err);
> > -            error_free(local_err);
> > +        Error *errp = NULL;
> > +        target_ulong htabsize = value & SDR_64_HTABSIZE;
> > +
> > +        if (htabsize > 28) {
> > +            error_setg(&errp,
> > +                       "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored
> > in SDR1",
> > +                       htabsize);
> > +            error_report_err(errp);
> > +            error_free(errp);
> >              return;
> The above is basically equivalent to just an error_report() and
> return;

Thats what I get for reusing the code...

> 
> > 
> >          }
> >      }