[PATCH 0/2] target/arm: two LDAPR/STLR fixes

[PATCH 0/2] target/arm: two LDAPR/STLR fixes

[Peter Maydell]

Patch 1 here fixes https://gitlab.com/qemu-project/qemu/-/issues/2419
which is a bug in the LDAPR/STLR-immediate insns that I introduced
when I did the decodetree conversion: the immediate field should be
signed, not unsigned.

Patch 2 is an unrelated bug that I happened to notice when I was
looking at the code: the LDAPR-register form incorrectly uses the
stricter check_atomic_align() rather than check_ordered_align()
(a bug that's been present since we added the FEAT_LSE2 support).

thanks
-- PMM

Peter Maydell (2):
  target/arm: Fix handling of LDAPR/STLR with negative offset
  target/arm: LDAPR should honour SCTLR_ELx.nAA

 target/arm/tcg/a64.decode      | 2 +-
 target/arm/tcg/translate-a64.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] target/arm: Fix handling of LDAPR/STLR with negative offset

[Peter Maydell]

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

Re: [PATCH 1/2] target/arm: Fix handling of LDAPR/STLR with negative offset

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 1/2] target/arm: Fix handling of LDAPR/STLR with negative offset

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 1/2] target/arm: Fix handling of LDAPR/STLR with negative offset

[Richard Henderson]

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~

Re: [PATCH 1/2] target/arm: Fix handling of LDAPR/STLR with negative offset

[Richard Henderson]

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~

[PATCH 2/2] target/arm: LDAPR should honour SCTLR_ELx.nAA

[Peter Maydell]

In commit c1a1f80518d360b when we added the FEAT_LSE2 relaxations to
the alignment requirements for atomic and ordered loads and stores,
we didn't quite get it right for LDAPR/LDAPRH/LDAPRB with no
immediate offset.  These instructions were handled in the old decoder
as part of disas_ldst_atomic(), but unlike all the other insns that
function decoded (LDADD, LDCLR, etc) these insns are "ordered", not
"atomic", so they should be using check_ordered_align() rather than
check_atomic_align().  Commit c1a1f80518d360b used
check_atomic_align() regardless for everything in
disas_ldst_atomic().  We then carried that incorrect check over in
the decodetree conversion, where LDAPR/LDAPRH/LDAPRB are now handled
by trans_LDAPR().

The effect is that when FEAT_LSE2 is implemented, these instructions
don't honour the SCTLR_ELx.nAA bit and will generate alignment
faults when they should not.

(The LDAPR insns with an immediate offset were in disas_ldst_ldapr_stlr()
and then in trans_LDAPR_i() and trans_STLR_i(), and have always used
the correct check_ordered_align().)

Use check_ordered_align() in trans_LDAPR().

Cc: qemu-stable@nongnu.org
Fixes: c1a1f80518d360b ("target/arm: Relax ordered/atomic alignment checks for LSE2")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/tcg/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
index 6c07aeaf3bd..5ea204d5009 100644
--- a/target/arm/tcg/translate-a64.c
+++ b/target/arm/tcg/translate-a64.c
@@ -3543,7 +3543,7 @@ static bool trans_LDAPR(DisasContext *s, arg_LDAPR *a)
     if (a->rn == 31) {
         gen_check_sp_alignment(s);
     }
-    mop = check_atomic_align(s, a->rn, a->sz);
+    mop = check_ordered_align(s, a->rn, 0, false, a->sz);
     clean_addr = gen_mte_check1(s, cpu_reg_sp(s, a->rn), false,
                                 a->rn != 31, mop);
     /*
-- 
2.34.1

Re: [PATCH 2/2] target/arm: LDAPR should honour SCTLR_ELx.nAA

[Richard Henderson]

On 7/9/24 06:45, Peter Maydell wrote:
> In commit c1a1f80518d360b when we added the FEAT_LSE2 relaxations to
> the alignment requirements for atomic and ordered loads and stores,
> we didn't quite get it right for LDAPR/LDAPRH/LDAPRB with no
> immediate offset.  These instructions were handled in the old decoder
> as part of disas_ldst_atomic(), but unlike all the other insns that
> function decoded (LDADD, LDCLR, etc) these insns are "ordered", not
> "atomic", so they should be using check_ordered_align() rather than
> check_atomic_align().  Commit c1a1f80518d360b used
> check_atomic_align() regardless for everything in
> disas_ldst_atomic().  We then carried that incorrect check over in
> the decodetree conversion, where LDAPR/LDAPRH/LDAPRB are now handled
> by trans_LDAPR().
> 
> The effect is that when FEAT_LSE2 is implemented, these instructions
> don't honour the SCTLR_ELx.nAA bit and will generate alignment
> faults when they should not.
> 
> (The LDAPR insns with an immediate offset were in disas_ldst_ldapr_stlr()
> and then in trans_LDAPR_i() and trans_STLR_i(), and have always used
> the correct check_ordered_align().)
> 
> Use check_ordered_align() in trans_LDAPR().
> 
> Cc:qemu-stable@nongnu.org
> Fixes: c1a1f80518d360b ("target/arm: Relax ordered/atomic alignment checks for LSE2")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/tcg/translate-a64.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~