On 29 June 2018 at 22:37, Richard Henderson <richard.hender...@linaro.org> wrote: > When installing a TLB entry, remove any cached version of the > same page in the VTLB. If the existing TLB entry matches, do > not copy into the VTLB, but overwrite it. > > Signed-off-by: Richard Henderson <richard.hender...@linaro.org> > --- > > This may fix some problems with Q800 that Laurent reported. > > On IRC, Peter suggested that regardless of the m68k ptest insn, we > need to be more careful with installed TLB entries. > > I added some temporary logging and concur. This sort of overwrite > happens often when writable pages are marked read-only in order to > track a dirty bit. After the dirty bit is set, we re-install the > TLB entry as read-write. > > I'm mildly surprised we haven't run into problems before... > > > r~ > > --- > accel/tcg/cputlb.c | 60 +++++++++++++++++++++++++--------------------- > 1 file changed, 33 insertions(+), 27 deletions(-) > > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index cc90a5fe92..250b024c5d 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -235,17 +235,30 @@ void tlb_flush_by_mmuidx_all_cpus_synced(CPUState > *src_cpu, > async_safe_run_on_cpu(src_cpu, fn, RUN_ON_CPU_HOST_INT(idxmap)); > } > > - > - > -static inline void tlb_flush_entry(CPUTLBEntry *tlb_entry, target_ulong addr) > +static inline bool tlb_hit_page_anyprot(CPUTLBEntry *tlb_entry, > + target_ulong page) > { > - if (tlb_hit_page(tlb_entry->addr_read, addr) || > - tlb_hit_page(tlb_entry->addr_write, addr) || > - tlb_hit_page(tlb_entry->addr_code, addr)) { > + return (tlb_hit_page(tlb_entry->addr_read, page) || > + tlb_hit_page(tlb_entry->addr_write, page) || > + tlb_hit_page(tlb_entry->addr_code, page));
Checkpatch warns that the outer brackets here are not required. > + /* If the old entry matches the new page, just overwrite TE. */ > + if (!tlb_hit_page_anyprot(te, vaddr_page)) { I found it slightly confusing that the sense of the "if" in the comment is backwards from the "if" in the code. Maybe /* * Only evict the old entry to the victim tlb if it's for a * different page; otherwise just overwrite the stale data. */ ? (I was only slightly confused, though, so I don't feel very strongly here.) > + unsigned vidx = env->vtlb_index++ % CPU_VTLB_SIZE; > + CPUTLBEntry *tv = &env->tlb_v_table[mmu_idx][vidx]; > > - env->iotlb_v[mmu_idx][vidx] = env->iotlb[mmu_idx][index]; > + /* Evict the old entry into the victim tlb. */ > + copy_tlb_helper(tv, te, true); > + env->iotlb_v[mmu_idx][vidx] = env->iotlb[mmu_idx][index]; > + } Otherwise Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> thanks -- PMM