From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aczfE-0002eq-Hs for qemu-devel@nongnu.org; Mon, 07 Mar 2016 13:13:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aczfB-0004aC-9i for qemu-devel@nongnu.org; Mon, 07 Mar 2016 13:13:52 -0500 Received: from mail-ig0-x235.google.com ([2607:f8b0:4001:c05::235]:38020) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aczfA-0004a2-TX for qemu-devel@nongnu.org; Mon, 07 Mar 2016 13:13:49 -0500 Received: by mail-ig0-x235.google.com with SMTP id ig19so24287604igb.1 for ; Mon, 07 Mar 2016 10:13:48 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <874md6dlym.fsf@linaro.org> References: <1454059965-23402-1-git-send-email-a.rigo@virtualopensystems.com> <1454059965-23402-11-git-send-email-a.rigo@virtualopensystems.com> <878u2jdv4a.fsf@linaro.org> <874md6dlym.fsf@linaro.org> Date: Mon, 7 Mar 2016 19:13:48 +0100 Message-ID: From: alvise rigo Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC v7 10/16] softmmu: Protect MMIO exclusive range List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: MTTCG Devel , Claudio Fontana , QEMU Developers , Paolo Bonzini , Jani Kokkonen , VirtualOpenSystems Technical Team , Richard Henderson On Thu, Feb 18, 2016 at 5:25 PM, Alex Benn=C3=A9e = wrote: > > alvise rigo writes: > >> On Wed, Feb 17, 2016 at 7:55 PM, Alex Benn=C3=A9e wrote: >>> >>> Alvise Rigo writes: >>> >>>> As for the RAM case, also the MMIO exclusive ranges have to be protect= ed >>>> 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 i= s >>>> not concluded yet. >>>> >>>> This flag will force the other CPUs to invalidate the exclusive range = in >>>> case of collision. >>>> >>>> Suggested-by: Jani Kokkonen >>>> Suggested-by: Claudio Fontana >>>> Signed-off-by: Alvise Rigo >>>> --- >>>> 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 h= eld 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 =3D false; >>>> >>>> CPU_FOREACH(cpu) { >>>> - if (cpu->excl_protected_range.begin !=3D 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 =3D EXCLUSIVE_RESET_ADDR; >>>> + if (current_cpu !=3D 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. > > But aren't there other users before the functional change was made to > skip the current_cpu? Where their expectations wrong or should we have > always skipped the current CPU? I see your point now. When current_cpu was skipped, there was no need of the line cpu->excl_protected_range.begin =3D EXCLUSIVE_RESET_ADDR; in helper_stcond_name() when we return back from softmmu_template.h. The error is that that line should have been added in this patch, not in PATCH 07/16. Fixing it for the next version. > > The additional of the bool return I agree only needs to be brought in > now when there are functions that care. > >> >>> >>>> + if (cpu->excl_protected_range.begin !=3D 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 =3D EXCLUSIVE_RESET_A= DDR; >>>> + ret =3D 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, ta= rget_ulong addr, >>>> } >>>> } >>>> } >>>> + /* For this vCPU, just update the TLB entry, no need to flush= . */ >>>> + env->tlb_table[mmu_idx][index].addr_write |=3D TLB_EXCL; >>>> } else { >>>> - hw_error("EXCL accesses to MMIO regions not supported yet."); >>>> + /* Set a pending exclusive access in the MemoryRegion */ >>>> + MemoryRegion *mr =3D iotlb_to_region(this, >>>> + env->iotlb[mmu_idx][index]= .addr, >>>> + env->iotlb[mmu_idx][index]= .attrs); >>>> + mr->pending_excl_access =3D 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 |=3D TLB_EXCL; >>>> - >>>> /* From now on we are in LL/SC context */ >>>> this->ll_sc_context =3D 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)(CPUArch= State *env, >>>> MemoryRegion *mr =3D iotlb_to_region(cpu, physaddr, iotlbentry->a= ttrs); >>>> >>>> physaddr =3D (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? > > We want as short and semantically informative as possible. Naming things = is hard ;-) > > - reset_other_cpus_colliding_ll_addr > - reset_other_cpus_overlapping_ll_addr > > Any other options? Umm, these sound fine to me. Probably the first one since shorter. Thank you, alvise > >> >> Thank you, >> alvise >> >>>> + mr->pending_excl_access =3D false; >>>> + } >>>> + } >>>> + >>>> if (mr !=3D &io_mem_rom && mr !=3D &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, ad= dr, oi, >>>> mmu_idx, ind= ex, >>>> retaddr); >>>> + /* N.B.: Here excl_succeeded =3D=3D true means that t= his access >>>> + * comes from an exclusive instruction. */ >>>> + if (cpu->excl_succeeded) { >>>> + MemoryRegion *mr =3D iotlb_to_region(cpu, iotlben= try->addr, >>>> + iotlbentry->at= trs); >>>> + mr->pending_excl_access =3D false; >>>> + } >>>> } else { >>>> glue(helper_le_st_name, _do_ram_access)(env, val, add= r, oi, >>>> mmu_idx, inde= x, >>>> @@ -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, ad= dr, oi, >>>> mmu_idx, ind= ex, >>>> retaddr); >>>> + /* N.B.: Here excl_succeeded =3D=3D true means that t= his access >>>> + * comes from an exclusive instruction. */ >>>> + if (cpu->excl_succeeded) { >>>> + MemoryRegion *mr =3D iotlb_to_region(cpu, iotlben= try->addr, >>>> + iotlbentry->at= trs); >>>> + mr->pending_excl_access =3D 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, add= r, oi, >>>> mmu_idx, inde= x, >>> >>> >>> -- >>> Alex Benn=C3=A9e > > > -- > Alex Benn=C3=A9e