Re: [Qemu-devel] [Qemu-arm] [PATCH 09/13] armv7m: Implement M profile default memory map

[Peter Maydell]

On 30 May 2017 at 15:56, Philippe Mathieu-Daudé <address@hidden> wrote:
> Hi Peter,
>
> On 04/25/2017 09:07 AM, Peter Maydell wrote:
>>
>> From: Michael Davidsaver <address@hidden>
>>
>> Add support for the M profile default memory map which is used
>> if the MPU is not present or disabled.
>>
>> The main differences in behaviour from implementing this
>> correctly are that we set the PAGE_EXEC attribute on
>> the right regions of memory, such that device regions
>> are not executable.
>>
>> Signed-off-by: Michael Davidsaver <address@hidden>
>> [PMM: rephrased comment and commit message; don't mark
>>  the flash memory region as not-writable]
>
>
> "not-writable by system caches" maybe to clarify?

(Note that this is a comment describing something I
deleted from Michael's original patch.)
No, by 'not-writable' here I mean "not writeable by the CPU".

>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  target/arm/helper.c | 35 ++++++++++++++++++++++++++---------
>>  1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 9e1ed1c..51662ad 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -8129,18 +8129,35 @@ 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 */
>> +    if (!arm_feature(env, ARM_FEATURE_M)) {
>> +        *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;
>
> I checked at Table B3-1 on the ARMv7-M Architecture Reference Manual I got
> at https://static.docs.arm.com/ddi0403/e/DDI0403E_B_armv7m_arm.pdf and the
> on-chip peripheral address space at 0x40000000 is eXecute Never.

This is the arm of the if() that deals with R profile, and R profile's
background region is completely different to M profile. (In any
case the patch shouldn't change the behaviour there.)

>> +            break;
>> +        }
>> +    } else {
>> +        /* Default system address map for M profile cores.
>> +         * The architecture specifies which regions are execute-never;
>> +         * at the MPU level no other checks are defined.
>> +         */
>> +        switch (address) {
>> +        case 0x00000000 ... 0x1fffffff: /* ROM */
>> +        case 0x20000000 ... 0x3fffffff: /* SRAM */
>> +        case 0x60000000 ... 0x7fffffff: /* RAM */
>> +        case 0x80000000 ... 0x9fffffff: /* RAM */
>> +            *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>> +            break;
>> +        default: /* Peripheral, 2x Device, and System */
>> +            *prot = PAGE_READ | PAGE_WRITE;
>
>
> This body is correct, however what do you think about using cases with
> comments instead of 'default'? This would be clearer.

Yeah, we could do that. I think this is one of those cases where
I opted to go with how Michael had already written the code rather
than rewriting it.

        /* Default system address map for M profile cores.
         * The architecture specifies which regions are execute-never;
         * at the MPU level no other checks are defined.
         */
        switch (address) {
        case 0x00000000 ... 0x1fffffff: /* ROM */
        case 0x20000000 ... 0x3fffffff: /* SRAM */
        case 0x60000000 ... 0x7fffffff: /* RAM */
        case 0x80000000 ... 0x9fffffff: /* RAM */
            *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
            break;
        case 0x40000000 ... 0x5fffffff: /* Peripheral */
        case 0xa0000000 ... 0xbfffffff: /* Device */
        case 0xc0000000 ... 0xdfffffff: /* Device */
        case 0xe0000000 ... 0xffffffff: /* System */
            *prot = PAGE_READ | PAGE_WRITE;
            break;
        default:
            g_assert_not_reached();
        }

would be the explicit version.

>>          }
>> -        break;
>> -    case 0x00000000 ... 0x7FFFFFFF:
>> -        *prot |= PAGE_EXEC;
>> -        break;
>>      }
>> -
>>  }
>>
>>  static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,

thanks
-- PMM