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

[Alex Bennée]

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?

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