Re: [PATCH v2 05/24] target/arm: Fix SCTLR_B test for TCGv_i64 load/store

[Peter Maydell]

On Tue, 8 Dec 2020 at 18:01, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Just because operating on a TCGv_i64 temporary does not
> mean that we're performing a 64-bit operation.  Restrict
> the frobbing to actual 64-bit operations.

If I understand correctly, this patch isn't actually a behaviour
change because at this point the only users of gen_aa32_ld_i64()
and gen_aa32_st_i64() are in fact performing 64-bit operations
so the (opc & MO_SIZE) == MO_64 test is always true. (Presumably
subsequent patches are going to add uses of these functions that
want to load smaller sizes?) If that's right, worth mentioning
explicitly in the commit message, I think.

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index f35d376341..ef9192cf6b 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -949,7 +949,7 @@ static void gen_aa32_ld_i64(DisasContext *s, TCGv_i64 
> val, TCGv_i32 a32,
>      tcg_gen_qemu_ld_i64(val, addr, index, opc);
>
>      /* Not needed for user-mode BE32, where we use MO_BE instead.  */
> -    if (!IS_USER_ONLY && s->sctlr_b) {
> +    if (!IS_USER_ONLY && s->sctlr_b && (opc & MO_SIZE) == MO_64) {
>          tcg_gen_rotri_i64(val, val, 32);
>      }
>
> @@ -968,7 +968,7 @@ static void gen_aa32_st_i64(DisasContext *s, TCGv_i64 
> val, TCGv_i32 a32,
>      TCGv addr = gen_aa32_addr(s, a32, opc);
>
>      /* Not needed for user-mode BE32, where we use MO_BE instead.  */
> -    if (!IS_USER_ONLY && s->sctlr_b) {
> +    if (!IS_USER_ONLY && s->sctlr_b && (opc & MO_SIZE) == MO_64) {
>          TCGv_i64 tmp = tcg_temp_new_i64();
>          tcg_gen_rotri_i64(tmp, val, 32);
>          tcg_gen_qemu_st_i64(tmp, addr, index, opc);

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM