| Message ID | 87df776b2534cc0ad2523d17c99453edb5de3459.1716763435.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Remaining MMU clean up patches | expand |
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? > + } > + } > } Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > #if defined(DUMP_PAGE_TABLES) > if (qemu_loglevel_mask(CPU_LOG_MMU)) {
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 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)) { > >
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)) { > > > >
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; + } + } } #if defined(DUMP_PAGE_TABLES) if (qemu_loglevel_mask(CPU_LOG_MMU)) {
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(-)