qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC v7 10/16] softmmu: Protect MMIO exclusive range


From: alvise rigo
Subject: Re: [Qemu-devel] [RFC v7 10/16] softmmu: Protect MMIO exclusive range
Date: Mon, 7 Mar 2016 19:13:48 +0100

On Thu, Feb 18, 2016 at 5:25 PM, Alex Bennée <address@hidden> wrote:
>
> alvise rigo <address@hidden> writes:
>
>> On Wed, Feb 17, 2016 at 7:55 PM, Alex Bennée <address@hidden> wrote:
>>>
>>> Alvise Rigo <address@hidden> 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 <address@hidden>
>>>> Suggested-by: Claudio Fontana <address@hidden>
>>>> Signed-off-by: Alvise Rigo <address@hidden>
>>>> ---
>>>>  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?

I see your point now. When current_cpu was skipped, there was no need
of the line
cpu->excl_protected_range.begin = 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 != 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?

Umm, these sound fine to me. Probably the first one since shorter.

Thank you,
alvise

>
>>
>> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]