Re: [Qemu-devel] [PATCH target-arm v2 10/13] target-arm: Implement PMSAv7 MPU

[Peter Crosthwaite]

On Fri, Jun 12, 2015 at 12:10 PM, 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.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> changed since v1 (PMM review):
> Add comment about PMSAv6 non-support
> Fix case where MPU is completely disabled
> Ignore regions with 0 size.
> GUEST_ERROR invalid base address alignments
> UNIMP regions that are smaller than TARGET_PAGE_SIZE
> use extract32 to get SR disable bits
> Fixed up DRACR AP bit error message.
> Correct bullet point about MPU FSR format
> Rebased against new FSR return system
> removed *prot switch-case
>
>  target-arm/cpu.h    |   1 +
>  target-arm/helper.c | 173 
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 173 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 43c1f85..bfba377 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -561,6 +561,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 588dbc9..6436c93 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5827,6 +5827,166 @@ do_fault:
>      return true;
>  }
>
> +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 bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
> +                                 int access_type, ARMMMUIdx mmu_idx,
> +                                 hwaddr *phys_ptr, int *prot, uint32_t *fsr)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    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);
> +    } else { /* MPU enabled */
> +        for (n = cpu->pmsav7_dregion; n >= 0; n--) { /* region search */

-1 on initialiser. I guess I have been getting lucky with 0s in the
right place on my test guest.

> +            uint32_t base = env->pmsav7.drbar[n];
> +            uint32_t rsize = extract32(env->pmsav7.drsr[n], 1, 5);
> +            uint32_t rmask;
> +            bool srdis = false;
> +
> +            if (!(env->pmsav7.drsr[n] & 0x1)) {
> +                continue;
> +            }
> +
> +            if (!rsize) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "DRSR.Rsize field can not be 
> 0");
> +                continue;
> +            }
> +            rsize++;
> +            rmask = (1ull << rsize) - 1;
> +
> +            if (base & rmask) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "DRBAR %" PRIx32 " misaligned 
> "
> +                              "to DRSR region size, mask = %" PRIx32,
> +                              base, rmask);
> +                continue;
> +            }
> +
> +            if (address < base || address > base + rmask) {
> +                continue;
> +            }
> +
> +            /* Region matched */
> +
> +            if (rsize >= 8) { /* no subregions for regions < 256 bytes */
> +                int i, snd;
> +                uint32_t srdis_mask;
> +
> +                rsize -= 3; /* sub region size (power of 2) */
> +                snd = ((address - base) >> rsize) & 0x7;
> +                srdis = extract32(env->pmsav7.drsr[n], snd + 8, 1);
> +
> +                srdis_mask = srdis ? 0x3 : 0x0;
> +                for (i = 2; i <= 8 && rsize < TARGET_PAGE_BITS; i *= 2) {
> +                    /* This will check in groups of 2, 4 and then 8, whether
> +                     * the subregion bits are consistent. rsize is 
> incremented
> +                     * back up to give the region size, considering 
> consistent
> +                     * adjacent subregions as one region. Stop testing if 
> rsize
> +                     * is already big enough for an entire QEMU page.
> +                     */
> +                    int snd_rounded = snd & ~(i - 1);
> +                    uint32_t srdis_multi = extract32(env->pmsav7.drsr[n],
> +                                                     snd_rounded + 8, i);
> +                    if (srdis_mask ^ srdis_multi) {
> +                        break;
> +                    }
> +                    srdis_mask = (srdis_mask << i) | srdis_mask;
> +                    rsize++;
> +                }
> +            }
> +            if (rsize < TARGET_PAGE_BITS) {
> +                qemu_log_mask(LOG_UNIMP, "No support for MPU (sub)region"
> +                              "alignment of %" PRIu32 " bits. Minimum is 
> %d\n",
> +                              rsize, TARGET_PAGE_BITS);
> +                continue;
> +            }
> +            if (srdis) {
> +                continue;
> +            }
> +            break;
> +        }
> +
> +        if (n == -1) { /* no hits */
> +            if (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> +                /* background fault */
> +                *fsr = 0;
> +                return true;
> +            } else {

This should be the behavior if dregions == 0 to handle a PMSA with no
MPU. I have patched the if above to fallthrough to the else in this
case.

else not actually needed due to short return in if. Removed.

Regards,
Peter

> +                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +            }
> +        } else { /* a MPU hit! */
> +            uint32_t ap = extract32(env->pmsav7.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 %"
> +                                  PRIx32 "\n", ap);
> +                }
> +            } 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 %"
> +                                  PRIx32 "\n", ap);
> +                }
> +            }
> +
> +            /* execute never */
> +            if (env->pmsav7.dracr[n] & (1 << 12)) {
> +                *prot &= ~PAGE_EXEC;
> +            }
> +        }
> +    }
> +
> +    *fsr = 0x00d; /* Permission fault */
> +    return !(*prot & (1 << access_type));
> +}
> +
>  static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>                                   int access_type, ARMMMUIdx mmu_idx,
>                                   hwaddr *phys_ptr, int *prot, uint32_t *fsr)
> @@ -5912,7 +6072,7 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, 
> uint32_t address,
>   * 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
> + *  * for PSMAv5 based systems we don't bother to return a full FSR format
>   *    value.
>   *
>   * @env: CPUARMState
> @@ -5960,6 +6120,16 @@ static inline bool get_phys_addr(CPUARMState *env, 
> target_ulong address,
>          }
>      }
>
> +    /* pmsav7 has special handling for when MPU is disabled so call it before
> +     * the common MMU/MPU disabled check below.
> +     */
> +    if (arm_feature(env, ARM_FEATURE_MPU) &&
> +        arm_feature(env, ARM_FEATURE_V7)) {
> +        *page_size = TARGET_PAGE_SIZE;
> +        return get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
> +                                    phys_ptr, prot, fsr);
> +    }
> +
>      if (regime_translation_disabled(env, mmu_idx)) {
>          /* MMU/MPU disabled.  */
>          *phys_ptr = address;
> @@ -5969,6 +6139,7 @@ static inline bool get_phys_addr(CPUARMState *env, 
> target_ulong address,
>      }
>
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
> +        /* Pre-v7 MPU */
>          *page_size = TARGET_PAGE_SIZE;
>          return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
>                                      phys_ptr, prot, fsr);
> --
> 2.4.3.3.g905f831
>