Re: [PATCH v7 28/61] target/ppc/mmu_common.c: Remove pte_update_flags()

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

On Mon May 13, 2024 at 9:28 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.

Debatable. It's definitely clunky code that could use some
love.

But without looking at details I would bet it's actually cleaner
to inline this into ppc6xx_tlb_pte_check since that is what deals
with the ptes.

Might leave this patch out for the first PR and see how things
settle.

Logic is odd too, or at least I don't really understand it or
intricacies of 6xx mmu. . Access bit is set even for access
violation? Store rejection logic I don't quite understand. Not
that I suggest changing anything in a cleanup series, but
would be nice to untangle and comment unusual cases a bit more
at least.

Thanks,
Nick

>
> 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 34200d9cb1..4fb93cbf40 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -179,39 +179,14 @@ static int ppc6xx_tlb_pte_check(mmu_ctx_t *ctx, target_ulong pte0,
>      return ret;
>  }
>  
> -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 */
> @@ -264,7 +239,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;
> +            }
> +        }
>      }
>  #if defined(DUMP_PAGE_TABLES)
>      if (qemu_loglevel_mask(CPU_LOG_MMU)) {