From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35947) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c4s07-0004PC-NT for qemu-devel@nongnu.org; Thu, 10 Nov 2016 11:14:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c4s02-0000vV-Kh for qemu-devel@nongnu.org; Thu, 10 Nov 2016 11:14:55 -0500 Received: from mail-wm0-x234.google.com ([2a00:1450:400c:c09::234]:35192) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c4s02-0000uZ-Cq for qemu-devel@nongnu.org; Thu, 10 Nov 2016 11:14:50 -0500 Received: by mail-wm0-x234.google.com with SMTP id a197so377482382wmd.0 for ; Thu, 10 Nov 2016 08:14:50 -0800 (PST) References: <20161109145748.27282-1-alex.bennee@linaro.org> <20161109145748.27282-14-alex.bennee@linaro.org> <87wpgcmomy.fsf@gmail.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <87wpgcmomy.fsf@gmail.com> Date: Thu, 10 Nov 2016 16:14:47 +0000 Message-ID: <87eg2j8g7c.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v6 13/19] cputlb: atomically update tlb fields used by tlb_reset_dirty List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pranith Kumar Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, mttcg@listserver.greensocs.com, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, nikunj@linux.vnet.ibm.com, mark.burton@greensocs.com, jan.kiszka@siemens.com, serge.fdrv@gmail.com, rth@twiddle.net, peter.maydell@linaro.org, claudio.fontana@huawei.com, Peter Crosthwaite Pranith Kumar 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, > >> +/* >> + * 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