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

["Nicholas Piggin" <npiggin@gmail.com>]

On Thu Jul 4, 2024 at 10:34 PM AEST, BALATON Zoltan wrote:
> 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 

Yeah that's fine I wouldn't expect it to change here, just wondering
if you'd dug into it more. I *think* that is the reaon for it.

Thanks,
Nick

> 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)) {
> >
> >