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

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

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?

> +            }
> +        }
>      }

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>  #if defined(DUMP_PAGE_TABLES)
>      if (qemu_loglevel_mask(CPU_LOG_MMU)) {