Re: [Qemu-devel] [PATCH 08/13] target/arm: Add v8M stack checks for LDRD/STRD (imm)

[Philippe Mathieu-Daudé]

On 02/10/2018 18:35, Peter Maydell wrote:
> Add the v8M stack checks for:
>  * LDRD (immediate)
>  * STRD (immediate)
> 
> Loads and stores are more complicated than ADD/SUB/MOV, because we
> must ensure that memory accesses below the stack limit are not
> performed, so we can't simply do the check when we actually update
> SP.
> 
> For these instructions, if the stack limit check triggers
> we must not:
>  * perform any memory access below the SP limit
>  * update PC, SP or the load/store base register
> but it is IMPDEF whether we:
>  * perform any accesses above or equal to the SP limit
>  * update destination registers for loads
> 
> For QEMU we choose to always check the limit before doing any other
> part of the load or store, so we won't update any registers or
> perform any memory accesses.
> 
> It is UNKNOWN whether the limit check triggers for a load or store
> where the initial SP value is below the limit and one of the stores
> would be below the limit, but the writeback moves SP to above the
> limit.  For QEMU we choose to trigger the check in this situation.
> 
> Note that limit checks happen only for loads and stores which update
> SP via writeback; they do not happen for loads and stores which
> simply use SP as a base register.
> 
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> ---
>  target/arm/translate.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index fcb33b8a503..c16d6075d94 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10278,6 +10278,8 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>                   * 0b1111_1001_x11x_xxxx_xxxx_xxxx_xxxx_xxxx
>                   *  - load/store dual (pre-indexed)
>                   */
> +                bool wback = extract32(insn, 21, 1);
> +
>                  if (rn == 15) {
>                      if (insn & (1 << 21)) {
>                          /* UNPREDICTABLE */
> @@ -10289,8 +10291,29 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>                      addr = load_reg(s, rn);
>                  }
>                  offset = (insn & 0xff) * 4;
> -                if ((insn & (1 << 23)) == 0)
> +                if ((insn & (1 << 23)) == 0) {
>                      offset = -offset;
> +                }
> +
> +                if (s->v8m_stackcheck && rn == 13 && wback) {
> +                    /*
> +                     * Here 'addr' is the current SP; if offset is +ve we're
> +                     * moving SP up, else down. It is UNKNOWN whether the 
> limit
> +                     * check triggers when SP starts below the limit and ends
> +                     * up above it; check whichever of the current and final
> +                     * SP is lower, so QEMU will trigger in that situation.
> +                     */
> +                    if ((int32_t)offset < 0) {
> +                        TCGv_i32 newsp = tcg_temp_new_i32();
> +
> +                        tcg_gen_addi_i32(newsp, addr, offset);
> +                        gen_helper_v8m_stackcheck(cpu_env, newsp);
> +                        tcg_temp_free_i32(newsp);
> +                    } else {
> +                        gen_helper_v8m_stackcheck(cpu_env, addr);
> +                    }
> +                }
> +
>                  if (insn & (1 << 24)) {
>                      tcg_gen_addi_i32(addr, addr, offset);
>                      offset = 0;
> @@ -10314,7 +10337,7 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>                      gen_aa32_st32(s, tmp, addr, get_mem_index(s));
>                      tcg_temp_free_i32(tmp);
>                  }
> -                if (insn & (1 << 21)) {
> +                if (wback) {
>                      /* Base writeback.  */
>                      tcg_gen_addi_i32(addr, addr, offset - 4);
>                      store_reg(s, rn, addr);
>