Hi Richard,
Thank you for looking at this.
Some comments below.
On Thu, May 7, 2015 at 7:25 PM, Richard Henderson <r...@twiddle.net> 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 <jani.kokko...@huawei.com>
> > Suggested-by: Claudio Fontana <claudio.font...@huawei.com>
> > Signed-off-by: Alvise Rigo <a.r...@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?
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~
