| Message ID | 1447698531-4751-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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 >
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
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); - } - } } }
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(-) -- 1.9.1