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 <bala...@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)) {
