From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42731) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBOFY-0000ql-H1 for qemu-devel@nongnu.org; Fri, 10 Jun 2016 11:21:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bBOFV-0005cH-97 for qemu-devel@nongnu.org; Fri, 10 Jun 2016 11:21:32 -0400 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:34152) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBOFU-0005cA-SC for qemu-devel@nongnu.org; Fri, 10 Jun 2016 11:21:29 -0400 Received: by mail-lf0-x241.google.com with SMTP id k192so6366713lfb.1 for ; Fri, 10 Jun 2016 08:21:28 -0700 (PDT) References: <20160526163549.3276-1-a.rigo@virtualopensystems.com> <20160526163549.3276-3-a.rigo@virtualopensystems.com> From: Sergey Fedorov Message-ID: <575ADAF4.3010900@gmail.com> Date: Fri, 10 Jun 2016 18:21:24 +0300 MIME-Version: 1.0 In-Reply-To: <20160526163549.3276-3-a.rigo@virtualopensystems.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC 02/10] softmmu_llsc_template.h: Move to multi-threading List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alvise Rigo , mttcg@listserver.greensocs.com, alex.bennee@linaro.org Cc: qemu-devel@nongnu.org, jani.kokkonen@huawei.com, claudio.fontana@huawei.com, tech@virtualopensystems.com, fred.konrad@greensocs.com, pbonzini@redhat.com, rth@twiddle.net, cota@braap.org, peter.maydell@linaro.org On 26/05/16 19:35, Alvise Rigo wrote: > Using tcg_exclusive_{lock,unlock}(), make the emulation of > LoadLink/StoreConditional thread safe. > > During an LL access, this lock protects the load access itself, the > update of the exclusive history and the update of the VCPU's protected > range. In a SC access, the lock protects the store access itself, the > possible reset of other VCPUs' protected range and the reset of the > exclusive context of calling VCPU. > > The lock is also taken when a normal store happens to access an > exclusive page to reset other VCPUs' protected range in case of > collision. I think the key problem here is that the load in LL helper can race with a concurrent regular fast-path store. It's probably easier to annotate the source here: 1 WORD_TYPE helper_ldlink_name(CPUArchState *env, target_ulong addr, 2 TCGMemOpIdx oi, uintptr_t retaddr) 3 { 4 WORD_TYPE ret; 5 int index; 6 CPUState *this_cpu = ENV_GET_CPU(env); 7 CPUClass *cc = CPU_GET_CLASS(this_cpu); 8 hwaddr hw_addr; 9 unsigned mmu_idx = get_mmuidx(oi); 10 index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); 11 tcg_exclusive_lock(); 12 /* Use the proper load helper from cpu_ldst.h */ 13 ret = helper_ld(env, addr, oi, retaddr); 14 /* hw_addr = hwaddr of the page (i.e. section->mr->ram_addr + xlat) 15 * plus the offset (i.e. addr & ~TARGET_PAGE_MASK) */ 16 hw_addr = (env->iotlb[mmu_idx][index].addr & TARGET_PAGE_MASK) + addr; 17 if (likely(!(env->tlb_table[mmu_idx][index].addr_read & TLB_MMIO))) { 18 /* If all the vCPUs have the EXCL bit set for this page there is no need 19 * to request any flush. */ 20 if (cpu_physical_memory_set_excl(hw_addr)) { 21 CPUState *cpu; 22 excl_history_put_addr(hw_addr); 23 CPU_FOREACH(cpu) { 24 if (this_cpu != cpu) { 25 tlb_flush_other(this_cpu, cpu, 1); 26 } 27 } 28 } 29 /* For this vCPU, just update the TLB entry, no need to flush. */ 30 env->tlb_table[mmu_idx][index].addr_write |= TLB_EXCL; 31 } else { 32 /* Set a pending exclusive access in the MemoryRegion */ 33 MemoryRegion *mr = iotlb_to_region(this_cpu, 34 env->iotlb[mmu_idx][index].addr, 35 env->iotlb[mmu_idx][index].attrs); 36 mr->pending_excl_access = true; 37 } 38 cc->cpu_set_excl_protected_range(this_cpu, hw_addr, DATA_SIZE); 39 tcg_exclusive_unlock(); 40 /* From now on we are in LL/SC context */ 41 this_cpu->ll_sc_context = true; 42 return ret; 43 } The exclusive lock at line 11 doesn't help if concurrent fast-patch store at this address occurs after we finished load at line 13 but before TLB is flushed as a result of line 25. If we reorder the load to happen after the TLB flush request we still must be sure that the flush is complete before we can do the load safely. > > Moreover, adapt target-arm to also cope with the new multi-threaded > execution. > > Signed-off-by: Alvise Rigo > --- > softmmu_llsc_template.h | 11 +++++++++-- > softmmu_template.h | 6 ++++++ > target-arm/op_helper.c | 6 ++++++ > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/softmmu_llsc_template.h b/softmmu_llsc_template.h > index 2c4a494..d3810c0 100644 > --- a/softmmu_llsc_template.h > +++ b/softmmu_llsc_template.h > @@ -62,11 +62,13 @@ WORD_TYPE helper_ldlink_name(CPUArchState *env, target_ulong addr, > hwaddr hw_addr; > unsigned mmu_idx = get_mmuidx(oi); > > + index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); > + > + tcg_exclusive_lock(); > + > /* Use the proper load helper from cpu_ldst.h */ > ret = helper_ld(env, addr, oi, retaddr); > > - index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); > - > /* hw_addr = hwaddr of the page (i.e. section->mr->ram_addr + xlat) > * plus the offset (i.e. addr & ~TARGET_PAGE_MASK) */ > hw_addr = (env->iotlb[mmu_idx][index].addr & TARGET_PAGE_MASK) + addr; > @@ -95,6 +97,8 @@ WORD_TYPE helper_ldlink_name(CPUArchState *env, target_ulong addr, > > cc->cpu_set_excl_protected_range(this_cpu, hw_addr, DATA_SIZE); > > + tcg_exclusive_unlock(); > + > /* From now on we are in LL/SC context */ > this_cpu->ll_sc_context = true; > > @@ -114,6 +118,8 @@ WORD_TYPE helper_stcond_name(CPUArchState *env, target_ulong addr, > * access as one made by the store conditional wrapper. If the store > * conditional does not succeed, the value will be set to 0.*/ > cpu->excl_succeeded = true; > + > + tcg_exclusive_lock(); > helper_st(env, addr, val, oi, retaddr); > > if (cpu->excl_succeeded) { > @@ -123,6 +129,7 @@ WORD_TYPE helper_stcond_name(CPUArchState *env, target_ulong addr, > > /* Unset LL/SC context */ > cc->cpu_reset_excl_context(cpu); > + tcg_exclusive_unlock(); > > return ret; > } > diff --git a/softmmu_template.h b/softmmu_template.h > index 76fe37e..9363a7b 100644 > --- a/softmmu_template.h > +++ b/softmmu_template.h > @@ -537,11 +537,16 @@ static inline void smmu_helper(do_excl_store)(CPUArchState *env, > } > } > > + /* Take the lock in case we are not coming from a SC */ > + tcg_exclusive_lock(); > + > smmu_helper(do_ram_store)(env, little_endian, val, addr, oi, > get_mmuidx(oi), index, retaddr); > > reset_other_cpus_colliding_ll_addr(hw_addr, DATA_SIZE); > > + tcg_exclusive_unlock(); > + > return; > } > > @@ -572,6 +577,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, > /* Handle an IO access or exclusive access. */ > if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) { > if (tlb_addr & TLB_EXCL) { > + > smmu_helper(do_excl_store)(env, true, val, addr, oi, index, > retaddr); > return; > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > index e22afc5..19ea52d 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -35,7 +35,9 @@ static void raise_exception(CPUARMState *env, uint32_t excp, > cs->exception_index = excp; > env->exception.syndrome = syndrome; > env->exception.target_el = target_el; > + tcg_exclusive_lock(); > cc->cpu_reset_excl_context(cs); > + tcg_exclusive_unlock(); > cpu_loop_exit(cs); > } > > @@ -58,7 +60,9 @@ void HELPER(atomic_clear)(CPUARMState *env) > CPUState *cs = ENV_GET_CPU(env); > CPUClass *cc = CPU_GET_CLASS(cs); > > + tcg_exclusive_lock(); > cc->cpu_reset_excl_context(cs); > + tcg_exclusive_unlock(); > } > > uint32_t HELPER(neon_tbl)(CPUARMState *env, uint32_t ireg, uint32_t def, > @@ -874,7 +878,9 @@ void HELPER(exception_return)(CPUARMState *env) > > aarch64_save_sp(env, cur_el); > > + tcg_exclusive_lock(); > cc->cpu_reset_excl_context(cs); > + tcg_exclusive_unlock(); > > /* We must squash the PSTATE.SS bit to zero unless both of the > * following hold: