* [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).