Re: [Qemu-devel] [RFC 2/5] Add new TLB_EXCL flag

[Richard Henderson <rth@twiddle.net>]

On 05/06/2015 08:38 AM, Alvise Rigo wrote:
> Add a new flag for the TLB entries to force all the accesses made to a
> page to follow the slow-path.
> 
> Mark the accessed page as dirty to invalidate any pending operation of
> LL/SC.
> 
> 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               |  7 ++++++-
>  include/exec/cpu-all.h |  1 +
>  softmmu_template.h     | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index 38f2151..3e4ccba 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -324,7 +324,12 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                                                     + xlat)) {
>              te->addr_write = address | TLB_NOTDIRTY;
>          } else {
> -            te->addr_write = address;
> +            if (!cpu_physical_memory_excl_is_dirty(section->mr->ram_addr
> +                                                   + xlat)) {
> +                te->addr_write = address | TLB_EXCL;
> +            } else {
> +                te->addr_write = address;
> +            }

I don't see that you initialize this new bitmap to all ones?  That would
generate exclusive accesses on all of memory until each unit (page?) is touched.

Perhaps you should invert the sense of your use of the dirty bitmap, such that
a set bit means that some vcpu is monitoring the unit, and no vcpu has written
to the unit?

Your nesting above would seem to fail when TLB_NOTDIRTY needs to be set as well.

> @@ -262,7 +263,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
>      uintptr_t haddr;
>      DATA_TYPE res;
>  
> -    /* Adjust the given return address.  */
> +        /* Adjust the given return address.  */
>      retaddr -= GETPC_ADJ;

Careful...

> @@ -387,6 +388,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
>      int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>      target_ulong tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
>      uintptr_t haddr;
> +    bool to_excl_mem = false;
>  
>      /* Adjust the given return address.  */
>      retaddr -= GETPC_ADJ;
> @@ -406,6 +408,14 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
>          tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
>      }
>  
> +    if (unlikely(tlb_addr & TLB_EXCL &&
> +        !(tlb_addr & ~(TARGET_PAGE_MASK | TLB_EXCL)))) {

This is (tlb_addr & ~TARGET_PAGE_MASK) == TLB_EXCL, but...

> +        /* The slow-path has been forced since we are reading a page used for a
> +         * load-link operation. */
> +        to_excl_mem = true;
> +        goto skip_io;

I'm not fond of either the boolean or the goto.

> +    }
> +
>      /* Handle an IO access.  */
>      if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) {

Nesting your unlikely code under one unlikely test seems better.  And would be
more likely to work in the case you need to handle both TLB_NOTDIRTY and TLB_EXCL.


r~