From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54722) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZG80g-0001jQ-UX for qemu-devel@nongnu.org; Fri, 17 Jul 2015 11:57:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZG80d-00011f-O1 for qemu-devel@nongnu.org; Fri, 17 Jul 2015 11:57:14 -0400 Received: from mail-wg0-f43.google.com ([74.125.82.43]:36273) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZG80d-00011O-Fn for qemu-devel@nongnu.org; Fri, 17 Jul 2015 11:57:11 -0400 Received: by wgxm20 with SMTP id m20so85565624wgx.3 for ; Fri, 17 Jul 2015 08:57:10 -0700 (PDT) References: <1436516626-8322-1-git-send-email-a.rigo@virtualopensystems.com> <1436516626-8322-14-git-send-email-a.rigo@virtualopensystems.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1436516626-8322-14-git-send-email-a.rigo@virtualopensystems.com> Date: Fri, 17 Jul 2015 16:57:08 +0100 Message-ID: <87615i6aln.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC v3 13/13] softmmu_template.h: move to multithreading List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alvise Rigo Cc: mttcg@listserver.greensocs.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, pbonzini@redhat.com, jani.kokkonen@huawei.com, tech@virtualopensystems.com Alvise Rigo writes: > Exploiting the tcg_excl_access_lock, port the helper_{le,be}_st_name to > work in real multithreading. > > - The macro lookup_cpus_ll_addr now uses directly the > env->excl_protected_addr to invalidate others' LL/SC operations > > Suggested-by: Jani Kokkonen > Suggested-by: Claudio Fontana > Signed-off-by: Alvise Rigo > --- > softmmu_template.h | 110 +++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 89 insertions(+), 21 deletions(-) > > diff --git a/softmmu_template.h b/softmmu_template.h > index bc767f6..522454f 100644 > --- a/softmmu_template.h > +++ b/softmmu_template.h > @@ -141,21 +141,24 @@ > vidx >= 0; \ > }) > > +#define EXCLUSIVE_RESET_ADDR ULLONG_MAX > + > +/* This macro requires the caller to have the tcg_excl_access_lock lock since > + * it modifies the excl_protected_hwaddr of a running vCPU. > + * The macros scans all the excl_protected_hwaddr of all the vCPUs and compare > + * them with the address the current vCPU is writing to. If there is a match, > + * we reset the value, making the SC fail. */ It would have been nice if we had started with a comment when the function^H^H^H^H^H macro was first introduced and then updated here. > #define lookup_cpus_ll_addr(addr) \ > ({ \ > CPUState *cpu; \ > CPUArchState *acpu; \ > - bool hit = false; \ > \ > CPU_FOREACH(cpu) { \ > acpu = (CPUArchState *)cpu->env_ptr; \ > if (cpu != current_cpu && acpu->excl_protected_hwaddr == addr) { \ > - hit = true; \ > - break; \ > + acpu->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR; \ > } \ > } \ > - \ > - hit; \ > }) My comment about using an inline function in the earlier patch stands. > > #ifndef SOFTMMU_CODE_ACCESS > @@ -439,18 +442,52 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, > * exclusive-protected memory. */ > hwaddr hw_addr = (iotlbentry->addr & TARGET_PAGE_MASK) + addr; > > - bool set_to_dirty; > - > /* Two cases of invalidation: the current vCPU is writing to another > * vCPU's exclusive address or the vCPU that issued the LoadLink is > * writing to it, but not through a StoreCond. */ > - set_to_dirty = lookup_cpus_ll_addr(hw_addr); > - set_to_dirty |= env->ll_sc_context && > - (env->excl_protected_hwaddr == hw_addr); > + qemu_mutex_lock(&tcg_excl_access_lock); > + > + /* The macro lookup_cpus_ll_addr could have reset the exclusive > + * address. Fail the SC in this case. > + * N.B.: Here excl_succeeded == 0 means that we don't come from a > + * store conditional. */ > + if (env->excl_succeeded && > + (env->excl_protected_hwaddr == EXCLUSIVE_RESET_ADDR)) { > + env->excl_succeeded = 0; > + qemu_mutex_unlock(&tcg_excl_access_lock); > + > + return; > + } > + > + lookup_cpus_ll_addr(hw_addr); Add comment for side effect, also confused by the above comment given we call lookups_cpus_ll_addr() after the comment about it tweaking excl_succedded. > + > + if (!env->excl_succeeded) { > + if (env->ll_sc_context && > + (env->excl_protected_hwaddr == hw_addr)) { > + cpu_physical_memory_set_excl_dirty(hw_addr); > + } > + } else { > + if (cpu_physical_memory_excl_is_dirty(hw_addr) || > + env->excl_protected_hwaddr != hw_addr) { > + env->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR; > + qemu_mutex_unlock(&tcg_excl_access_lock); > + env->excl_succeeded = 0; > + > + return; > + } > + } I'm wondering if it can be more naturally written the other way round to aid comprehension: if (env->excl_succeeded) { if (cpu_physical_memory_excl_is_dirty(hw_addr) || env->excl_protected_hwaddr != hw_addr) { ..do stuff.. return } } else { if (env->ll_sc_context && (env->excl_protected_hwaddr == hw_addr)) { cpu_physical_memory_set_excl_dirty(hw_addr); } } Although now I'm confused as to why we push on in 3 of the 4 cases. > + > + haddr = addr + env->tlb_table[mmu_idx][index].addend; > + #if DATA_SIZE == 1 > + glue(glue(st, SUFFIX), _p)((uint8_t *)haddr, val); > + #else > + glue(glue(st, SUFFIX), _le_p)((uint8_t *)haddr, val); > + #endif > + > + env->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR; > + qemu_mutex_unlock(&tcg_excl_access_lock); > > - if (set_to_dirty) { > - cpu_physical_memory_set_excl_dirty(hw_addr); > - } /* the vCPU is legitimately writing to the protected address */ > + return; > } else { > if ((addr & (DATA_SIZE - 1)) != 0) { > goto do_unaligned_access; > @@ -537,18 +574,49 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, > * exclusive-protected memory. */ > hwaddr hw_addr = (iotlbentry->addr & TARGET_PAGE_MASK) + addr; > > - bool set_to_dirty; > - > /* Two cases of invalidation: the current vCPU is writing to another > * vCPU's exclusive address or the vCPU that issued the LoadLink is > * writing to it, but not through a StoreCond. */ > - set_to_dirty = lookup_cpus_ll_addr(hw_addr); > - set_to_dirty |= env->ll_sc_context && > - (env->excl_protected_hwaddr == hw_addr); > + qemu_mutex_lock(&tcg_excl_access_lock); > + > + /* The macro lookup_cpus_ll_addr could have reset the exclusive > + * address. Fail the SC in this case. > + * N.B.: Here excl_succeeded == 0 means that we don't come from a > + * store conditional. */ > + if (env->excl_succeeded && > + (env->excl_protected_hwaddr == EXCLUSIVE_RESET_ADDR)) { > + env->excl_succeeded = 0; > + qemu_mutex_unlock(&tcg_excl_access_lock); > + > + return; > + } > + > + lookup_cpus_ll_addr(hw_addr); > + > + if (!env->excl_succeeded) { > + if (env->ll_sc_context && > + (env->excl_protected_hwaddr == hw_addr)) { > + cpu_physical_memory_set_excl_dirty(hw_addr); > + } > + } else { > + if (cpu_physical_memory_excl_is_dirty(hw_addr) || > + env->excl_protected_hwaddr != hw_addr) { > + env->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR; > + qemu_mutex_unlock(&tcg_excl_access_lock); > + env->excl_succeeded = 0; > + > + return; > + } > + } Given the amount of copy/paste between the two functions I wonder if there is some commonality to be re-factored out here? > > - if (set_to_dirty) { > - cpu_physical_memory_set_excl_dirty(hw_addr); > - } /* the vCPU is legitimately writing to the protected address */ > + haddr = addr + env->tlb_table[mmu_idx][index].addend; > + > + glue(glue(st, SUFFIX), _be_p)((uint8_t *)haddr, val); > + > + qemu_mutex_unlock(&tcg_excl_access_lock); > + env->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR; > + > + return; > } else { > if ((addr & (DATA_SIZE - 1)) != 0) { > goto do_unaligned_access; -- Alex Bennée