On 07/02/2018 03:37 AM, Peter Maydell wrote: > 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.
Yeah, well, emacs requires them for alignment. Checkpatch isn't too smart about multi-line expressions. > > >> + /* 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. > */ > ? The if got reorganized a few times before settling on the current. I agree that your comment matches the current form much better. r~