| Message ID | cd05e33ea86d301cf64aa3925a6302b92249ee3c.1714606359.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Misc PPC exception and BookE MMU clean ups | expand |
On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote: > This function is no longer called for BookE MMU model so remove parts > related to it. This has uncovered a few may be used uninitialised > warnings that are also fixed. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > target/ppc/mmu_common.c | 25 +++++-------------------- > 1 file changed, 5 insertions(+), 20 deletions(-) > > diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c > index a1f98f8de4..d61c41d8c9 100644 > --- a/target/ppc/mmu_common.c > +++ b/target/ppc/mmu_common.c > @@ -684,12 +684,10 @@ static int mmubooke_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > ret = mmubooke_check_tlb(env, tlb, &raddr, &ctx->prot, address, > access_type, i); > if (ret != -1) { > - if (ret >= 0) { > - ctx->raddr = raddr; > - } > break; > } > } > + ctx->raddr = raddr; > qemu_log_mask(CPU_LOG_MMU, > "%s: access %s " TARGET_FMT_lx " => " HWADDR_FMT_plx > " %d %d\n", __func__, ret < 0 ? "refused" : "granted", > @@ -897,9 +895,6 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > ret = mmubooke206_check_tlb(env, tlb, &raddr, &ctx->prot, address, > access_type, mmu_idx); > if (ret != -1) { > - if (ret >= 0) { > - ctx->raddr = raddr; > - } > goto found_tlb; > } > } > @@ -907,6 +902,7 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > > found_tlb: > > + ctx->raddr = raddr; Not sure about the uninitialized warnings here either, caller probably should not be using ctx->raddr unless we returned 0... > qemu_log_mask(CPU_LOG_MMU, "%s: access %s " TARGET_FMT_lx " => " > HWADDR_FMT_plx " %d %d\n", __func__, > ret < 0 ? "refused" : "granted", address, raddr, > @@ -1163,20 +1159,9 @@ static int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, > MMUAccessType access_type, int type, > int mmu_idx) > { > - bool real_mode; > - > - if (env->mmu_model == POWERPC_MMU_BOOKE) { > - return mmubooke_get_physical_address(env, ctx, eaddr, access_type); > - } else if (env->mmu_model == POWERPC_MMU_BOOKE206) { > - return mmubooke206_get_physical_address(env, ctx, eaddr, access_type, > - mmu_idx); > - } This could just go in the previous patch when you split booke xlate? > - > - real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR) > - : !FIELD_EX64(env->msr, MSR, DR); > - if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx || > - env->mmu_model == POWERPC_MMU_SOFT_4xx || > - env->mmu_model == POWERPC_MMU_REAL)) { > + bool real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR) > + : !FIELD_EX64(env->msr, MSR, DR); > + if (real_mode) { > memset(ctx, 0, sizeof(*ctx)); > ctx->raddr = eaddr; > ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; This still changes beahviour of MPC8xx MMU doesn't it? It's supposed to abort always. Thanks, Nick
On Tue, 7 May 2024, Nicholas Piggin wrote: > On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote: >> This function is no longer called for BookE MMU model so remove parts >> related to it. This has uncovered a few may be used uninitialised >> warnings that are also fixed. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> target/ppc/mmu_common.c | 25 +++++-------------------- >> 1 file changed, 5 insertions(+), 20 deletions(-) >> >> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c >> index a1f98f8de4..d61c41d8c9 100644 >> --- a/target/ppc/mmu_common.c >> +++ b/target/ppc/mmu_common.c >> @@ -684,12 +684,10 @@ static int mmubooke_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, >> ret = mmubooke_check_tlb(env, tlb, &raddr, &ctx->prot, address, >> access_type, i); >> if (ret != -1) { >> - if (ret >= 0) { >> - ctx->raddr = raddr; >> - } >> break; >> } >> } >> + ctx->raddr = raddr; >> qemu_log_mask(CPU_LOG_MMU, >> "%s: access %s " TARGET_FMT_lx " => " HWADDR_FMT_plx >> " %d %d\n", __func__, ret < 0 ? "refused" : "granted", >> @@ -897,9 +895,6 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, >> ret = mmubooke206_check_tlb(env, tlb, &raddr, &ctx->prot, address, >> access_type, mmu_idx); >> if (ret != -1) { >> - if (ret >= 0) { >> - ctx->raddr = raddr; >> - } >> goto found_tlb; >> } >> } >> @@ -907,6 +902,7 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, >> >> found_tlb: >> >> + ctx->raddr = raddr; > > Not sure about the uninitialized warnings here either, caller probably > should not be using ctx->raddr unless we returned 0... > >> qemu_log_mask(CPU_LOG_MMU, "%s: access %s " TARGET_FMT_lx " => " >> HWADDR_FMT_plx " %d %d\n", __func__, >> ret < 0 ? "refused" : "granted", address, raddr, >> @@ -1163,20 +1159,9 @@ static int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, >> MMUAccessType access_type, int type, >> int mmu_idx) >> { >> - bool real_mode; >> - >> - if (env->mmu_model == POWERPC_MMU_BOOKE) { >> - return mmubooke_get_physical_address(env, ctx, eaddr, access_type); >> - } else if (env->mmu_model == POWERPC_MMU_BOOKE206) { >> - return mmubooke206_get_physical_address(env, ctx, eaddr, access_type, >> - mmu_idx); >> - } > > This could just go in the previous patch when you split booke xlate? Removing this uncovers the warnings so I keep it here to separate it from the previous change. I gave up on trying to resolve these warnings and untangle the embedded functions from mmu_ctx_t which would be needed to move these booke functions out from this file. The other problem is that these booke get_physical_address functions and mmu40x_get_physical_address all use ppcemb_tlb_check which then needs to be in the same file and static to be inlined and not run too slow but 40x is still in jumbo_xlate so I just leave it for now and may return to it later or let somebody else continue from here. I think this series moves forward enough for now and I don't have more time now. >> - >> - real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR) >> - : !FIELD_EX64(env->msr, MSR, DR); >> - if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx || >> - env->mmu_model == POWERPC_MMU_SOFT_4xx || >> - env->mmu_model == POWERPC_MMU_REAL)) { >> + bool real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR) >> + : !FIELD_EX64(env->msr, MSR, DR); >> + if (real_mode) { >> memset(ctx, 0, sizeof(*ctx)); >> ctx->raddr = eaddr; >> ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; > > This still changes beahviour of MPC8xx MMU doesn't it? It's supposed > to abort always. I don't think it can get here because there's still an abort case in ppc_tlb_invalidate_all() which is called from ppc_cpu_reset_hold() so it will likely crash before it could call anything here. But if you think it's necessary I could add a case for it in ppc_xlate() maybe. Regards, BALATON Zoltan
On Wed May 8, 2024 at 9:40 AM AEST, BALATON Zoltan wrote: > On Tue, 7 May 2024, Nicholas Piggin wrote: > > On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote: > >> This function is no longer called for BookE MMU model so remove parts > >> related to it. This has uncovered a few may be used uninitialised > >> warnings that are also fixed. > >> > >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > >> --- > >> target/ppc/mmu_common.c | 25 +++++-------------------- > >> 1 file changed, 5 insertions(+), 20 deletions(-) > >> > >> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c > >> index a1f98f8de4..d61c41d8c9 100644 > >> --- a/target/ppc/mmu_common.c > >> +++ b/target/ppc/mmu_common.c > >> @@ -684,12 +684,10 @@ static int mmubooke_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > >> ret = mmubooke_check_tlb(env, tlb, &raddr, &ctx->prot, address, > >> access_type, i); > >> if (ret != -1) { > >> - if (ret >= 0) { > >> - ctx->raddr = raddr; > >> - } > >> break; > >> } > >> } > >> + ctx->raddr = raddr; > >> qemu_log_mask(CPU_LOG_MMU, > >> "%s: access %s " TARGET_FMT_lx " => " HWADDR_FMT_plx > >> " %d %d\n", __func__, ret < 0 ? "refused" : "granted", > >> @@ -897,9 +895,6 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > >> ret = mmubooke206_check_tlb(env, tlb, &raddr, &ctx->prot, address, > >> access_type, mmu_idx); > >> if (ret != -1) { > >> - if (ret >= 0) { > >> - ctx->raddr = raddr; > >> - } > >> goto found_tlb; > >> } > >> } > >> @@ -907,6 +902,7 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > >> > >> found_tlb: > >> > >> + ctx->raddr = raddr; > > > > Not sure about the uninitialized warnings here either, caller probably > > should not be using ctx->raddr unless we returned 0... > > > >> qemu_log_mask(CPU_LOG_MMU, "%s: access %s " TARGET_FMT_lx " => " > >> HWADDR_FMT_plx " %d %d\n", __func__, > >> ret < 0 ? "refused" : "granted", address, raddr, > >> @@ -1163,20 +1159,9 @@ static int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, > >> MMUAccessType access_type, int type, > >> int mmu_idx) > >> { > >> - bool real_mode; > >> - > >> - if (env->mmu_model == POWERPC_MMU_BOOKE) { > >> - return mmubooke_get_physical_address(env, ctx, eaddr, access_type); > >> - } else if (env->mmu_model == POWERPC_MMU_BOOKE206) { > >> - return mmubooke206_get_physical_address(env, ctx, eaddr, access_type, > >> - mmu_idx); > >> - } > > > > This could just go in the previous patch when you split booke xlate? > > Removing this uncovers the warnings so I keep it here to separate it from > the previous change. I gave up on trying to resolve these warnings and > untangle the embedded functions from mmu_ctx_t which would be needed to > move these booke functions out from this file. The other problem is that > these booke get_physical_address functions and mmu40x_get_physical_address > all use ppcemb_tlb_check which then needs to be in the same file and > static to be inlined and not run too slow but 40x is still in jumbo_xlate > so I just leave it for now and may return to it later or let somebody else > continue from here. I think this series moves forward enough for now and I > don't have more time now. If you can't easily drop the path or solve the problem okay, just put a comment or something on the zeroing and I'll take a closer look when I merge. > > >> - > >> - real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR) > >> - : !FIELD_EX64(env->msr, MSR, DR); > >> - if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx || > >> - env->mmu_model == POWERPC_MMU_SOFT_4xx || > >> - env->mmu_model == POWERPC_MMU_REAL)) { > >> + bool real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR) > >> + : !FIELD_EX64(env->msr, MSR, DR); > >> + if (real_mode) { > >> memset(ctx, 0, sizeof(*ctx)); > >> ctx->raddr = eaddr; > >> ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; > > > > This still changes beahviour of MPC8xx MMU doesn't it? It's supposed > > to abort always. > > I don't think it can get here because there's still an abort case in > ppc_tlb_invalidate_all() which is called from ppc_cpu_reset_hold() so it > will likely crash before it could call anything here. But if you think > it's necessary I could add a case for it in ppc_xlate() maybe. I would rather not change it here. You can remove it with another patch. Thanks, Nick
diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c index a1f98f8de4..d61c41d8c9 100644 --- a/target/ppc/mmu_common.c +++ b/target/ppc/mmu_common.c @@ -684,12 +684,10 @@ static int mmubooke_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, ret = mmubooke_check_tlb(env, tlb, &raddr, &ctx->prot, address, access_type, i); if (ret != -1) { - if (ret >= 0) { - ctx->raddr = raddr; - } break; } } + ctx->raddr = raddr; qemu_log_mask(CPU_LOG_MMU, "%s: access %s " TARGET_FMT_lx " => " HWADDR_FMT_plx " %d %d\n", __func__, ret < 0 ? "refused" : "granted", @@ -897,9 +895,6 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, ret = mmubooke206_check_tlb(env, tlb, &raddr, &ctx->prot, address, access_type, mmu_idx); if (ret != -1) { - if (ret >= 0) { - ctx->raddr = raddr; - } goto found_tlb; } } @@ -907,6 +902,7 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, found_tlb: + ctx->raddr = raddr; qemu_log_mask(CPU_LOG_MMU, "%s: access %s " TARGET_FMT_lx " => " HWADDR_FMT_plx " %d %d\n", __func__, ret < 0 ? "refused" : "granted", address, raddr, @@ -1163,20 +1159,9 @@ static int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, MMUAccessType access_type, int type, int mmu_idx) { - bool real_mode; - - if (env->mmu_model == POWERPC_MMU_BOOKE) { - return mmubooke_get_physical_address(env, ctx, eaddr, access_type); - } else if (env->mmu_model == POWERPC_MMU_BOOKE206) { - return mmubooke206_get_physical_address(env, ctx, eaddr, access_type, - mmu_idx); - } - - real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR) - : !FIELD_EX64(env->msr, MSR, DR); - if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx || - env->mmu_model == POWERPC_MMU_SOFT_4xx || - env->mmu_model == POWERPC_MMU_REAL)) { + bool real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR) + : !FIELD_EX64(env->msr, MSR, DR); + if (real_mode) { memset(ctx, 0, sizeof(*ctx)); ctx->raddr = eaddr; ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
This function is no longer called for BookE MMU model so remove parts related to it. This has uncovered a few may be used uninitialised warnings that are also fixed. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- target/ppc/mmu_common.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-)