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
> ---
> 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 & 0x0100)) {
> -/* Update accessed flag */
> -*pte1p |= 0x0100;
> -store = 1;
> -}
> -if (!(*pte1p & 0x0080)) {
> -if (access_type == MMU_DATA_STORE && ret == 0) {
> -/* Update changed flag */
> -*pte1p |= 0x0080;
> -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, >tlb.tlb6[best].pte1, ret, access_type);
> +pte1p = >tlb.tlb6[best].pte1;
> +*pte1p |= 0x0100; /* Update accessed flag */
> +if (!(*pte1p & 0x0080)) {
> +if (access_type == MMU_DATA_STORE && ret == 0) {
> +/* Update changed flag */
> +*pte1p |= 0x0080;
> +} else {
> +/* Force page fault for first write access */
> +ctx->prot &= ~PAGE_WRITE;
> +}
> +}
> }
> #if defined(DUMP_PAGE_TABLES)
> if (qemu_loglevel_mask(CPU_LOG_MMU)) {
