| Message ID | 20240709134504.3500007-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: two LDAPR/STLR fixes | expand |
On 9/7/24 15:45, Peter Maydell wrote: > When we converted the LDAPR/STLR instructions to decodetree we > accidentally introduced a regression where the offset is negative. > The 9-bit immediate field is signed, and the old hand decoder > correctly used sextract32() to get it out of the insn word, > but the ldapr_stlr_i pattern in the decode file used "imm:9" > instead of "imm:s9", so it treated the field as unsigned. > > Fix the pattern to treat the field as a signed immediate. > > Cc: qemu-stable@nongnu.org > Fixes: 2521b6073b7 ("target/arm: Convert LDAPR/STLR (imm) to decodetree") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2419 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/tcg/a64.decode | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 9/7/24 15:45, Peter Maydell wrote: > When we converted the LDAPR/STLR instructions to decodetree we > accidentally introduced a regression where the offset is negative. > The 9-bit immediate field is signed, and the old hand decoder > correctly used sextract32() to get it out of the insn word, > but the ldapr_stlr_i pattern in the decode file used "imm:9" > instead of "imm:s9", so it treated the field as unsigned. > > Fix the pattern to treat the field as a signed immediate. > > Cc: qemu-stable@nongnu.org > Fixes: 2521b6073b7 ("target/arm: Convert LDAPR/STLR (imm) to decodetree") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2419 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/tcg/a64.decode | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode > index 223eac3cac2..f873e8bc8b9 100644 > --- a/target/arm/tcg/a64.decode > +++ b/target/arm/tcg/a64.decode > @@ -520,7 +520,7 @@ LDAPR sz:2 111 0 00 1 0 1 11111 1100 00 rn:5 rt:5 > LDRA 11 111 0 00 m:1 . 1 ......... w:1 1 rn:5 rt:5 imm=%ldra_imm > > &ldapr_stlr_i rn rt imm sz sign ext > -@ldapr_stlr_i .. ...... .. . imm:9 .. rn:5 rt:5 &ldapr_stlr_i > +@ldapr_stlr_i .. ...... .. . imm:s9 .. rn:5 rt:5 &ldapr_stlr_i BTW I noted some instr formats use 'uimm*' for unsigned immediate. Maybe we could recommend/enforce that, having 'imm*' always signed.
On 7/9/24 07:11, Philippe Mathieu-Daudé wrote: > BTW I noted some instr formats use 'uimm*' for unsigned immediate. > Maybe we could recommend/enforce that, having 'imm*' always signed. With Arm, especially with load/store, some insns have unsigned fields and some have signed fields, but for implementation purposes we need to use the same argument set. Therefore we cannot mix names like this. This naming convention works better with more regular encodings such as riscv or loongarch. r~
On 7/9/24 06:45, Peter Maydell wrote: > When we converted the LDAPR/STLR instructions to decodetree we > accidentally introduced a regression where the offset is negative. > The 9-bit immediate field is signed, and the old hand decoder > correctly used sextract32() to get it out of the insn word, > but the ldapr_stlr_i pattern in the decode file used"imm:9" > instead of"imm:s9", so it treated the field as unsigned. > > Fix the pattern to treat the field as a signed immediate. > > Cc:qemu-stable@nongnu.org > Fixes: 2521b6073b7 ("target/arm: Convert LDAPR/STLR (imm) to decodetree") > Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2419 > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > target/arm/tcg/a64.decode | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode index 223eac3cac2..f873e8bc8b9 100644 --- a/target/arm/tcg/a64.decode +++ b/target/arm/tcg/a64.decode @@ -520,7 +520,7 @@ LDAPR sz:2 111 0 00 1 0 1 11111 1100 00 rn:5 rt:5 LDRA 11 111 0 00 m:1 . 1 ......... w:1 1 rn:5 rt:5 imm=%ldra_imm &ldapr_stlr_i rn rt imm sz sign ext -@ldapr_stlr_i .. ...... .. . imm:9 .. rn:5 rt:5 &ldapr_stlr_i +@ldapr_stlr_i .. ...... .. . imm:s9 .. rn:5 rt:5 &ldapr_stlr_i STLR_i sz:2 011001 00 0 ......... 00 ..... ..... @ldapr_stlr_i sign=0 ext=0 LDAPR_i sz:2 011001 01 0 ......... 00 ..... ..... @ldapr_stlr_i sign=0 ext=0 LDAPR_i 00 011001 10 0 ......... 00 ..... ..... @ldapr_stlr_i sign=1 ext=0 sz=0
When we converted the LDAPR/STLR instructions to decodetree we accidentally introduced a regression where the offset is negative. The 9-bit immediate field is signed, and the old hand decoder correctly used sextract32() to get it out of the insn word, but the ldapr_stlr_i pattern in the decode file used "imm:9" instead of "imm:s9", so it treated the field as unsigned. Fix the pattern to treat the field as a signed immediate. Cc: qemu-stable@nongnu.org Fixes: 2521b6073b7 ("target/arm: Convert LDAPR/STLR (imm) to decodetree") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2419 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/tcg/a64.decode | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)