Re: [Qemu-devel] [PATCH 05/11] target-arm: Use correct mmu_idx for unprivileged loads and stores

[Greg Bellows <greg.bellows@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 4263 bytes --]

On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> The MMU index to use for unprivileged loads and stores is more
> complicated than we currently implement:
>  * for A64, it should be "if at EL1, access as if EL0; otherwise
>    access at current EL"
>  * for A32/T32, it should be "if EL2, UNPREDICTABLE; otherwise
>    access as if at EL0".
>
>
​The wording between the specs appears to be almost identical, curious why
the handling is different?​



> In both cases, if we want to make the access for Secure EL0
> this is not the same mmu_idx as for Non-Secure EL0.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/translate-a64.c | 19 ++++++++++++++++++-
>  target-arm/translate.c     | 26 ++++++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 96f14ff..acf4b16 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -123,6 +123,23 @@ void a64_translate_init(void)
>  #endif
>  }
>
> +static inline ARMMMUIdx get_a64_user_mem_index(DisasContext *s)
> +{
> +    /* Return the mmu_idx to use for A64 "unprivileged load/store" insns:
> +     *  if EL1, access as if EL0; otherwise access at current EL
> +     */
> +    switch (s->mmu_idx) {
> +    case ARMMMUIdx_S12NSE1:
> +        return ARMMMUIdx_S12NSE0;
> +    case ARMMMUIdx_S1SE1:
> +        return ARMMMUIdx_S1SE0;
> +    case ARMMMUIdx_S2NS:
> +        g_assert_not_reached();
> +    default:
> +        return s->mmu_idx;
> +    }
> +}
> +
>  void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
>                              fprintf_function cpu_fprintf, int flags)
>  {
> @@ -2107,7 +2124,7 @@ static void disas_ldst_reg_imm9(DisasContext *s,
> uint32_t insn)
>          }
>      } else {
>          TCGv_i64 tcg_rt = cpu_reg(s, rt);
> -        int memidx = is_unpriv ? MMU_USER_IDX : get_mem_index(s);
> +        int memidx = is_unpriv ? get_a64_user_mem_index(s) :
> get_mem_index(s);
>
>          if (is_store) {
>              do_gpr_st_memidx(s, tcg_rt, tcg_addr, size, memidx);
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 7163649..715f65d 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -113,6 +113,28 @@ void arm_translate_init(void)
>      a64_translate_init();
>  }
>
> +static inline ARMMMUIdx get_a32_user_mem_index(DisasContext *s)
> +{
> +    /* Return the mmu_idx to use for A32/T32 "unprivileged load/store"
> +     * insns:
> +     *  if PL2, UNPREDICTABLE (we choose to implement as if PL0)
> +     *  otherwise, access as if at PL0.
> +     */
> +    switch (s->mmu_idx) {
> +    case ARMMMUIdx_S1E2:        /* this one is UNPREDICTABLE */
> +    case ARMMMUIdx_S12NSE0:
> +    case ARMMMUIdx_S12NSE1:
> +        return ARMMMUIdx_S12NSE0;
> +    case ARMMMUIdx_S1E3:
> +    case ARMMMUIdx_S1SE0:
> +    case ARMMMUIdx_S1SE1:
> +        return ARMMMUIdx_S1SE0;
> +    case ARMMMUIdx_S2NS:
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  static inline TCGv_i32 load_cpu_offset(int offset)
>  {
>      TCGv_i32 tmp = tcg_temp_new_i32();
> @@ -8793,7 +8815,7 @@ static void disas_arm_insn(DisasContext *s, unsigned
> int insn)
>              tmp2 = load_reg(s, rn);
>              if ((insn & 0x01200000) == 0x00200000) {
>                  /* ldrt/strt */
> -                i = MMU_USER_IDX;
> +                i = get_a32_user_mem_index(s);
>              } else {
>                  i = get_mem_index(s);
>              }
> @@ -10173,7 +10195,7 @@ static int disas_thumb2_insn(CPUARMState *env,
> DisasContext *s, uint16_t insn_hw
>                      break;
>                  case 0xe: /* User privilege.  */
>                      tcg_gen_addi_i32(addr, addr, imm);
> -                    memidx = MMU_USER_IDX;
> +                    memidx = get_a32_user_mem_index(s);
>                      break;
>                  case 0x9: /* Post-decrement.  */
>                      imm = -imm;
> --
> 1.9.1
>
> ​Otherwise,

Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

[-- Attachment #2: Type: text/html, Size: 5839 bytes --]