On 02/06/2021 16:26, Richard Henderson wrote: > On 6/2/21 12:18 PM, Bruno Larsen (billionai) wrote: >> Based-on: <20210518201146.794854-1-richard.henderson@linaro.org> >> >> This commit attempts to implement a first draft of a solution to the >> first bug mentioned by Richard Henderson in this e-mail >> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html >> The second bug was not touched, which is basically implementing the >> solution C >> >> To sumarize the first bug here, from my understanding, when an address >> translation is asked of a 64bit mmu that uses hashtables, the code >> attempts to check some permission bits, but checks them from the wrong >> location. >> >> The solution implemented here is more complex than necessary on >> purpose, to make it more readable (and make sure I understand what is >> going on). If that would really fix the problem, I'll move to >> implementing an actual solution, and to all affected functions. >> >> Signed-off-by: Bruno Larsen (billionai) >> --- >>   target/ppc/mmu-hash64.c | 12 ++++++++++-- >>   1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c >> index c1b98a97e9..63f10f1be7 100644 >> --- a/target/ppc/mmu-hash64.c >> +++ b/target/ppc/mmu-hash64.c >> @@ -887,6 +887,14 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr >> eaddr, MMUAccessType access_type, >>       int exec_prot, pp_prot, amr_prot, prot; >>       int need_prot; >>       hwaddr raddr; >> +    unsigned immu_idx, dmmu_idx; >> +    immu_idx = (env->hflags >> HFLAGS_IMMU_IDX) & 7; >> +    dmmu_idx = (env->hflags >> HFLAGS_DMMU_IDX) & 7; > > This doesn't help at all with the reported bug. You're still reading > from env. You need the mmu_idx that was passed to ppc_cpu_tlb_fill. Ah, I saw a macro for MMU_IDX and assumed they pointed to different locations. Ok, the fix for ppc_cpu_tlb_fill should be easy, then > > For the use from ppc_cpu_get_phys_page_debug, you'd pass in > cpu_mmu_index(env, false). ppc_cpu_get_phys_page_debug has 2 calls to ppc_xlate, one using the data MMU, the other using the instruction MMU. I'm guessing I should pass both, right? But here we have another bit that confuses me: cpu_mmu_index returns 0 if in user mode, or uses the information stored in env to get it, so I don't see how that would be different from getting directly. Unless the point is to have ppc_*_xlate be generic and pc_*_debug knows the info in env is correct. Is that it? > > >> +    const short HV = 1, IR = 2, DR = 3; >> +    bool MSR[3]; >> +    MSR[HV] = dmmu_idx & 2, >> +    MSR[IR] = immu_idx & 4, >> +    MSR[DR] = dmmu_idx & 4; > > There's no point in the array.  Just use three different scalars > (real_mode, hv, and pr (note that pr is the major portion of the bug > as reported)). Additionally, you'll not be distinguishing immu_idx and > dmmu_idx, but using the single idx that's given. Ah, yeah, that's the "more complex than necessary, but it was easy for me to read" part. Scalars are a good solution. In this function in specific, PR doesn't actually show up anywhere, so I would actually only need 2. Anyway, will start working on this. > >> -    if (access_type == MMU_INST_FETCH ? !msr_ir : !msr_dr) { >> +    if (access_type == MMU_INST_FETCH ? !MSR[IR] : !MSR[DR]) { > > Which simplifies this condition to just a single test. > > > r~ -- Bruno Piazera Larsen Instituto de Pesquisas ELDORADO Departamento Computação Embarcada Analista de Software Trainee Aviso Legal - Disclaimer