[Qemu-devel] [RFC v1 0/3] Fixup exclusive store logic

[Qemu-devel] [RFC v1 0/3] Fixup exclusive store logic

[Alistair Francis]

I found some issues with the way exclusive store was working. This patch
series seems to fix the test cases that were failing for me and also
seem to follow what the ARM ARM says.

The first patch is just a simple adjustment.

The second patch is just preparing for the third patch.

The third patch is where the big change is. It includes details of the
limited testing that I have done.

Alistair Francis (3):
  target/arm: Update the memops for exclusive load
  tcg/tcg-op: Expose the tcg_gen_ext_i* functions
  target/arm: Correct exclusive store return value

 target/arm/translate-a64.c | 24 +++++++++++++-----------
 tcg/tcg-op.c               |  4 ++--
 tcg/tcg-op.h               |  2 ++
 3 files changed, 17 insertions(+), 13 deletions(-)

-- 
2.11.0

[Qemu-devel] [RFC v1 1/3] target/arm: Update the memops for exclusive load

[Alistair Francis]

Acording to the ARM ARM exclusive loads require the same allignment as
exclusive stores. Let's update the memops used for the load to match
that of the store. This adds the alignment requirement to the memops.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target/arm/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 58ed4c6d05..245175e2f1 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1854,7 +1854,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
                                TCGv_i64 addr, int size, bool is_pair)
 {
     TCGv_i64 tmp = tcg_temp_new_i64();
-    TCGMemOp memop = s->be_data + size;
+    TCGMemOp memop = size | MO_ALIGN | s->be_data;
 
     g_assert(size <= 3);
     tcg_gen_qemu_ld_i64(tmp, addr, get_mem_index(s), memop);
-- 
2.11.0

Re: [Qemu-devel] [RFC v1 1/3] target/arm: Update the memops for exclusive load

[Richard Henderson]

On 08/11/2017 11:19 AM, Alistair Francis wrote:
> Acording to the ARM ARM exclusive loads require the same allignment as
> exclusive stores. Let's update the memops used for the load to match
> that of the store. This adds the alignment requirement to the memops.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>  target/arm/translate-a64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [RFC v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions

[Alistair Francis]

Expose the tcg_gen_ext_i32() and tcg_gen_ext_i64() functions as we are
going to use them later.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 tcg/tcg-op.c | 4 ++--
 tcg/tcg-op.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 87f673ef49..d25e3003ef 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2709,7 +2709,7 @@ void tcg_gen_qemu_st_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
     gen_ldst_i64(INDEX_op_qemu_st_i64, val, addr, memop, idx);
 }
 
-static void tcg_gen_ext_i32(TCGv_i32 ret, TCGv_i32 val, TCGMemOp opc)
+void tcg_gen_ext_i32(TCGv_i32 ret, TCGv_i32 val, TCGMemOp opc)
 {
     switch (opc & MO_SSIZE) {
     case MO_SB:
@@ -2730,7 +2730,7 @@ static void tcg_gen_ext_i32(TCGv_i32 ret, TCGv_i32 val, TCGMemOp opc)
     }
 }
 
-static void tcg_gen_ext_i64(TCGv_i64 ret, TCGv_i64 val, TCGMemOp opc)
+void tcg_gen_ext_i64(TCGv_i64 ret, TCGv_i64 val, TCGMemOp opc)
 {
     switch (opc & MO_SSIZE) {
     case MO_SB:
diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index 5d3278f243..8c45b79a92 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-op.h
@@ -835,6 +835,8 @@ void tcg_gen_qemu_ld_i32(TCGv_i32, TCGv, TCGArg, TCGMemOp);
 void tcg_gen_qemu_st_i32(TCGv_i32, TCGv, TCGArg, TCGMemOp);
 void tcg_gen_qemu_ld_i64(TCGv_i64, TCGv, TCGArg, TCGMemOp);
 void tcg_gen_qemu_st_i64(TCGv_i64, TCGv, TCGArg, TCGMemOp);
+void tcg_gen_ext_i32(TCGv_i32 ret, TCGv_i32 val, TCGMemOp opc);
+void tcg_gen_ext_i64(TCGv_i64 ret, TCGv_i64 val, TCGMemOp opc);
 
 static inline void tcg_gen_qemu_ld8u(TCGv ret, TCGv addr, int mem_index)
 {
-- 
2.11.0

Re: [Qemu-devel] [RFC v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions

[Richard Henderson]

On 08/11/2017 11:19 AM, Alistair Francis wrote:
> Expose the tcg_gen_ext_i32() and tcg_gen_ext_i64() functions as we are
> going to use them later.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>  tcg/tcg-op.c | 4 ++--
>  tcg/tcg-op.h | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)

This is a good idea, since I'm certain we have several copies of this in
several of the target translators.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [RFC v1 3/3] target/arm: Correct exclusive store return value

[Alistair Francis]

The exclusive store operation should return 0 if the operation updates
memory and 1 if it doesn't. This means that storing tmp in the rd
register is incorrect.

This patch updates the succesful opertion to store 0 into the rd
register instead of tmp. It also adds a branch to fail if the memory
isn't updated.

In order to add a branch for the pair case when size equals 2 we first
need to apply the same memory operation on the exclusive value in order
for the comparison to work.

There is still no value checks added if we are doing a 64-bit store with
pairs.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
This was caught with an internal fuzzy tester. These patches fix the
Xilinx 2.10-rc2 tree. I tested with the fuzzy tester (single CPU) and
Linux boot (4 CPUs) on the Xilinx tree. I don't have a good test case to
run on mainline at the moment, but I'm working on getting one. Also
linux-user is fully untested.

All tests were with MTTCG enabled.

 target/arm/translate-a64.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 245175e2f1..ea7c61bc6f 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1894,10 +1894,11 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
      * }
      * env->exclusive_addr = -1;
      */
+    TCGMemOp memop = size | MO_ALIGN | s->be_data;
     TCGLabel *fail_label = gen_new_label();
     TCGLabel *done_label = gen_new_label();
     TCGv_i64 addr = tcg_temp_local_new_i64();
-    TCGv_i64 tmp;
+    TCGv_i64 tmp, val;
 
     /* Copy input into a local temp so it is not trashed when the
      * basic block ends at the branch insn.
@@ -1907,15 +1908,15 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
 
     tmp = tcg_temp_new_i64();
     if (is_pair) {
+        val = tcg_temp_new_i64();
         if (size == 2) {
-            TCGv_i64 val = tcg_temp_new_i64();
             tcg_gen_concat32_i64(tmp, cpu_reg(s, rt), cpu_reg(s, rt2));
             tcg_gen_concat32_i64(val, cpu_exclusive_val, cpu_exclusive_high);
             tcg_gen_atomic_cmpxchg_i64(tmp, addr, val, tmp,
                                        get_mem_index(s),
-                                       size | MO_ALIGN | s->be_data);
-            tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
-            tcg_temp_free_i64(val);
+                                       memop);
+            tcg_gen_ext_i64(val, val, memop);
+            tcg_gen_brcond_i64(TCG_COND_NE, tmp, val, fail_label);
         } else if (s->be_data == MO_LE) {
             gen_helper_paired_cmpxchg64_le(tmp, cpu_env, addr, cpu_reg(s, rt),
                                            cpu_reg(s, rt2));
@@ -1924,22 +1925,23 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
                                            cpu_reg(s, rt2));
         }
     } else {
-        TCGv_i64 val = cpu_reg(s, rt);
+        val = cpu_reg(s, rt);
         tcg_gen_atomic_cmpxchg_i64(tmp, addr, cpu_exclusive_val, val,
                                    get_mem_index(s),
-                                   size | MO_ALIGN | s->be_data);
-        tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, cpu_exclusive_val);
+                                   memop);
+        tcg_gen_brcond_i64(TCG_COND_NE, tmp, cpu_exclusive_val, fail_label);
     }
 
     tcg_temp_free_i64(addr);
 
-    tcg_gen_mov_i64(cpu_reg(s, rd), tmp);
-    tcg_temp_free_i64(tmp);
+    tcg_gen_movi_i64(cpu_reg(s, rd), 0);
     tcg_gen_br(done_label);
 
     gen_set_label(fail_label);
     tcg_gen_movi_i64(cpu_reg(s, rd), 1);
     gen_set_label(done_label);
+    tcg_temp_free_i64(tmp);
+    tcg_temp_free_i64(val);
     tcg_gen_movi_i64(cpu_exclusive_addr, -1);
 }
 
-- 
2.11.0

Re: [Qemu-devel] [RFC v1 3/3] target/arm: Correct exclusive store return value

[Richard Henderson]

On 08/11/2017 11:19 AM, Alistair Francis wrote:
> The exclusive store operation should return 0 if the operation updates
> memory and 1 if it doesn't. This means that storing tmp in the rd
> register is incorrect.

I'm confused as to what you believe is wrong.

>              tcg_gen_atomic_cmpxchg_i64(tmp, addr, val, tmp,
>                                         get_mem_index(s),
> -                                       size | MO_ALIGN | s->be_data);
> -            tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);

Yes, we load the result of the cmpxchg into tmp, but then we immediately
overwrite tmp with 0/1 depending on whether the operation succeeded.

> 
> This patch updates the succesful opertion to store 0 into the rd
> register instead of tmp. It also adds a branch to fail if the memory
> isn't updated.

Since we use setcond, a branch is not required.

> +            tcg_gen_ext_i64(val, val, memop);

What is this addition intended to accomplish?  Because of the position within
the code, you know that memop contains MO_64, so that this is a no-op.


r~

Re: [Qemu-devel] [RFC v1 3/3] target/arm: Correct exclusive store return value

[Alistair Francis]

On Fri, Aug 11, 2017 at 12:46 PM, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 08/11/2017 11:19 AM, Alistair Francis wrote:
>> The exclusive store operation should return 0 if the operation updates
>> memory and 1 if it doesn't. This means that storing tmp in the rd
>> register is incorrect.
>
> I'm confused as to what you believe is wrong.
>
>>              tcg_gen_atomic_cmpxchg_i64(tmp, addr, val, tmp,
>>                                         get_mem_index(s),
>> -                                       size | MO_ALIGN | s->be_data);
>> -            tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
>
> Yes, we load the result of the cmpxchg into tmp, but then we immediately
> overwrite tmp with 0/1 depending on whether the operation succeeded.

Hmmm... When I looked at the values in tmp I was seeing non 0/1 values in there.

>
>>
>> This patch updates the succesful opertion to store 0 into the rd
>> register instead of tmp. It also adds a branch to fail if the memory
>> isn't updated.
>
> Since we use setcond, a branch is not required.
>
>> +            tcg_gen_ext_i64(val, val, memop);
>
> What is this addition intended to accomplish?  Because of the position within
> the code, you know that memop contains MO_64, so that this is a no-op.

This is when size == 2 so it's a 32bit operation so memop contains MO_32.

Thanks,
Alistair

>
>
> r~

Re: [Qemu-devel] [RFC v1 3/3] target/arm: Correct exclusive store return value

[Richard Henderson]

On 08/11/2017 01:13 PM, Alistair Francis wrote:
>>> +            tcg_gen_ext_i64(val, val, memop);
>>
>> What is this addition intended to accomplish?  Because of the position within
>> the code, you know that memop contains MO_64, so that this is a no-op.
> 
> This is when size == 2 so it's a 32bit operation so memop contains MO_32.

It's a paired 32-bit operation, so we're operating on a 64-bit quantity.  So
extending from 32-bits would be actively wrong.


r~

Re: [Qemu-devel] [RFC v1 3/3] target/arm: Correct exclusive store return value

[Alistair Francis]

On Fri, Aug 11, 2017 at 1:24 PM, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 08/11/2017 01:13 PM, Alistair Francis wrote:
>>>> +            tcg_gen_ext_i64(val, val, memop);
>>>
>>> What is this addition intended to accomplish?  Because of the position within
>>> the code, you know that memop contains MO_64, so that this is a no-op.
>>
>> This is when size == 2 so it's a 32bit operation so memop contains MO_32.
>
> It's a paired 32-bit operation, so we're operating on a 64-bit quantity.  So
> extending from 32-bits would be actively wrong.

>From what I can see though, the 32bit memop is carried into the
tcg_gen_atomic_cmpxchg_i64() call so the value returned to tmp is
masked by the 32bit operation.

Is passing down MO_32 into tcg_gen_atomic_cmpxchg_i64() wrong then as
it ends up as a 64-bit operation?

My TCG knowledge is pretty limited here.

Thanks,
Alistair

>
>
> r~
>

Re: [Qemu-devel] [RFC v1 3/3] target/arm: Correct exclusive store return value

[Richard Henderson]

On 08/11/2017 01:29 PM, Alistair Francis wrote:
> On Fri, Aug 11, 2017 at 1:24 PM, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> On 08/11/2017 01:13 PM, Alistair Francis wrote:
>>>>> +            tcg_gen_ext_i64(val, val, memop);
>>>>
>>>> What is this addition intended to accomplish?  Because of the position within
>>>> the code, you know that memop contains MO_64, so that this is a no-op.
>>>
>>> This is when size == 2 so it's a 32bit operation so memop contains MO_32.
>>
>> It's a paired 32-bit operation, so we're operating on a 64-bit quantity.  So
>> extending from 32-bits would be actively wrong.
> 
> From what I can see though, the 32bit memop is carried into the
> tcg_gen_atomic_cmpxchg_i64() call so the value returned to tmp is
> masked by the 32bit operation.
> 
> Is passing down MO_32 into tcg_gen_atomic_cmpxchg_i64() wrong then as
> it ends up as a 64-bit operation?

If we're passing MO_32 down to cmpxchg_i64 for this case, you have indeed found
a bug.  I'll investigate this further on Monday.


r~

Re: [Qemu-devel] [RFC v1 3/3] target/arm: Correct exclusive store return value

[Alistair Francis]

On Fri, Aug 11, 2017 at 1:38 PM, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 08/11/2017 01:29 PM, Alistair Francis wrote:
>> On Fri, Aug 11, 2017 at 1:24 PM, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>> On 08/11/2017 01:13 PM, Alistair Francis wrote:
>>>>>> +            tcg_gen_ext_i64(val, val, memop);
>>>>>
>>>>> What is this addition intended to accomplish?  Because of the position within
>>>>> the code, you know that memop contains MO_64, so that this is a no-op.
>>>>
>>>> This is when size == 2 so it's a 32bit operation so memop contains MO_32.
>>>
>>> It's a paired 32-bit operation, so we're operating on a 64-bit quantity.  So
>>> extending from 32-bits would be actively wrong.
>>
>> From what I can see though, the 32bit memop is carried into the
>> tcg_gen_atomic_cmpxchg_i64() call so the value returned to tmp is
>> masked by the 32bit operation.
>>
>> Is passing down MO_32 into tcg_gen_atomic_cmpxchg_i64() wrong then as
>> it ends up as a 64-bit operation?
>
> If we're passing MO_32 down to cmpxchg_i64 for this case, you have indeed found
> a bug.  I'll investigate this further on Monday.

Maybe that is why I'm seeing these failures. I'll have a look as well
to see if this fixes my problems.

Thanks,
Alistair

>
>
> r~
>

Re: [Qemu-devel] [RFC v1 3/3] target/arm: Correct exclusive store return value

[Alistair Francis]

On Fri, Aug 11, 2017 at 1:39 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Fri, Aug 11, 2017 at 1:38 PM, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> On 08/11/2017 01:29 PM, Alistair Francis wrote:
>>> On Fri, Aug 11, 2017 at 1:24 PM, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>> On 08/11/2017 01:13 PM, Alistair Francis wrote:
>>>>>>> +            tcg_gen_ext_i64(val, val, memop);
>>>>>>
>>>>>> What is this addition intended to accomplish?  Because of the position within
>>>>>> the code, you know that memop contains MO_64, so that this is a no-op.
>>>>>
>>>>> This is when size == 2 so it's a 32bit operation so memop contains MO_32.
>>>>
>>>> It's a paired 32-bit operation, so we're operating on a 64-bit quantity.  So
>>>> extending from 32-bits would be actively wrong.
>>>
>>> From what I can see though, the 32bit memop is carried into the
>>> tcg_gen_atomic_cmpxchg_i64() call so the value returned to tmp is
>>> masked by the 32bit operation.
>>>
>>> Is passing down MO_32 into tcg_gen_atomic_cmpxchg_i64() wrong then as
>>> it ends up as a 64-bit operation?
>>
>> If we're passing MO_32 down to cmpxchg_i64 for this case, you have indeed found
>> a bug.  I'll investigate this further on Monday.
>
> Maybe that is why I'm seeing these failures. I'll have a look as well
> to see if this fixes my problems.

That's it. That wrong mask was causing all my breakages.

I'll send out a new series, thanks for your help.

Thanks,
Alistair

>
> Thanks,
> Alistair
>
>>
>>
>> r~
>>