* [Qemu-devel] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl
@ 2015-11-16 18:28 Peter Maydell
2015-11-23 16:49 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2015-11-16 18:28 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Desnogues, qemu-arm, Alex Bennée, patches
The checks for the unallocated encodings in the ldst_excl group
(exclusives and load-acquire/store-release) were not correct. This
error meant that in turn we ended up with code attempting to handle
the non-existent case of "non-exclusive load-acquire/store-release
pair". Delete that broken and now unreachable code.
Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The easiest way to validate that we have the unallocated
conditions correct now is to look at C4.4.6 "load/store exclusive"
in the v8 ARM ARM rev A.3h: our three conditions correspond
to the three "unallocated" rows in the decode table.
PS: Laurent originally reported this way back in 2014:
http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg01255.html
target-arm/translate-a64.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index fe485a4..890ace4 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1833,7 +1833,8 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
int size = extract32(insn, 30, 2);
TCGv_i64 tcg_addr;
- if ((!is_excl && !is_lasr) ||
+ if ((!is_excl && !is_pair && !is_lasr) ||
+ (!is_excl && is_pair) ||
(is_pair && size < 2)) {
unallocated_encoding(s);
return;
@@ -1862,15 +1863,6 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
} else {
do_gpr_ld(s, tcg_rt, tcg_addr, size, false, false);
}
- if (is_pair) {
- TCGv_i64 tcg_rt2 = cpu_reg(s, rt);
- tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
- if (is_store) {
- do_gpr_st(s, tcg_rt2, tcg_addr, size);
- } else {
- do_gpr_ld(s, tcg_rt2, tcg_addr, size, false, false);
- }
- }
}
}
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl
2015-11-16 18:28 [Qemu-devel] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl Peter Maydell
@ 2015-11-23 16:49 ` Peter Maydell
2015-11-23 18:42 ` Sergey Fedorov
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2015-11-23 16:49 UTC (permalink / raw)
To: QEMU Developers
Cc: Laurent Desnogues, Edgar E. Iglesias, qemu-arm, Alex Bennée,
Patch Tracking
Ping? I forgot to mark this for-2.5, and given how long the bug's
been hanging around there's not much urgency to fixing it, but
we might as well put the fix into 2.5 if it gets reviewed.
thanks
-- PMM
On 16 November 2015 at 18:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> The checks for the unallocated encodings in the ldst_excl group
> (exclusives and load-acquire/store-release) were not correct. This
> error meant that in turn we ended up with code attempting to handle
> the non-existent case of "non-exclusive load-acquire/store-release
> pair". Delete that broken and now unreachable code.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The easiest way to validate that we have the unallocated
> conditions correct now is to look at C4.4.6 "load/store exclusive"
> in the v8 ARM ARM rev A.h: our three conditions correspond
> to the three "unallocated" rows in the decode table.
>
> PS: Laurent originally reported this way back in 2014:
> http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg01255.html
>
> target-arm/translate-a64.c | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index fe485a4..890ace4 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1833,7 +1833,8 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
> int size = extract32(insn, 30, 2);
> TCGv_i64 tcg_addr;
>
> - if ((!is_excl && !is_lasr) ||
> + if ((!is_excl && !is_pair && !is_lasr) ||
> + (!is_excl && is_pair) ||
> (is_pair && size < 2)) {
> unallocated_encoding(s);
> return;
> @@ -1862,15 +1863,6 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
> } else {
> do_gpr_ld(s, tcg_rt, tcg_addr, size, false, false);
> }
> - if (is_pair) {
> - TCGv_i64 tcg_rt2 = cpu_reg(s, rt);
> - tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
> - if (is_store) {
> - do_gpr_st(s, tcg_rt2, tcg_addr, size);
> - } else {
> - do_gpr_ld(s, tcg_rt2, tcg_addr, size, false, false);
> - }
> - }
> }
> }
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl
2015-11-23 16:49 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
@ 2015-11-23 18:42 ` Sergey Fedorov
2015-11-23 18:54 ` Sergey Fedorov
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Fedorov @ 2015-11-23 18:42 UTC (permalink / raw)
To: Peter Maydell, QEMU Developers
Cc: Laurent Desnogues, qemu-arm, Patch Tracking
On 23.11.2015 19:49, Peter Maydell wrote:
> Ping? I forgot to mark this for-2.5, and given how long the bug's
> been hanging around there's not much urgency to fixing it, but
> we might as well put the fix into 2.5 if it gets reviewed.
>
Hi, Peter. I'm going to review this carefully in a few days :)
For now, I see that the comment for this function should be updated to
match new code.
Best,
Sergey
>
> On 16 November 2015 at 18:28, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The checks for the unallocated encodings in the ldst_excl group
>> (exclusives and load-acquire/store-release) were not correct. This
>> error meant that in turn we ended up with code attempting to handle
>> the non-existent case of "non-exclusive load-acquire/store-release
>> pair". Delete that broken and now unreachable code.
>>
>> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> The easiest way to validate that we have the unallocated
>> conditions correct now is to look at C4.4.6 "load/store exclusive"
>> in the v8 ARM ARM rev A.h: our three conditions correspond
>> to the three "unallocated" rows in the decode table.
>>
>> PS: Laurent originally reported this way back in 2014:
>> http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg01255.html
>>
>> target-arm/translate-a64.c | 12 ++----------
>> 1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> index fe485a4..890ace4 100644
>> --- a/target-arm/translate-a64.c
>> +++ b/target-arm/translate-a64.c
>> @@ -1833,7 +1833,8 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
>> int size = extract32(insn, 30, 2);
>> TCGv_i64 tcg_addr;
>>
>> - if ((!is_excl && !is_lasr) ||
>> + if ((!is_excl && !is_pair && !is_lasr) ||
>> + (!is_excl && is_pair) ||
>> (is_pair && size < 2)) {
>> unallocated_encoding(s);
>> return;
>> @@ -1862,15 +1863,6 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
>> } else {
>> do_gpr_ld(s, tcg_rt, tcg_addr, size, false, false);
>> }
>> - if (is_pair) {
>> - TCGv_i64 tcg_rt2 = cpu_reg(s, rt);
>> - tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
>> - if (is_store) {
>> - do_gpr_st(s, tcg_rt2, tcg_addr, size);
>> - } else {
>> - do_gpr_ld(s, tcg_rt2, tcg_addr, size, false, false);
>> - }
>> - }
>> }
>> }
>>
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl
2015-11-23 18:42 ` Sergey Fedorov
@ 2015-11-23 18:54 ` Sergey Fedorov
2015-11-24 11:03 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Fedorov @ 2015-11-23 18:54 UTC (permalink / raw)
To: Peter Maydell, QEMU Developers
Cc: Laurent Desnogues, qemu-arm, Patch Tracking
On 23.11.2015 21:42, Sergey Fedorov wrote:
> On 23.11.2015 19:49, Peter Maydell wrote:
>> Ping? I forgot to mark this for-2.5, and given how long the bug's
>> been hanging around there's not much urgency to fixing it, but
>> we might as well put the fix into 2.5 if it gets reviewed.
>>
> Hi, Peter. I'm going to review this carefully in a few days :)
> For now, I see that the comment for this function should be updated to
> match new code.
>
However, I've just checked the logic of the patch was correct. Just the
comment needs to be adjusted.
Best regards,
Sergey
>> On 16 November 2015 at 18:28, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> The checks for the unallocated encodings in the ldst_excl group
>>> (exclusives and load-acquire/store-release) were not correct. This
>>> error meant that in turn we ended up with code attempting to handle
>>> the non-existent case of "non-exclusive load-acquire/store-release
>>> pair". Delete that broken and now unreachable code.
>>>
>>> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> The easiest way to validate that we have the unallocated
>>> conditions correct now is to look at C4.4.6 "load/store exclusive"
>>> in the v8 ARM ARM rev A.h: our three conditions correspond
>>> to the three "unallocated" rows in the decode table.
>>>
>>> PS: Laurent originally reported this way back in 2014:
>>> http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg01255.html
>>>
>>> target-arm/translate-a64.c | 12 ++----------
>>> 1 file changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>>> index fe485a4..890ace4 100644
>>> --- a/target-arm/translate-a64.c
>>> +++ b/target-arm/translate-a64.c
>>> @@ -1833,7 +1833,8 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
>>> int size = extract32(insn, 30, 2);
>>> TCGv_i64 tcg_addr;
>>>
>>> - if ((!is_excl && !is_lasr) ||
>>> + if ((!is_excl && !is_pair && !is_lasr) ||
>>> + (!is_excl && is_pair) ||
>>> (is_pair && size < 2)) {
>>> unallocated_encoding(s);
>>> return;
>>> @@ -1862,15 +1863,6 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
>>> } else {
>>> do_gpr_ld(s, tcg_rt, tcg_addr, size, false, false);
>>> }
>>> - if (is_pair) {
>>> - TCGv_i64 tcg_rt2 = cpu_reg(s, rt);
>>> - tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
>>> - if (is_store) {
>>> - do_gpr_st(s, tcg_rt2, tcg_addr, size);
>>> - } else {
>>> - do_gpr_ld(s, tcg_rt2, tcg_addr, size, false, false);
>>> - }
>>> - }
>>> }
>>> }
>>>
>>> --
>>> 1.9.1
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl
2015-11-23 18:54 ` Sergey Fedorov
@ 2015-11-24 11:03 ` Peter Maydell
2015-11-24 12:14 ` Sergey Fedorov
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2015-11-24 11:03 UTC (permalink / raw)
To: Sergey Fedorov
Cc: Laurent Desnogues, qemu-arm, QEMU Developers, Patch Tracking
On 23 November 2015 at 18:54, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 23.11.2015 21:42, Sergey Fedorov wrote:
>> On 23.11.2015 19:49, Peter Maydell wrote:
>>> Ping? I forgot to mark this for-2.5, and given how long the bug's
>>> been hanging around there's not much urgency to fixing it, but
>>> we might as well put the fix into 2.5 if it gets reviewed.
>>>
>> Hi, Peter. I'm going to review this carefully in a few days :)
>> For now, I see that the comment for this function should be updated to
>> match new code.
>>
>
> However, I've just checked the logic of the patch was correct. Just the
> comment needs to be adjusted.
Looking through the rest of the file, in general we don't try
to note in the instruction-format comments all the unallocated
encoding possibilities, so I think the simplest thing will be
just to delete that part of the comment. I don't think duplicating
the information in the code itself really gains us much.
I'll send out a v2.
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl
2015-11-24 11:03 ` Peter Maydell
@ 2015-11-24 12:14 ` Sergey Fedorov
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Fedorov @ 2015-11-24 12:14 UTC (permalink / raw)
To: Peter Maydell
Cc: Laurent Desnogues, qemu-arm, QEMU Developers, Patch Tracking
On 24.11.2015 14:03, Peter Maydell wrote:
> On 23 November 2015 at 18:54, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 23.11.2015 21:42, Sergey Fedorov wrote:
>>> On 23.11.2015 19:49, Peter Maydell wrote:
>>>> Ping? I forgot to mark this for-2.5, and given how long the bug's
>>>> been hanging around there's not much urgency to fixing it, but
>>>> we might as well put the fix into 2.5 if it gets reviewed.
>>>>
>>> Hi, Peter. I'm going to review this carefully in a few days :)
>>> For now, I see that the comment for this function should be updated to
>>> match new code.
>>>
>> However, I've just checked the logic of the patch was correct. Just the
>> comment needs to be adjusted.
> Looking through the rest of the file, in general we don't try
> to note in the instruction-format comments all the unallocated
> encoding possibilities, so I think the simplest thing will be
> just to delete that part of the comment. I don't think duplicating
> the information in the code itself really gains us much.
>
> I'll send out a v2.
>
I would just agree with you. No need to duplicate that low-level
knowledge which is obvious from the code.
Best regards,
Sergey
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-24 12:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-16 18:28 [Qemu-devel] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl Peter Maydell
2015-11-23 16:49 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
2015-11-23 18:42 ` Sergey Fedorov
2015-11-23 18:54 ` Sergey Fedorov
2015-11-24 11:03 ` Peter Maydell
2015-11-24 12:14 ` Sergey Fedorov
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).