[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v3 13/13] softmmu_template.h: move to multithreadi
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [RFC v3 13/13] softmmu_template.h: move to multithreading |
Date: |
Fri, 17 Jul 2015 16:57:08 +0100 |
Alvise Rigo <address@hidden> 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 <address@hidden>
> Suggested-by: Claudio Fontana <address@hidden>
> Signed-off-by: Alvise Rigo <address@hidden>
> ---
> 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
- [Qemu-devel] [RFC v3 09/13] cpus.c: introduce simple callback support, (continued)
[Qemu-devel] [RFC v3 11/13] Introduce exit_flush_req and tcg_excl_access_lock, Alvise Rigo, 2015/07/10
[Qemu-devel] [RFC v3 10/13] Simple TLB flush wrap to use as exit callback, Alvise Rigo, 2015/07/10
[Qemu-devel] [RFC v3 13/13] softmmu_template.h: move to multithreading, Alvise Rigo, 2015/07/10
- Re: [Qemu-devel] [RFC v3 13/13] softmmu_template.h: move to multithreading,
Alex Bennée <=
[Qemu-devel] [RFC v3 12/13] softmmu_llsc_template.h: move to multithreading, Alvise Rigo, 2015/07/10
Re: [Qemu-devel] [RFC v3 00/13] Slow-path for atomic instruction translation, Mark Burton, 2015/07/10
Re: [Qemu-devel] [RFC v3 00/13] Slow-path for atomic instruction translation, Frederic Konrad, 2015/07/10