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

[alvise rigo]

Hi Richard,

Thank you for looking at this.
Some comments below.

On Thu, May 7, 2015 at 7:25 PM, Richard Henderson <address@hidden> wrote:
>
> 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 <address@hidden>
> > Suggested-by: Claudio Fontana <address@hidden>
> > Signed-off-by: Alvise Rigo <address@hidden>
> > ---
> >  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?


Actually the sense of the bitmap is already inverted (0 means dirty),
in this way I didn't have to touch the code to initialize the bitmap
in exec.c.
In the next iteration I will make this more clear by naming it
differently or by initializing it with all ones.

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

I missed this..

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

In this case the goto seems to make the code a bit cleaner, but if
it's necessary I will remove them.

>
> > +    }
> > +
> >      /* 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.

OK, I will keep this in mind for the next version.

Thank you,
alvise

>
>
> r~