| Message ID | 55ea002eb081fd1fdc6ff6f0e3f25ba892044e9d.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 not used from any other files so make it static and > fix the maybe used uninitialised warnings this has uncovered. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > target/ppc/internal.h | 5 +---- > target/ppc/mmu_common.c | 5 ++++- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/target/ppc/internal.h b/target/ppc/internal.h > index 601c0b533f..7a99f08dc8 100644 > --- a/target/ppc/internal.h > +++ b/target/ppc/internal.h > @@ -261,10 +261,7 @@ typedef struct mmu_ctx_t mmu_ctx_t; > bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type, > hwaddr *raddrp, int *psizep, int *protp, > int mmu_idx, bool guest_visible); > -int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, > - target_ulong eaddr, > - MMUAccessType access_type, int type, > - int mmu_idx); > + > /* Software driven TLB helpers */ > int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr, > int way, int is_code); > diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c > index 762b13805b..4852cb5571 100644 > --- a/target/ppc/mmu_common.c > +++ b/target/ppc/mmu_common.c > @@ -666,6 +666,7 @@ static int mmubooke_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > hwaddr raddr = (hwaddr)-1ULL; > int i, ret = -1; > > + ctx->prot = 0; > for (i = 0; i < env->nb_tlb; i++) { > tlb = &env->tlb.tlbe[i]; > ret = mmubooke_check_tlb(env, tlb, &raddr, &ctx->prot, address, > @@ -873,6 +874,7 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > hwaddr raddr = (hwaddr)-1ULL; > int i, j, ways, ret = -1; > > + ctx->prot = 0; > for (i = 0; i < BOOKE206_MAX_TLBN; i++) { > ways = booke206_tlb_ways(env, i); > for (j = 0; j < ways; j++) { The prot warnings are valid AFAIKS, used uninit by qemu_log_mask call. So, I see what the booke _check_tlb() functions are doing with *prot now and that is to assign it iff return value is 0 or -2 or -3, matching TLB address (and possibly mismatch prot). Would it be better to fix it as: 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, ret == -1 ? 0 : ctx->prot, ret); This way it's clearer that we're only printing it when a TLB was found, and it won't silence other possible use-uninitialised? > @@ -1144,7 +1146,7 @@ void dump_mmu(CPUPPCState *env) > } > } > > -int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, > +static int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, > target_ulong eaddr, > MMUAccessType access_type, int type, > int mmu_idx) > @@ -1163,6 +1165,7 @@ int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, > if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx || > env->mmu_model == POWERPC_MMU_SOFT_4xx || > env->mmu_model == POWERPC_MMU_REAL)) { > + memset(ctx, 0, sizeof(*ctx)); > ctx->raddr = eaddr; > ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; > return 0; I wonder why the compiler doesn't see these, they are all in the return not-zero cases that should be quite visible? What if you leave the static change to the end of your series, do the simplifications allow the compiler to work it out? I prefer not to squash such compiler warnings if it can be avoided. 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 not used from any other files so make it static and >> fix the maybe used uninitialised warnings this has uncovered. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> target/ppc/internal.h | 5 +---- >> target/ppc/mmu_common.c | 5 ++++- >> 2 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/target/ppc/internal.h b/target/ppc/internal.h >> index 601c0b533f..7a99f08dc8 100644 >> --- a/target/ppc/internal.h >> +++ b/target/ppc/internal.h >> @@ -261,10 +261,7 @@ typedef struct mmu_ctx_t mmu_ctx_t; >> bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type, >> hwaddr *raddrp, int *psizep, int *protp, >> int mmu_idx, bool guest_visible); >> -int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, >> - target_ulong eaddr, >> - MMUAccessType access_type, int type, >> - int mmu_idx); >> + >> /* Software driven TLB helpers */ >> int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr, >> int way, int is_code); >> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c >> index 762b13805b..4852cb5571 100644 >> --- a/target/ppc/mmu_common.c >> +++ b/target/ppc/mmu_common.c >> @@ -666,6 +666,7 @@ static int mmubooke_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, >> hwaddr raddr = (hwaddr)-1ULL; >> int i, ret = -1; >> >> + ctx->prot = 0; >> for (i = 0; i < env->nb_tlb; i++) { >> tlb = &env->tlb.tlbe[i]; >> ret = mmubooke_check_tlb(env, tlb, &raddr, &ctx->prot, address, >> @@ -873,6 +874,7 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, >> hwaddr raddr = (hwaddr)-1ULL; >> int i, j, ways, ret = -1; >> >> + ctx->prot = 0; >> for (i = 0; i < BOOKE206_MAX_TLBN; i++) { >> ways = booke206_tlb_ways(env, i); >> for (j = 0; j < ways; j++) { > > The prot warnings are valid AFAIKS, used uninit by qemu_log_mask call. > > So, I see what the booke _check_tlb() functions are doing with > *prot now and that is to assign it iff return value is 0 or -2 or > -3, matching TLB address (and possibly mismatch prot). > > Would it be better to fix it as: > > 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, ret == -1 ? 0 : ctx->prot, ret); > > This way it's clearer that we're only printing it when a TLB was > found, and it won't silence other possible use-uninitialised? I can do that. >> @@ -1144,7 +1146,7 @@ void dump_mmu(CPUPPCState *env) >> } >> } >> >> -int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, >> +static int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, >> target_ulong eaddr, >> MMUAccessType access_type, int type, >> int mmu_idx) >> @@ -1163,6 +1165,7 @@ int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, >> if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx || >> env->mmu_model == POWERPC_MMU_SOFT_4xx || >> env->mmu_model == POWERPC_MMU_REAL)) { >> + memset(ctx, 0, sizeof(*ctx)); >> ctx->raddr = eaddr; >> ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; >> return 0; > > I wonder why the compiler doesn't see these, they are all in the return > not-zero cases that should be quite visible? > > What if you leave the static change to the end of your series, do the > simplifications allow the compiler to work it out? I prefer not to > squash such compiler warnings if it can be avoided. Even removing this memser at the end of the series brings back the warnings so this has to stay for now. Maybe this can be cleaned up later but I'd like to focus on booke now. Regards, BALATON Zoltan
diff --git a/target/ppc/internal.h b/target/ppc/internal.h index 601c0b533f..7a99f08dc8 100644 --- a/target/ppc/internal.h +++ b/target/ppc/internal.h @@ -261,10 +261,7 @@ typedef struct mmu_ctx_t mmu_ctx_t; bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type, hwaddr *raddrp, int *psizep, int *protp, int mmu_idx, bool guest_visible); -int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, - target_ulong eaddr, - MMUAccessType access_type, int type, - int mmu_idx); + /* Software driven TLB helpers */ int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr, int way, int is_code); diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c index 762b13805b..4852cb5571 100644 --- a/target/ppc/mmu_common.c +++ b/target/ppc/mmu_common.c @@ -666,6 +666,7 @@ static int mmubooke_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, hwaddr raddr = (hwaddr)-1ULL; int i, ret = -1; + ctx->prot = 0; for (i = 0; i < env->nb_tlb; i++) { tlb = &env->tlb.tlbe[i]; ret = mmubooke_check_tlb(env, tlb, &raddr, &ctx->prot, address, @@ -873,6 +874,7 @@ static int mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, hwaddr raddr = (hwaddr)-1ULL; int i, j, ways, ret = -1; + ctx->prot = 0; for (i = 0; i < BOOKE206_MAX_TLBN; i++) { ways = booke206_tlb_ways(env, i); for (j = 0; j < ways; j++) { @@ -1144,7 +1146,7 @@ void dump_mmu(CPUPPCState *env) } } -int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, +static int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, target_ulong eaddr, MMUAccessType access_type, int type, int mmu_idx) @@ -1163,6 +1165,7 @@ int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx, if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx || env->mmu_model == POWERPC_MMU_SOFT_4xx || env->mmu_model == POWERPC_MMU_REAL)) { + memset(ctx, 0, sizeof(*ctx)); ctx->raddr = eaddr; ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; return 0;
This function is not used from any other files so make it static and fix the maybe used uninitialised warnings this has uncovered. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- target/ppc/internal.h | 5 +---- target/ppc/mmu_common.c | 5 ++++- 2 files changed, 5 insertions(+), 5 deletions(-)