[PATCH v1 0/3] Fixup exclusive store logic

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 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.

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 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 <[hidden email]>
Reviewed-by: Richard Henderson <[hidden email]>
---

 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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v1 2/3] tcg/tcg-op: Expose the tcg_gen_ext_i* functions

Alistair Francis
In reply to this post by Alistair Francis
Expose the tcg_gen_ext_i32() and tcg_gen_ext_i64() functions.

Signed-off-by: Alistair Francis <[hidden email]>
Reviewed-by: Richard Henderson <[hidden email]>
---

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask

Alistair Francis
In reply to this post by Alistair Francis
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 <[hidden email]>
---

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 0/3] Fixup exclusive store logic

Alistair Francis-2
In reply to this post by Alistair Francis
On Fri, Aug 11, 2017 at 3:17 PM, Alistair Francis
<[hidden email]> 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 0/3] Fixup exclusive store logic

Alistair Francis
On Fri, Aug 11, 2017 at 4:21 PM, Alistair Francis <[hidden email]> wrote:

> On Fri, Aug 11, 2017 at 3:17 PM, Alistair Francis
> <[hidden email]> 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
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 0/3] Fixup exclusive store logic

Portia Stephens
> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On Behalf Of
> Alistair Francis
> Sent: Friday, August 11, 2017 4:23 PM
> To: Alistair Francis <[hidden email]>; Portia Stephens
> <[hidden email]>
> Cc: [hidden email] Developers <[hidden email]>;
> Peter Maydell <[hidden email]>; Edgar Iglesias
> <[hidden email]>; Edgar Iglesias <[hidden email]>; qemu-arm
> <[hidden email]>
> Subject: Re: [PATCH v1 0/3] Fixup exclusive store logic
>
> On Fri, Aug 11, 2017 at 4:21 PM, Alistair Francis <[hidden email]> wrote:
> > On Fri, Aug 11, 2017 at 3:17 PM, Alistair Francis
> > <[hidden email]> 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 <[hidden email]>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 0/3] Fixup exclusive store logic

Peter Maydell-5
In reply to this post by Alistair Francis
On 11 August 2017 at 23:17, Alistair Francis
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask

Edgar E. Iglesias-2
In reply to this post by Alistair Francis
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 <[hidden email]>



>
> Signed-off-by: Alistair Francis <[hidden email]>
> ---
>
> 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 1/3] target/arm: Update the memops for exclusive load

Edgar E. Iglesias-2
In reply to this post by Alistair Francis
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 <[hidden email]>
> Reviewed-by: Richard Henderson <[hidden email]>

Reviewed-by: Edgar E. Iglesias <[hidden email]>


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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Edgar E. Iglesias-2
In reply to this post by Alistair Francis
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 <[hidden email]>
> Reviewed-by: Richard Henderson <[hidden email]>

Reviewed-by: Edgar E. Iglesias <[hidden email]>


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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 0/3] Fixup exclusive store logic

Edgar E. Iglesias-2
In reply to this post by Peter Maydell-5
On Sat, Aug 12, 2017 at 11:24:30AM +0100, Peter Maydell wrote:

> On 11 August 2017 at 23:17, Alistair Francis
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 0/3] Fixup exclusive store logic

Alistair Francis-2
On Sat, Aug 12, 2017 at 4:42 AM, Edgar E. Iglesias
<[hidden email]> wrote:

> On Sat, Aug 12, 2017 at 11:24:30AM +0100, Peter Maydell wrote:
>> On 11 August 2017 at 23:17, Alistair Francis
>> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v1 3/3] target/arm: Correct exclusive store cmpxchg memop mask

Richard Henderson-3
In reply to this post by Alistair Francis
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 <[hidden email]>
> ---
>
> 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~

Loading...