Re: [PATCH v2 10/28] target/ppc/mmu_common.c: Introduce mmu6xx_get_physical_address()

[Nicholas Piggin]

On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote:
> Repurpose get_segment_6xx_tlb() to do the whole address translation
> for POWERPC_MMU_SOFT_6xx MMU model by moving the BAT check there and
> renaming it to match other similar functions. These are only called
> once together so no need to keep these separate functions and
> combining them simplifies the caller allowing further restructuring.

Looks good...

>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  target/ppc/mmu_common.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index 98730035b1..ef1669b01d 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -359,19 +359,25 @@ static int get_bat_6xx_tlb(CPUPPCState *env, mmu_ctx_t 
> *ctx,
>      return ret;
>  }
>  
> -/* Perform segment based translation */
> -static int get_segment_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx,
> -                               target_ulong eaddr, MMUAccessType access_type,
> -                               int type)
> +static int mmu6xx_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
> +                                       target_ulong eaddr,
> +                                       MMUAccessType access_type, int type)
>  {
>      PowerPCCPU *cpu = env_archcpu(env);
>      hwaddr hash;
> -    target_ulong vsid;
> -    int ds, target_page_bits;
> +    target_ulong vsid, sr, pgidx;
>      bool pr;
> -    int ret;
> -    target_ulong sr, pgidx;
> +    int ds, target_page_bits, ret = -1;
>  
> +    /* First try to find a BAT entry if there are any */
> +    if (env->nb_BATs != 0) {
> +        ret = get_bat_6xx_tlb(env, ctx, eaddr, access_type);
> +    }
> +    if (ret >= 0) {
> +        return ret;
> +    }

Would you consider not doing any rearranging of local variables there
and change this as:

    /* First try to find a BAT entry if there are any */
    if (env->nb_BATs != 0) {
        int ret = get_bat_6xx_tlb(env, ctx, eaddr, access_type);
        if (ret >= 0) {
            return ret;
        }
    }

Otherwise,

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

> +    /* Perform segment based translation when no BATs matched */
>      pr = FIELD_EX64(env->msr, MSR, PR);
>      ctx->eaddr = eaddr;
>  
> @@ -1193,14 +1199,8 @@ int get_physical_address_wtlb(CPUPPCState *env, 
> mmu_ctx_t *ctx,
>          if (real_mode) {
>              ret = check_physical(env, ctx, eaddr, access_type);
>          } else {
> -            /* Try to find a BAT */
> -            if (env->nb_BATs != 0) {
> -                ret = get_bat_6xx_tlb(env, ctx, eaddr, access_type);
> -            }
> -            if (ret < 0) {
> -                /* We didn't match any BAT entry or don't have BATs */
> -                ret = get_segment_6xx_tlb(env, ctx, eaddr, access_type, 
> type);
> -            }
> +            ret = mmu6xx_get_physical_address(env, ctx, eaddr, access_type,
> +                                              type);
>          }
>          break;
>