Re: [Qemu-devel] [PATCH target-arm v1 6/9] target-arm: Implement PMSAv7 MPU

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <address@hidden> wrote:
> Unified MPU only. Uses ARM architecture major revision to switch
> between PMSAv5 and v7 when ARM_FEATURE_MPU is set. PMSA v6 remains
> unsupported and is asserted against.
>
> The return code from get_phys_addr has to patched to handle the case
> of a 0 FSR with error. Use -1 to indicate this condition.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>  target-arm/cpu.h    |   1 +
>  target-arm/helper.c | 153 
> ++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 144 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 9cb2e49..73e2619 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -559,6 +559,7 @@ void pmccntr_sync(CPUARMState *env);
>  #define SCTLR_DT      (1U << 16) /* up to ??, RAO in v6 and v7 */
>  #define SCTLR_nTWI    (1U << 16) /* v8 onward */
>  #define SCTLR_HA      (1U << 17)
> +#define SCTLR_BR      (1U << 17) /* PMSA only */
>  #define SCTLR_IT      (1U << 18) /* up to ??, RAO in v6 and v7 */
>  #define SCTLR_nTWE    (1U << 18) /* v8 onward */
>  #define SCTLR_WXN     (1U << 19)
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 63859a4..09210d3 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3323,13 +3323,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_one_arm_cp_reg(cpu, &rvbar);
>      }
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
> -        /* These are the MPU registers prior to PMSAv6. Any new
> -         * PMSA core later than the ARM946 will require that we
> -         * implement the PMSAv6 or PMSAv7 registers, which are
> -         * completely different.
> -         */
> -        assert(!arm_feature(env, ARM_FEATURE_V6));
> -        define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
> +        if (arm_feature(env, ARM_FEATURE_V6)) {
> +            assert(arm_feature(env, ARM_FEATURE_V7));

Could use a comment /* PMSAv6 is not implemented */

> +            define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);

It's a bit unfortunate that we've managed to end up with
a cpreg array for "present in both VMSA and PMSA" but the
code path doesn't then allow for calling the define_ function
just once. Oh well.

> +            define_arm_cp_regs(cpu, pmsav7_cp_reginfo);
> +        } else {
> +            define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
> +        }
>      } else {
>          define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
>          define_arm_cp_regs(cpu, vmsa_cp_reginfo);
> @@ -5720,6 +5720,130 @@ do_fault:
>      return (1 << 9) | (fault_type << 2) | level;
>  }
>
> +static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
> +                                                ARMMMUIdx mmu_idx,
> +                                                int32_t address, int *prot)
> +{
> +    *prot = PAGE_READ | PAGE_WRITE;
> +    switch (address) {
> +    case 0xF0000000 ... 0xFFFFFFFF:
> +        if (regime_sctlr(env, mmu_idx) & SCTLR_V) { /* hivecs execing is ok 
> */
> +            *prot |= PAGE_EXEC;
> +        }
> +        break;
> +    case 0x00000000 ... 0x7FFFFFFF:
> +        *prot |= PAGE_EXEC;
> +        break;
> +    }
> +
> +}
> +
> +static int get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
> +                                int access_type, ARMMMUIdx mmu_idx,
> +                                hwaddr *phys_ptr, int *prot)
> +{
> +    int n;
> +    bool is_user = regime_is_user(env, mmu_idx);
> +
> +    *phys_ptr = address;
> +    *prot = 0;
> +
> +    if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
> +        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);

When will this be reached? get_phys_addr() has already caught the
'MPU disabled' case.

