diff mbox series

[v2,20/28] target/ppc/mmu_common.c: Make get_physical_address_wtlb() static

Message ID 55ea002eb081fd1fdc6ff6f0e3f25ba892044e9d.1714606359.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show
Series Misc PPC exception and BookE MMU clean ups | expand

Commit Message

BALATON Zoltan May 1, 2024, 11:43 p.m. UTC
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(-)

Comments

Nicholas Piggin May 7, 2024, 10:47 a.m. UTC | #1
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
BALATON Zoltan May 7, 2024, 3:31 p.m. UTC | #2
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 mbox series

Patch

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;