Re: [Qemu-devel] Making cputlb.c operations safe for MTTCG

[Alex Bennée]

Paolo Bonzini <address@hidden> writes:

> On 26/07/2016 14:09, Alex Bennée wrote:
>>
>> As the eventual operation is the setting of a flag I'm wondering if we
>> can simply use atomic primitives to ensure we don't corrupt the lookup
>> address when setting the TLB_NOTDIRTY flag?
>
> In theory tlb_reset_dirty and tlb_set_dirty1 can use atomic_* on
> tlb_entry->addr_write, but careful because:
>
> - you need to order reads and writes to tlb_entry->addr_write and
> tlb_entry->addend properly
>
> - addr_write cannot be written atomically for 32-bit host/64-bit target.
>  Probably you can use something like
>
>     union {
>         target_ulong addr_write;
> #if TARGET_LONG_BITS == 32
>         struct { uint32_t lo_and_lfags; } addr_write_w;
> #elif defined HOST_WORDS_BIGENDIAN
>         struct { uint32_t hi, lo_and_flags; } addr_write_w;
> #else
>         struct { uint32_t lo_and_flags, hi; } addr_write_w;
> #endif
>     };


This will work but I wonder if it is time to call it a day for 32 on 64
support? I mean all this can be worked around but I wonder if it is
worth the effort if no one actually uses this combination.

I might prepare a patch for the next dev cycle to promote discussion.

>
> IIRC "foreign" accesses only set TLB_NOTDIRTY, so they can use a cmpxchg
> on lo_and_flags (worst case you end up with an unnecessary call to
> notdirty_mem_write).

Yeah I used a cmpxchg in the RFC patch. AFAICT you wouldn't see a change
to addend that didn't involve addr_write changing. So as long as
addr_write was always the last thing written things should be fine.

>
> - When removing TLB_NOTDIRTY from a TLB entry
> (notdirty_mem_write/tlb_unprotect_code), as well as filling in a TLB
> entry without TLB_NOTDIRTY (tlb_set_page_with_attrs) you need to protect
> from a concurrent tb_alloc_page and hence take the tb_lock.

tlb_set_page_with_attrs is also only ever filled by the vCPU in question
so I just ensured the final addr_write was calculated and written in one
go at the end, after all other entries had been updated.

>
> In particular:
>
> - in notdirty_mem_write, care must be put in the ordering of
> tb_invalidate_phys_page_fast (which itself calls tlb_unprotect_code and
> takes the tb_lock in tb_invalidate_phys_page_range) and tlb_set_dirty.
> At least it seems to me that the call to tb_invalidate_phys_page_fast
> should be after the write, but that's not all.  Perhaps merge this part
> of notdirty_mem_write:
>
>     /* Set both VGA and migration bits for simplicity and to remove
>      * the notdirty callback faster.
>      */
>     cpu_physical_memory_set_dirty_range(ram_addr, size,
>                                         DIRTY_CLIENTS_NOCODE);
>     /* we remove the notdirty callback only if the code has been
>        flushed */
>     if (!cpu_physical_memory_is_clean(ram_addr)) {
>         tlb_set_dirty(current_cpu, current_cpu->mem_io_vaddr);
>     }
>
> into tlb_unprotect_code?!?  Or perhaps do tlb_set_dirty _first_, and
> then add back the callback if cpu_physical_memory_is_clean(ram_addr) is
> true.  I haven't put much thought into it.

I'll have a closer look at these bits.

>
> - tlb_set_page_with_attrs is also hard-ish to get right, but perhaps the
> same idea of adding the callback last would work:
>
>     /* First set addr_write so that concurrent tlb_reset_dirty_range
>      * finds a match.
>      */
>     te->addr_write = address;
>     if (memory_region_is_ram(section->mr)) {
>         if (cpu_physical_memory_is_clean(
>                      memory_region_get_ram_addr(section->mr) + xlat)) {
>             te->addr_write = address | TLB_NOTDIRTY;
>         }
>     }

Why not just write addr_write completely at the end?

>
> Paolo
>
>> Of course the TLB structure itself covers a number of values but AFAICT
>> erroneously setting TLB_NOTDIRTY on a entry that gets updated to a new
>> address wouldn't cause a problem except triggering an additional
>> slow-path write. If we are careful about the filling of the TLB entries
>> can we be sure we are always safe?


--
Alex Bennée