Re: [PATCH 11/43] target/ppc/mmu_common.c: Remove pte_update_flags()

[BALATON Zoltan]

On Thu, 4 Jul 2024, Nicholas Piggin wrote:
> On Mon May 27, 2024 at 9:12 AM AEST, BALATON Zoltan wrote:
> > This function is used only once, its return value is ignored and one
> > of its parameter is a return value from a previous call. It is better
> > to inline it in the caller and remove it.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> >  target/ppc/mmu_common.c | 41 +++++++++++++----------------------------
> >  1 file changed, 13 insertions(+), 28 deletions(-)
> >
> > diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> > index e3537c63c0..c4902b7632 100644
> > --- a/target/ppc/mmu_common.c
> > +++ b/target/ppc/mmu_common.c
> > @@ -119,39 +119,14 @@ static int ppc6xx_tlb_pte_check(mmu_ctx_t *ctx, 
> > target_ulong pte0,
> >      }
> >  }
> >
> > -static int pte_update_flags(mmu_ctx_t *ctx, target_ulong *pte1p,
> > -                            int ret, MMUAccessType access_type)
> > -{
> > -    int store = 0;
> > -
> > -    /* Update page flags */
> > -    if (!(*pte1p & 0x00000100)) {
> > -        /* Update accessed flag */
> > -        *pte1p |= 0x00000100;
> > -        store = 1;
> > -    }
> > -    if (!(*pte1p & 0x00000080)) {
> > -        if (access_type == MMU_DATA_STORE && ret == 0) {
> > -            /* Update changed flag */
> > -            *pte1p |= 0x00000080;
> > -            store = 1;
> > -        } else {
> > -            /* Force page fault for first write access */
> > -            ctx->prot &= ~PAGE_WRITE;
> > -        }
> > -    }
> > -
> > -    return store;
> > -}
> > -
> >  /* Software driven TLB helpers */
> >
> >  static int ppc6xx_tlb_check(CPUPPCState *env, mmu_ctx_t *ctx,
> >                              target_ulong eaddr, MMUAccessType access_type)
> >  {
> >      ppc6xx_tlb_t *tlb;
> > -    int nr, best, way;
> > -    int ret;
> > +    target_ulong *pte1p;
> > +    int nr, best, way, ret;
> >
> >      best = -1;
> >      ret = -1; /* No TLB found */
> > @@ -204,7 +179,17 @@ done:
> >                        " prot=%01x ret=%d\n",
> >                        ctx->raddr & TARGET_PAGE_MASK, ctx->prot, ret);
> >          /* Update page flags */
> > -        pte_update_flags(ctx, &env->tlb.tlb6[best].pte1, ret, access_type);
> > +        pte1p = &env->tlb.tlb6[best].pte1;
> > +        *pte1p |= 0x00000100; /* Update accessed flag */
> > +        if (!(*pte1p & 0x00000080)) {
> > +            if (access_type == MMU_DATA_STORE && ret == 0) {
> > +                /* Update changed flag */
> > +                *pte1p |= 0x00000080;
> > +            } else {
> > +                /* Force page fault for first write access */
> > +                ctx->prot &= ~PAGE_WRITE;
>
> Out of curiosity, I guess this unusual part is because ctx->prot can get
> PAGE_WRITE set in the bat lookup, then it has to be cleared if the PTE
> does not have changed bit?

I have no idea. I was just trying to clean up this code to make it simpler 
with this series. I think historically there was a single function that 
handled all models but as these became too different it was split up by 
MMU models. It could be some of this are remnants of some old code where 
some other model needed it and not needed any more or this could be merged 
with hash32 but I did not try to find that out, just try to make sure not 
to break it any more than it might already be broken.

Regards,
BALATON Zoltan

> > +            }
> > +        }
> >      }
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> >  #if defined(DUMP_PAGE_TABLES)
> >      if (qemu_loglevel_mask(CPU_LOG_MMU)) {