Re: [PATCH 1/3] target/arm: Correct LDRD atomicity and fault behaviour

[Richard Henderson <richard.henderson@linaro.org>]

On 2/27/25 06:27, Peter Maydell wrote:
> Our LDRD implementation is wrong in two respects:
> 
>   * if the address is 4-aligned and the load crosses a page boundary
>     and the second load faults and the first load was to the
>     base register (as in cases like "ldrd r2, r3, [r2]", then we
>     must not update the base register before taking the fault
>   * if the address is 8-aligned the access must be a 64-bit
>     single-copy atomic access, not two 32-bit accesses
> 
> Rewrite the handling of the loads in LDRD to use a single
> tcg_gen_qemu_ld_i64() and split the result into the destination
> registers. This allows us to get the atomicity requirements
> right, and also implicitly means that we won't update the
> base register too early for the page-crossing case.
> 
> Note that because we no longer increment 'addr' by 4 in the course of
> performing the LDRD we must change the adjustment value we pass to
> op_addr_ri_post() and op_addr_rr_post(): it no longer needs to
> subtract 4 to get the correct value to use if doing base register
> writeback.
> 
> STRD has the same problem with not getting the atomicity right;
> we will deal with that in the following commit.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Stu Grossman <stu.grossman@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/tcg/translate.c | 64 ++++++++++++++++++++++++--------------
>   1 file changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
> index d8225b77c8c..e10a1240c17 100644
> --- a/target/arm/tcg/translate.c
> +++ b/target/arm/tcg/translate.c
> @@ -5003,10 +5003,43 @@ static bool op_store_rr(DisasContext *s, arg_ldst_rr *a,
>       return true;
>   }
>   
> +static void do_ldrd_load(DisasContext *s, TCGv_i32 addr, int rt, int rt2)
> +{
> +    /*
> +     * LDRD is required to be an atomic 64-bit access if the
> +     * address is 8-aligned, two atomic 32-bit accesses if
> +     * it's only 4-aligned, and to give an alignemnt fault
> +     * if it's not 4-aligned.
> +     * Rt is always the word from the lower address, and Rt2 the
> +     * data from the higher address, regardless of endianness.
> +     * So (like gen_load_exclusive) we avoid gen_aa32_ld_i64()
> +     * so we don't get its SCTLR_B check, and instead do a 64-bit access
> +     * using MO_BE if appropriate and then split the two halves.
> +     *
> +     * This also gives us the correct behaviour of not updating
> +     * rt if the load of rt2 faults; this is required for cases
> +     * like "ldrd r2, r3, [r2]" where rt is also the base register.
> +     */
> +    int mem_idx = get_mem_index(s);
> +    MemOp opc = MO_64 | MO_ALIGN_4 | MO_ATOM_SUBALIGN | s->be_data;

The 64-bit atomicity begins with armv7 + LPAE, and not present for any m-profile.
Worth checking ARM_FEATURE_LPAE, or at least adding to the comment?

Getting 2 x 4-byte atomicity, but not require 8-byte atomicity, would use 
MO_ATOM_IFALIGN_PAIR.

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


r~