On Wed, Feb 17, 2016 at 7:55 PM, Alex Bennée <alex.ben...@linaro.org> wrote: > > Alvise Rigo <a.r...@virtualopensystems.com> writes: > >> As for the RAM case, also the MMIO exclusive ranges have to be protected >> by other CPU's accesses. In order to do that, we flag the accessed >> MemoryRegion to mark that an exclusive access has been performed and is >> not concluded yet. >> >> This flag will force the other CPUs to invalidate the exclusive range in >> case of collision. >> >> 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 | 20 +++++++++++++------- >> include/exec/memory.h | 1 + >> softmmu_llsc_template.h | 11 +++++++---- >> softmmu_template.h | 22 ++++++++++++++++++++++ >> 4 files changed, 43 insertions(+), 11 deletions(-) >> >> diff --git a/cputlb.c b/cputlb.c >> index 87d09c8..06ce2da 100644 >> --- a/cputlb.c >> +++ b/cputlb.c >> @@ -496,19 +496,25 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, >> target_ulong addr) >> /* For every vCPU compare the exclusive address and reset it in case of a >> * match. Since only one vCPU is running at once, no lock has to be held to >> * guard this operation. */ >> -static inline void lookup_and_reset_cpus_ll_addr(hwaddr addr, hwaddr size) >> +static inline bool lookup_and_reset_cpus_ll_addr(hwaddr addr, hwaddr size) >> { >> CPUState *cpu; >> + bool ret = false; >> >> CPU_FOREACH(cpu) { >> - if (cpu->excl_protected_range.begin != EXCLUSIVE_RESET_ADDR && >> - ranges_overlap(cpu->excl_protected_range.begin, >> - cpu->excl_protected_range.end - >> - cpu->excl_protected_range.begin, >> - addr, size)) { >> - cpu->excl_protected_range.begin = EXCLUSIVE_RESET_ADDR; >> + if (current_cpu != cpu) { > > I'm confused by this change. I don't see anywhere in the MMIO handling > why we would want to change skipping the CPU. Perhaps this belongs in > the previous patch? Maybe the function should really be > lookup_and_maybe_reset_other_cpu_ll_addr?
This is actually used later on in this patch. > >> + if (cpu->excl_protected_range.begin != EXCLUSIVE_RESET_ADDR && >> + ranges_overlap(cpu->excl_protected_range.begin, >> + cpu->excl_protected_range.end - >> + cpu->excl_protected_range.begin, >> + addr, size)) { >> + cpu->excl_protected_range.begin = EXCLUSIVE_RESET_ADDR; >> + ret = true; >> + } >> } >> } >> + >> + return ret; >> } >> >> #define MMUSUFFIX _mmu >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 71e0480..bacb3ad 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -171,6 +171,7 @@ struct MemoryRegion { >> bool rom_device; >> bool flush_coalesced_mmio; >> bool global_locking; >> + bool pending_excl_access; /* A vCPU issued an exclusive access */ >> uint8_t dirty_log_mask; >> ram_addr_t ram_addr; >> Object *owner; >> diff --git a/softmmu_llsc_template.h b/softmmu_llsc_template.h >> index 101f5e8..b4712ba 100644 >> --- a/softmmu_llsc_template.h >> +++ b/softmmu_llsc_template.h >> @@ -81,15 +81,18 @@ WORD_TYPE helper_ldlink_name(CPUArchState *env, >> target_ulong addr, >> } >> } >> } >> + /* For this vCPU, just update the TLB entry, no need to flush. */ >> + env->tlb_table[mmu_idx][index].addr_write |= TLB_EXCL; >> } else { >> - hw_error("EXCL accesses to MMIO regions not supported yet."); >> + /* Set a pending exclusive access in the MemoryRegion */ >> + MemoryRegion *mr = iotlb_to_region(this, >> + env->iotlb[mmu_idx][index].addr, >> + >> env->iotlb[mmu_idx][index].attrs); >> + mr->pending_excl_access = true; >> } >> >> cc->cpu_set_excl_protected_range(this, hw_addr, DATA_SIZE); >> >> - /* For this vCPU, just update the TLB entry, no need to flush. */ >> - env->tlb_table[mmu_idx][index].addr_write |= TLB_EXCL; >> - >> /* From now on we are in LL/SC context */ >> this->ll_sc_context = true; >> >> diff --git a/softmmu_template.h b/softmmu_template.h >> index c54bdc9..71c5152 100644 >> --- a/softmmu_template.h >> +++ b/softmmu_template.h >> @@ -360,6 +360,14 @@ static inline void glue(io_write, SUFFIX)(CPUArchState >> *env, >> MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs); >> >> physaddr = (physaddr & TARGET_PAGE_MASK) + addr; >> + >> + /* Invalidate the exclusive range that overlaps this access */ >> + if (mr->pending_excl_access) { >> + if (lookup_and_reset_cpus_ll_addr(physaddr, 1 << SHIFT)) { Here precisely. As you wrote, we can rename it to lookup_and_maybe_reset_other_cpu_ll_addr even if this name does not convince me. What about other_cpus_reset_colliding_ll_addr? Thank you, alvise >> + mr->pending_excl_access = false; >> + } >> + } >> + >> if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) { >> cpu_io_recompile(cpu, retaddr); >> } >> @@ -504,6 +512,13 @@ void helper_le_st_name(CPUArchState *env, target_ulong >> addr, DATA_TYPE val, >> glue(helper_le_st_name, _do_mmio_access)(env, val, addr, oi, >> mmu_idx, index, >> retaddr); >> + /* N.B.: Here excl_succeeded == true means that this access >> + * comes from an exclusive instruction. */ >> + if (cpu->excl_succeeded) { >> + MemoryRegion *mr = iotlb_to_region(cpu, >> iotlbentry->addr, >> + iotlbentry->attrs); >> + mr->pending_excl_access = false; >> + } >> } else { >> glue(helper_le_st_name, _do_ram_access)(env, val, addr, oi, >> mmu_idx, index, >> @@ -655,6 +670,13 @@ void helper_be_st_name(CPUArchState *env, target_ulong >> addr, DATA_TYPE val, >> glue(helper_be_st_name, _do_mmio_access)(env, val, addr, oi, >> mmu_idx, index, >> retaddr); >> + /* N.B.: Here excl_succeeded == true means that this access >> + * comes from an exclusive instruction. */ >> + if (cpu->excl_succeeded) { >> + MemoryRegion *mr = iotlb_to_region(cpu, >> iotlbentry->addr, >> + iotlbentry->attrs); >> + mr->pending_excl_access = false; >> + } > > My comments about duplication on previous patches still stand. Indeed. Thank you, alvise > >> } else { >> glue(helper_be_st_name, _do_ram_access)(env, val, addr, oi, >> mmu_idx, index, > > > -- > Alex Bennée