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

[Richard Henderson]

On 07/02/2018 03:37 AM, Peter Maydell wrote:
> 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.

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~