On Wed, Sep 30, 2015 at 5:58 AM, Richard Henderson <r...@twiddle.net> wrote: > On 09/24/2015 06:32 PM, Alvise Rigo wrote: >> >> The new helpers rely on the legacy ones to perform the actual read/write. >> >> The LoadLink helper (helper_ldlink_name) prepares the way for the >> following SC operation. It sets the linked address and the size of the >> access. >> These helper also update the TLB entry of the page involved in the >> LL/SC for those vCPUs that have the bit set (dirty), so that the >> following accesses made by all the vCPUs will follow the slow path. >> >> The StoreConditional helper (helper_stcond_name) returns 1 if the >> store has to fail due to a concurrent access to the same page by >> another vCPU. A 'concurrent access' can be a store made by *any* vCPU >> (although, some implementations allow stores made by the CPU that issued >> the LoadLink). >> >> Suggested-by: Jani Kokkonen <jani.kokko...@huawei.com> >> Suggested-by: Claudio Fontana <claudio.font...@huawei.com> >> Signed-off-by: Alvise Rigo <a.r...@virtualopensystems.com> >> --- >> cputlb.c | 3 ++ >> softmmu_llsc_template.h | 124 >> ++++++++++++++++++++++++++++++++++++++++++++++++ >> softmmu_template.h | 12 +++++ >> tcg/tcg.h | 30 ++++++++++++ >> 4 files changed, 169 insertions(+) >> create mode 100644 softmmu_llsc_template.h >> >> diff --git a/cputlb.c b/cputlb.c >> index 1e25a2a..d5aae7c 100644 >> --- a/cputlb.c >> +++ b/cputlb.c >> @@ -416,6 +416,8 @@ static inline void >> lookup_and_reset_cpus_ll_addr(hwaddr addr, hwaddr size) >> >> #define MMUSUFFIX _mmu >> >> +/* Generates LoadLink/StoreConditional helpers in softmmu_template.h */ >> +#define GEN_EXCLUSIVE_HELPERS >> #define SHIFT 0 >> #include "softmmu_template.h" >> >> @@ -428,6 +430,7 @@ static inline void >> lookup_and_reset_cpus_ll_addr(hwaddr addr, hwaddr size) >> #define SHIFT 3 >> #include "softmmu_template.h" >> #undef MMUSUFFIX >> +#undef GEN_EXCLUSIVE_HELPERS >> >> #define MMUSUFFIX _cmmu >> #undef GETPC_ADJ >> diff --git a/softmmu_llsc_template.h b/softmmu_llsc_template.h >> new file mode 100644 >> index 0000000..9f22834 >> --- /dev/null >> +++ b/softmmu_llsc_template.h >> @@ -0,0 +1,124 @@ >> +/* >> + * Software MMU support (esclusive load/store operations) >> + * >> + * Generate helpers used by TCG for qemu_ldlink/stcond ops. >> + * >> + * Included from softmmu_template.h only. >> + * >> + * Copyright (c) 2015 Virtual Open Systems >> + * >> + * Authors: >> + * Alvise Rigo <a.r...@virtualopensystems.com> >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library; if not, see >> <http://www.gnu.org/licenses/>. >> + */ >> + >> +/* This template does not generate together the le and be version, but >> only one >> + * of the two depending on whether BIGENDIAN_EXCLUSIVE_HELPERS has been >> set. >> + * The same nomenclature as softmmu_template.h is used for the exclusive >> + * helpers. */ >> + >> +#ifdef BIGENDIAN_EXCLUSIVE_HELPERS >> + >> +#define helper_ldlink_name glue(glue(helper_be_ldlink, USUFFIX), >> MMUSUFFIX) >> +#define helper_stcond_name glue(glue(helper_be_stcond, SUFFIX), >> MMUSUFFIX) >> +#define helper_ld glue(glue(helper_be_ld, USUFFIX), MMUSUFFIX) >> +#define helper_st glue(glue(helper_be_st, SUFFIX), MMUSUFFIX) >> + >> +#else /* LE helpers + 8bit helpers (generated only once for both LE end >> BE) */ >> + >> +#if DATA_SIZE > 1 >> +#define helper_ldlink_name glue(glue(helper_le_ldlink, USUFFIX), >> MMUSUFFIX) >> +#define helper_stcond_name glue(glue(helper_le_stcond, SUFFIX), >> MMUSUFFIX) >> +#define helper_ld glue(glue(helper_le_ld, USUFFIX), MMUSUFFIX) >> +#define helper_st glue(glue(helper_le_st, SUFFIX), MMUSUFFIX) >> +#else /* DATA_SIZE <= 1 */ >> +#define helper_ldlink_name glue(glue(helper_ret_ldlink, USUFFIX), >> MMUSUFFIX) >> +#define helper_stcond_name glue(glue(helper_ret_stcond, SUFFIX), >> MMUSUFFIX) >> +#define helper_ld glue(glue(helper_ret_ld, USUFFIX), MMUSUFFIX) >> +#define helper_st glue(glue(helper_ret_st, SUFFIX), MMUSUFFIX) >> +#endif >> + >> +#endif >> + >> +WORD_TYPE helper_ldlink_name(CPUArchState *env, target_ulong addr, >> + TCGMemOpIdx oi, uintptr_t retaddr) >> +{ >> + WORD_TYPE ret; >> + int index; >> + CPUState *cpu; >> + hwaddr hw_addr; >> + unsigned mmu_idx = get_mmuidx(oi); >> + >> + /* Use the proper load helper from cpu_ldst.h */ >> + ret = helper_ld(env, addr, mmu_idx, 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; >> + >> + cpu_physical_memory_clear_excl_dirty(hw_addr, >> ENV_GET_CPU(env)->cpu_index); >> + /* If all the vCPUs have the EXCL bit set for this page there is no >> need >> + * to request any flush. */ >> + if (cpu_physical_memory_excl_is_dirty(hw_addr, smp_cpus)) { >> + CPU_FOREACH(cpu) { >> + if (current_cpu != cpu) { >> + if (cpu_physical_memory_excl_is_dirty(hw_addr, >> + cpu->cpu_index)) { >> + cpu_physical_memory_clear_excl_dirty(hw_addr, >> + cpu->cpu_index); >> + tlb_flush(cpu, 1); >> + } > > > Why would you need to indicate that another cpu has started an exclusive > operation on this page? That seems definitely wrong.
The cpu_physical_memory_clear_excl_dirty() sets the flag to generate the TLB entry with the EXCL flag. > > I think that all you wanted was to flush the other cpu, so that it notices > that this cpu has started an exclusive operation. Indeed, after calling cpu_physical_memory_clear_excl_dirty and flushing the TLB, the CPU will be forced to create the TLB entries from scratch, with the TLB flag if necessary. > > It would be great if most of this were pulled out to a subroutine so that > these helper functions didn't accrue so much duplicate code. > > It would be fantastic to implement a tlb_flush_phys_page to that we didn't > have to flush the entire tlb of the other cpus. Yes, this would be great, but it would also require a structure mapping PHYS_ADDR -> TLB_ENTRIES, which is not provided by the softmmu at the moment. > >> + /* We set it preventively to true to distinguish the following legacy >> + * access as one made by the store conditional wrapper. If the store >> + * conditional does not succeed, the value will be set to 0.*/ >> + env->excl_succeeded = 1; > > > Ah -- "distinguish" is the word that was missing from the comments in the > previous patches. > >> diff --git a/softmmu_template.h b/softmmu_template.h >> index e4431e8..ad65d20 100644 >> --- a/softmmu_template.h >> +++ b/softmmu_template.h >> @@ -640,6 +640,18 @@ void probe_write(CPUArchState *env, target_ulong >> addr, int mmu_idx, >> #endif >> #endif /* !defined(SOFTMMU_CODE_ACCESS) */ >> >> +#ifdef GEN_EXCLUSIVE_HELPERS >> + >> +#if DATA_SIZE > 1 /* The 8-bit helpers are generate along with LE helpers >> */ >> +#define BIGENDIAN_EXCLUSIVE_HELPERS >> +#include "softmmu_llsc_template.h" >> +#undef BIGENDIAN_EXCLUSIVE_HELPERS >> +#endif >> + >> +#include "softmmu_llsc_template.h" >> + >> +#endif /* !defined(GEN_EXCLUSIVE_HELPERS) */ > > > I'm not especially keen on this. Not that what we currently have in > softmmu_template.h is terribly better. What I've already proposed in the past was to copy the actual store code from softmmu_template to softmmu_llsc_template. But since now we need to support all the TLB_ flags, this will really produce a lot of code duplication. > > I wonder what can be done to clean all of this up, short of actually > transitioning to c++ templates... > Is there already an example in QEMU I can look at? Wouldn't this require a new compiler dependency? Thanks, alvise > > r~
