qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v1 0/3]  Fixup exclusive store logic
@ 2017-08-11 22:17 Alistair Francis
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 1/3] target/arm: Update the memops for exclusive load Alistair Francis
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Alistair Francis @ 2017-08-11 22:17 UTC (permalink / raw)
  To: qemu-devel, peter.maydell
  Cc: alistair.francis, alistair23, edgar.iglesias, edgar.iglesias,
	qemu-arm

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.

The first patch is just a simple adjustment.

The third patch fixes the main bug I was seeing.

The second patch is left over from the RFC that seems like it is still a
good idea.

Changes from RFC:
 - Rewrite the third patch to correctly fix the issue.

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 cmpxchg memop mask

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

-- 
2.11.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH v1 1/3] target/arm: Update the memops for exclusive load
  2017-08-11 22:17 [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
@ 2017-08-11 22:17 ` Alistair Francis
  2017-08-12 11:38   ` Edgar E. Iglesias
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions Alistair Francis
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Alistair Francis @ 2017-08-11 22:17 UTC (permalink / raw)
  To: qemu-devel, peter.maydell
  Cc: alistair.francis, alistair23, edgar.iglesias, edgar.iglesias,
	qemu-arm

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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---

 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions
  2017-08-11 22:17 [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 1/3] target/arm: Update the memops for exclusive load Alistair Francis
@ 2017-08-11 22:17 ` Alistair Francis
  2017-08-12 11:39   ` Edgar E. Iglesias
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask Alistair Francis
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Alistair Francis @ 2017-08-11 22:17 UTC (permalink / raw)
  To: qemu-devel, peter.maydell
  Cc: alistair.francis, alistair23, edgar.iglesias, edgar.iglesias,
	qemu-arm

Expose the tcg_gen_ext_i32() and tcg_gen_ext_i64() functions.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---

Although I no longer am using these functions I have left this patch in
as Richard thought it was a good idea.

 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask
  2017-08-11 22:17 [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 1/3] target/arm: Update the memops for exclusive load Alistair Francis
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions Alistair Francis
@ 2017-08-11 22:17 ` Alistair Francis
  2017-08-12 11:36   ` Edgar E. Iglesias
  2017-08-12 15:01   ` Richard Henderson
  2017-08-11 23:21 ` [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
  2017-08-12 10:24 ` Peter Maydell
  4 siblings, 2 replies; 14+ messages in thread
From: Alistair Francis @ 2017-08-11 22:17 UTC (permalink / raw)
  To: qemu-devel, peter.maydell
  Cc: alistair.francis, alistair23, edgar.iglesias, edgar.iglesias,
	qemu-arm

When we perform the atomic_cmpxchg operation we want to perform the
operation on a pair of 32-bit registers. Previously we were just passing
the register size in which was set to MO_32. This would result in the
high register to be ignored. To fix this issue we hardcode the size to
be 64-bits long when operating on 32-bit 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, but am working with some internal teams to get one.
Also linux-user is fully untested.

All tests were with MTTCG enabled.

 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 245175e2f1..49b4d6918d 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1913,7 +1913,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int 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);
+                                       MO_64 | MO_ALIGN | s->be_data);
             tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
             tcg_temp_free_i64(val);
         } else if (s->be_data == MO_LE) {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic
  2017-08-11 22:17 [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
                   ` (2 preceding siblings ...)
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask Alistair Francis
@ 2017-08-11 23:21 ` Alistair Francis
  2017-08-11 23:22   ` Alistair Francis
  2017-08-12 10:24 ` Peter Maydell
  4 siblings, 1 reply; 14+ messages in thread
From: Alistair Francis @ 2017-08-11 23:21 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell, Edgar Iglesias,
	Edgar Iglesias, qemu-arm

On Fri, Aug 11, 2017 at 3:17 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> 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.
>
> The first patch is just a simple adjustment.
>
> The third patch fixes the main bug I was seeing.
>
> The second patch is left over from the RFC that seems like it is still a
> good idea.

After working with the internal fuzzy testing team I have a test case
where exclusive operations are failing on master but working on top of
this patch series.

Thanks,
Alistair

>
> Changes from RFC:
>  - Rewrite the third patch to correctly fix the issue.
>
> 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 cmpxchg memop mask
>
>  target/arm/translate-a64.c | 4 ++--
>  tcg/tcg-op.c               | 4 ++--
>  tcg/tcg-op.h               | 2 ++
>  3 files changed, 6 insertions(+), 4 deletions(-)
>
> --
> 2.11.0
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic
  2017-08-11 23:21 ` [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
@ 2017-08-11 23:22   ` Alistair Francis
  2017-08-11 23:31     ` Portia Stephens
  0 siblings, 1 reply; 14+ messages in thread
From: Alistair Francis @ 2017-08-11 23:22 UTC (permalink / raw)
  To: Alistair Francis, portia.stephens
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell, Edgar Iglesias,
	Edgar Iglesias, qemu-arm

On Fri, Aug 11, 2017 at 4:21 PM, Alistair Francis <alistair23@gmail.com> wrote:
> On Fri, Aug 11, 2017 at 3:17 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> 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.
>>
>> The first patch is just a simple adjustment.
>>
>> The third patch fixes the main bug I was seeing.
>>
>> The second patch is left over from the RFC that seems like it is still a
>> good idea.

+ Portia from fuzzy testing team.

>
> After working with the internal fuzzy testing team I have a test case
> where exclusive operations are failing on master but working on top of
> this patch series.
>
> Thanks,
> Alistair
>
>>
>> Changes from RFC:
>>  - Rewrite the third patch to correctly fix the issue.
>>
>> 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 cmpxchg memop mask
>>
>>  target/arm/translate-a64.c | 4 ++--
>>  tcg/tcg-op.c               | 4 ++--
>>  tcg/tcg-op.h               | 2 ++
>>  3 files changed, 6 insertions(+), 4 deletions(-)
>>
>> --
>> 2.11.0
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic
  2017-08-11 23:22   ` Alistair Francis
@ 2017-08-11 23:31     ` Portia Stephens
  0 siblings, 0 replies; 14+ messages in thread
From: Portia Stephens @ 2017-08-11 23:31 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell, Edgar Iglesias,
	Edgar Iglesias, qemu-arm

> -----Original Message-----
> From: alistair23@gmail.com [mailto:alistair23@gmail.com] On Behalf Of
> Alistair Francis
> Sent: Friday, August 11, 2017 4:23 PM
> To: Alistair Francis <alistai@xilinx.com>; Portia Stephens
> <portias@xilinx.com>
> Cc: qemu-devel@nongnu.org Developers <qemu-devel@nongnu.org>;
> Peter Maydell <peter.maydell@linaro.org>; Edgar Iglesias
> <edgari@xilinx.com>; Edgar Iglesias <edgar.iglesias@gmail.com>; qemu-arm
> <qemu-arm@nongnu.org>
> Subject: Re: [PATCH v1 0/3] Fixup exclusive store logic
>
> On Fri, Aug 11, 2017 at 4:21 PM, Alistair Francis <alistair23@gmail.com> wrote:
> > On Fri, Aug 11, 2017 at 3:17 PM, Alistair Francis
> > <alistair.francis@xilinx.com> wrote:
> >> 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.
> >>
> >> The first patch is just a simple adjustment.
> >>
> >> The third patch fixes the main bug I was seeing.
> >>
> >> The second patch is left over from the RFC that seems like it is still a
> >> good idea.
>
> + Portia from fuzzy testing team.
>
> >
> > After working with the internal fuzzy testing team I have a test case
> > where exclusive operations are failing on master but working on top of
> > this patch series.
> >

This patch series fixes the failures previously seen with exclusive stores
by our internal tests on AArch64.

Tested-by: Portia Stephens <portia.stephens@xilinx.com>

> > Thanks,
> > Alistair
> >
> >>
> >> Changes from RFC:
> >>  - Rewrite the third patch to correctly fix the issue.
> >>
> >> 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 cmpxchg memop mask
> >>
> >>  target/arm/translate-a64.c | 4 ++--
> >>  tcg/tcg-op.c               | 4 ++--
> >>  tcg/tcg-op.h               | 2 ++
> >>  3 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> --
> >> 2.11.0
> >>


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic
  2017-08-11 22:17 [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
                   ` (3 preceding siblings ...)
  2017-08-11 23:21 ` [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
@ 2017-08-12 10:24 ` Peter Maydell
  2017-08-12 11:42   ` Edgar E. Iglesias
  4 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2017-08-12 10:24 UTC (permalink / raw)
  To: Alistair Francis
  Cc: QEMU Developers, Alistair Francis, Edgar Iglesias,
	Edgar E. Iglesias, qemu-arm

On 11 August 2017 at 23:17, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> 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.
>
> The first patch is just a simple adjustment.
>
> The third patch fixes the main bug I was seeing.
>
> The second patch is left over from the RFC that seems like it is still a
> good idea.
>
> Changes from RFC:
>  - Rewrite the third patch to correctly fix the issue.
>
> 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 cmpxchg memop mask
>
>  target/arm/translate-a64.c | 4 ++--
>  tcg/tcg-op.c               | 4 ++--
>  tcg/tcg-op.h               | 2 ++
>  3 files changed, 6 insertions(+), 4 deletions(-)

Is this series (or at least patches 1 and 3) worth putting
into 2.10 ?

thanks
-- PMM

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask Alistair Francis
@ 2017-08-12 11:36   ` Edgar E. Iglesias
  2017-08-12 15:01   ` Richard Henderson
  1 sibling, 0 replies; 14+ messages in thread
From: Edgar E. Iglesias @ 2017-08-12 11:36 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel, peter.maydell, alistair23, edgar.iglesias, qemu-arm

On Fri, Aug 11, 2017 at 03:17:41PM -0700, Alistair Francis wrote:
> When we perform the atomic_cmpxchg operation we want to perform the
> operation on a pair of 32-bit registers. Previously we were just passing
> the register size in which was set to MO_32. This would result in the
> high register to be ignored. To fix this issue we hardcode the size to
> be 64-bits long when operating on 32-bit pairs.

Good catch Alistair!
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> 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, but am working with some internal teams to get one.
> Also linux-user is fully untested.
> 
> All tests were with MTTCG enabled.
> 
>  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 245175e2f1..49b4d6918d 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1913,7 +1913,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int 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);
> +                                       MO_64 | MO_ALIGN | s->be_data);
>              tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
>              tcg_temp_free_i64(val);
>          } else if (s->be_data == MO_LE) {
> -- 
> 2.11.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 1/3] target/arm: Update the memops for exclusive load
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 1/3] target/arm: Update the memops for exclusive load Alistair Francis
@ 2017-08-12 11:38   ` Edgar E. Iglesias
  0 siblings, 0 replies; 14+ messages in thread
From: Edgar E. Iglesias @ 2017-08-12 11:38 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel, peter.maydell, alistair23, edgar.iglesias, qemu-arm

On Fri, Aug 11, 2017 at 03:17:36PM -0700, 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@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
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions Alistair Francis
@ 2017-08-12 11:39   ` Edgar E. Iglesias
  0 siblings, 0 replies; 14+ messages in thread
From: Edgar E. Iglesias @ 2017-08-12 11:39 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel, peter.maydell, alistair23, edgar.iglesias, qemu-arm

On Fri, Aug 11, 2017 at 03:17:38PM -0700, Alistair Francis wrote:
> Expose the tcg_gen_ext_i32() and tcg_gen_ext_i64() functions.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
> 
> Although I no longer am using these functions I have left this patch in
> as Richard thought it was a good idea.
> 
>  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
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic
  2017-08-12 10:24 ` Peter Maydell
@ 2017-08-12 11:42   ` Edgar E. Iglesias
  2017-08-12 13:52     ` Alistair Francis
  0 siblings, 1 reply; 14+ messages in thread
From: Edgar E. Iglesias @ 2017-08-12 11:42 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Alistair Francis, QEMU Developers, Alistair Francis,
	Edgar E. Iglesias, qemu-arm

On Sat, Aug 12, 2017 at 11:24:30AM +0100, Peter Maydell wrote:
> On 11 August 2017 at 23:17, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
> > 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.
> >
> > The first patch is just a simple adjustment.
> >
> > The third patch fixes the main bug I was seeing.
> >
> > The second patch is left over from the RFC that seems like it is still a
> > good idea.
> >
> > Changes from RFC:
> >  - Rewrite the third patch to correctly fix the issue.
> >
> > 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 cmpxchg memop mask
> >
> >  target/arm/translate-a64.c | 4 ++--
> >  tcg/tcg-op.c               | 4 ++--
> >  tcg/tcg-op.h               | 2 ++
> >  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> Is this series (or at least patches 1 and 3) worth putting
> into 2.10 ?


I would vote for including it...

Cheers,
Edgar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic
  2017-08-12 11:42   ` Edgar E. Iglesias
@ 2017-08-12 13:52     ` Alistair Francis
  0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2017-08-12 13:52 UTC (permalink / raw)
  To: Edgar E. Iglesias
  Cc: Peter Maydell, Alistair Francis, QEMU Developers,
	Edgar E. Iglesias, qemu-arm

On Sat, Aug 12, 2017 at 4:42 AM, Edgar E. Iglesias
<edgar.iglesias@xilinx.com> wrote:
> On Sat, Aug 12, 2017 at 11:24:30AM +0100, Peter Maydell wrote:
>> On 11 August 2017 at 23:17, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>> > 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.
>> >
>> > The first patch is just a simple adjustment.
>> >
>> > The third patch fixes the main bug I was seeing.
>> >
>> > The second patch is left over from the RFC that seems like it is still a
>> > good idea.
>> >
>> > Changes from RFC:
>> >  - Rewrite the third patch to correctly fix the issue.
>> >
>> > 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 cmpxchg memop mask
>> >
>> >  target/arm/translate-a64.c | 4 ++--
>> >  tcg/tcg-op.c               | 4 ++--
>> >  tcg/tcg-op.h               | 2 ++
>> >  3 files changed, 6 insertions(+), 4 deletions(-)
>>
>> Is this series (or at least patches 1 and 3) worth putting
>> into 2.10 ?
>
>
> I would vote for including it...

The only reason not to is because this bug has been in QEMU for a
while, so it obviously isn't hit very often. In saying that it is a
pretty big issue (32-bit pair stores are completely broken) which
might become an issue during the 2.10 support window and I don't see
many complications from including the series.

I agree with Edgar, probably worth including.

Thanks,
Alistair

>
> Cheers,
> Edgar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask
  2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask Alistair Francis
  2017-08-12 11:36   ` Edgar E. Iglesias
@ 2017-08-12 15:01   ` Richard Henderson
  1 sibling, 0 replies; 14+ messages in thread
From: Richard Henderson @ 2017-08-12 15:01 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel, peter.maydell
  Cc: edgar.iglesias, alistair23, qemu-arm, edgar.iglesias

On 08/11/2017 03:17 PM, Alistair Francis wrote:
> When we perform the atomic_cmpxchg operation we want to perform the
> operation on a pair of 32-bit registers. Previously we were just passing
> the register size in which was set to MO_32. This would result in the
> high register to be ignored. To fix this issue we hardcode the size to
> be 64-bits long when operating on 32-bit 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, but am working with some internal teams to get one.
> Also linux-user is fully untested.
> 
> All tests were with MTTCG enabled.
> 
>  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 245175e2f1..49b4d6918d 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1913,7 +1913,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int 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);
> +                                       MO_64 | MO_ALIGN | s->be_data);
>              tcg_gen_setcond_i64(TCG_COND_NE, tmp, tmp, val);
>              tcg_temp_free_i64(val);
>          } else if (s->be_data == MO_LE) {
> 

Reading the ARM pseudocode again, especially wrt SetExclusiveMonitors, I think
there are other bugs here wrt 32-bit LDXP/STXP.

Since SetExclusiveMonitors is invoked only with address + dsize, one should be
able to write

	ldxp	w0, w1, [x5]
	stxr	w3, x2, [x5]
or
	ldxr	x0, [x5]
	stxp	w3, w1, w2, [x5]

However, the LDXR and LDXP above do not store the cpu_exclusive_* metadata in
the same format.  Fixing this is simply a matter of ignoring cpu_exclusive_high
for 32-bit pair operations and store it all in cpu_exclusive_val, as the 64-bit
single-register operation does.

In addition, 32-bit LDXP must be single-copy atomic, and we're issuing 2 loads,
this is trivially fixed with the rest of the required changes, but perhaps
worth noting.

I'll post a patch shortly.


r~

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-08-12 15:02 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-11 22:17 [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 1/3] target/arm: Update the memops for exclusive load Alistair Francis
2017-08-12 11:38   ` Edgar E. Iglesias
2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions Alistair Francis
2017-08-12 11:39   ` Edgar E. Iglesias
2017-08-11 22:17 ` [Qemu-devel] [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask Alistair Francis
2017-08-12 11:36   ` Edgar E. Iglesias
2017-08-12 15:01   ` Richard Henderson
2017-08-11 23:21 ` [Qemu-devel] [PATCH v1 0/3] Fixup exclusive store logic Alistair Francis
2017-08-11 23:22   ` Alistair Francis
2017-08-11 23:31     ` Portia Stephens
2017-08-12 10:24 ` Peter Maydell
2017-08-12 11:42   ` Edgar E. Iglesias
2017-08-12 13:52     ` Alistair Francis

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).