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

[Paolo Bonzini]

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
    };

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).

- 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.

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.

- 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;
        }
    }

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?