On Thu Jul 4, 2024 at 10:34 PM AEST, BALATON Zoltan wrote:
> On Thu, 4 Jul 2024, Nicholas Piggin wrote:
> > 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?
>
> I have no idea. I was just trying to clean up this code to make it simpler
Yeah that's fine I wouldn't expect it to change here, just wondering
if you'd dug into it more. I *think* that is the reaon for it.
Thanks,
Nick
> with this series. I think historically there was a single function that
> handled all models but as these became too different it was split up by
> MMU models. It could be some of this are remnants of some old code where
> some other model needed it and not needed any more or this could be merged
> with hash32 but I did not try to find that out, just try to make sure not
> to break it any more than it might already be broken.
>
> Regards,
> BALATON Zoltan
>
> >> + }
> >> + }
> >> }
> >
> > Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> >
> >> #if defined(DUMP_PAGE_TABLES)
> >> if (qemu_loglevel_mask(CPU_LOG_MMU)) {
> >
> >