| Message ID | 20230602142219.1999756-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Fix return value from LDSMIN/LDSMAX 8/16 bit atomics | expand |
On 2/6/23 16:22, Peter Maydell wrote: > The atomic memory operations are supposed to return the old memory > data value in the destination register. This value is not > sign-extended, even if the operation is the signed minimum or > maximum. (In the pseudocode for the instructions the returned data > value is passed to ZeroExtend() to create the value in the register.) > > We got this wrong because we were doing a 32-to-64 zero extend on the > result for 8 and 16 bit data values, rather than the correct amount > of zero extension. > > Fix the bug by using ext8u and ext16u for the MO_8 and MO_16 data > sizes rather than ext32u. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/tcg/translate-a64.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 6/2/23 07:22, Peter Maydell wrote: > The atomic memory operations are supposed to return the old memory > data value in the destination register. This value is not > sign-extended, even if the operation is the signed minimum or > maximum. (In the pseudocode for the instructions the returned data > value is passed to ZeroExtend() to create the value in the register.) > > We got this wrong because we were doing a 32-to-64 zero extend on the > result for 8 and 16 bit data values, rather than the correct amount > of zero extension. > > Fix the bug by using ext8u and ext16u for the MO_8 and MO_16 data > sizes rather than ext32u. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/tcg/translate-a64.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c > index 741a6087399..075553e15f5 100644 > --- a/target/arm/tcg/translate-a64.c > +++ b/target/arm/tcg/translate-a64.c > @@ -3401,8 +3401,22 @@ static void disas_ldst_atomic(DisasContext *s, uint32_t insn, > */ > fn(tcg_rt, clean_addr, tcg_rs, get_mem_index(s), mop); > > - if ((mop & MO_SIGN) && size != MO_64) { > - tcg_gen_ext32u_i64(tcg_rt, tcg_rt); > + if (mop & MO_SIGN) { > + switch (size) { > + case MO_8: > + tcg_gen_ext8u_i64(tcg_rt, tcg_rt); > + break; > + case MO_16: > + tcg_gen_ext16u_i64(tcg_rt, tcg_rt); > + break; > + case MO_32: > + tcg_gen_ext32u_i64(tcg_rt, tcg_rt); > + break; > + case MO_64: > + break; > + default: > + g_assert_not_reached(); > + } This reminds me that we have a function in tcg to handle this switch, but it isn't exposed. I keep meaning to do that... Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c index 741a6087399..075553e15f5 100644 --- a/target/arm/tcg/translate-a64.c +++ b/target/arm/tcg/translate-a64.c @@ -3401,8 +3401,22 @@ static void disas_ldst_atomic(DisasContext *s, uint32_t insn, */ fn(tcg_rt, clean_addr, tcg_rs, get_mem_index(s), mop); - if ((mop & MO_SIGN) && size != MO_64) { - tcg_gen_ext32u_i64(tcg_rt, tcg_rt); + if (mop & MO_SIGN) { + switch (size) { + case MO_8: + tcg_gen_ext8u_i64(tcg_rt, tcg_rt); + break; + case MO_16: + tcg_gen_ext16u_i64(tcg_rt, tcg_rt); + break; + case MO_32: + tcg_gen_ext32u_i64(tcg_rt, tcg_rt); + break; + case MO_64: + break; + default: + g_assert_not_reached(); + } } }
The atomic memory operations are supposed to return the old memory data value in the destination register. This value is not sign-extended, even if the operation is the signed minimum or maximum. (In the pseudocode for the instructions the returned data value is passed to ZeroExtend() to create the value in the register.) We got this wrong because we were doing a 32-to-64 zero extend on the result for 8 and 16 bit data values, rather than the correct amount of zero extension. Fix the bug by using ext8u and ext16u for the MO_8 and MO_16 data sizes rather than ext32u. Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/tcg/translate-a64.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)