Re: [Qemu-devel] [PATCH v6 13/19] cputlb: atomically update tlb fields used by tlb_reset_dirty

[Alex Bennée]

Pranith Kumar <address@hidden> writes:

> Hi Alex,
>
> This patch is causing some build errors on a 32-bit box:
>
> In file included from /home/pranith/qemu/include/exec/exec-all.h:44:0,
>                  from /home/pranith/qemu/cputlb.c:23:
> /home/pranith/qemu/cputlb.c: In function 
> ‘tlb_flush_page_by_mmuidx_async_work’:
> /home/pranith/qemu/cputlb.c:54:36: error: format ‘%x’ expects argument of 
> type ‘unsigned int’, but argument 5 has type ‘long unsigned int’ 
> [-Werror=format=]
>          qemu_log_mask(CPU_LOG_MMU, "%s: " fmt, __func__, \
>                                     ^
> /home/pranith/qemu/include/qemu/log.h:94:22: note: in definition of macro 
> ‘qemu_log_mask’
>              qemu_log(FMT, ## __VA_ARGS__);              \
>                       ^~~
> /home/pranith/qemu/cputlb.c:286:5: note: in expansion of macro ‘tlb_debug’
>      tlb_debug("page:%d addr:"TARGET_FMT_lx" mmu_idx%" PRIxPTR "\n",
>      ^~~~~~~~~
> /home/pranith/qemu/cputlb.c:57:25: error: format ‘%x’ expects argument of 
> type ‘unsigned int’, but argument 6 has type ‘long unsigned int’ 
> [-Werror=format=]
>          fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
>                          ^
> /home/pranith/qemu/cputlb.c:286:5: note: in expansion of macro ‘tlb_debug’
>      tlb_debug("page:%d addr:"TARGET_FMT_lx" mmu_idx%" PRIxPTR "\n",
>      ^~~~~~~~~
> cc1: all warnings being treated as errors
>
> Thanks,
>
<snip>
>> +/*
>> + * Dirty write flag handling
>> + *
>> + * When the TCG code writes to a location it looks up the address in
>> + * the TLB and uses that data to compute the final address. If any of
>> + * the lower bits of the address are set then the slow path is forced.
>> + * There are a number of reasons to do this but for normal RAM the
>> + * most usual is detecting writes to code regions which may invalidate
>> + * generated code.
>> + *
>> + * Because we want other vCPUs to respond to changes straight away we
>> + * update the te->addr_write field atomically. If the TLB entry has
>> + * been changed by the vCPU in the mean time we skip the update.
>> + */
>> +
>> +static void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry, uintptr_t start,
>>                             uintptr_t length)
>>  {
>> -    uintptr_t addr;
>> +    /* paired with atomic_mb_set in tlb_set_page_with_attrs */
>> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);
>> +    uintptr_t addr = orig_addr;
>>
>> -    if (tlb_is_dirty_ram(tlb_entry)) {
>> -        addr = (tlb_entry->addr_write & TARGET_PAGE_MASK) + 
>> tlb_entry->addend;
>> +    if ((addr & (TLB_INVALID_MASK | TLB_MMIO | TLB_NOTDIRTY)) == 0) {
>> +        addr &= TARGET_PAGE_MASK;
>> +        addr += atomic_read(&tlb_entry->addend);
>>          if ((addr - start) < length) {
>> -            tlb_entry->addr_write |= TLB_NOTDIRTY;
>> +            uintptr_t notdirty_addr = orig_addr | TLB_NOTDIRTY;
>> +            atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, 
>> notdirty_addr);
>>          }
>>      }
>>  }

Even worse than that we trip up the atomic.h QEMU_BUILD_BUG_ON with the
atomic_cmpxchg. Now I believe we can use atomic_cmpxchg__nocheck without
too much issue on x86 but we'll need to #ifdef it on detection of wide
atomics.

--
Alex Bennée