From: "Alex Bennée" <alex.bennee@linaro.org>
To: alvise rigo <a.rigo@virtualopensystems.com>
Cc: MTTCG Devel <mttcg@listserver.greensocs.com>,
Claudio Fontana <claudio.fontana@huawei.com>,
QEMU Developers <qemu-devel@nongnu.org>,
Paolo Bonzini <pbonzini@redhat.com>,
Jani Kokkonen <jani.kokkonen@huawei.com>,
VirtualOpenSystems Technical Team <tech@virtualopensystems.com>,
Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [RFC v7 10/16] softmmu: Protect MMIO exclusive range
Date: Thu, 18 Feb 2016 16:25:53 +0000 [thread overview]
Message-ID: <874md6dlym.fsf@linaro.org> (raw)
In-Reply-To: <CAH47eN23LcBcCWB0WiioBnNBDFmBicx8_XZ7rH0rxMgemuhgEg@mail.gmail.com>
alvise rigo <a.rigo@virtualopensystems.com> writes:
> On Wed, Feb 17, 2016 at 7:55 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Alvise Rigo <a.rigo@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.kokkonen@huawei.com>
>>> Suggested-by: Claudio Fontana <claudio.fontana@huawei.com>
>>> Signed-off-by: Alvise Rigo <a.rigo@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.
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?
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 != 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?
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?
>
> 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
--
Alex Bennée
next prev parent reply other threads:[~2016-02-18 16:26 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-29 9:32 [Qemu-devel] [RFC v7 00/16] Slow-path for atomic instruction translation Alvise Rigo
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 01/16] exec.c: Add new exclusive bitmap to ram_list Alvise Rigo
2016-02-11 13:00 ` Alex Bennée
2016-02-11 13:21 ` alvise rigo
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 02/16] softmmu: Simplify helper_*_st_name, wrap unaligned code Alvise Rigo
2016-02-11 13:07 ` Alex Bennée
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 03/16] softmmu: Simplify helper_*_st_name, wrap MMIO code Alvise Rigo
2016-02-11 13:15 ` Alex Bennée
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 04/16] softmmu: Simplify helper_*_st_name, wrap RAM code Alvise Rigo
2016-02-11 13:18 ` Alex Bennée
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 05/16] softmmu: Add new TLB_EXCL flag Alvise Rigo
2016-02-11 13:18 ` Alex Bennée
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 06/16] qom: cpu: Add CPUClass hooks for exclusive range Alvise Rigo
2016-02-11 13:22 ` Alex Bennée
2016-02-18 13:53 ` alvise rigo
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 07/16] softmmu: Add helpers for a new slowpath Alvise Rigo
2016-02-11 16:33 ` Alex Bennée
2016-02-18 13:58 ` alvise rigo
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 08/16] softmmu: Honor the new exclusive bitmap Alvise Rigo
2016-02-16 17:39 ` Alex Bennée
2016-02-18 14:18 ` alvise rigo
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 09/16] softmmu: Include MMIO/invalid exclusive accesses Alvise Rigo
2016-02-16 17:49 ` Alex Bennée
2016-02-18 14:18 ` alvise rigo
2016-02-18 16:26 ` Alex Bennée
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 10/16] softmmu: Protect MMIO exclusive range Alvise Rigo
2016-02-17 18:55 ` Alex Bennée
2016-02-18 14:15 ` alvise rigo
2016-02-18 16:25 ` Alex Bennée [this message]
2016-03-07 18:13 ` alvise rigo
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 11/16] tcg: Create new runtime helpers for excl accesses Alvise Rigo
2016-02-18 16:16 ` Alex Bennée
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 12/16] configure: Use slow-path for atomic only when the softmmu is enabled Alvise Rigo
2016-02-18 16:40 ` Alex Bennée
2016-02-18 16:43 ` Alex Bennée
2016-03-07 17:21 ` alvise rigo
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 13/16] softmmu: Add history of excl accesses Alvise Rigo
2016-02-16 17:07 ` Alex Bennée
2016-02-18 14:17 ` alvise rigo
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 14/16] target-arm: translate: Use ld/st excl for atomic insns Alvise Rigo
2016-02-18 17:02 ` Alex Bennée
2016-03-07 18:39 ` alvise rigo
2016-03-07 20:06 ` Alex Bennée
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 15/16] target-arm: cpu64: use custom set_excl hook Alvise Rigo
2016-02-18 18:19 ` Alex Bennée
2016-01-29 9:32 ` [Qemu-devel] [RFC v7 16/16] target-arm: aarch64: add atomic instructions Alvise Rigo
2016-02-19 11:34 ` Alex Bennée
2016-02-19 11:44 ` [Qemu-devel] [RFC v7 00/16] Slow-path for atomic instruction translation Alex Bennée
2016-02-19 12:01 ` alvise rigo
2016-02-19 12:19 ` Alex Bennée
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874md6dlym.fsf@linaro.org \
--to=alex.bennee@linaro.org \
--cc=a.rigo@virtualopensystems.com \
--cc=claudio.fontana@huawei.com \
--cc=jani.kokkonen@huawei.com \
--cc=mttcg@listserver.greensocs.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
--cc=tech@virtualopensystems.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).