Message ID | 20220104073121.3784280-7-clg@kaod.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [PULL,01/26] ppc/pnv: Change the maximum of PHB3 devices for Power8NVL | expand |
On Tue, 4 Jan 2022 at 07:31, Cédric Le Goater <clg@kaod.org> wrote: > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Message-Id: <20211222071002.1568894-1-clg@kaod.org> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > target/ppc/mmu-radix64.c | 55 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 52 insertions(+), 3 deletions(-) Hi; Coverity reports that this change introduces a use of an uninitialized variable: > @@ -306,6 +321,15 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, > hwaddr pte_addr; > uint64_t pte; > > + qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx > + " mmu_idx %u (prot %c%c%c) 0x%"HWADDR_PRIx"\n", > + __func__, access_str(access_type), > + eaddr, mmu_idx, > + *h_prot & PAGE_READ ? 'r' : '-', > + *h_prot & PAGE_WRITE ? 'w' : '-', > + *h_prot & PAGE_EXEC ? 'x' : '-', > + g_raddr); At the top of this function we now read the value at h_prot to do this logging, but all the callsites pass in an uninitialized value: int h_prot; ... ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr, pate, &h_raddr, &h_prot, etc); Are the callsites wrong (should be initializing h_prot), or is the logging wrong (and it's this function itself which is supposed to initialize *h_prot as an extra return value) ? This is CID 1468942. thanks -- PMM
On 1/11/22 17:51, Peter Maydell wrote: > On Tue, 4 Jan 2022 at 07:31, Cédric Le Goater <clg@kaod.org> wrote: >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Message-Id: <20211222071002.1568894-1-clg@kaod.org> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> target/ppc/mmu-radix64.c | 55 +++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 52 insertions(+), 3 deletions(-) > > Hi; Coverity reports that this change introduces a use of > an uninitialized variable: > >> @@ -306,6 +321,15 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, >> hwaddr pte_addr; >> uint64_t pte; >> >> + qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx >> + " mmu_idx %u (prot %c%c%c) 0x%"HWADDR_PRIx"\n", >> + __func__, access_str(access_type), >> + eaddr, mmu_idx, >> + *h_prot & PAGE_READ ? 'r' : '-', >> + *h_prot & PAGE_WRITE ? 'w' : '-', >> + *h_prot & PAGE_EXEC ? 'x' : '-', >> + g_raddr); > > At the top of this function we now read the value at h_prot to > do this logging, but all the callsites pass in an uninitialized > value: >> > int h_prot; > ... > ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr, > pate, &h_raddr, &h_prot, etc); > > > Are the callsites wrong (should be initializing h_prot), or is the > logging wrong (and it's this function itself which is supposed to > initialize *h_prot as an extra return value) ?> > This is CID 1468942. h_prot is initialized after the logging, in ppc_radix64_check_prot(). Logging is wrong. Thanks, C.
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index 5b0e62e676dc..d10ae001d7c9 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -97,12 +97,22 @@ static void ppc_radix64_raise_segi(PowerPCCPU *cpu, MMUAccessType access_type, env->error_code = 0; } +static inline const char *access_str(MMUAccessType access_type) +{ + return access_type == MMU_DATA_LOAD ? "reading" : + (access_type == MMU_DATA_STORE ? "writing" : "execute"); +} + static void ppc_radix64_raise_si(PowerPCCPU *cpu, MMUAccessType access_type, vaddr eaddr, uint32_t cause) { CPUState *cs = CPU(cpu); CPUPPCState *env = &cpu->env; + qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx" cause %08x\n", + __func__, access_str(access_type), + eaddr, cause); + switch (access_type) { case MMU_INST_FETCH: /* Instruction Storage Interrupt */ @@ -130,6 +140,11 @@ static void ppc_radix64_raise_hsi(PowerPCCPU *cpu, MMUAccessType access_type, CPUState *cs = CPU(cpu); CPUPPCState *env = &cpu->env; + qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx" 0x%" + HWADDR_PRIx" cause %08x\n", + __func__, access_str(access_type), + eaddr, g_raddr, cause); + switch (access_type) { case MMU_INST_FETCH: /* H Instruction Storage Interrupt */ @@ -306,6 +321,15 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, hwaddr pte_addr; uint64_t pte; + qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx + " mmu_idx %u (prot %c%c%c) 0x%"HWADDR_PRIx"\n", + __func__, access_str(access_type), + eaddr, mmu_idx, + *h_prot & PAGE_READ ? 'r' : '-', + *h_prot & PAGE_WRITE ? 'w' : '-', + *h_prot & PAGE_EXEC ? 'x' : '-', + g_raddr); + *h_page_size = PRTBE_R_GET_RTS(pate.dw0); /* No valid pte or access denied due to protection */ if (ppc_radix64_walk_tree(CPU(cpu)->as, g_raddr, pate.dw0 & PRTBE_R_RPDB, @@ -343,6 +367,11 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, hwaddr h_raddr, pte_addr; int ret; + qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx + " mmu_idx %u pid %"PRIu64"\n", + __func__, access_str(access_type), + eaddr, mmu_idx, pid); + /* Index Process Table by PID to Find Corresponding Process Table Entry */ offset = pid * sizeof(struct prtb_entry); size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12); @@ -468,9 +497,10 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, * | = On | Process Scoped | Scoped | * +-------------+----------------+---------------+ */ -bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type, - hwaddr *raddr, int *psizep, int *protp, int mmu_idx, - bool guest_visible) +static bool ppc_radix64_xlate_impl(PowerPCCPU *cpu, vaddr eaddr, + MMUAccessType access_type, hwaddr *raddr, + int *psizep, int *protp, int mmu_idx, + bool guest_visible) { CPUPPCState *env = &cpu->env; uint64_t lpid, pid; @@ -588,3 +618,22 @@ bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type, return true; } + +bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type, + hwaddr *raddrp, int *psizep, int *protp, int mmu_idx, + bool guest_visible) +{ + bool ret = ppc_radix64_xlate_impl(cpu, eaddr, access_type, raddrp, + psizep, protp, mmu_idx, guest_visible); + + qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx + " mmu_idx %u (prot %c%c%c) -> 0x%"HWADDR_PRIx"\n", + __func__, access_str(access_type), + eaddr, mmu_idx, + *protp & PAGE_READ ? 'r' : '-', + *protp & PAGE_WRITE ? 'w' : '-', + *protp & PAGE_EXEC ? 'x' : '-', + *raddrp); + + return ret; +}