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

[Richard Henderson]

On 06/27/2016 02:48 PM, Peter Maydell wrote:

On 27 June 2016 at 22:43, Richard Henderson  wrote:

All you need to do is byte-reverse the data.

  bswap(a + b) == bswap(a) + bswap(b).


?

0xFF + 0xFF == 0x1FE, bswap(0x1FE) == 0xFE01
bswap(0xFF) + bswap(0xFF) == 0xFF00 + 0xFF00 == 0x1FE00
(or 0xFE00 with truncate to 32-bit).

Or am I missing something?


No, you're quite right.  I think I didn't have enough coffee this morning.

So we'd need a cmpxchg loop for reverse-endian add.


r~

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

[Peter Maydell]

On 27 June 2016 at 22:43, Richard Henderson  wrote:
> All you need to do is byte-reverse the data.
>
>   bswap(a + b) == bswap(a) + bswap(b).

?

0xFF + 0xFF == 0x1FE, bswap(0x1FE) == 0xFE01
bswap(0xFF) + bswap(0xFF) == 0xFF00 + 0xFF00 == 0x1FE00
(or 0xFE00 with truncate to 32-bit).

Or am I missing something?

thanks
-- PMM

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

[Richard Henderson]

On 06/27/2016 02:19 PM, Emilio G. Cota wrote:

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?


Sometimes.  The guest can also explicitly request a reversed byte load/store. 
This is used both for explicit byte-reversing instructions (e.g. x86 movbe) and 
toggling the system byte order (arm setbe).




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.


All you need to do is byte-reverse the data.

  bswap(a + b) == bswap(a) + bswap(b).


r~

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

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

[Richard Henderson]

On 06/27/2016 12:01 PM, Emilio G. Cota wrote:

Signed-off-by: Emilio G. Cota 
---
 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.



+/*
+ * 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?


r~