> +    } else { /* MPU enabled */
> +        for (n = 15; n >= 0; n--) { /* region search */
> +            uint32_t base = env->cp15.c6_drbar[n];
> +            uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1;
> +            int snd;
> +
> +            if (!(env->cp15.c6_drsr[n] & 0x1)) {
> +                continue;
> +            }
> +            if (rsize < 2) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "DRSR.Rsize field can not be 
> 0");

This case is UNPREDICTABLE so I would continue here (ie treat the
region as disabled) rather than doing something potentially odd
with an out of range value later.

> +            }

Our effective minimum region size is 1K because that's the
target pagesize setting. Should we enforce that (minimum region
size is impdef so it would be architecturally ok) or will that
break otherwise ok guests?

Base address not aligned to the region size is also UNPREDICTABLE,
incidentally.

> +
> +            if (address < base || address > base - 1 + (1ull << rsize)) {
> +                continue;
> +            }
> +
> +            if (rsize < 8) { /* no subregions for regions < 256 bytes */
> +                break;
> +            }
> +
> +            rsize -= 3; /* sub region size (power of 2) */
> +            snd = (address - base) >> rsize & 0x7;
> +            if (env->cp15.c6_drsr[n] & 1 << (snd + 8)) {

I think I would find these easier to read with more parens
so I didn't have to look up & vs << precedence.

> +                continue;
> +            }
> +            break;
> +        }
> +
> +        if (n == -1) { /* no hits */
> +            if (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> +                /* background fault */
> +                return -1;
> +            } else {
> +                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +            }
> +        } else { /* a MPU hit! */
> +            uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3);
> +
> +            if (is_user) { /* User mode AP bit decoding */
> +                switch (ap) {
> +                case 0:
> +                case 1:
> +                case 5:
> +                    break; /* no access */
> +                case 3:
> +                    *prot |= PAGE_WRITE;
> +                    /* fall through */
> +                case 2:
> +                case 6:
> +                    *prot |= PAGE_READ | PAGE_EXEC;
> +                    break;
> +                default:
> +                    qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in 
> "
> +                                  "DRACR");

Putting the line break after the ',' would let you avoid
splitting the string.

> +                }
> +            } else { /* Priv. mode AP bits decoding */
> +                switch (ap) {
> +                case 0:
> +                    break; /* no access */
> +                case 1:
> +                case 2:
> +                case 3:
> +                    *prot |= PAGE_WRITE;
> +                    /* fall through */
> +                case 5:
> +                case 6:
> +                    *prot |= PAGE_READ | PAGE_EXEC;
> +                    break;
> +                default:
> +                    qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in 
> "
> +                                  "DRACR");
> +                }
> +            }
> +
> +            /* execute never */
> +            *prot &= env->cp15.c6_dracr[n] & 1 << 12 ? ~PAGE_EXEC : ~0;

Break this down into something more readable, please.

> +        }
> +    }
> +
> +    switch (access_type) {
> +    case 0:
> +        return *prot & PAGE_READ ? 0 : 0x00D;
> +    case 1:
> +        return *prot & PAGE_WRITE ? 0 : 0x00D;
> +    case 2:
> +        return *prot & PAGE_EXEC ? 0 : 0x00D;

This is
   if (!(*prot & (1 << access_type))) {
       return 0xD; /* Permission fault */
   } else {
       return 0;
   }

isn't it?

> +    default:
> +        abort(); /* should be unreachable */
> +        return 0;

The usual way to write this is g_assert_not_reached(), and the return
isn't needed.

> +    }
> +
> +}
> +
>  static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>                                  int access_type, ARMMMUIdx mmu_idx,
>                                  hwaddr *phys_ptr, int *prot)
> @@ -5795,13 +5919,14 @@ static int get_phys_addr_pmsav5(CPUARMState *env, 
> uint32_t address,
>   * MPU state on MPU based systems.
>   *
>   * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs,
> - * prot and page_size may not be filled in, and the return value provides
> + * prot and page_size may or may-not be filled in, and the return value 
> provides

No hyphen.

>   * information on why the translation aborted, in the format of a
>   * DFSR/IFSR fault register, with the following caveats:
>   *  * we honour the short vs long DFSR format differences.
>   *  * the WnR bit is never set (the caller must do this).
>   *  * for MPU based systems we don't bother to return a full FSR format
>   *    value.

Is this bullet point still true? If it is, is that a bug
we now need to fix?

> + *  * -1 return value indicates a 0 FSR.
>   *
>   * @env: CPUARMState
>   * @address: virtual address to get physical address for
> @@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, 
> target_ulong address,
>
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>          *page_size = TARGET_PAGE_SIZE;
> -        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> -                                    phys_ptr, prot);
> +        if (arm_feature(env, ARM_FEATURE_V6)) {
> +            assert(arm_feature(env, ARM_FEATURE_V7));
> +            return get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
> +                                        phys_ptr, prot);

v7M will take this code path now (which is the right thing, it is
closer to v7R than to the 946); did you cross check against
the M profile spec to see if any of this is R-profile specific?

(We don't actually implement the M profile MPU right now, but
it would be nice to avoid leaving beartraps for whoever does.)

> +        } else {
> +            return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> +                                        phys_ptr, prot);
> +        }
>      }
>
>      if (regime_using_lpae_format(env, mmu_idx)) {
> @@ -5897,6 +6028,8 @@ int arm_tlb_fill(CPUState *cs, vaddr address,
>          tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
>                                  prot, mmu_idx, page_size);
>          return 0;
> +    } else if (ret == -1) {
> +        ret = 0;
>      }
>
>      return ret;

This isn't going to work, because tlb_fill() which calls this
function making the same "0 means OK, non-0 means FSR value"
assumption.

I think the best thing to do here is to switch
get_phys_addr() and friends to a bool return type for
success/failure and pass in a uint32_t* for FSR value.

thanks
-- PMM