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

[Peter Crosthwaite]

On Tue, Jun 2, 2015 at 4:59 AM, Peter Maydell <address@hidden> wrote:
> 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 */
>

Added.

>> +            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.
>

Yeh, so I thought this preferrable to having to dup the arm_feature() iffery.

>> +            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.
>

Nice catch. It is actually a bug in get_phys_addr. Unlike the existing
get_phys_addr_foo implementations, get_phys_addr_pmsav7 needs to
handle the disabled case, as the behaviour should still default to the
backgrounding rather than full system access. That check needs to be
disabled in our 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.
>

Fixed.

>> +            }
>
> 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?
>

So this gets complex due to subregions. The real permissions
granularity in on the subregion level. To be safe we would have to go
lowest common denominator with 8K regions so that subregions are only
1k. I have instead added a check to allow use for a 1k region with
consistent subregion settings but disallow inconsistent SRs. The
solution is generalised for all region sizes and subregion bit
consistency combinations (e.g. 2k region with 4 consistent SR bits is
ok too).

I'm disallowing accesses with an UNIMP (ignoring region and
continuing) in the inconsistent case.

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

Check added. region is ignored (continue) in this case.

>> +
>> +            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.
>

changed to extract32.

>> +                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.
>

Fixed. This message should probably also include said bad value of ap
so I have added that.

>> +                }
>> +            } 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");

Same.

>> +                }
>> +            }
>> +
>> +            /* execute never */
>> +            *prot &= env->cp15.c6_dracr[n] & 1 << 12 ? ~PAGE_EXEC : ~0;
>
> Break this down into something more readable, please.
>

With the FSR return refactoring this is now an if anyway.

>> +        }
>> +    }
>> +
>> +    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?
>

Yes but that assumes that access_type encoding is correlated to
PAGE_FOO masks so I didn't want this to break if one or the other was
re-encoded.

>> +    default:
>> +        abort(); /* should be unreachable */
>> +        return 0;
>
> The usual way to write this is g_assert_not_reached(), and the return
> isn't needed.
>

Fixed.

>> +    }
>> +
>> +}
>> +
>>  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.
>

Fixed.

>>   * 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?

Not quite. It should read "PMSAv5 based systems". Fixed

> If it is, is that a bug
> we now need to fix?
>

Don't think so. Should be ok.

>> + *  * -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?
>

Nope. Just add !ARM_FEATURE_M to avoid the beartrap?

> (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.
>

And done (new patch that'll go up front of v2).

Regards,
Peter

> thanks
> -- PMM
>