Re: [Qemu-devel] [PATCH] accel/tcg: Avoid caching overwritten tlb entries

[Peter Maydell]

On 29 June 2018 at 22:37, Richard Henderson
<address@hidden> 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 <address@hidden>
> ---
>
> 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 <address@hidden>

thanks
-- PMM