Re: [Qemu-devel] [RFC 01/30] softmmu: add cmpxchg helpers

[Emilio G. Cota]

On Mon, Jun 27, 2016 at 13:11:28 -0700, Richard Henderson wrote:
> On 06/27/2016 12:01 PM, Emilio G. Cota wrote:
> >Signed-off-by: Emilio G. Cota <address@hidden>
> >---
> > softmmu_template.h | 58 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > tcg/tcg.h          | 16 +++++++++++++++
> > 2 files changed, 74 insertions(+)
> >
> >diff --git a/softmmu_template.h b/softmmu_template.h
> >index 208f808..7b519dc 100644
> >--- a/softmmu_template.h
> >+++ b/softmmu_template.h
> >@@ -548,6 +548,64 @@ void probe_write(CPUArchState *env, target_ulong addr, 
> >int mmu_idx,
> >     }
> > }
> > #endif
> >+
> >+DATA_TYPE
> >+glue(glue(helper_cmpxchg, SUFFIX),
> >+     MMUSUFFIX)(CPUArchState *env, target_ulong addr, DATA_TYPE old,
> >+                DATA_TYPE new, TCGMemOpIdx oi, uintptr_t retaddr)
> >+{
> >+    unsigned mmu_idx = get_mmuidx(oi);
> >+    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;
> >+
> >+    /* Adjust the given return address.  */
> >+    retaddr -= GETPC_ADJ;
> >+
> >+    /* If the TLB entry is for a different page, reload and try again.  */
> >+    if ((addr & TARGET_PAGE_MASK)
> >+        != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
> >+        if (unlikely((addr & (DATA_SIZE - 1)) != 0
> >+                     && (get_memop(oi) & MO_AMASK) == MO_ALIGN)) {
> >+            cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
> >+                                 mmu_idx, retaddr);
> >+        }
> >+        if (!VICTIM_TLB_HIT(addr_write)) {
> >+            tlb_fill(ENV_GET_CPU(env), addr, MMU_DATA_STORE, mmu_idx, 
> >retaddr);
> >+        }
> >+        tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
> >+    }
> 
> You need to verify that addr_read == addr_write as well, so that tlb_fill
> can signal an exception in the rare case of a guest attempting a cmpxchg on
> a write-only page.

Will do.

> >+    /*
> >+     * If the host allows unaligned accesses, then let the compiler
> >+     * do its thing when performing the access on the host.
> >+     */
> >+    haddr = addr + env->tlb_table[mmu_idx][index].addend;
> >+    return atomic_cmpxchg((DATA_TYPE *)haddr, old, new);
> 
> Host endian operation?

I forgot to add byte ordering in the cover letter under "why this is
an RFC" -- I admit I'm confused by all the macro trickery done for
regular loads and stores.

We store data in memory as per the guests' byte ordering, right?
If so, I don't see how it would be possible to leverage the
host compiler for things like atomic_add -- we'd increment garbage,
not a meaningful value.

                Emilio