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

[Peter Maydell]

On 10 June 2015 at 23:17, Peter Crosthwaite
<address@hidden> wrote:
> 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:
>>> +    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.

We already do this in the lpae code path; I think it's safe.

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

Currently we don't set the MPU feature bit for M3; anybody
adding MPU support will need to check the code anyway, so
having the function not be called at all for M profile is
probably more confusing than helpful. I just wondered if
you'd looked at whether the two are really identical or not...

thanks
-- PMM