Re: [PATCH 28/43] target/ppc/mmu-hash32.c: Inline and remove ppc_hash32_pte_raddr()

[Nicholas Piggin]

On Mon May 27, 2024 at 9:13 AM AEST, BALATON Zoltan wrote:
> This function is used only once and does not add more clarity than
> doing it inline.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Ah, not really sure I agree. Yes I suppose in this case because it
has that comment. But you could instead remove the comment and
leave the function there (because the comment is redundant with
the function name), and then your main function is 1 line
instead of 4.

Don't remove functions just because they're called once, if they
are a nice self-contained and well named thing. But okay for here
I suppose.

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

> ---
>  target/ppc/mmu-hash32.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
> index 6f0f0bbb00..c4de1647e2 100644
> --- a/target/ppc/mmu-hash32.c
> +++ b/target/ppc/mmu-hash32.c
> @@ -298,15 +298,6 @@ static hwaddr ppc_hash32_htab_lookup(PowerPCCPU *cpu,
>      return pte_offset;
>  }
>  
> -static hwaddr ppc_hash32_pte_raddr(target_ulong sr, ppc_hash_pte32_t pte,
> -                                   target_ulong eaddr)
> -{
> -    hwaddr rpn = pte.pte1 & HPTE32_R_RPN;
> -    hwaddr mask = ~TARGET_PAGE_MASK;
> -
> -    return (rpn & ~mask) | (eaddr & mask);
> -}
> -
>  bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType 
> access_type,
>                        hwaddr *raddrp, int *psizep, int *protp, int mmu_idx,
>                        bool guest_visible)
> @@ -440,11 +431,12 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, 
> MMUAccessType access_type,
>               */
>              prot &= ~PAGE_WRITE;
>          }
> -     }
> +    }
> +    *protp = prot;
>  
>      /* 9. Determine the real address from the PTE */
> -
> -    *raddrp = ppc_hash32_pte_raddr(sr, pte, eaddr);
> -    *protp = prot;
> +    *raddrp = pte.pte1 & HPTE32_R_RPN;
> +    *raddrp &= TARGET_PAGE_MASK;
> +    *raddrp |= eaddr & ~TARGET_PAGE_MASK;
>      return true;
>  }